From 461dae20de966011003dd111057ce1870dfde87f Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Fri, 16 Nov 2018 14:39:50 +0100 Subject: [PATCH] KEYCLOAK-8731 Ensure password history is kept in line with password policy --- .../PasswordCredentialProvider.java | 5 +- .../account/AccountFormServiceTest.java | 110 ++++++++++++++++-- 2 files changed, 102 insertions(+), 13 deletions(-) diff --git a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java index 2be347130a..98fd9286a1 100644 --- a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java +++ b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java @@ -111,8 +111,8 @@ public class PasswordCredentialProvider implements CredentialProvider, Credentia CredentialModel oldPassword = getPassword(realm, user); if (oldPassword == null) return; int expiredPasswordsPolicyValue = policy.getExpiredPasswords(); + List list = getCredentialStore().getStoredCredentialsByType(realm, user, CredentialModel.PASSWORD_HISTORY); if (expiredPasswordsPolicyValue > 1) { - List list = getCredentialStore().getStoredCredentialsByType(realm, user, CredentialModel.PASSWORD_HISTORY); // oldPassword will expire few lines below, and there is one active password, // hence (expiredPasswordsPolicyValue - 2) passwords should be left in history final int passwordsToLeave = expiredPasswordsPolicyValue - 2; @@ -129,7 +129,8 @@ public class PasswordCredentialProvider implements CredentialProvider, Credentia oldPassword.setType(CredentialModel.PASSWORD_HISTORY); getCredentialStore().updateCredential(realm, user, oldPassword); } else { - session.userCredentialManager().removeStoredCredential(realm, user, oldPassword.getId()); + list.stream().forEach(p -> getCredentialStore().removeStoredCredential(realm, user, p.getId())); + getCredentialStore().removeStoredCredential(realm, user, oldPassword.getId()); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java index fdda24253a..cfae10337d 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java @@ -23,6 +23,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.keycloak.admin.client.resource.RealmResource; +import org.keycloak.credential.CredentialModel; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.events.EventType; @@ -30,6 +31,8 @@ import org.keycloak.models.AccountRoles; import org.keycloak.models.AdminRoles; import org.keycloak.models.Constants; import org.keycloak.models.PasswordPolicy; +import org.keycloak.models.RealmModel; +import org.keycloak.models.UserModel; import org.keycloak.models.utils.TimeBasedOTP; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.EventRepresentation; @@ -40,6 +43,7 @@ import org.keycloak.services.resources.account.AccountFormService; import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.admin.ApiUtil; +import org.keycloak.testsuite.arquillian.AuthServerTestEnricher; import org.keycloak.testsuite.drone.Different; import org.keycloak.testsuite.pages.AccountApplicationsPage; import org.keycloak.testsuite.pages.AccountFederatedIdentityPage; @@ -66,11 +70,14 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import org.hamcrest.Matchers; +import org.junit.Assume; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItems; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; /** @@ -410,87 +417,168 @@ public class AccountFormServiceTest extends AbstractTestRealmKeycloakTest { private void assertChangePasswordSucceeds(String currentPassword, String newPassword) { changePasswordPage.changePassword(currentPassword, newPassword, newPassword); Assert.assertEquals("Your password has been updated.", profilePage.getSuccess()); - events.expectAccount(EventType.UPDATE_PASSWORD).assertEvent(); + events.expectAccount(EventType.UPDATE_PASSWORD).user(userId).assertEvent(); } private void assertChangePasswordFails(String currentPassword, String newPassword) { changePasswordPage.changePassword(currentPassword, newPassword, newPassword); Assert.assertThat(profilePage.getError(), containsString("Invalid password: must not be equal to any of last")); - events.expectAccount(EventType.UPDATE_PASSWORD_ERROR).error(Errors.PASSWORD_REJECTED).assertEvent(); + events.expectAccount(EventType.UPDATE_PASSWORD_ERROR).user(userId).error(Errors.PASSWORD_REJECTED).assertEvent(); } - @Test + private void assertNumberOfStoredCredentials(int expectedNumberOfStoredCredentials) { + Assume.assumeTrue("Works only on auth-server-undertow", + AuthServerTestEnricher.AUTH_SERVER_CONTAINER.equals(AuthServerTestEnricher.AUTH_SERVER_CONTAINER_DEFAULT)); + + final String uId = userId; // Needed for run-on-server + testingClient.server("test").run(session -> { + RealmModel realm = session.getContext().getRealm(); + UserModel user = session.users().getUserById(uId, realm); + assertThat(user, Matchers.notNullValue()); + List storedCredentials = session.userCredentialManager().getStoredCredentials(realm, user); + assertThat(storedCredentials, Matchers.hasSize(expectedNumberOfStoredCredentials)); + }); + } + + @Test public void changePasswordWithPasswordHistoryPolicyThreePasswords() { + userId = createUser("test", "user-changePasswordWithPasswordHistoryPolicyThreePasswords", "password"); + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(3)"); changePasswordPage.open(); - loginPage.login("test-user@localhost", "password"); - events.expectLogin().client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); + loginPage.login("user-changePasswordWithPasswordHistoryPolicyThreePasswords", "password"); + events.expectLogin().user(userId).client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); assertChangePasswordFails ("password", "password"); // current: password + assertNumberOfStoredCredentials(1); assertChangePasswordSucceeds("password", "password3"); // current: password + assertNumberOfStoredCredentials(2); assertChangePasswordFails ("password3", "password"); // current: password1, history: password + assertNumberOfStoredCredentials(2); assertChangePasswordFails ("password3", "password3"); // current: password1, history: password + assertNumberOfStoredCredentials(2); assertChangePasswordSucceeds("password3", "password4"); // current: password1, history: password + assertNumberOfStoredCredentials(3); assertChangePasswordFails ("password4", "password"); // current: password2, history: password, password1 + assertNumberOfStoredCredentials(3); assertChangePasswordFails ("password4", "password3"); // current: password2, history: password, password1 + assertNumberOfStoredCredentials(3); assertChangePasswordFails ("password4", "password4"); // current: password2, history: password, password1 + assertNumberOfStoredCredentials(3); assertChangePasswordSucceeds("password4", "password5"); // current: password2, history: password, password1 + assertNumberOfStoredCredentials(3); assertChangePasswordSucceeds("password5", "password"); // current: password3, history: password1, password2 + assertNumberOfStoredCredentials(3); } @Test public void changePasswordWithPasswordHistoryPolicyTwoPasswords() { + userId = createUser("test", "user-changePasswordWithPasswordHistoryPolicyTwoPasswords", "password"); + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(2)"); changePasswordPage.open(); - loginPage.login("test-user@localhost", "password"); - events.expectLogin().client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); + loginPage.login("user-changePasswordWithPasswordHistoryPolicyTwoPasswords", "password"); + events.expectLogin().user(userId).client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); assertChangePasswordFails ("password", "password"); // current: password + assertNumberOfStoredCredentials(1); assertChangePasswordSucceeds("password", "password1"); // current: password + assertNumberOfStoredCredentials(2); assertChangePasswordFails ("password1", "password"); // current: password1, history: password + assertNumberOfStoredCredentials(2); assertChangePasswordFails ("password1", "password1"); // current: password1, history: password + assertNumberOfStoredCredentials(2); assertChangePasswordSucceeds("password1", "password2"); // current: password1, history: password + assertNumberOfStoredCredentials(2); assertChangePasswordFails ("password2", "password1"); // current: password2, history: password1 + assertNumberOfStoredCredentials(2); assertChangePasswordFails ("password2", "password2"); // current: password2, history: password1 + assertNumberOfStoredCredentials(2); assertChangePasswordSucceeds("password2", "password"); // current: password2, history: password1 + assertNumberOfStoredCredentials(2); } @Test public void changePasswordWithPasswordHistoryPolicyOnePwds() { + userId = createUser("test", "user-changePasswordWithPasswordHistoryPolicyOnePwds", "password"); + // One password means only the active password is checked setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(1)"); changePasswordPage.open(); - loginPage.login("test-user@localhost", "password"); - events.expectLogin().client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); + loginPage.login("user-changePasswordWithPasswordHistoryPolicyOnePwds", "password"); + events.expectLogin().user(userId).client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); assertChangePasswordFails ("password", "password"); // current: password + assertNumberOfStoredCredentials(1); assertChangePasswordSucceeds("password", "password6"); // current: password + assertNumberOfStoredCredentials(1); assertChangePasswordFails ("password6", "password6"); // current: password1 + assertNumberOfStoredCredentials(1); assertChangePasswordSucceeds("password6", "password"); // current: password1 + assertNumberOfStoredCredentials(1); } @Test public void changePasswordWithPasswordHistoryPolicyZeroPwdsInHistory() { + userId = createUser("test", "user-changePasswordWithPasswordHistoryPolicyZeroPwdsInHistory", "password"); + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(0)"); changePasswordPage.open(); - loginPage.login("test-user@localhost", "password"); - events.expectLogin().client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); + loginPage.login("user-changePasswordWithPasswordHistoryPolicyZeroPwdsInHistory", "password"); + events.expectLogin().user(userId).client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); assertChangePasswordFails ("password", "password"); // current: password + assertNumberOfStoredCredentials(1); assertChangePasswordSucceeds("password", "password1"); // current: password + assertNumberOfStoredCredentials(1); assertChangePasswordFails ("password1", "password1"); // current: password1 + assertNumberOfStoredCredentials(1); assertChangePasswordSucceeds("password1", "password"); // current: password1 + assertNumberOfStoredCredentials(1); + } + + @Test + public void changePasswordWithPasswordHistoryPolicyExpiration() { + userId = createUser("test", "user-changePasswordWithPasswordHistoryPolicyExpiration", "password"); + + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(3)"); + + changePasswordPage.open(); + loginPage.login("user-changePasswordWithPasswordHistoryPolicyExpiration", "password"); + events.expectLogin().user(userId).client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT + "?path=password").assertEvent(); + + assertNumberOfStoredCredentials(1); + assertChangePasswordSucceeds("password", "password2"); // current: password + assertNumberOfStoredCredentials(2); + assertChangePasswordSucceeds("password2", "password4"); // current: password2, history: password + assertNumberOfStoredCredentials(3); + + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(2)"); + assertChangePasswordSucceeds("password4", "password5"); // current: password4, history: password2 + assertNumberOfStoredCredentials(2); + + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(1)"); + assertChangePasswordSucceeds("password5", "password6"); // current: password5, history: - + assertNumberOfStoredCredentials(1); + + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(2)"); + assertChangePasswordSucceeds("password6", "password7"); // current: password6, history: password5 + assertNumberOfStoredCredentials(2); + + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(0)"); + assertChangePasswordSucceeds("password7", "password8"); // current: password5, history: - + assertNumberOfStoredCredentials(1); } @Test