KEYCLOAK-4095 Fix for expiring passwords

services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java

@@ -31,9 +31,7 @@ import org.keycloak.policy.PasswordPolicyManagerProvider;
 import org.keycloak.policy.PolicyError;
 
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.HashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 
@@ -57,7 +55,7 @@ public class PasswordCredentialProvider implements CredentialProvider, Credentia
     }
 
     public CredentialModel getPassword(RealmModel realm, UserModel user) {
-        List<CredentialModel> passwords = null;
+        List<CredentialModel> passwords;
         if (user instanceof CachedUserModel && !((CachedUserModel)user).isMarkedForEviction()) {
             CachedUserModel cached = (CachedUserModel)user;
             passwords = (List<CredentialModel>)cached.getCachedWith().get(PASSWORD_CACHE_KEY);
@@ -107,25 +105,20 @@ public class PasswordCredentialProvider implements CredentialProvider, Credentia
         CredentialModel oldPassword = getPassword(realm, user);
         if (oldPassword == null) return;
         int expiredPasswordsPolicyValue = policy.getExpiredPasswords();
-        if (expiredPasswordsPolicyValue > -1) {
+        if (expiredPasswordsPolicyValue > 1) {
             List<CredentialModel> list = getCredentialStore().getStoredCredentialsByType(realm, user, CredentialModel.PASSWORD_HISTORY);
-            List<CredentialModel> history = new LinkedList<>();
+            // oldPassword will expire few lines below, and there is one active password,
-            history.addAll(list);
+            // hence (expiredPasswordsPolicyValue - 2) passwords should be left in history
-            if (history.size() + 1 >= expiredPasswordsPolicyValue) {
+            final int passwordsToLeave = expiredPasswordsPolicyValue - 2;
-                Collections.sort(history, new Comparator<CredentialModel>() {
+            if (list.size() > passwordsToLeave) {
-                    @Override
+                list.stream()
-                    public int compare(CredentialModel o1, CredentialModel o2) {
+                  .sorted((o1, o2) -> { // sort by date descending
-                        long o1Date = o1.getCreatedDate() == null ? 0 : o1.getCreatedDate().longValue();
+                      Long o1Date = o1.getCreatedDate() == null ? Long.MIN_VALUE : o1.getCreatedDate();
-                        long o2Date = o2.getCreatedDate() == null ? 0 : o2.getCreatedDate().longValue();
+                      Long o2Date = o2.getCreatedDate() == null ? Long.MIN_VALUE : o2.getCreatedDate();
-                        if (o1Date > o2Date) return 1;
+                      return (- o1Date.compareTo(o2Date));
-                        else if (o1Date < o2Date) return -1;
+                  })
-                        else return 0;
+                  .skip(passwordsToLeave)
-                    }
+                  .forEach(p -> getCredentialStore().removeStoredCredential(realm, user, p.getId()));
-                });
-                for (int i = 0; i < history.size() + 2 - expiredPasswordsPolicyValue; i++) {
-                    getCredentialStore().removeStoredCredential(realm, user, history.get(i).getId());
-                }
-
             }
             oldPassword.setType(CredentialModel.PASSWORD_HISTORY);
             getCredentialStore().updateCredential(realm, user, oldPassword);

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountTest.java

@@ -22,9 +22,11 @@ import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+
 import org.keycloak.events.Details;
 import org.keycloak.events.Errors;
 import org.keycloak.events.EventType;
+import org.keycloak.models.PasswordPolicy;
 import org.keycloak.models.utils.TimeBasedOTP;
 import org.keycloak.representations.idm.EventRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
@@ -50,6 +52,7 @@ import org.keycloak.testsuite.util.IdentityProviderBuilder;
 import org.keycloak.testsuite.util.OAuthClient;
 import org.keycloak.testsuite.util.RealmBuilder;
 import org.keycloak.testsuite.util.UserBuilder;
+
 import org.openqa.selenium.By;
 import org.openqa.selenium.WebDriver;
 
@@ -363,33 +366,90 @@ public class AccountTest extends TestRealmKeycloakTest {
         events.expectAccount(EventType.UPDATE_PASSWORD).assertEvent();
     }
 
-    @Test
+     private void assertChangePasswordSucceeds(String currentPassword, String newPassword) {
-    public void changePasswordWithPasswordHistoryPolicy() {
+        changePasswordPage.changePassword(currentPassword, newPassword, newPassword);
-        setPasswordPolicy("passwordHistory(2)");
+        Assert.assertEquals("Your password has been updated.", profilePage.getSuccess());
+        events.expectAccount(EventType.UPDATE_PASSWORD).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();
+    }
+
+   @Test
+    public void changePasswordWithPasswordHistoryPolicyThreePasswords() {
+        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();
 
-        changePasswordPage.changePassword("password", "password", "password");
+        assertChangePasswordFails   ("password",  "password");  // current: password
-        Assert.assertEquals("Invalid password: must not be equal to any of last 2 passwords.", profilePage.getError());
+        assertChangePasswordSucceeds("password",  "password1"); // current: password
-        events.expectAccount(EventType.UPDATE_PASSWORD_ERROR).error(Errors.PASSWORD_REJECTED).assertEvent();
 
-        changePasswordPage.changePassword("password", "password1", "password1");
+        assertChangePasswordFails   ("password1", "password");  // current: password1, history: password
-        Assert.assertEquals("Your password has been updated.", profilePage.getSuccess());
+        assertChangePasswordFails   ("password1", "password1"); // current: password1, history: password
-        events.expectAccount(EventType.UPDATE_PASSWORD).assertEvent();
+        assertChangePasswordSucceeds("password1", "password2"); // current: password1, history: password
 
-        changePasswordPage.changePassword("password1", "password", "password");
+        assertChangePasswordFails   ("password2", "password");  // current: password2, history: password, password1
-        Assert.assertEquals("Invalid password: must not be equal to any of last 2 passwords.", profilePage.getError());
+        assertChangePasswordFails   ("password2", "password1"); // current: password2, history: password, password1
-        events.expectAccount(EventType.UPDATE_PASSWORD_ERROR).error(Errors.PASSWORD_REJECTED).assertEvent();
+        assertChangePasswordFails   ("password2", "password2"); // current: password2, history: password, password1
+        assertChangePasswordSucceeds("password2", "password3"); // current: password2, history: password, password1
 
-        changePasswordPage.changePassword("password1", "password1", "password1");
+        assertChangePasswordSucceeds("password3", "password");  // current: password3, history: password1, password2
-        Assert.assertEquals("Invalid password: must not be equal to any of last 2 passwords.", profilePage.getError());
+    }
-        events.expectAccount(EventType.UPDATE_PASSWORD_ERROR).error(Errors.PASSWORD_REJECTED).assertEvent();
 
-        changePasswordPage.changePassword("password1", "password2", "password2");
+    @Test
-        Assert.assertEquals("Your password has been updated.", profilePage.getSuccess());
+    public void changePasswordWithPasswordHistoryPolicyTwoPasswords() {
-        events.expectAccount(EventType.UPDATE_PASSWORD).assertEvent();
+        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();
+
+        assertChangePasswordFails   ("password",  "password");  // current: password
+        assertChangePasswordSucceeds("password",  "password1"); // current: password
+
+        assertChangePasswordFails   ("password1", "password");  // current: password1, history: password
+        assertChangePasswordFails   ("password1", "password1"); // current: password1, history: password
+        assertChangePasswordSucceeds("password1", "password2"); // current: password1, history: password
+
+        assertChangePasswordFails   ("password2", "password1"); // current: password2, history: password1
+        assertChangePasswordFails   ("password2", "password2"); // current: password2, history: password1
+        assertChangePasswordSucceeds("password2", "password");  // current: password2, history: password1
+    }
+
+    @Test
+    public void changePasswordWithPasswordHistoryPolicyOnePwds() {
+        // 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();
+
+        assertChangePasswordFails   ("password",  "password");  // current: password
+        assertChangePasswordSucceeds("password",  "password1"); // current: password
+
+        assertChangePasswordFails   ("password1", "password1"); // current: password1
+        assertChangePasswordSucceeds("password1", "password");  // current: password1
+    }
+
+    @Test
+    public void changePasswordWithPasswordHistoryPolicyZeroPwdsInHistory() {
+        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();
+
+        assertChangePasswordFails   ("password",  "password");  // current: password
+        assertChangePasswordSucceeds("password",  "password1"); // current: password
+
+        assertChangePasswordFails   ("password1", "password1"); // current: password1
+        assertChangePasswordSucceeds("password1", "password");  // current: password1
     }
 
     @Test