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

test: authentication, adding generic password policy tests #7728

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

test: authentication, adding generic password policy tests #7728

wants to merge 2 commits into from

Conversation

danlavu
Copy link

@danlavu danlavu commented Nov 30, 2024 •
edited
Loading

@danlavu danlavu marked this pull request as draft November 30, 2024 23:07
@danlavu danlavu mentioned this pull request Nov 30, 2024
Open
@danlavu danlavu force-pushed the tests-authentication-password-changes branch 3 times, most recently from bfc5ed5 to aea7f4e Compare December 4, 2024 01:51
spoore1
spoore1 requested changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

Couple of places missing ()'s I think.

@spoore1 spoore1 self-assigned this Feb 5, 2025
@danlavu danlavu force-pushed the tests-authentication-password-changes branch from aea7f4e to 1242992 Compare February 5, 2025 22:49
@danlavu danlavu force-pushed the tests-authentication-password-changes branch from 1242992 to 20248d2 Compare March 15, 2025 20:08
@danlavu danlavu marked this pull request as ready for review March 15, 2025 20:12
@danlavu
Copy link
Author

danlavu commented Mar 15, 2025 

ests/test_ldap.py::test_ldap__ppolicy_change_password_with_complexity_requirement[exop] (ldap) 
tests/test_ldap.py::test_ldap__ppolicy_change_password_with_complexity_requirement[ldap_modify] (ldap) 
tests/test_ldap.py::test_ldap__ppolicy_change_password_with_complexity_requirement[exop_force] (ldap) 
tests/test_ldap.py::test_ldap__authenticate_user_with_whitespace_prefix_in_userid (ldap) 
tests/test_ldap.py::test_ldap__shadow_policy_change_password[su] (ldap) 
tests/test_ldap.py::test_ldap__shadow_policy_change_password[ssh] (ldap) 
tests/test_ldap.py::test_ldap__search_base_is_discovered_and_defaults_to_root_dse (ldap) 
tests/test_ldap.py::test_ldap__search_base_is_discovered_and_defaults_to_root_dse_users_groups_and_netgroups[ldap_user_search_base-ou=People,dc=ldap,dc=test] (ldap) 
tests/test_ldap.py::test_ldap__search_base_is_discovered_and_defaults_to_root_dse_users_groups_and_netgroups[ldap_group_search_base-ou=Groups,dc=ldap,dc=test] (ldap) 
tests/test_ldap.py::test_ldap__search_base_is_discovered_and_defaults_to_root_dse_users_groups_and_netgroups[ldap_netgroup_search_base-ou=Netgroup,dc=ldap,dc=test] (ldap) 
tests/test_ldap.py::test_ldap__lookup_user_with_search_bases (ldap) 
tests/test_ldap.py::test_ldap__lookup_and_authenticate_as_user_with_different_object_search_bases[dc=ldap,dc=test] (ldap) 
tests/test_ldap.py::test_ldap__lookup_and_authenticate_as_user_with_different_object_search_bases[dc=shanks,dc=com] (ldap) 
tests/test_ldap.py::test_ldap__password_change_no_grace_logins_left[exop-1-Expected login failure] (ldap) 
tests/test_ldap.py::test_ldap__password_change_no_grace_logins_left[exop_force-3-Expected password change request] (ldap) 
tests/test_ldap.py::test_ldap__empty_attribute (ldap) 
tests/test_ldap.py::test_ldap__limit_search_base_group (ldap) 


================ 17 passed, 670 deselected in 164.95s (0:02:44) ================
PASSED [  5%]PASSED [ 11%]PASSED [ 17%]PASSED [ 23%]PASSED [ 29%]PASSED [ 35%]PASSED [ 41%]PASSED [ 47%]PASSED [ 52%]PASSED [ 58%]PASSED [ 64%]PASSED [ 70%]PASSED [ 76%]PASSED [ 82%]PASSED [ 88%]PASSED             [ 94%]PASSED     [100%]
Process finished with exit code 0

Will post the test_authentication runs when they are done, but all the test are passing with SSSD/sssd-test-framework#139

@danlavu danlavu marked this pull request as draft March 15, 2025 20:14
@danlavu danlavu marked this pull request as ready for review March 17, 2025 22:42
@danlavu danlavu force-pushed the tests-authentication-password-changes branch 2 times, most recently from bf3564c to a1a7718 Compare March 17, 2025 22:52
@danlavu danlavu requested a review from aplopez March 17, 2025 22:53
@danlavu
Copy link
Author

danlavu commented Mar 17, 2025

This is ready for review.

@danlavu danlavu requested a review from spoore1 March 17, 2025 22:53
tests: adding generic password change tests
8811c77 
* user is forced to changed password at login
* user logins and issues a password change
@danlavu danlavu force-pushed the tests-authentication-password-changes branch from a1a7718 to 28554ea Compare March 18, 2025 17:05
spoore1
spoore1 requested changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

Mostly comment/wording changes suggested with a few questions mixed in. Looks like a good set of tests for password policy.

src/tests/system/tests/test_authentication.py Outdated
provider: GenericProvider,
):
"""
:title: User logins and issues a password change
Copy link
Contributor

Choose a reason for hiding this comment

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

The wording for the title seems difficult to read to me. Maybe something like user logs in and issues a password change? Or maybe just user authenticates and issues a password change? Or User issues a password change after login? I'm not sure which makes the most sense.

Copy link
Author

Choose a reason for hiding this comment

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

I like the last one, it clear on what it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@danlavu danlavu force-pushed the tests-authentication-password-changes branch 3 times, most recently from 29bec62 to 1da3edb Compare March 18, 2025 23:36
tests: removed overlapping test scenarios from authentication tests
922826c 
* few scenarios have been removed
* ppolicy tests have been made into ppolicy tests only, since normal ldap is covered by the generic provider now
* renamed some of the test cases
* removed su from a password change test
* removed some test cases that are now covered by the new test cases
@danlavu danlavu force-pushed the tests-authentication-password-changes branch from 1da3edb to 922826c Compare March 19, 2025 18:32
aplopez
aplopez reviewed Mar 20, 2025
View reviewed changes
src/tests/system/tests/test_authentication.py
1. User is authenticated
2. Password change is unsuccessful
3. Password change is successful
4. User cannot log in
Copy link
Contributor

@aplopez aplopez Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see the required topology is AnyProvider, so, if I understood correctly, this test could be run against an AD.

If I remember correctly, AD will allow you to log in with your old password during 1 hour (this is the default but configurable) after you changed it. In this situation this check (and thus the test) will fail.

I think this behavior should be disabled if the provider is AD. Or is it already done by the framework?

src/tests/system/tests/test_authentication.py
provider: GenericProvider,
):
"""
:title: User issues a password change after login with password policy complexity enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any difference with the previous test? The previous test also has complexity enabled.

src/tests/system/tests/test_authentication.py

# 389ds 'Must change password' needs to be triggered by an administrative password reset first.
if isinstance(provider, LDAP):
user.modify(password=old_pass)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be part of the LDAPUser.password_change_at_logon() method if this is required by the LDAP server?

src/tests/system/tests/test_authentication.py
:steps
1. Login as user
2. Offline, login as user
3. Offline, login as user with bad password
3. Offline user authentication with incorrect password
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two lines with number 3.

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

@spoore1 spoore1 spoore1 requested changes

@aplopez aplopez aplopez left review comments

Assignees

@spoore1 spoore1

@aplopez aplopez

Labels
branch: sssd-2-9-4 Corresponds to C8S branch: sssd-2-9 branch: sssd-2-10 Changes requested Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danlavu @spoore1 @aplopez