Fix Nutanix Prism Central tests by using uuid for hypervisor_id

[jnagare-redhat]

What Was Fixed:
Nutanix:

1. Root cause: Nutanix Prism Central doesn't expose hostname property in API, only uuid
2. Solution: Set hypervisor_id='uuid' for all Prism Central tests
3. Infrastructure: Added missing register_sat_and_enable_aps_repo fixture
4. Timeout: Increased from 20s to 60s for Nutanix (slower than other hypervisors)
5. Consistency: Added target_sat parameter to CLI deploy functions

Libvirt:

• Uses qemu+ssh:// connection URI
• SSH to the hypervisor and communicates via libvirt protocol
• SSH itself can use either password OR keys, but...
• Virt-who service running as a daemon can't interactively provide passwords
• Requires SSH key-based authentication for non-interactive access

PRT test Cases example

trigger: test-robottelo
pytest: tests/foreman/virtwho

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

Blocking issues:

• Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)

General comments:

• In setup_libvirt_ssh_auth, interpolating hypervisor_password directly into the shell command with double quotes is fragile (e.g. passwords containing ", $, or spaces); consider escaping the password or writing it to a temporary file and using sshpass -f to avoid shell-interpretation issues.
• The logic that infers hypervisor_type from request.module.__name__ in setup_libvirt_ssh_auth is brittle and depends on naming conventions; it would be more robust to derive the hypervisor type from existing configuration/fixtures (or an explicit marker) instead of parsing module names.
• The new libvirt SSH setup assumes sshpass, ssh-copy-id, and ssh-keyscan are present on the Satellite host but never checks for them; adding a lightweight pre-check with a clear failure message would make debugging environment issues much easier.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `setup_libvirt_ssh_auth`, interpolating `hypervisor_password` directly into the shell command with double quotes is fragile (e.g. passwords containing `"`, `$`, or spaces); consider escaping the password or writing it to a temporary file and using `sshpass -f` to avoid shell-interpretation issues.
- The logic that infers `hypervisor_type` from `request.module.__name__` in `setup_libvirt_ssh_auth` is brittle and depends on naming conventions; it would be more robust to derive the hypervisor type from existing configuration/fixtures (or an explicit marker) instead of parsing module names.
- The new libvirt SSH setup assumes `sshpass`, `ssh-copy-id`, and `ssh-keyscan` are present on the Satellite host but never checks for them; adding a lightweight pre-check with a clear failure message would make debugging environment issues much easier.

## Individual Comments

### Comment 1
<location> `pytest_fixtures/component/virtwho_config.py:257-266` </location>
<code_context>
+
+    hypervisor_server = settings.virtwho.libvirt.hypervisor_server
+    hypervisor_username = settings.virtwho.libvirt.hypervisor_username
+    hypervisor_password = settings.virtwho.libvirt.hypervisor_password
+
+    # Generate SSH key on Satellite if it doesn't exist
+    result = target_sat.execute(
+        'test -f /root/.ssh/id_rsa || ssh-keygen -t rsa -N "" -f /root/.ssh/id_rsa'
+    )
+
+    # Add hypervisor to known_hosts
+    target_sat.execute(
+        f'ssh-keyscan -H {hypervisor_server} >> /root/.ssh/known_hosts 2>/dev/null || true'
+    )
+
+    # Copy public key to hypervisor using sshpass
+    target_sat.execute(
+        f'sshpass -p "{hypervisor_password}" ssh-copy-id '
</code_context>

<issue_to_address>
**🚨 issue (security):** Embedding the hypervisor password directly in a shell command risks issues with special characters and potential injection.

`hypervisor_password` is interpolated directly into the command string:

```python
f'sshpass -p "{hypervisor_password}" ssh-copy-id ...'
```
If the password includes characters like `"`, `$`, backticks, or other shell metacharacters, this can break the command or allow injection. Consider either:
- Using `sshpass -f` with a temp file containing the password, or
- Properly shell-escaping the password (e.g., `shlex.quote` and adjusting the surrounding quotes), if `target_sat.execute` accepts a raw command string.

This keeps the fixture safe for arbitrary password values.
</issue_to_address>

### Comment 2
<location> `pytest_fixtures/component/virtwho_config.py:259-262` </location>
<code_context>
+    hypervisor_password = settings.virtwho.libvirt.hypervisor_password
+
+    # Generate SSH key on Satellite if it doesn't exist
+    result = target_sat.execute(
+        'test -f /root/.ssh/id_rsa || ssh-keygen -t rsa -N "" -f /root/.ssh/id_rsa'
+    )
+
</code_context>

<issue_to_address>
**suggestion (bug_risk):** The `result` variable from the key-generation command is unused and can be removed.

If you don’t need the output here, just call `target_sat.execute(...)` without assigning it. If you do, add a check on the return code (and fail early if key creation fails) so later SSH steps don’t proceed without a valid key.

```suggestion
    # Generate SSH key on Satellite if it doesn't exist
    target_sat.execute(
        'test -f /root/.ssh/id_rsa || ssh-keygen -t rsa -N "" -f /root/.ssh/id_rsa'
    )
```
</issue_to_address>

### Comment 3
<location> `tests/foreman/virtwho/cli/test_nutanix_sca.py:89-94` </location>
<code_context>
         :CaseImportance: High
         """
         form_data_cli['prism-flavor'] = "central"
+        # Prism Central doesn't expose hostname property, must use uuid for hypervisor_id
+        form_data_cli['hypervisor-id'] = "uuid"
</code_context>

<issue_to_address>
**suggestion (testing):** Strengthen Nutanix Prism Central CLI test to assert that the resulting virt-who config actually uses `hypervisor-id=uuid`

This updates the CLI form data as expected, but the test still only verifies successful deployment. To directly validate the bugfix, please add an assertion that the created virt-who configuration (or generated config file) uses `hypervisor_id=uuid` and not `hostname`. That way the test explicitly guards this behavior instead of relying on indirect signals.

Suggested implementation:

```python
        virtwho_config = target_sat.cli.VirtWhoConfig.create(form_data_cli)['general-information']
        # Ensure Prism Central virt-who config uses hypervisor_id=uuid instead of hostname
        assert virtwho_config.get('hypervisor_id') == 'uuid'
        assert virtwho_config['status'] == 'No Report Yet'

```

If the returned structure uses a different key for this field (e.g. `"hypervisor-id"` or nested under another section), adjust the assertion accordingly, for example:
- `virtwho_config.get('hypervisor-id') == 'uuid'`, or
- `virtwho_config['connection']['hypervisor_id'] == 'uuid'`.

If available in this test suite, you may also want to:
1. For the `"script"` deploy_type branch, assert that the fetched script contains a line like `hypervisor_id=uuid`.
2. For the `"id"` deploy_type branch, if there is a helper to retrieve the rendered `/etc/virt-who.d/*.conf` content, assert it also contains `hypervisor_id=uuid`.

These extra checks would further harden the test against regressions in how the configuration is rendered.
</issue_to_address>

### Comment 4
<location> `tests/foreman/virtwho/api/test_nutanix_sca.py:87-89` </location>
<code_context>
         :CaseImportance: High
         """
         form_data_api['prism_flavor'] = "central"
+        # Prism Central doesn't expose hostname property, must use uuid for hypervisor_id
+        form_data_api['hypervisor_id'] = "uuid"
         virtwho_config = target_sat.api.VirtWhoConfig(**form_data_api).create()
         assert virtwho_config.status == 'unknown'
</code_context>

<issue_to_address>
**suggestion (testing):** Add an explicit assertion that the created API virt-who config for Prism Central stores `hypervisor_id=uuid`

This test only asserts the status; please also assert that the created `virtwho_config` (or underlying Satellite resource) actually has `hypervisor_id="uuid"` when `prism_flavor` is central, ideally via an API field that exposes this value. That will directly validate the Prism Central behavior and prevent regressions.
</issue_to_address>

### Comment 5
<location> `tests/foreman/virtwho/ui/test_nutanix_sca.py:108-110` </location>
<code_context>
         name = gen_string('alpha')
         form_data_ui['name'] = name
         form_data_ui['hypervisor_content.prism_flavor'] = "Prism Central"
+        # Prism Central doesn't expose hostname property, must use uuid for hypervisor_id
+        form_data_ui['hypervisor_id'] = 'uuid'
         with org_session:
             org_session.virtwho_configure.create(form_data_ui)
</code_context>

<issue_to_address>
**suggestion (testing):** UI test could explicitly verify that the created configuration in the UI has hypervisor_id set to `uuid` for Prism Central

The test now sets `hypervisor_id` for Prism Central but only validates a subset of the saved config. Please extend the `values` assertions to verify that the persisted UI configuration includes `hypervisor_id='uuid'` (and that `hostname` is not used), so the UI path is validated consistently with the CLI/API paths.

Suggested implementation:

```python
        form_data_ui['hypervisor_content.prism_flavor'] = "Prism Central"
        # Prism Central doesn't expose hostname property, must use uuid for hypervisor_id
        form_data_ui['hypervisor_id'] = 'uuid'
        with org_session:
            org_session.virtwho_configure.create(form_data_ui)
            values = org_session.virtwho_configure.read(name)

            # Verify that the UI-persisted configuration uses uuid for hypervisor_id
            assert values.get('hypervisor_id') == 'uuid'

            # Verify that hostname-based hypervisor identification is not used for Prism Central
            # (adjust the key name if the UI uses a different field for hostname)
            assert not values.get('hostname') and not values.get('hypervisor_hostname')

```

1. If the persisted configuration uses a different key for the hostname (e.g. `hypervisor_hostname`, `hypervisor_fqdn`, or nested under another structure), update the hostname assertion to match the actual key(s).
2. If there is already an existing `values` assertion block (e.g. comparing against a dict subset), consider folding these new assertions into that block or extending the expected dict to include `"hypervisor_id": "uuid"` and the appropriate hostname field as empty/None for consistency.
</issue_to_address>

### Comment 6
<location> `pytest_fixtures/component/virtwho_config.py:265-267` </location>
<code_context>
    target_sat.execute(
        f'ssh-keyscan -H {hypervisor_server} >> /root/.ssh/known_hosts 2>/dev/null || true'
    )
</code_context>

<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

*Source: opengrep*
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

New security issues found

[jnagare-redhat]

pre-commit.ci autofix

[jnagare-redhat]

trigger: test-robottelo
pytest: tests/foreman/virtwho

[Satellite-QE]

PRT Result

Build Number: 13925
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/virtwho --external-logging
Test Result : === 3 failed, 87 passed, 9 deselected, 1183 warnings in 15899.85s (4:24:59) ====

[jnagare-redhat]

3 failed test cases are not related to this PR changes. Those failure can be address separately.
I am running PRT now with only specific to change

[jnagare-redhat]

trigger: test-robottelo
pytest: tests/foreman/virtwho/ui/test_nutanix_sca.py tests/foreman/virtwho/cli/test_nutanix_sca.py tests/foreman/virtwho/api/test_nutanix_sca.py

[Satellite-QE]

PRT Result

Build Number: 13932
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/virtwho/ui/test_nutanix_sca.py  tests/foreman/virtwho/cli/test_nutanix_sca.py  tests/foreman/virtwho/api/test_nutanix_sca.py --external-logging
Test Result : =========== 1 failed, 18 passed, 206 warnings in 4076.78s (1:07:56) ============