Add tests for IPv4/IPv6 with CIDR mask in sudoHost#32
Conversation
Reviewer's GuideAdds two critical BareLDAP sudo tests that validate sudoHost behavior for IPv4/IPv6 addresses with and without CIDR masks, covering both allowed and denied access cases based on client IP matching. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The two tests share a lot of setup/teardown logic (configuring client IP, adding user/sudorule, starting SSSD, and deleting IP); consider extracting this into a helper or fixture and parameterizing only the expected outcome to reduce duplication and make future changes easier.
- Using
if "ipv6" in ip_typeto decide whether to use IPv6-specific commands is a bit fragile; switching to an explicit enum or boolean flag (e.g.,is_ipv6) in the parametrization would make the logic clearer and less error-prone if additional IP types are added later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The two tests share a lot of setup/teardown logic (configuring client IP, adding user/sudorule, starting SSSD, and deleting IP); consider extracting this into a helper or fixture and parameterizing only the expected outcome to reduce duplication and make future changes easier.
- Using `if "ipv6" in ip_type` to decide whether to use IPv6-specific commands is a bit fragile; switching to an explicit enum or boolean flag (e.g., `is_ipv6`) in the parametrization would make the logic clearer and less error-prone if additional IP types are added later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a689f6e to
00c7b82
Compare
| # IPv4 with CIDR mask - positive | ||
| (False, "192.168.10.0/26", "192.168.10.5", True), | ||
| # IPv4 with CIDR mask - negative | ||
| (False, "192.168.10.0/26", "192.168.20.5", False), |
There was a problem hiding this comment.
Maybe you could add two more cases, one positive and one negative for IPv4 addresses without a mask.
There was a problem hiding this comment.
Hi,
I've added two cases(positive and negative) for IPv4. Please check.
00c7b82 to
71c03e3
Compare
|
I see the tests fail as expected. Have you tried running them against LDAP without SSSD to confirm they pass? |
|
static code failures are of unrelated test files. |
|
Please try to rebase as the static analysis should be fixed now. |
- 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
Signed-off-by: shridhargadekar <shridhar.always@gmail.com>
71c03e3 to
dab4986
Compare
|
Branch is rebased, is ready for review |
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
Summary by Sourcery
Add critical sudoHost IP-based sudo rule tests covering IPv4/IPv6 address and CIDR mask handling for allowed and denied access scenarios.
Tests: