From 9412e339a8f74c8032875046fe2d5e098e995310 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Tue, 10 Jun 2025 11:28:55 -0300 Subject: [PATCH] Password modification time attribute as an operational and read-only attribute Closes #40270 Signed-off-by: Pedro Igor --- .../storage/ldap/LDAPStorageProvider.java | 10 +++------- .../org/keycloak/storage/ldap/LDAPUtils.java | 6 ++++++ .../ldap/idm/store/ldap/LDAPIdentityStore.java | 11 +++++++++++ .../MSADUserAccountControlStorageMapper.java | 3 +++ .../ldap/mappers/msad/UserAccountControl.java | 6 +++++- .../keycloak/testsuite/util/LDAPTestUtils.java | 2 +- .../org/keycloak/testsuite/util/LDAPRule.java | 4 ++++ .../federation/ldap/LDAPAccountRestApiTest.java | 17 +++++++++++++---- .../ldap/LDAPBinaryAttributesTest.java | 2 +- 9 files changed, 47 insertions(+), 14 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java index b0c9ad6da92..52a44f00b32 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java @@ -1228,20 +1228,16 @@ public class LDAPStorageProvider implements UserStorageProvider, } private long getPasswordChangedTime(LDAPObject ldapObject) { - String value = ldapObject.getAttributeAsString(getAttributeName()); + String attributeName = getLdapIdentityStore().getPasswordModificationTimeAttributeName(); + String value = ldapObject.getAttributeAsString(attributeName); if (StringUtil.isBlank(value)) { return -1L; } - if (LDAPConstants.PWD_LAST_SET.equals(getAttributeName())) { + if (LDAPConstants.PWD_LAST_SET.equals(attributeName)) { return (Long.parseLong(value) / 10000L) - 11644473600000L; } return LDAPUtils.generalizedTimeToDate(value).getTime(); } - - private String getAttributeName() { - LDAPConfig config = getLdapIdentityStore().getConfig(); - return config.isActiveDirectory() ? LDAPConstants.PWD_LAST_SET : LDAPConstants.PWD_CHANGED_TIME; - } } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPUtils.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPUtils.java index ce61137f2e2..63d7e70abda 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPUtils.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPUtils.java @@ -113,6 +113,12 @@ public class LDAPUtils { .collect(Collectors.toSet()); mandatoryAttrs.add(ldapConfig.getRdnLdapAttribute()); + String passwordModifiedTimeAttributeName = ldapStore.getPasswordModificationTimeAttributeName(); + String passwordModifiedTime = user.getFirstAttribute(passwordModifiedTimeAttributeName); + if (passwordModifiedTime != null) { + ldapUser.setSingleAttribute(passwordModifiedTimeAttributeName, passwordModifiedTime); + } + ldapUser.executeOnMandatoryAttributesComplete(mandatoryAttrs, ldapObject -> { LDAPUtils.computeAndSetDn(ldapConfig, ldapObject); ldapStore.add(ldapObject); diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java index d0016d9f438..386deda4fb3 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java @@ -524,6 +524,13 @@ public class LDAPIdentityStore implements IdentityStore { .map(String::toLowerCase) .collect(Collectors.toSet()); + if (!isCreate) { + // for updates, assume the PWD_CHANGED_TIME attribute is an operational attribute and read-only + // otherwise, updates will fail when trying to modify the attribute + // vendors like AD, support the same type of attribute differently and using a mapper + ldapObject.addReadOnlyAttributeName(LDAPConstants.PWD_CHANGED_TIME); + } + for (Map.Entry> attrEntry : ldapObject.getAttributes().entrySet()) { String attrName = attrEntry.getKey(); Set attrValue = attrEntry.getValue(); @@ -602,4 +609,8 @@ public class LDAPIdentityStore implements IdentityStore { return attr; } + public String getPasswordModificationTimeAttributeName() { + return getConfig().isActiveDirectory() ? LDAPConstants.PWD_LAST_SET : LDAPConstants.PWD_CHANGED_TIME; + } + } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java index 15973c14cb1..99e050a0625 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java @@ -65,6 +65,9 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp public static final Set PASSWORD_UPDATE_MSAD_ERROR_CODES = Set.of("52D"); private final Function GET_USER_ACCOUNT_CONTROL = ldapUser -> { + if (ldapUser == null) { + return UserAccountControl.empty(); + } String userAccountControl = ldapUser.getAttributeAsString(LDAPConstants.USER_ACCOUNT_CONTROL); return UserAccountControl.of(userAccountControl); }; diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/UserAccountControl.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/UserAccountControl.java index 20263e28feb..4cf2674f103 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/UserAccountControl.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/UserAccountControl.java @@ -49,9 +49,13 @@ public class UserAccountControl { private static final UserAccountControl EMPTY = new UserAccountControl(0); + public static UserAccountControl empty() { + return EMPTY; + } + public static UserAccountControl of(String userAccountControl) { if (userAccountControl == null) { - return EMPTY; + return empty(); } return new UserAccountControl(Long.parseLong(userAccountControl)); } diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java index e4460ab8423..76960895360 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java @@ -138,7 +138,7 @@ public class LDAPTestUtils { } else if (otherAttrs.containsKey(name)) { return otherAttrs.getFirst(name); } - return super.getFirstAttribute(name); + return null; } @Override diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/LDAPRule.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/LDAPRule.java index 2f50b41b462..41116dfdeb7 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/LDAPRule.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/LDAPRule.java @@ -307,4 +307,8 @@ public class LDAPRule extends ExternalResource { STARTTLS } } + + public boolean isEmbeddedServer() { + return ldapTestConfiguration.isStartEmbeddedLdapServer(); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java index 27ba791f8a6..b772a2c516e 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java @@ -34,6 +34,7 @@ import org.junit.Test; import org.junit.runners.MethodSorters; import org.keycloak.admin.client.resource.UserProfileResource; import org.keycloak.broker.provider.util.SimpleHttp; +import org.keycloak.common.util.MultivaluedHashMap; import org.keycloak.federation.kerberos.KerberosFederationProvider; import org.keycloak.models.LDAPConstants; import org.keycloak.models.RealmModel; @@ -90,6 +91,7 @@ public class LDAPAccountRestApiTest extends AbstractLDAPTest { @Override protected void afterImportTestRealm() { + boolean isEmbeddedServer = ldapRule.isEmbeddedServer(); testingClient.server().run(session -> { LDAPTestContext ctx = LDAPTestContext.init(session); RealmModel appRealm = ctx.getRealm(); @@ -97,10 +99,17 @@ public class LDAPAccountRestApiTest extends AbstractLDAPTest { // Delete all LDAP users and add some new for testing LDAPTestUtils.removeAllLDAPUsers(ctx.getLdapProvider(), appRealm); - LDAPObject john = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "johnkeycloak", "John", "Doe", "john@email.org", null, "1234"); - LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), john, "Password1"); - john.setSingleAttribute(LDAPConstants.PWD_CHANGED_TIME, "22000101000000Z"); - ctx.getLdapProvider().getLdapIdentityStore().update(john); + if (isEmbeddedServer) { + MultivaluedHashMap otherAttrs = new MultivaluedHashMap<>(); + + otherAttrs.putSingle(LDAPConstants.PWD_CHANGED_TIME, "22000101000000Z"); + + LDAPObject john = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "johnkeycloak", "John", "Doe", "john@email.org", otherAttrs); + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), john, "Password1"); + } else { + LDAPObject john = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "johnkeycloak", "John", "Doe", "john@email.org", null, "1234"); + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), john, "Password1"); + } }); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPBinaryAttributesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPBinaryAttributesTest.java index fa7816cbf39..3535ee725f4 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPBinaryAttributesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPBinaryAttributesTest.java @@ -246,7 +246,7 @@ public class LDAPBinaryAttributesTest extends AbstractLDAPTest { } else if (UserModel.USERNAME.equals(name)) { return username; } - return super.getFirstAttribute(name); + return null; } @Override