Skip to content

Commit

Permalink
KEYCLOAK-8731 Ensure password history is kept in line with password p…
Browse files Browse the repository at this point in the history 
…olicy
  • Loading branch information
@hmlnarik
hmlnarik committed Nov 19, 2018
1 parent a3d4612 commit 461dae2
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 13 deletions.
5 changes: 3 additions & 2 deletions services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,8 @@ protected void expirePassword(RealmModel realm, UserModel user, PasswordPolicy p
CredentialModel oldPassword = getPassword(realm, user);
if (oldPassword == null) return;
int expiredPasswordsPolicyValue = policy.getExpiredPasswords();
List<CredentialModel> list = getCredentialStore().getStoredCredentialsByType(realm, user, CredentialModel.PASSWORD_HISTORY);
if (expiredPasswordsPolicyValue > 1) {
List<CredentialModel> 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;
Expand All @@ -129,7 +129,8 @@ protected void expirePassword(RealmModel realm, UserModel user, PasswordPolicy p
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());
}

}
Expand Down
110 changes: 99 additions & 11 deletions ...llian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,16 @@
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;
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;
Expand All @@ -40,6 +43,7 @@
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;
Expand All @@ -66,11 +70,14 @@
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;

/**
Expand Down Expand Up @@ -410,87 +417,168 @@ public void changePasswordWithRegexPatternsPolicy() {
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<CredentialModel> 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
Expand Down

0 comments on commit 461dae2

Please sign in to comment.