Add tests for aliases#28
Merged
Merged
Conversation
Reviewer's GuideExtends sudo-related pytest coverage for the local (bare client) provider by enabling an existing netgroup test and adding focused tests for sudoers aliases (user, command, host, and runas), and aligns flake8’s max line length with Black via pyproject.toml updates. 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 found 3 issues, and left some high level feedback:
- Several tests hard-code the password string "Secret123" multiple times; consider centralizing this into a shared constant or fixture to avoid repetition and ease future changes.
- The alias-based sudo tests share a lot of setup logic (user creation, alias creation, sudo rule creation, and SSSD restart); extracting common helpers or fixtures would make the tests more concise and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests hard-code the password string "Secret123" multiple times; consider centralizing this into a shared constant or fixture to avoid repetition and ease future changes.
- The alias-based sudo tests share a lot of setup logic (user creation, alias creation, sudo rule creation, and SSSD restart); extracting common helpers or fixtures would make the tests more concise and easier to maintain.
## Individual Comments
### Comment 1
<location path="pytest/tests/test_basic.py" line_range="1108-1117" />
<code_context>
+def test_basic__user_alias_with_group_member(
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative case for a user not in the group-based User_Alias.
Consider also adding a user who is not in `group-1` and asserting that sudo is denied for that user, to confirm the `User_Alias` is not applied to non-members.
Suggested implementation:
```python
:setup:
1. Create users "user-1" and "user-2" and group "group-1" with user-1 as member and user-2 not a member
2. Define ``User_Alias SUDO_SUBJECTS`` listing ``%group-1``
3. Create sudorule for ``SUDO_SUBJECTS`` to run /bin/ls on all hosts
4. Enable SSSD sudo responder and start SSSD
5. Verify that sudo is allowed for "user-1" and denied for "user-2", confirming that the User_Alias is not applied to non-members of ``group-1``
```
To fully implement the negative case:
1. In `test_basic__user_alias_with_group_member`, ensure a second user (e.g. `user_2` or similar) is created who is *not* added to `group-1` (matching the naming and helper APIs already used in this file for creating users and groups).
2. After the existing positive sudo assertion for the group member, add an assertion similar to:
```python
assert not client.auth.sudo.run(
user_2.name,
"Secret123",
command="/bin/ls /root",
), f"User {user_2.name} should be denied (not in group-1 User_Alias)"
```
adapting `user_2`/password/command to whatever variables and helpers are already used in this test file.
3. Keep the style consistent with the earlier test that asserts a non-member is denied (the `u3` negative case shown at the top of your snippet), including any helper functions or fixtures used to provision users and set passwords.
</issue_to_address>
### Comment 2
<location path="pytest/tests/test_basic.py" line_range="1141-1150" />
<code_context>
+def test_basic__command_alias(
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the Cmnd_Alias test with a command that should be denied.
To better verify the alias behavior, please also assert that a different command (e.g. `/bin/id` or `/bin/cat`) is denied for the same user and rule. This ensures only the aliased command is permitted and guards against overly broad aliases.
Suggested implementation:
```python
"""
:title: Sudo rule may reference a Cmnd_Alias
:setup:
1. Create user "user-1"
2. Define ``Cmnd_Alias LSHELP`` as /bin/ls
3. Create sudorule allowing user-1 to run ``LSHELP`` on all hosts
4. Enable SSSD sudo responder and start SSSD
5. Verify that the same user is *denied* sudo for a non-aliased command (e.g. ``/bin/id``)
```
To fully implement the suggestion, you should also add a negative assertion in the body of `test_basic__command_alias`, reusing the same user and rule that allow running the `LSHELP` alias. For example, after the existing positive check that `/bin/ls` (or `LSHELP`) works, add something like:
```python
assert not client.auth.sudo.run(
user.name, "Secret123", command="/bin/id"
), f"User {user.name} should not be allowed to run /bin/id via sudo (Cmnd_Alias)"
```
Replace `user` and the password with the actual variable names/values used in the test for the allowed command. Place this assertion close to the existing successful sudo assertion so that the test clearly verifies both permitted and denied behavior for the same rule.
</issue_to_address>
### Comment 3
<location path="pytest/tests/test_basic.py" line_range="1173-1182" />
<code_context>
+def test_basic__host_alias(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative assertion for Host_Alias with a non-matching host.
Since this only covers the positive case, please also add a `Host_Alias` that does not match the current host (e.g., a bogus hostname) and assert that a rule using that alias is not applied, to confirm that host alias matching can fail as expected.
Suggested implementation:
```python
"""
:title: Sudo rule may reference a Host_Alias for sudoHost
:setup:
1. Create user "user-1"
2. Define ``Host_Alias TRUSTED`` with the client's short hostname
3. Create sudorule allowing user-1 to run /bin/ls on ``TRUSTED``
4. Enable SSSD sudo responder and start SSSD
5. Define ``Host_Alias UNTRUSTED`` with a bogus/non-matching hostname
6. Create a sudorule using ``UNTRUSTED`` and assert that it is not applied
```
To fully implement the requested negative assertion, extend the body of `test_basic__host_alias` to:
1. Create a second Host_Alias (e.g. `UNTRUSTED`) with a hostname that does not match the client (for example `"definitely-not-" + client.system.hostname` or a fixed bogus hostname).
2. Create a sudo rule for the same user and command, but with `sudoHost` set to the `UNTRUSTED` Host_Alias.
3. After configuring SSSD and refreshing sudo rules (using whatever helper is already present in this test file, e.g. `client.sssd.restart()` or a dedicated sudo refresh), assert that:
- The positive case still works when using the `TRUSTED` alias (existing assertion).
- A sudo attempt matching the `UNTRUSTED` rule does *not* succeed. Depending on your helper API this will likely look like:
- `assert not client.auth.sudo.run(u.name, "Secret123", command="/bin/ls /root", expect_failure=True)` or
- wrapping `client.auth.sudo.run(...)` in a `with pytest.raises(...)` or checking the returned object’s exit code, following the patterns used elsewhere in `test_basic.py`.
Make sure the negative assertion explicitly exercises the rule that references the non-matching `UNTRUSTED` Host_Alias, so the test confirms that host alias matching can fail as expected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
shridhargadekar
requested changes
Apr 28, 2026
shridhargadekar
requested changes
Apr 28, 2026
Collaborator
shridhargadekar
left a comment
There was a problem hiding this comment.
apart from minor change request. rest looks good.
Add tests for aliases on local provider (sudoers files). Add enable netgroup test for local provider. Add flake8 line length rule to pytest/pyproject.toml
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
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.
Add tests for aliases on local provider (sudoers files).
Add enable netgroup test for local provider.
Add flake8 line length rule to pytest/pyproject.toml
Depends on: SSSD/sssd-test-framework#242
Summary by Sourcery
Add coverage for sudoers alias handling and enable netgroup testing on bare client topology while aligning flake8 configuration with Black’s line length.
Build:
Tests: