Sudo ipv4 ipv6 mask tests#31
Closed
shridhargadekar wants to merge 2 commits into
Closed
Conversation
Reviewer's GuideExtends sudo tests to cover IP-based sudoHost matching for IPv4/IPv6 (with and without CIDR masks) and strengthens the duplicate sudoUser test by using multiple users and iterating over them to verify rule resolution is not mangled. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_sudo__duplicate_sudo_userthe setup docstring mentions creatinguser-4, but the test never creates or uses it; either add the missing user usage or update the description to match the actual behavior. - In the new IP-based sudo tests you call
client.host.ssh.run(..., raise_on_error=False)but never check the result; consider asserting or at least validating the command succeeded so the tests don't silently pass when IP configuration fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_sudo__duplicate_sudo_user` the setup docstring mentions creating `user-4`, but the test never creates or uses it; either add the missing user usage or update the description to match the actual behavior.
- In the new IP-based sudo tests you call `client.host.ssh.run(..., raise_on_error=False)` but never check the result; consider asserting or at least validating the command succeeded so the tests don't silently pass when IP configuration fails.
## Individual Comments
### Comment 1
<location path="pytest/tests/test_sudo.py" line_range="662-618" />
<code_context>
+ client.sssd.common.sudo()
+ client.sssd.start()
+
+ assert client.auth.sudo.list("user-1", "Secret123"), f"Sudo list failed for sudoHost={host_value}!"
+ assert client.auth.sudo.run("user-1", "Secret123", command="/bin/ls /root"), "Sudo command failed!"
+
+ finally:
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions by checking expected sudo rules, not just success/failure
Here you only assert that `sudo.list(...)` is truthy, while other tests in this file use the `expected=` parameter to validate the returned rules. Where possible, assert the specific expected rule(s) (e.g. that `ALL` or `/bin/ls` appears) so the test verifies not just that access is granted but that the resolved rule content is correct for IPv4/IPv6 and CIDR sudoHost values.
Suggested implementation:
```python
assert client.auth.sudo.list(
"user-1",
"Secret123",
expected=[host_value, "ALL"],
), f"Sudo list failed for sudoHost={host_value}!"
```
The `expected` parameter usage here assumes that:
1. `client.auth.sudo.list(...)` accepts an `expected` keyword argument.
2. The list output contains both the `sudoHost` value (`host_value`) and the command (`ALL`) as strings that can be matched directly.
You should align the `expected=[host_value, "ALL"]` structure with the patterns used elsewhere in `pytest/tests/test_sudo.py`. For example, if other tests pass `expected` as a single string, a dict, or a more specific pattern (e.g. `expected=[f"sudoHost: {host_value}", "sudoCommand: ALL"]`), adjust this call to match that exact convention so the assertion validates the resolved rule content in the same way.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -611,3 +616,105 @@ def test_sudo__defaults_set_no_auth_and_sudo_rule_has_mandatory_auth(client: Cli | |||
| assert client.auth.sudo.list("user-1", expected=["(root) PASSWD: ALL"]), "Sudo list failed!" | |||
| assert not client.auth.sudo.run("user-1", command="/bin/ls /root"), "Sudo command successful!" | |||
| assert client.auth.sudo.run("user-1", "Secret123", command="/bin/ls /root"), "Sudo command failed!" | |||
There was a problem hiding this comment.
suggestion (testing): Strengthen assertions by checking expected sudo rules, not just success/failure
Here you only assert that sudo.list(...) is truthy, while other tests in this file use the expected= parameter to validate the returned rules. Where possible, assert the specific expected rule(s) (e.g. that ALL or /bin/ls appears) so the test verifies not just that access is granted but that the resolved rule content is correct for IPv4/IPv6 and CIDR sudoHost values.
Suggested implementation:
assert client.auth.sudo.list(
"user-1",
"Secret123",
expected=[host_value, "ALL"],
), f"Sudo list failed for sudoHost={host_value}!"The expected parameter usage here assumes that:
client.auth.sudo.list(...)accepts anexpectedkeyword argument.- The list output contains both the
sudoHostvalue (host_value) and the command (ALL) as strings that can be matched directly.
You should align the expected=[host_value, "ALL"] structure with the patterns used elsewhere in pytest/tests/test_sudo.py. For example, if other tests pass expected as a single string, a dict, or a more specific pattern (e.g. expected=[f"sudoHost: {host_value}", "sudoCommand: ALL"]), adjust this call to match that exact convention so the assertion validates the resolved rule content in the same way.
- test_sudo__host_ipv4_ipv6_with_mask_allowed: allowed access when client IP matches sudoHost (IPv4 with mask, IPv6, IPv6 with mask) - test_sudo__host_ipv4_ipv6_with_mask_denied: Tests denied access when client IP doesn't match sudoHost Each test is parameterized to cover 3 scenarios (6 total test runs). Tests verify that sudoHost works correctly with IPv4/IPv6 addresses and CIDR notation.
shridhargadekar
force-pushed
the
sudo-ipv4-ipv6-mask-tests
branch
from
685931e to
349862d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Extend sudo integration tests to cover IP-based sudoHost matching with IPv4/IPv6 addresses and CIDR masks, and broaden existing duplicate sudo user coverage across multiple users.
New Features:
Enhancements:
Tests: