-
Notifications
You must be signed in to change notification settings - Fork 247
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.
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
Tests: Add passkey test cases for following scenario. #6727
Conversation
4480a5a
to
0f9b578
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
074f569
to
5782acb
Compare
5782acb
to
c3e5e90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good but there are some minor comments.
c3e5e90
to
111b070
Compare
111b070
to
59c80fb
Compare
In general terms, it looks good to me. We are still missing the marker to skip the tests in those distributions where passkey isn't available, but you can't add them until we implement them. So, I'd keep this PR as it is until we can add those markers. Thanks for your hard work! |
59c80fb
to
6d62489
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two commits, one with the changes and one merge, you should remove the former as we don't use merges but rebases.
Apart from that, there's a minor comment inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I marked many instances of "authenticate" with a comment to "Add su
" because we received input a few weeks ago about being more specific when we talk about authentication. We need to specify how the user is authenticating as the method could be different in different cases/scenarios and we need to make sure we're describing what we're actually testing. I agree with this and think we should follow that advice.
Besides that mostly just a question about your use of checking provider name for suffix as the way to check which one is in use.
7cb24fe
to
7cb59eb
Compare
e67c96f
to
4150763
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor correction in the module docstring.
6485f73
to
4f96319
Compare
6727c97
to
a9387c7
Compare
a9387c7
to
243f729
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor change that I missed earlier for the comment in test_passkey__su_fail_mapping.
Test cases are as follows: 4. Check auth deny for incorrect pin for LDAP, IPA, Ad and Samba. 5. Check auth deny for incorrect passkey mapping for LDAP, IPA, AD and Samba. 6. Check auth of user when server is not resolvable for IPA, LDAP, AD and Samba. First PR is under review, SSSD#6634 Signed-off-by: Madhuri Upadhye <mupadhye@redhat.com>
243f729
to
41574ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for your patience and hard work. I know it has taken time to get everything polished and ready to be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for all of your effort here!
Test cases are as follows:
4. Check auth deny for incorrect pin for LDAP and IPA.
5. Check auth deny for incorrect passkey mapping for LDAP and IPA.
6. Check auth of user when server is not resolvable for IPA and LDAP.
First PR is under review, #6634