PR#51044: Issue 51027 - Test passwordHistory is not rewritten on a fail attempt - 389-ds-base

389-ds-base

#51044 Issue 51027 - Test passwordHistory is not rewritten on a fail attempt

Closed 3 years ago by spichugi. Opened 4 years ago by spichugi.

spichugi/389-ds-base i51027 into master

Issue 51027 - Test passwordHistory is not rewritten on a fail attempt

Simon Pichugin • 4 years ago

05f8661

dirsrvtests/tests/suites/password/pwp_history_test.py

file modified

+102 -32

		`@@ -22,43 +22,122 @@`
		`logging.getLogger(__name__).setLevel(logging.DEBUG)`
		`log = logging.getLogger(__name__)`

		`+ USER_PWD = 'password'`

		`- def test_basic(topology_st):`
		`+`
		`+ @pytest.fixture(scope="function")`
		`+ def user(topology_st, request):`
		`+ """Add and remove a test user"""`
		`+`
		`+ dm = DirectoryManager(topology_st.standalone)`
		`+`
		`+ # Add aci so users can change their own password`
		`+ USER_ACI = '(targetattr="userpassword \|\| passwordHistory")(version 3.0; acl "pwp test"; allow (all) userdn="ldap:///self";)'`
		`+ ous = OrganizationalUnits(topology_st.standalone, DEFAULT_SUFFIX)`
		`+ ou = ous.get('people')`
		`+ ou.add('aci', USER_ACI)`
		`+`
		`+ # Create a user`
		`+ users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)`
		`+ user = users.create_test_user()`
		`+ user.set('userpassword', USER_PWD)`
		`+ def fin():`
		`+ dm.rebind()`
		`+ user.delete()`
		`+ ou.remove('aci', USER_ACI)`
		`+ request.addfinalizer(fin)`
		`+ return user`
		`+`
		`+`
		`+ def test_history_is_not_overwritten(topology_st, user):`
		`+ """Test that passwordHistory user attribute is not overwritten`
		`+`
		`+ :id: 1b311532-dd55-4072-88a9-1f960cb371bd`
		`+ :setup: Standalone instance, a test user`
		`+ :steps:`
		`+ 1. Configure password history policy as bellow:`
		`+ passwordHistory: on`
		`+ passwordInHistory: 3`
		`+ 2. Change the password 3 times`
		`+ 3. Try to change the password 2 more times to see`
		`+ if it rewrites passwordHistory even on a failure attempt`
		`+ 4. Try to change the password to the initial value (it should be`
		`+ still in history)`
		`+ :expectedresults:`
		`+ 1. Password history policy should be configured successfully`
		`+ 2. Success`
		`+ 3. Password changes should be correctly rejected`
		`+ with Constrant Violation error`
		`+ 4. Password change should be correctly rejected`
		`+ with Constrant Violation error`
		`+ """`
		`+`
		`+ topology_st.standalone.config.replace_many(('passwordHistory', 'on'),`
		`+ ('passwordInHistory', '3'))`
		`+ log.info('Configured password policy.')`
		`+ time.sleep(1)`
		`+`
		`+ # Bind as the test user`
		`+ user.rebind(USER_PWD)`
		`+ time.sleep(.5)`
