From 82fc4012983ea3f6dc2a24f157ab7ced57150465 Mon Sep 17 00:00:00 2001 From: Pascal Euhus Date: Mon, 1 Mar 2021 08:59:27 +0100 Subject: [PATCH] [KEYCLOAK-9841] use LDAPUser UUID as an identifier instead of username --- .../storage/ldap/LDAPStorageProvider.java | 33 +++++++++- .../ldap/LDAPStorageProviderFactory.java | 13 ++-- .../ldap/LDAPProvidersIntegrationTest.java | 54 ++++++++++++++++ .../federation/ldap/LDAPSyncTest.java | 61 +++++++++++++++++-- .../LDAPProvidersIntegrationNoImportTest.java | 6 ++ 5 files changed, 153 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 89804f9554..925206c321 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 @@ -486,8 +486,11 @@ public class LDAPStorageProvider implements UserStorageProvider, return existing; } - LDAPObject ldapUser = loadLDAPUserByUsername(realm, local.getUsername()); - if (ldapUser == null) { + String uuidLdapAttribute = local.getFirstAttribute(LDAPConstants.LDAP_ID); + + LDAPObject ldapUser = loadLDAPUserByUuid(realm, uuidLdapAttribute); + + if(ldapUser == null){ return null; } LDAPUtils.checkUuid(ldapUser, ldapIdentityStore.getConfig()); @@ -516,7 +519,17 @@ public class LDAPStorageProvider implements UserStorageProvider, UserModel imported = null; if (model.isImportEnabled()) { - imported = session.userLocalStorage().addUser(realm, ldapUsername); + // Search if there is already an existing user, which means the username might have changed in LDAP without Keycloak knowing about it + UserModel existingLocalUser = session.userLocalStorage() + .searchForUserByUserAttributeStream(realm, LDAPConstants.LDAP_ID, ldapUser.getUuid()).findFirst().orElse(null); + if(existingLocalUser != null){ + imported = existingLocalUser; + // Need to evict the existing user from cache + session.userCache().evict(realm, existingLocalUser); + } else { + imported = session.userLocalStorage().addUser(realm, ldapUsername); + } + } else { InMemoryUserAdapter adapter = new InMemoryUserAdapter(session, realm, new StorageId(model.getId(), ldapUsername).getId()); adapter.addDefaults(); @@ -803,5 +816,19 @@ public class LDAPStorageProvider implements UserStorageProvider, } } + public LDAPObject loadLDAPUserByUuid(RealmModel realm, String uuid) { + if(uuid == null){ + return null; + } + try (LDAPQuery ldapQuery = LDAPUtils.createQueryForUserSearch(this, realm)) { + LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); + + String uuidLDAPAttributeName = this.ldapIdentityStore.getConfig().getUuidLDAPAttributeName(); + Condition usernameCondition = conditionsBuilder.equal(uuidLDAPAttributeName, uuid, EscapeStrategy.DEFAULT); + ldapQuery.addWhereCondition(usernameCondition); + + return ldapQuery.getFirstResult(); + } + } } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java index 76ac3ee750..07facdb3d3 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java @@ -64,6 +64,7 @@ import org.keycloak.utils.CredentialHelper; import java.util.Date; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.function.Function; import java.util.stream.Collectors; @@ -593,16 +594,18 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory userModelOptional = session.userLocalStorage() + .searchForUserByUserAttributeStream(currentRealm, LDAPConstants.LDAP_ID, ldapUser.getUuid()) + .findFirst(); + if (!userModelOptional.isPresent() && currentUserLocal == null) { // Add new user to Keycloak exists.value = false; ldapFedProvider.importUserFromLDAP(session, currentRealm, ldapUser); syncResult.increaseAdded(); } else { + UserModel currentUser = userModelOptional.isPresent() ? userModelOptional.get() : currentUserLocal; if ((fedModel.getId().equals(currentUser.getFederationLink())) && (ldapUser.getUuid().equals(currentUser.getFirstAttribute(LDAPConstants.LDAP_ID)))) { // Update keycloak user @@ -621,7 +624,7 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory { + LDAPTestContext ctx = LDAPTestContext.init(session); + // Add user to LDAP + LDAPObject becky = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), ctx.getRealm(), "beckybecks", "Becky", "Becks", "becky-becks@email.org", null, "123"); + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), becky, "Password1"); + }); + + loginSuccessAndLogout("beckybecks", "Password1"); + + String origKeycloakUserId = testingClient.server().fetchString(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel testRealm = ctx.getRealm(); + + UserModel importedUser = session.userLocalStorage().getUserByUsername(testRealm, "beckybecks"); + + // Update user 'beckybecks' in LDAP + LDAPObject becky = ctx.getLdapProvider().loadLDAPUserByUsername(testRealm, importedUser.getUsername()); + // NOTE: Changing LDAP Username directly here + String userNameLdapAttributeName = ctx.getLdapProvider().getLdapIdentityStore().getConfig().getUsernameLdapAttribute(); + becky.setSingleAttribute(userNameLdapAttributeName, "beckyupdated"); + becky.setSingleAttribute(LDAPConstants.EMAIL, "becky-updated@email.org"); + ctx.getLdapProvider().getLdapIdentityStore().update(becky); + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), becky, "MyChangedPassword11"); + return importedUser.getId(); + }); + + loginSuccessAndLogout("beckyupdated", "MyChangedPassword11"); + + loginPage.open(); + loginPage.login("beckybecks", "Password1"); + Assert.assertEquals("Invalid username or password.", loginPage.getInputError()); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + + // The original username is not possible to use as username was changed in LDAP. + // However the call to LDAPStorageProvider.loadAndValidateUser shouldn't delete the user just because his username changed in LDAP + UserModel user = session.users().getUserByUsername(ctx.getRealm(), "beckybecks"); + Assert.assertNull(user); + + // Assert user can be found with new username from LDAP. And it is same user as before + user = session.users().getUserByUsername(ctx.getRealm(), "beckyupdated"); + Assert.assertNotNull(user); + String newKeycloakUserId = user.getId(); + // Need to remove double quotes from server response + Assert.assertEquals(origKeycloakUserId.replace("\"",""), newKeycloakUserId); + }); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSyncTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSyncTest.java index aa0936db65..fc572d5881 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSyncTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSyncTest.java @@ -240,10 +240,60 @@ public class LDAPSyncTest extends AbstractLDAPTest { }); } + @Test + public void test03LDAPSyncWhenUsernameChanged() { + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory(); + + // Add user to LDAP + LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), ctx.getRealm(), "beckybecks", "Becky", "Becks", "becky-becks@email.org", null, "123"); + SynchronizationResult syncResult = new UserStorageSyncManager().syncAllUsers(sessionFactory, "test", ctx.getLdapModel()); + Assert.assertEquals(0, syncResult.getFailed()); + }); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel testRealm = ctx.getRealm(); + UserStorageSyncManager usersSyncManager = new UserStorageSyncManager(); + + // Update user 'beckybecks' in LDAP + LDAPObject ldapUser = ctx.getLdapProvider().loadLDAPUserByUsername(testRealm, "beckybecks"); + // NOTE: Changing LDAP Username directly here + String userNameLdapAttributeName = ctx.getLdapProvider().getLdapIdentityStore().getConfig().getUsernameLdapAttribute(); + ldapUser.setSingleAttribute(userNameLdapAttributeName, "beckyupdated"); + ldapUser.setSingleAttribute(LDAPConstants.EMAIL, "becky-updated@email.org"); + ctx.getLdapProvider().getLdapIdentityStore().update(ldapUser); + + // Assert still old users in local provider + LDAPTestAsserts.assertUserImported(session.userLocalStorage(), testRealm, "beckybecks", "Becky", "Becks", "becky-becks@email.org", "123"); + + // Trigger partial sync + KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory(); + SynchronizationResult syncResult = usersSyncManager.syncChangedUsers(sessionFactory, "test", ctx.getLdapModel()); + Assert.assertEquals(0, syncResult.getFailed()); + }); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel testRealm = session.realms().getRealm("test"); + UserProvider userProvider = session.userLocalStorage(); + // Assert users updated in local provider + LDAPTestAsserts.assertUserImported(session.users(), testRealm, "beckyupdated", "Becky", "Becks", "becky-updated@email.org", "123"); + UserModel updatedLocalUser = userProvider.getUserByUsername(testRealm, "beckyupdated"); + LDAPObject ldapUser = ctx.getLdapProvider().loadLDAPUserByUsername(testRealm, "beckyupdated"); + // Assert old user 'beckybecks' does not exists locally + Assert.assertNull(userProvider.getUserByUsername(testRealm, "beckybecks")); + // Assert UUID didn't change + Assert.assertEquals(updatedLocalUser.getAttributeStream(LDAPConstants.LDAP_ID).findFirst().get(),ldapUser.getUuid()); + }); + } + // KEYCLOAK-1571 @Test - public void test03SameUUIDAndUsernameSync() { + public void test04SameUUIDAndUsernameSync() { String origUuidAttrName = testingClient.server().fetch(session -> { LDAPTestContext ctx = LDAPTestContext.init(session); @@ -290,10 +340,9 @@ public class LDAPSyncTest extends AbstractLDAPTest { testRealm().components().component(ldapModelId).update(ldapRep); } - // KEYCLOAK-1728 @Test - public void test04MissingLDAPUsernameSync() { + public void test05MissingLDAPUsernameSync() { String origUsernameAttrName = testingClient.server().fetch(session -> { LDAPTestContext ctx = LDAPTestContext.init(session); @@ -355,7 +404,7 @@ public class LDAPSyncTest extends AbstractLDAPTest { // KEYCLOAK-10770 user-storage/{id}/sync should return 400 instead of 404 @Test - public void test05SyncRestAPIMissingAction() { + public void test06SyncRestAPIMissingAction() { ComponentRepresentation ldapRep = testRealm().components().component(ldapModelId).toRepresentation(); try { @@ -368,7 +417,7 @@ public class LDAPSyncTest extends AbstractLDAPTest { // KEYCLOAK-10770 user-storage/{id}/sync should return 400 instead of 404 @Test - public void test06SyncRestAPIWrongAction() { + public void test07SyncRestAPIWrongAction() { ComponentRepresentation ldapRep = testRealm().components().component(ldapModelId).toRepresentation(); try { @@ -380,7 +429,7 @@ public class LDAPSyncTest extends AbstractLDAPTest { } @Test - public void test07LDAPGroupSyncAfterGroupRename() { + public void test08LDAPGroupSyncAfterGroupRename() { testingClient.server().run(session -> { LDAPTestContext ctx = LDAPTestContext.init(session); RealmModel appRealm = ctx.getRealm(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java index fb2e21ff1c..aacf4acc06 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java @@ -334,4 +334,10 @@ public class LDAPProvidersIntegrationNoImportTest extends LDAPProvidersIntegrati } } + // No need to test this in no-import mode. There won't be any users in localStorage. + @Test + @Ignore + @Override + public void updateLDAPUsernameTest() { + } }