From 4aa70f4623d3fb03fa3e618ee9de924f6a5440f4 Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Wed, 4 Jun 2025 09:01:02 +0200 Subject: [PATCH] Sequential transactions instead of nested transactions Closes #40171 Signed-off-by: Alexander Schwartz --- .../PasswordCredentialProvider.java | 29 ++++++++---- .../testsuite/forms/PasswordHashingTest.java | 45 +++++++++++++++++++ 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java index 1ca522ae18dc..fe31a83fdd5d 100644 --- a/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java +++ b/services/src/main/java/org/keycloak/credential/PasswordCredentialProvider.java @@ -22,6 +22,7 @@ import org.jboss.logging.Logger; import org.keycloak.common.util.Time; import org.keycloak.credential.hash.PasswordHashProvider; +import org.keycloak.models.AbstractKeycloakTransaction; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelException; import org.keycloak.models.PasswordPolicy; @@ -282,14 +283,26 @@ private void rehashPasswordIfRequired(KeycloakSession session, RealmModel realm, if (!provider.policyCheck(passwordPolicy, password)) { final int iterations = passwordPolicy != null ? passwordPolicy.getHashIterations() : -1; final String hashAlgorithm = passwordPolicy != null ? passwordPolicy.getHashAlgorithm() : null; - try { - // refresh the password in a different transaction, do not fail if there is a model exception - KeycloakModelUtils.runJobInTransaction(session.getKeycloakSessionFactory(), session.getContext(), - (KeycloakSession s) -> refreshPassword(s, hashAlgorithm, iterations, input.getChallengeResponse(), - password.getId(), password.getCreatedDate(), password.getUserLabel(), user.getId())); - } catch (ModelException e) { - logger.info("Error re-hashing the password in a different transaction", e); - } + // Refresh the password in a different transaction, do not fail if there is a model exception on current modifications due to concurrent logins. + // Also do not start it as a nested transaction, as the current transaction might have auto-migrated the credential. + // see: JpaUserCredentialStore#toModel for the on-the-fly migration of the salt column + session.getTransactionManager().enlistAfterCompletion(new AbstractKeycloakTransaction() { + @Override + protected void commitImpl() { + try { + KeycloakModelUtils.runJobInTransaction(session.getKeycloakSessionFactory(), session.getContext(), + (KeycloakSession s) -> refreshPassword(s, hashAlgorithm, iterations, input.getChallengeResponse(), + password.getId(), password.getCreatedDate(), password.getUserLabel(), user.getId())); + } catch (ModelException e) { + logger.info("Error re-hashing the password in a different transaction", e); + } + } + + @Override + protected void rollbackImpl() { + + } + }); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java index 362982fca4e7..24573482b42a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/PasswordHashingTest.java @@ -16,6 +16,7 @@ */ package org.keycloak.testsuite.forms; +import jakarta.persistence.EntityManager; import jakarta.ws.rs.BadRequestException; import org.bouncycastle.crypto.generators.Argon2BytesGenerator; import org.jboss.arquillian.graphene.page.Page; @@ -24,6 +25,7 @@ import org.junit.Test; import org.keycloak.common.crypto.FipsMode; import org.keycloak.common.util.Base64; +import org.keycloak.connections.jpa.JpaConnectionProvider; import org.keycloak.credential.CredentialModel; import org.keycloak.credential.hash.PasswordHashProvider; import org.keycloak.credential.hash.PasswordHashProviderFactory; @@ -35,8 +37,10 @@ import org.keycloak.crypto.hash.Argon2PasswordHashProviderFactory; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; +import org.keycloak.models.cache.UserCache; import org.keycloak.models.credential.PasswordCredentialModel; import org.keycloak.models.credential.dto.PasswordCredentialData; +import org.keycloak.models.jpa.entities.CredentialEntity; import org.keycloak.representations.idm.CredentialRepresentation; import org.keycloak.representations.idm.ErrorRepresentation; import org.keycloak.representations.idm.RealmRepresentation; @@ -52,6 +56,7 @@ import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; +import java.nio.charset.StandardCharsets; import java.security.spec.KeySpec; import java.time.Duration; import java.util.List; @@ -109,6 +114,42 @@ public void testPasswordRehashedOnAlgorithmChanged() throws Exception { loginPage.open(); loginPage.login(username, password); + appPage.assertCurrent(); + + credential = PasswordCredentialModel.createFromCredentialModel(fetchCredentials(username)); + + assertEquals(Pbkdf2PasswordHashProviderFactory.ID, credential.getPasswordCredentialData().getAlgorithm()); + assertEncoded(credential, password, credential.getPasswordSecretData().getSalt(), "PBKDF2WithHmacSHA1", 1); + } + + @Test + public void testPasswordRehashedOnAlgorithmChangedWithMigratedSalt() throws Exception { + setPasswordPolicy("hashAlgorithm(" + Pbkdf2Sha256PasswordHashProviderFactory.ID + ") and hashIterations(1)"); + + String username = "testPasswordRehashedOnAlgorithmChangedWithMigratedSalt"; + final String password = createUser(username); + + PasswordCredentialModel credential = PasswordCredentialModel.createFromCredentialModel(fetchCredentials(username)); + + assertEquals(Pbkdf2Sha256PasswordHashProviderFactory.ID, credential.getPasswordCredentialData().getAlgorithm()); + + assertEncoded(credential, password, credential.getPasswordSecretData().getSalt(), "PBKDF2WithHmacSHA256", 1); + + setPasswordPolicy("hashAlgorithm(" + Pbkdf2PasswordHashProviderFactory.ID + ") and hashIterations(1)"); + + String credentialId = credential.getId(); + testingClient.server().run(session -> { + EntityManager em = session.getProvider(JpaConnectionProvider.class).getEntityManager(); + CredentialEntity credentialEntity = em.find(CredentialEntity.class, credentialId); + // adding a dummy value to the salt column to trigger migration in JpaUserCredentialStore#toModel on next fetch of the credential + credentialEntity.setSalt("dummy".getBytes(StandardCharsets.UTF_8)); + // Clearing the user cache as we updated the database directly + session.getProvider(UserCache.class).clear(); + }); + + loginPage.open(); + loginPage.login(username, password); + appPage.assertCurrent(); credential = PasswordCredentialModel.createFromCredentialModel(fetchCredentials(username)); @@ -131,6 +172,7 @@ public void testPasswordRehashedToDefaultProviderIfHashAlgorithmRemoved() { loginPage.open(); loginPage.login(username, password); + appPage.assertCurrent(); credential = PasswordCredentialModel.createFromCredentialModel(fetchCredentials(username)); @@ -152,6 +194,7 @@ public void testPasswordRehashedOnIterationsChanged() throws Exception { loginPage.open(); loginPage.login(username, password); + appPage.assertCurrent(); credential = PasswordCredentialModel.createFromCredentialModel(fetchCredentials(username)); @@ -180,6 +223,7 @@ public void testPasswordNotRehasedUnchangedIterations() { loginPage.open(); loginPage.login(username, password); + appPage.assertCurrent(); credential = PasswordCredentialModel.createFromCredentialModel(fetchCredentials(username)); @@ -192,6 +236,7 @@ public void testPasswordNotRehasedUnchangedIterations() { loginPage.open(); loginPage.login(username, password); + appPage.assertCurrent(); credential = PasswordCredentialModel.createFromCredentialModel(fetchCredentials(username));