From ff551c554540f705f3cb51525058007e4a2d12c9 Mon Sep 17 00:00:00 2001 From: pkokush Date: Thu, 24 Oct 2019 22:33:21 +0300 Subject: [PATCH] KEYCLOAK-10307: check password history length in password verification (#6058) --- .../policy/HistoryPasswordPolicyProvider.java | 11 +++++++++- .../keycloak/credential/CredentialModel.java | 9 +++++++++ .../PasswordCredentialProvider.java | 6 +----- .../account/AccountFormServiceTest.java | 20 +++++++++++++++++++ 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java b/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java index fe7c9df9c36..f9084691aef 100644 --- a/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/policy/HistoryPasswordPolicyProvider.java @@ -26,6 +26,7 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import java.util.List; +import java.util.stream.Collectors; /** * @author Stian Thorgersen @@ -60,7 +61,8 @@ public class HistoryPasswordPolicyProvider implements PasswordPolicyProvider { } } List passwordHistory = session.userCredentialManager().getStoredCredentialsByType(realm, user, CredentialModel.PASSWORD_HISTORY); - for (CredentialModel cred : passwordHistory) { + List recentPasswordHistory = getRecent(passwordHistory, passwordHistoryPolicyValue - 1); + for (CredentialModel cred : recentPasswordHistory) { PasswordHashProvider hash = session.getProvider(PasswordHashProvider.class, cred.getAlgorithm()); if (hash.verify(password, cred)) { return new PolicyError(ERROR_MESSAGE, passwordHistoryPolicyValue); @@ -71,6 +73,13 @@ public class HistoryPasswordPolicyProvider implements PasswordPolicyProvider { return null; } + private List getRecent(List passwordHistory, int limit) { + return passwordHistory.stream() + .sorted(CredentialModel.comparingByStartDateDesc()) + .limit(limit) + .collect(Collectors.toList()); + } + @Override public Object parseConfig(String value) { return parseInteger(value, HistoryPasswordPolicyProviderFactory.DEFAULT_VALUE); diff --git a/server-spi/src/main/java/org/keycloak/credential/CredentialModel.java b/server-spi/src/main/java/org/keycloak/credential/CredentialModel.java index f4661835087..ef943e3129d 100755 --- a/server-spi/src/main/java/org/keycloak/credential/CredentialModel.java +++ b/server-spi/src/main/java/org/keycloak/credential/CredentialModel.java @@ -20,6 +20,7 @@ package org.keycloak.credential; import org.keycloak.common.util.MultivaluedHashMap; import java.io.Serializable; +import java.util.Comparator; /** * Used just in cases when we want to "directly" update or retrieve the hash or salt of user credential (For example during export/import) @@ -168,4 +169,12 @@ public class CredentialModel implements Serializable { public void setConfig(MultivaluedHashMap config) { this.config = config; } + + public static Comparator comparingByStartDateDesc() { + return (o1, o2) -> { // sort by date descending + Long o1Date = o1.getCreatedDate() == null ? Long.MIN_VALUE : o1.getCreatedDate(); + Long o2Date = o2.getCreatedDate() == null ? Long.MIN_VALUE : o2.getCreatedDate(); + return (-o1Date.compareTo(o2Date)); + }; + } } diff --git a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java index 98fd9286a19..5110179212c 100644 --- a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java +++ b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java @@ -118,11 +118,7 @@ public class PasswordCredentialProvider implements CredentialProvider, Credentia final int passwordsToLeave = expiredPasswordsPolicyValue - 2; if (list.size() > passwordsToLeave) { list.stream() - .sorted((o1, o2) -> { // sort by date descending - Long o1Date = o1.getCreatedDate() == null ? Long.MIN_VALUE : o1.getCreatedDate(); - Long o2Date = o2.getCreatedDate() == null ? Long.MIN_VALUE : o2.getCreatedDate(); - return (- o1Date.compareTo(o2Date)); - }) + .sorted(CredentialModel.comparingByStartDateDesc()) .skip(passwordsToLeave) .forEach(p -> getCredentialStore().removeStoredCredential(realm, user, p.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 5e4a9c1b567..b4c141a2abd 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 @@ -561,6 +561,26 @@ public class AccountFormServiceTest extends AbstractTestRealmKeycloakTest { assertNumberOfStoredCredentials(1); } + @Test + public void changePasswordToOldOneAfterPasswordHistoryPolicyExpirationChange() { + userId = createUser("test", "user-changePasswordToOldOneAfterPasswordHistoryPolicyExpirationChange", "password"); + + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(3)"); + + changePasswordPage.open(); + loginPage.login("user-changePasswordToOldOneAfterPasswordHistoryPolicyExpirationChange", "password"); + events.expectLogin().user(userId).client("account").detail(Details.REDIRECT_URI, getAccountRedirectUrl() + "?path=password").assertEvent(); + + assertNumberOfStoredCredentials(1); + assertChangePasswordSucceeds("password", "password1"); + assertNumberOfStoredCredentials(2); + assertChangePasswordSucceeds("password1", "password2"); + assertNumberOfStoredCredentials(3); + + setPasswordPolicy(PasswordPolicy.PASSWORD_HISTORY_ID + "(2)"); + assertChangePasswordSucceeds("password2", "password"); + } + @Test public void changePasswordWithPasswordHistoryPolicyExpiration() { userId = createUser("test", "user-changePasswordWithPasswordHistoryPolicyExpiration", "password");