diff --git a/common/src/main/java/org/keycloak/common/util/Time.java b/common/src/main/java/org/keycloak/common/util/Time.java index e48f217a9c..561dd4aaf2 100644 --- a/common/src/main/java/org/keycloak/common/util/Time.java +++ b/common/src/main/java/org/keycloak/common/util/Time.java @@ -39,7 +39,7 @@ public class Time { * @return see description */ public static long currentTimeMillis() { - return System.currentTimeMillis() + (offset * 1000); + return System.currentTimeMillis() + (offset * 1000L); } /** @@ -48,7 +48,7 @@ public class Time { * @return see description */ public static Date toDate(int time) { - return new Date(((long) time ) * 1000); + return new Date(time * 1000L); } /** @@ -66,7 +66,7 @@ public class Time { * @return Time in milliseconds */ public static long toMillis(int time) { - return ((long) time) * 1000; + return time * 1000L; } /** diff --git a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java index 1a69e9a1ec..043c1e0f5c 100644 --- a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java +++ b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java @@ -32,7 +32,6 @@ import org.keycloak.models.cache.UserCache; import org.keycloak.policy.PasswordPolicyManagerProvider; import org.keycloak.policy.PolicyError; -import java.io.IOException; import java.util.Collections; import java.util.List; import java.util.Set; @@ -90,21 +89,46 @@ public class PasswordCredentialProvider implements CredentialProvider create new + createdCredential = getCredentialStore().createCredential(realm, user, credentialModel); + } else { // password exists --> update existing + credentialModel.setId(oldPassword.getId()); + getCredentialStore().updateCredential(realm, user, credentialModel); + createdCredential = credentialModel; + + // 2) add a password history item based on the old password + if (expiredPasswordsPolicyValue > 1) { + oldPassword.setId(null); + oldPassword.setType(PasswordCredentialModel.PASSWORD_HISTORY); + getCredentialStore().createCredential(realm, user, oldPassword); + } + } + + // 3) remove old password history items + List passwordHistoryList = getCredentialStore().getStoredCredentialsByType(realm, user, PasswordCredentialModel.PASSWORD_HISTORY); + final int passwordHistoryListMaxSize = Math.max(0, expiredPasswordsPolicyValue - 1); + if (passwordHistoryList.size() > passwordHistoryListMaxSize) { + passwordHistoryList.stream() + .sorted(CredentialModel.comparingByStartDateDesc()) + .skip(passwordHistoryListMaxSize) + .forEach(p -> getCredentialStore().removeStoredCredential(realm, user, p.getId())); + } + + UserCache userCache = session.userCache(); + if (userCache != null) { + userCache.evict(realm, user); + } + return createdCredential; } @Override @@ -118,31 +142,6 @@ public class PasswordCredentialProvider implements CredentialProvider list = getCredentialStore().getStoredCredentialsByType(realm, user, PasswordCredentialModel.PASSWORD_HISTORY); - if (expiredPasswordsPolicyValue > 1) { - // 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; - if (list.size() > passwordsToLeave) { - list.stream() - .sorted(CredentialModel.comparingByStartDateDesc()) - .skip(passwordsToLeave) - .forEach(p -> getCredentialStore().removeStoredCredential(realm, user, p.getId())); - } - oldPassword.setType(PasswordCredentialModel.PASSWORD_HISTORY); - getCredentialStore().updateCredential(realm, user, oldPassword); - } else { - list.stream().forEach(p -> getCredentialStore().removeStoredCredential(realm, user, p.getId())); - getCredentialStore().removeStoredCredential(realm, user, oldPassword.getId()); - } - - } - protected PasswordHashProvider getHashProvider(PasswordPolicy policy) { PasswordHashProvider hash = session.getProvider(PasswordHashProvider.class, policy.getHashAlgorithm()); if (hash == null) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/policy/PasswordHistoryPolicyTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/policy/PasswordHistoryPolicyTest.java new file mode 100644 index 0000000000..124ff980e5 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/policy/PasswordHistoryPolicyTest.java @@ -0,0 +1,226 @@ +/* + * Copyright 2016 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.keycloak.testsuite.policy; + +import java.util.function.Consumer; +import javax.ws.rs.BadRequestException; +import javax.ws.rs.core.Response; +import org.junit.After; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.keycloak.admin.client.resource.UserResource; +import org.keycloak.representations.idm.CredentialRepresentation; +import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.testsuite.AbstractAuthTest; + +import static org.keycloak.representations.idm.CredentialRepresentation.PASSWORD; +import static org.keycloak.testsuite.admin.ApiUtil.getCreatedId; + +/** + * @author Tomas Kyjovsky + */ +public class PasswordHistoryPolicyTest extends AbstractAuthTest { + + UserResource user; + + private void setPasswordHistory(String passwordHistory) { + log.info(String.format("Setting %s", passwordHistory)); + RealmRepresentation testRealmRepresentation = testRealmResource().toRepresentation(); + testRealmRepresentation.setPasswordPolicy(passwordHistory); + testRealmResource().update(testRealmRepresentation); + } + + private void setPasswordHistoryValue(String value) { + setPasswordHistory(String.format("passwordHistory(%s)", value)); + } + + private void setPasswordHistoryValue(int value) { + setPasswordHistoryValue(String.valueOf(value)); + } + + public UserRepresentation createUserRepresentation(String username) { + UserRepresentation userRepresentation = new UserRepresentation(); + userRepresentation.setUsername(username); + userRepresentation.setEmail(String.format("%s@email.test", userRepresentation.getUsername())); + userRepresentation.setEmailVerified(true); + return userRepresentation; + } + + public UserResource createUser(UserRepresentation user) { + String createdUserId; + try (Response response = testRealmResource().users().create(user)) { + createdUserId = getCreatedId(response); + } + return testRealmResource().users().get(createdUserId); + } + + public void resetUserPassword(UserResource userResource, String newPassword) { + CredentialRepresentation newCredential = new CredentialRepresentation(); + newCredential.setType(PASSWORD); + newCredential.setValue(newPassword); + newCredential.setTemporary(false); + userResource.resetPassword(newCredential); + } + + private void expectBadRequestException(Consumer f) { + try { + f.accept(null); + throw new AssertionError("An expected BadRequestException was not thrown."); + } catch (BadRequestException bre) { + log.info("An expected BadRequestException was caught."); + } + } + + @Before + public void before() { + user = createUser(createUserRepresentation("test_user")); + } + + @After + public void after() { + user.remove(); + } + + @Test + public void testPasswordHistory_noHistory() { + setPasswordHistory(""); + resetUserPassword(user, "secret"); // history: + resetUserPassword(user, "secret"); // history: + } + + @Test + public void testPasswordHistory_length1() { + setPasswordHistoryValue(1); + resetUserPassword(user, "secret"); // history: secret + expectBadRequestException(f -> { + resetUserPassword(user, "secret"); + }); + resetUserPassword(user, "secret_2"); // history: secret_2 + } + + @Test + public void testPasswordHistory_length2() { + setPasswordHistoryValue(2); + resetUserPassword(user, "secret"); // history: secret + resetUserPassword(user, "secret_2"); // history: secret_2, secret + expectBadRequestException(f -> { + resetUserPassword(user, "secret_2"); + }); + expectBadRequestException(f -> { + resetUserPassword(user, "secret"); + }); + resetUserPassword(user, "secret_3"); // history: secret_3, secret_2 + + expectBadRequestException(f -> { + resetUserPassword(user, "secret_3"); + }); + expectBadRequestException(f -> { + resetUserPassword(user, "secret_2"); + }); + resetUserPassword(user, "secret"); // history: secret, secret_3 + } + + @Test + public void testPasswordHistory_length3() { + setPasswordHistoryValue(3); + resetUserPassword(user, "secret"); // history: secret + resetUserPassword(user, "secret_2"); // history: secret_2, secret + resetUserPassword(user, "secret_3"); // history: secret_3, secret_2, secret + expectBadRequestException(f -> { + resetUserPassword(user, "secret_3"); + }); + expectBadRequestException(f -> { + resetUserPassword(user, "secret_2"); + }); + expectBadRequestException(f -> { + resetUserPassword(user, "secret"); + }); + resetUserPassword(user, "secret_4"); // history: secret_4, secret_3, secret_2 + + expectBadRequestException(f -> { + resetUserPassword(user, "secret_4"); + }); + expectBadRequestException(f -> { + resetUserPassword(user, "secret_3"); + }); + expectBadRequestException(f -> { + resetUserPassword(user, "secret_2"); + }); + resetUserPassword(user, "secret"); // history: secret, secret_4, secret_3 + } + + @Test + public void testPasswordHistory_lengthChange() { + testPasswordHistory_length3(); // history: secret, secret_4, secret_3 + + setPasswordHistoryValue(2); // history: secret, secret_4 + expectBadRequestException(f -> { + resetUserPassword(user, "secret"); + }); + expectBadRequestException(f -> { + resetUserPassword(user, "secret_4"); + }); + resetUserPassword(user, "secret_2"); // history: secret_2, secret + + setPasswordHistoryValue(1); // history: secret_2 + expectBadRequestException(f -> { + resetUserPassword(user, "secret_2"); + }); + resetUserPassword(user, "secret"); // history: secret + + setPasswordHistory(""); // history: + resetUserPassword(user, "secret"); // history: + } + + @Test + public void testInvalidPasswordHistoryPolicyValue_notANumber() { + expectBadRequestException(f -> { + setPasswordHistoryValue("abc"); + }); + expectBadRequestException(f -> { + setPasswordHistoryValue("2-"); + }); + expectBadRequestException(f -> { + setPasswordHistoryValue("-2-"); + }); + } + + @Test + @Ignore("KEYCLOAK-12673") + public void testInvalidPasswordHistoryPolicyValue_zero() { + expectBadRequestException(f -> { + setPasswordHistoryValue(0); + }); + } + + @Test + @Ignore("KEYCLOAK-12673") + public void testInvalidPasswordHistoryPolicyValue_negative() { + expectBadRequestException(f -> { + setPasswordHistoryValue(-1); + }); + expectBadRequestException(f -> { + setPasswordHistoryValue(-2); + }); + expectBadRequestException(f -> { + setPasswordHistoryValue(-10); + }); + } + +} diff --git a/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authentication/PasswordPolicyTest.java b/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authentication/PasswordPolicyTest.java index 7478467ac6..5431fa7167 100644 --- a/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authentication/PasswordPolicyTest.java +++ b/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authentication/PasswordPolicyTest.java @@ -166,7 +166,6 @@ public class PasswordPolicyTest extends AbstractConsoleTest { } @Test - @Ignore("Disabled until KEYCLOAK-11922 is resolved.") public void testPasswordHistoryPolicy() { RealmRepresentation realm = testRealmResource().toRepresentation(); realm.setPasswordPolicy("passwordHistory(2)");