From d679c13040a838fd65aa5cc801232448d3a8f067 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Thu, 15 Feb 2024 10:31:50 +0100 Subject: [PATCH] Continue LDAP search if a duplicated user (ModelDuplicateException) is found Closes #25778 Signed-off-by: rmartinc --- .../storage/ldap/LDAPStorageProvider.java | 95 +++++++++++-------- .../LDAPSearchForUsersPaginationTest.java | 35 +++++++ ...PSearchForUsersPaginationNoImportTest.java | 35 +++++++ 3 files changed, 126 insertions(+), 39 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 fbb5da27d1..b257ee9ef4 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 @@ -62,6 +62,7 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserManager; import org.keycloak.models.UserModel; +import org.keycloak.models.UserProvider; import org.keycloak.models.cache.CachedUserModel; import org.keycloak.models.credential.PasswordCredentialModel; import org.keycloak.models.utils.ReadOnlyUserModelDelegate; @@ -648,37 +649,8 @@ public class LDAPStorageProvider implements UserStorageProvider, return importUserFromLDAP(session, realm, ldapUser, true); } - protected UserModel importUserFromLDAP(KeycloakSession session, RealmModel realm, LDAPObject ldapUser, boolean duplicates) { - String ldapUsername = LDAPUtils.getUsername(ldapUser, ldapIdentityStore.getConfig()); - LDAPUtils.checkUuid(ldapUser, ldapIdentityStore.getConfig()); - - UserModel imported; - if (model.isImportEnabled()) { - // Search if there is already an existing user, which means the username might have changed in LDAP without Keycloak knowing about it - UserModel existingLocalUser = UserStoragePrivateUtil.userLocalStorage(session) - .searchForUserByUserAttributeStream(realm, LDAPConstants.LDAP_ID, ldapUser.getUuid()).findFirst().orElse(null); - if(existingLocalUser != null){ - imported = existingLocalUser; - // Need to evict the existing user from cache - if (UserStorageUtil.userCache(session) != null) { - UserStorageUtil.userCache(session).evict(realm, existingLocalUser); - } - if (!duplicates) { - // if duplicates are not wanted return null - return null; - } - } else { - imported = UserStoragePrivateUtil.userLocalStorage(session).addUser(realm, ldapUsername); - } - - } else { - InMemoryUserAdapter adapter = new InMemoryUserAdapter(session, realm, new StorageId(model.getId(), ldapUsername).getId()); - adapter.addDefaults(); - imported = adapter; - } - imported.setEnabled(true); - - UserModel finalImported = imported; + private void doImportUser(final RealmModel realm, final UserModel user, final LDAPObject ldapUser) { + user.setEnabled(true); realm.getComponentsStream(model.getId(), LDAPStorageMapper.class.getName()) .sorted(ldapMappersComparator.sortDesc()) .forEachOrdered(mapperModel -> { @@ -686,15 +658,15 @@ public class LDAPStorageProvider implements UserStorageProvider, logger.tracef("Using mapper %s during import user from LDAP", mapperModel); } LDAPStorageMapper ldapMapper = mapperManager.getMapper(mapperModel); - ldapMapper.onImportUserFromLDAP(ldapUser, finalImported, realm, true); + ldapMapper.onImportUserFromLDAP(ldapUser, user, realm, true); }); String userDN = ldapUser.getDn().toString(); - if (model.isImportEnabled()) imported.setFederationLink(model.getId()); - imported.setSingleAttribute(LDAPConstants.LDAP_ID, ldapUser.getUuid()); - imported.setSingleAttribute(LDAPConstants.LDAP_ENTRY_DN, userDN); + if (model.isImportEnabled()) user.setFederationLink(model.getId()); + user.setSingleAttribute(LDAPConstants.LDAP_ID, ldapUser.getUuid()); + user.setSingleAttribute(LDAPConstants.LDAP_ENTRY_DN, userDN); if(getLdapIdentityStore().getConfig().isTrustEmail()){ - imported.setEmailVerified(true); + user.setEmailVerified(true); } if (kerberosConfig.isAllowKerberosAuthentication() && kerberosConfig.getKerberosPrincipalAttribute() != null) { String kerberosPrincipal = ldapUser.getAttributeAsString(kerberosConfig.getKerberosPrincipalAttribute()); @@ -702,11 +674,56 @@ public class LDAPStorageProvider implements UserStorageProvider, logger.warnf("Kerberos principal attribute not found on LDAP user [%s]. Configured kerberos principal attribute name is [%s]", ldapUser.getDn(), kerberosConfig.getKerberosPrincipalAttribute()); } else { KerberosPrincipal kerberosPrinc = new KerberosPrincipal(kerberosPrincipal); - imported.setSingleAttribute(KerberosConstants.KERBEROS_PRINCIPAL, kerberosPrinc.toString()); + user.setSingleAttribute(KerberosConstants.KERBEROS_PRINCIPAL, kerberosPrinc.toString()); } } - logger.debugf("Imported new user from LDAP to Keycloak DB. Username: [%s], Email: [%s], LDAP_ID: [%s], LDAP Entry DN: [%s]", imported.getUsername(), imported.getEmail(), - ldapUser.getUuid(), userDN); + logger.debugf("Imported new user from LDAP to Keycloak DB. Username: [%s], Email: [%s], LDAP_ID: [%s], LDAP Entry DN: [%s]", + user.getUsername(), user.getEmail(), ldapUser.getUuid(), userDN); + } + + protected UserModel importUserFromLDAP(KeycloakSession session, RealmModel realm, LDAPObject ldapUser, boolean forcedImport) { + String ldapUsername = LDAPUtils.getUsername(ldapUser, ldapIdentityStore.getConfig()); + LDAPUtils.checkUuid(ldapUser, ldapIdentityStore.getConfig()); + + UserModel imported = null; + UserModel existingLocalUser = null; + final UserProvider userProvider = UserStoragePrivateUtil.userLocalStorage(session); + try { + if (model.isImportEnabled()) { + // Search if there is already an existing user, which means the username might have changed in LDAP without Keycloak knowing about it + existingLocalUser = userProvider.searchForUserByUserAttributeStream(realm, LDAPConstants.LDAP_ID, ldapUser.getUuid()) + .findFirst().orElse(null); + if (existingLocalUser != null) { + imported = existingLocalUser; + // Need to evict the existing user from cache + if (UserStorageUtil.userCache(session) != null) { + UserStorageUtil.userCache(session).evict(realm, existingLocalUser); + } + if (!forcedImport) { + // if import is not forced return null as it was already imported + return null; + } + } else { + imported = userProvider.addUser(realm, ldapUsername); + } + } else { + InMemoryUserAdapter adapter = new InMemoryUserAdapter(session, realm, new StorageId(model.getId(), ldapUsername).getId()); + adapter.addDefaults(); + imported = adapter; + } + doImportUser(realm, imported, ldapUser); + } catch (ModelDuplicateException e) { + logger.warnf(e, "Duplicated user importing from LDAP. LDAP Entry DN: [%s], LDAP_ID: [%s]", ldapUser.getDn(), ldapUser.getUuid()); + if (!forcedImport && existingLocalUser == null) { + // try to continue if import was not forced, delete created db user if necessary + if (model.isImportEnabled() && imported != null) { + userProvider.removeUser(realm, imported); + } + return null; + } + throw e; + } + UserModel proxy = proxy(realm, imported, ldapUser, false); return proxy; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java index 5851b9c3f5..d3c483d369 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java @@ -36,10 +36,13 @@ import org.junit.FixMethodOrder; import org.junit.Test; import org.junit.runners.MethodSorters; import org.keycloak.models.LDAPConstants; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.util.LDAPRule; import org.keycloak.testsuite.util.LDAPTestUtils; +import org.keycloak.testsuite.util.UserBuilder; @FixMethodOrder(MethodSorters.NAME_ASCENDING) public class LDAPSearchForUsersPaginationTest extends AbstractLDAPTest { @@ -180,6 +183,38 @@ public class LDAPSearchForUsersPaginationTest extends AbstractLDAPTest { Assert.assertEquals(Set.of("john"), usernames); } + public void testDuplicateEmailInDatabase() { + setLDAPEnabled(false); + try { + // create a local db user with the same email than an a ldap user + String userId = ApiUtil.getCreatedId(testRealm().users().create(UserBuilder.create() + .username("jdoe").firstName("John").lastName("Doe") + .email("john14@email.org") + .build())); + Assert.assertNotNull("User not created", userId); + getCleanup().addUserId(userId); + } finally { + setLDAPEnabled(true); + } + + List search = adminClient.realm(TEST_REALM_NAME).users() + .search("john14@email.org", null, null) + .stream().collect(Collectors.toList()); + Assert.assertEquals("Incorrect users found", 1, search.size()); + Assert.assertEquals("Incorrect User", "jdoe", search.get(0).getUsername()); + Assert.assertTrue("Duplicated user created", adminClient.realm(TEST_REALM_NAME).users().search("john", true).isEmpty()); + } + + private void setLDAPEnabled(final boolean enabled) { + testingClient.server().run((KeycloakSession session) -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + ctx.getLdapModel().getConfig().putSingle("enabled", Boolean.toString(enabled)); + appRealm.updateComponent(ctx.getLdapModel()); + }); + } + private void assertLDAPSearchMatchesLocalDB(String searchString) { //this call should import some users into local database List importedUsers = adminClient.realm(TEST_REALM_NAME).users().search(searchString, null, null).stream().map(UserRepresentation::getUsername).collect(Collectors.toList()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java index 5052d641d2..d907be220e 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java @@ -28,12 +28,15 @@ import org.junit.FixMethodOrder; import org.junit.Test; import org.junit.runners.MethodSorters; import org.keycloak.models.LDAPConstants; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.federation.ldap.AbstractLDAPTest; import org.keycloak.testsuite.federation.ldap.LDAPTestContext; import org.keycloak.testsuite.util.LDAPRule; import org.keycloak.testsuite.util.LDAPTestUtils; +import org.keycloak.testsuite.util.UserBuilder; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasSize; @@ -192,4 +195,36 @@ public class LDAPSearchForUsersPaginationNoImportTest extends AbstractLDAPTest { .collect(Collectors.toSet()); Assert.assertEquals(Set.of("john"), usernames); } + + public void testDuplicateEmailInDatabase() { + setLDAPEnabled(false); + try { + // create a local db user with the same email than an a ldap user + String userId = ApiUtil.getCreatedId(testRealm().users().create(UserBuilder.create() + .username("jdoe").firstName("John").lastName("Doe") + .email("john14@email.org") + .build())); + Assert.assertNotNull("User not created", userId); + getCleanup().addUserId(userId); + } finally { + setLDAPEnabled(true); + } + + List search = adminClient.realm(TEST_REALM_NAME).users() + .search("john14@email.org", null, null) + .stream().collect(Collectors.toList()); + Assert.assertEquals("User not found", 1, search.size()); + Assert.assertEquals("Incorrect User", "jdoe", search.get(0).getUsername()); + Assert.assertTrue("Duplicated user created", adminClient.realm(TEST_REALM_NAME).users().search("john", true).isEmpty()); + } + + private void setLDAPEnabled(final boolean enabled) { + testingClient.server().run((KeycloakSession session) -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + ctx.getLdapModel().getConfig().putSingle("enabled", Boolean.toString(enabled)); + appRealm.updateComponent(ctx.getLdapModel()); + }); + } }