[6.18.z] feat: Add methods to manage autosign entries for puppet/openvox on a SmartProxy

[Satellite-QE]

Cherrypick of PR: #1376

Added two methods to SmartProxy to manage autosign entries on the puppetserver.
The methods allow to add and delete autosign entries.

This is an implementation of https://apidocs.theforeman.org/foreman/3.16/apidoc/v2/autosign.html for POST and DELETE requests.

Example:

host = target_sat.api.Host().search(query={'name': some_host_name})[0]
host.puppet_ca_proxy.add_autosign_entry(host.name)
host.puppet_ca_proxy.delete_autosign_entry(host.name)

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `nailgun/entities.py:8087-8098` </location>
<code_context>
             client.post(path, **kwargs), self._server_config, synchronous, timeout
         )

+    def add_autosign_entry(self, certname, **kwargs):
+        """Add an entry to the puppetserver's autosign file.
+
+        :param certname: Name the host is going to register with
+        """
+        kwargs = kwargs.copy()
+        kwargs.update(self._server_config.get_client_kwargs())
+        path = f'{self.path()}/autosign'
+        return _handle_response(
+            client.post(path, data={'id': certname}, **kwargs),
+            self._server_config,
</code_context>

<issue_to_address>
**🚨 suggestion (security):** Consider validating certname before using it in the request.

If certname is externally sourced, ensure it is properly validated or sanitized to prevent injection or malformed requests.

```suggestion
    def _validate_certname(self, certname):
        """Validate certname to prevent injection or malformed requests."""
        import re
        # Only allow alphanumeric, dash, dot, and underscore, 1-255 chars
        if not isinstance(certname, str) or not re.match(r'^[A-Za-z0-9_.-]{1,255}$', certname):
            raise ValueError(f"Invalid certname: {certname}")

    def add_autosign_entry(self, certname, **kwargs):
        """Add an entry to the puppetserver's autosign file.

        :param certname: Name the host is going to register with
        """
        self._validate_certname(certname)
        kwargs = kwargs.copy()
        kwargs.update(self._server_config.get_client_kwargs())
        path = f'{self.path()}/autosign'
        return _handle_response(
            client.post(path, data={'id': certname}, **kwargs),
            self._server_config,
        )
```
</issue_to_address>

### Comment 2
<location> `nailgun/entities.py:8100-8095` </location>
<code_context>
+            self._server_config,
+        )
+
+    def delete_autosign_entry(self, certname, **kwargs):
+        """Delete an entry from the puppetserver's autosign file.
+
+        :param certname: Name of the host to be deleted from the autosign file
+        """
+        kwargs = kwargs.copy()
+        kwargs.update(self._server_config.get_client_kwargs())
+        path = f'{self.path()}/autosign/{certname}'
+        return _handle_response(
+            client.delete(path, **kwargs),
+            self._server_config,
</code_context>

<issue_to_address>
**suggestion:** Check for edge cases with certname in URL path.

URL-encode certname or validate its format to avoid routing issues caused by special characters or slashes.
</issue_to_address>

### Comment 3
<location> `tests/test_entities.py:3462-3471` </location>
<code_context>
                 if 'environment' in param:
                     self.assertIn('/environments', post.call_args[0][0])

+    def test_add_autosign_entry(self):
+        """Call :meth:`nailgun.entities.SmartProxy.add_autosign_entry`.
+
+        Assert that
+        * correct fqdn is sent,
+        * proper path is built.
+        """
+        certname = "host.example.com"
+        with self.subTest():
+            with mock.patch.object(client, 'post') as post:
+                self.smart_proxy.add_autosign_entry(certname)
+                self.assertEqual(post.call_count, 1)
+                self.assertIn(f'{self.smart_proxy.path()}/autosign', post.call_args[0][0])
+                self.assertEqual(len(post.call_args[1]), 1)
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding tests for error handling and invalid input for add_autosign_entry.

Please add tests for invalid certname values and for client.post failures to ensure robust error handling.

Suggested implementation:

```python
    def test_add_autosign_entry(self):
        """Call :meth:`nailgun.entities.SmartProxy.add_autosign_entry`.

        Assert that
        * correct fqdn is sent,
        * proper path is built.
        """
        certname = "host.example.com"
        with self.subTest():
            with mock.patch.object(client, 'post') as post:
                self.smart_proxy.add_autosign_entry(certname)
                self.assertEqual(post.call_count, 1)
                self.assertIn(f'{self.smart_proxy.path()}/autosign', post.call_args[0][0])
                self.assertEqual(len(post.call_args[1]), 1)
                self.assertEqual(post.call_args[1], {'data': {'id': 'host.example.com'}})

    def test_add_autosign_entry_invalid_certname(self):
        """Test add_autosign_entry with invalid certname values."""
        invalid_certnames = ["", None, "invalid/certname", "   "]
        for certname in invalid_certnames:
            with self.subTest(certname=certname):
                with mock.patch.object(client, 'post') as post:
                    with self.assertRaises(ValueError):
                        self.smart_proxy.add_autosign_entry(certname)
                    post.assert_not_called()

    def test_add_autosign_entry_post_failure(self):
        """Test add_autosign_entry when client.post raises an exception."""
        certname = "host.example.com"
        with mock.patch.object(client, 'post', side_effect=Exception("API failure")) as post:
            with self.assertRaises(Exception) as excinfo:
                self.smart_proxy.add_autosign_entry(certname)
            self.assertIn("API failure", str(excinfo.exception))
            self.assertEqual(post.call_count, 1)

```

These tests assume that `add_autosign_entry` raises a `ValueError` for invalid certname values. If it does not currently do so, you will need to update the implementation of `add_autosign_entry` in `nailgun.entities.SmartProxy` to validate the certname and raise a `ValueError` for invalid input.
</issue_to_address>

### Comment 4
<location> `tests/test_entities.py:3478-3487` </location>
<code_context>
+                self.assertEqual(len(post.call_args[1]), 1)
+                self.assertEqual(post.call_args[1], {'data': {'id': 'host.example.com'}})
+
+    def test_delete_autosign_entry(self):
+        """Call :meth:`nailgun.entities.SmartProxy.add_autosign_entry`.
+
+        Assert that
+        * correct fqdn is sent,
+        * proper path is built.
+        """
+        certname = "host.example.com"
+        with self.subTest():
+            with mock.patch.object(client, 'delete') as delete:
+                self.smart_proxy.delete_autosign_entry(certname)
+                self.assertEqual(delete.call_count, 1)
+                self.assertIn(
+                    f'{self.smart_proxy.path()}/autosign/{certname}', delete.call_args[0][0]
+                )
+                self.assertEqual(len(delete.call_args[1]), 0)
+

</code_context>

<issue_to_address>
**suggestion (testing):** Add tests for error scenarios and edge cases in delete_autosign_entry.

Please add tests for cases like deleting a non-existent entry, invalid certname input, and client.delete exceptions to improve error handling coverage.

Suggested implementation:

```python
    def test_delete_autosign_entry(self):
        """Call :meth:`nailgun.entities.SmartProxy.delete_autosign_entry`.

        Assert that
        * correct fqdn is sent,
        * proper path is built.
        """
        certname = "host.example.com"
        with self.subTest():
            with mock.patch.object(client, 'delete') as delete:
                self.smart_proxy.delete_autosign_entry(certname)
                self.assertEqual(delete.call_count, 1)
                self.assertIn(
                    f'{self.smart_proxy.path()}/autosign/{certname}', delete.call_args[0][0]
                )
                self.assertEqual(len(delete.call_args[1]), 0)

    def test_delete_autosign_entry_nonexistent(self):
        """Test deleting a non-existent autosign entry."""
        certname = "nonexistent.example.com"
        with self.subTest():
            with mock.patch.object(client, 'delete', side_effect=Exception("Not Found")) as delete:
                with self.assertRaises(Exception) as exc:
                    self.smart_proxy.delete_autosign_entry(certname)
                self.assertIn("Not Found", str(exc.exception))
                self.assertEqual(delete.call_count, 1)

    def test_delete_autosign_entry_invalid_certname(self):
        """Test deleting with invalid certname input."""
        invalid_certnames = ["", None, "invalid/certname"]
        for certname in invalid_certnames:
            with self.subTest(certname=certname):
                with mock.patch.object(client, 'delete') as delete:
                    with self.assertRaises(Exception):
                        self.smart_proxy.delete_autosign_entry(certname)
                    self.assertEqual(delete.call_count, 0)

    def test_delete_autosign_entry_client_exception(self):
        """Test client.delete raising an exception."""
        certname = "host.example.com"
        with self.subTest():
            with mock.patch.object(client, 'delete', side_effect=RuntimeError("Network error")) as delete:
                with self.assertRaises(RuntimeError) as exc:
                    self.smart_proxy.delete_autosign_entry(certname)
                self.assertIn("Network error", str(exc.exception))
                self.assertEqual(delete.call_count, 1)

```

- If `delete_autosign_entry` does not currently raise exceptions for invalid certnames, you may need to add input validation and raise an appropriate exception in its implementation.
- Adjust the exception types in the tests to match the actual exceptions raised by your codebase (e.g., custom exceptions, HTTPError, etc.).
</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]

