Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TESTS: Add access control simple filter tests #7874

Closed
wants to merge 1 commit into from
Closed

TESTS: Add access control simple filter tests #7874

wants to merge 1 commit into from

Conversation

andreboscatto
Copy link
Contributor

@andreboscatto andreboscatto commented Mar 12, 2025 •
edited
Loading

Added 3 tests for access control simple filter using the new testing framework

@andreboscatto andreboscatto force-pushed the testAccessControl branch 3 times, most recently from d9779d9 to 7859d54 Compare March 12, 2025 13:38
@pbrezina
Copy link
Member

Hi, maybe, it would be wort to rename the file to test_access_control_simple.py. We have more access control providers, it may be better to split it for clarity.

@andreboscatto andreboscatto removed the request for review from sumit-bose March 13, 2025 14:03
@andreboscatto andreboscatto requested a review from pbrezina March 13, 2025 14:03
danlavu
danlavu requested changes Mar 15, 2025
View reviewed changes
Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove quite a few lines and simplify the test, other than those changes, it looks good, thank you Andre.

src/tests/system/tests/test_access_control.py

client.sssd.start(service_user=sssd_service_user)

assert client.auth.parametrize(method).password("user1", "Secret123"), "User login!"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client.auth.ssh.password(...)

The second part, is the stderr message, if the assertion fails. Should be the negation, ```User cannot login!"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all the messages afterwards the assertion should be the opposite? Is that right? In this case I will change all of them, because I understood it wrong ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked more this style I implemented in the last test case:

assert client.auth.ssh.password("user2", "Secret123"), "User2 should be able to log in!"
assert not client.auth.ssh.password("user3", "Secret123"), "User3 should NOT be able to log in!"

wdyt?

@andreboscatto
Copy link
Contributor Author

I was pushing the wrong branch ¬¬ changes should be applied now

@andreboscatto
TESTS: Add access control simple filter tests
43f3c27 
Added 3 tests for access control simple filter using the new testing framework
@andreboscatto andreboscatto deleted the testAccessControl branch March 15, 2025 23:02
@andreboscatto
Copy link
Contributor Author

I did a mistake with branches and renamed, thus the PR was closed :(

Let's keep the discussion here #7882

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexey-tikhonov alexey-tikhonov alexey-tikhonov left review comments

@danlavu danlavu danlavu requested changes

@pbrezina pbrezina Awaiting requested review from pbrezina

Assignees

@danlavu danlavu

@pbrezina pbrezina

Labels
branch: sssd-2-9 Tests Waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andreboscatto @pbrezina @danlavu @alexey-tikhonov @sumit-bose