tbordaz commented 4 years ago Test looks good. Is it failing without all these sleep ?
		`+`
		`+ # Change the password 3 times`
		`+ user.set('userpassword', 'password1')`
		`+ user.rebind('password1')`
		`+ time.sleep(.5)`
		`+ user.set('userpassword', 'password2')`
		`+ user.rebind('password2')`
		`+ time.sleep(.5)`
		`+ user.set('userpassword', 'password3')`
		`+ user.rebind('password3')`
		`+ time.sleep(.5)`
		`+`
		`+ # Try to change the password 2 more times to see`
		`+ # if it rewrites passwordHistory even on a failure attempt`
		`+ with pytest.raises(ldap.CONSTRAINT_VIOLATION):`
		`+ user.set('userpassword', 'password2')`
		`+ time.sleep(.5)`
		`+ with pytest.raises(ldap.CONSTRAINT_VIOLATION):`
		`+ user.set('userpassword', 'password1')`
		`+ time.sleep(.5)`
		`+`
		`+ # Try to change the password to the initial value (it should be still in history)`
		`+ with pytest.raises(ldap.CONSTRAINT_VIOLATION):`
		`+ user.set('userpassword', USER_PWD)`
		`+`
		`+`
		`+ def test_basic(topology_st, user):`
		`"""Test basic password policy history feature functionality`

		`:id: 83d74f7d-3036-4944-8839-1b40bbf265ff`
		`- :setup: Standalone instance`
		`+ :setup: Standalone instance, a test user`
		`:steps:`
		`1. Configure password history policy as bellow:`
		`passwordHistory: on`
		`passwordInHistory: 3`
		`passwordChange: on`
		`passwordStorageScheme: CLEAR`
		`- 2. Add a test user`
		`- 3. Attempt to change password to the same password`
		`- 4. Change password four times`
		`- 5. Check that we only have 3 passwords stored in history`
		`- 6. Attempt to change the password to previous passwords`
		`- 7. Reset password by Directory Manager (admin reset)`
		`- 8. Try and change the password to the previous password before the reset`
		`- 9. Test passwordInHistory set to "0" rejects only the current password`
		`- 10. Test passwordInHistory set to "2" rejects previous passwords`
		`-`
		`-`
		`+ 2. Attempt to change password to the same password`
		`+ 3. Change password four times`
		`+ 4. Check that we only have 3 passwords stored in history`
		`+ 5. Attempt to change the password to previous passwords`
		`+ 6. Reset password by Directory Manager (admin reset)`
		`+ 7. Try and change the password to the previous password before the reset`
		`+ 8. Test passwordInHistory set to "0" rejects only the current password`
		`+ 9. Test passwordInHistory set to "2" rejects previous passwords`
		`:expectedresults:`
		`1. Password history policy should be configured successfully`
		`- 2. User should be added successfully`
		`- 3. Password change should be correctly rejected`
		`+ 2. Password change should be correctly rejected`
		`with Constrant Violation error`
		`- 4. Password should be successfully changed`
		`- 5. Only 3 passwords should be stored in history`
		`- 6. Password changes should be correctly rejected`
		`+ 3. Password should be successfully changed`
		`+ 4. Only 3 passwords should be stored in history`
		`+ 5. Password changes should be correctly rejected`
		`with Constrant Violation error`
		`- 7. Password should be successfully reset`
		`- 8. Password change should be correctly rejected`
		`+ 6. Password should be successfully reset`
		`+ 7. Password change should be correctly rejected`
		`with Constrant Violation error`
		`+ 8. Success`
		`9. Success`
		`- 10. Success`
		`"""`

		`#`
		`@@ -76,17 +155,8 @@`
		`assert False`
		`time.sleep(1)`

		`- # Add aci so users can change their own password`
		`- USER_ACI = '(targetattr="userpassword \|\| passwordHistory")(version 3.0; acl "pwp test"; allow (all) userdn="ldap:///self";)'`
		`- ous = OrganizationalUnits(topology_st.standalone, DEFAULT_SUFFIX)`
		`- ou = ous.get('people')`
		`- ou.add('aci', USER_ACI)`
		`-`
		`- # Create user`
		`- users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)`
		`- user = users.create(properties=TEST_USER_PROPERTIES)`
		`- user.set('userpassword', 'password')`
		`- user.rebind('password')`
		`+ # Bind as the test user`
		`+ user.rebind(USER_PWD)`

		`#`
		`# Test that password history is enforced.`

spichugi commented 4 years ago

Description: Add a test that check that "passwordHistory" attribute
for a user doesn't get updated if a password change fails due to
password repetition.
Add a fixture for the test user and its ACI.

https://pagure.io/389-ds-base/issue/51027

Reviewed by: ?

tbordaz commented on line 63 of dirsrvtests/tests/suites/password/pwp_history_test.py 4 years ago

Test looks good. Is it failing without all these sleep ?

spichugi commented 4 years ago

Test looks good. Is it failing without all these sleep ?

Yes, it successfully sets a password even when it's in passwordHistory.

    # Try to change the password 2 more times to see
    # if it rewrites passwordHistory even on a failure attempt
    with pytest.raises(ldap.CONSTRAINT_VIOLATION):
>           user.set('userpassword', 'password2')
E           Failed: DID NOT RAISE <class 'ldap.CONSTRAINT_VIOLATION'>

But it is a known issue for some time (other tests are written in a certain way to honor it). We can improve the passwordHistory processing at some point but I think it is nearly a corner case and I don't see much room for security exploit here.
We can discuss and triage it though :)
I've opened an issue - https://pagure.io/389-ds-base/issue/51046

tbordaz commented 4 years ago

Thanks for the information. You have my ACK

rebased onto 05f8661

4 years ago

Pull-Request has been merged by spichugi

4 years ago

spichugi commented 3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4097

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago

Metadata

Assignee

None

Tags

No Tags

Changes Summary 1

+102 -32

file changed

dirsrvtests/tests/suites/password/pwp_history_test.py

389-ds-base

Source Code

#51044 Issue 51027 - Test passwordHistory is not rewritten on a fail attempt Closed 3 years ago by spichugi. Opened 4 years ago by spichugi. spichugi/389-ds-base i51027 into master

Metadata

Changes Summary 1

#51044 Issue 51027 - Test passwordHistory is not rewritten on a fail attempt

Closed 3 years ago by spichugi. Opened 4 years ago by spichugi.

spichugi/389-ds-base i51027 into master