🚨 suggestion (security): Consider validating certname before using it in the request.

If certname is externally sourced, ensure it is properly validated or sanitized to prevent injection or malformed requests.

Suggested change

	def add_autosign_entry(self, certname, **kwargs):
	"""Add an entry to the puppetserver's autosign file.
	:param certname: Name the host is going to register with
	"""
	kwargs = kwargs.copy()
	kwargs.update(self._server_config.get_client_kwargs())
	path = f'{self.path()}/autosign'
	return _handle_response(
	client.post(path, data={'id': certname}, **kwargs),
	self._server_config,
	)
	def _validate_certname(self, certname):
	"""Validate certname to prevent injection or malformed requests."""
	import re
	# Only allow alphanumeric, dash, dot, and underscore, 1-255 chars
	if not isinstance(certname, str) or not re.match(r'^[A-Za-z0-9_.-]{1,255}$', certname):
	raise ValueError(f"Invalid certname: {certname}")
	def add_autosign_entry(self, certname, **kwargs):
	"""Add an entry to the puppetserver's autosign file.
	:param certname: Name the host is going to register with
	"""
	self._validate_certname(certname)
	kwargs = kwargs.copy()
	kwargs.update(self._server_config.get_client_kwargs())
	path = f'{self.path()}/autosign'
	return _handle_response(
	client.post(path, data={'id': certname}, **kwargs),
	self._server_config,
	)