diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java index 507183279c..282cd06012 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java @@ -45,6 +45,8 @@ import org.keycloak.storage.ldap.mappers.membership.MembershipType; import org.keycloak.storage.ldap.mappers.membership.UserRolesRetrieveStrategy; import org.keycloak.storage.user.SynchronizationResult; +import java.util.Arrays; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -213,21 +215,68 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements // due to the realm cache being bloated with huge amount of (temporary) realm entities RealmModel currentRealm = session.realms().getRealm(realm.getId()); + // List of top-level groups known to the whole transaction + ArrayList transactionTopLevelGroups = new ArrayList(currentRealm.getTopLevelGroups()); + String[] transactionBinarySearchTopLevelGroupsArray = transactionTopLevelGroups.parallelStream().map(g -> g.getName()).toArray(String[]::new); + for (Map.Entry groupEntry : groupsInTransaction.entrySet()) { String groupName = groupEntry.getKey(); - GroupModel kcExistingGroup = KeycloakModelUtils.findGroupByPath(currentRealm, "/" + groupName); + + // Binary search the list of top-level groups known to the outer transaction for presence of the currently processed group + int transactionBinarySearchResult = Arrays.binarySearch(transactionBinarySearchTopLevelGroupsArray, groupName); + GroupModel kcExistingGroup = (transactionBinarySearchResult > 0) ? transactionTopLevelGroups.get(transactionBinarySearchResult) : null; if (kcExistingGroup != null) { - updateAttributesOfKCGroup(kcExistingGroup, groupEntry.getValue()); - syncResult.increaseUpdated(); - visitedGroupIds.add(kcExistingGroup.getId()); + + try { + + // Update each existing group to be synced in its own inner transaction to prevent race condition when + // the groups intended to be updated was already deleted via other channel in the meantime + KeycloakModelUtils.runJobInTransaction(ldapProvider.getSession().getKeycloakSessionFactory(), new KeycloakSessionTask() { + + @Override + public void run(KeycloakSession session) { + + updateAttributesOfKCGroup(kcExistingGroup, groupEntry.getValue()); + syncResult.increaseUpdated(); + visitedGroupIds.add(kcExistingGroup.getId()); + + } + + }); + + } catch (ModelException me) { + logger.error(String.format("Failed to update attributes of LDAP group %s: ", groupName), me); + syncResult.increaseFailed(); + } + } else { - GroupModel kcGroup = currentRealm.createGroup(groupName); - updateAttributesOfKCGroup(kcGroup, groupEntry.getValue()); - currentRealm.moveGroup(kcGroup, null); - syncResult.increaseAdded(); - visitedGroupIds.add(kcGroup.getId()); + + try { + + // Create each non-existing group to be synced in its own inner transaction to prevent race condition when + // the roup intended to be created was already created via other channel in the meantime + KeycloakModelUtils.runJobInTransaction(ldapProvider.getSession().getKeycloakSessionFactory(), new KeycloakSessionTask() { + + @Override + public void run(KeycloakSession session) { + + RealmModel innerTransactionRealm = session.realms().getRealm(realm.getId()); + GroupModel kcGroup = innerTransactionRealm.createGroup(groupName); + updateAttributesOfKCGroup(kcGroup, groupEntry.getValue()); + innerTransactionRealm.moveGroup(kcGroup, null); + syncResult.increaseAdded(); + visitedGroupIds.add(kcGroup.getId()); + + } + + }); + + } catch (ModelException me) { + logger.error(String.format("Failed to sync group %s from LDAP: ", groupName), me); + syncResult.increaseFailed(); + } } } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperSyncTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperSyncTest.java index 7fe5da7720..e6e03f504a 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperSyncTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperSyncTest.java @@ -422,9 +422,9 @@ public class LDAPGroupMapperSyncTest extends AbstractLDAPTest { LDAPTestUtils.createLDAPGroup(session, appRealm, ctx.getLdapModel(), - String.format("group-%s", i), + String.format("group-%s", j), descriptionAttrName, - String.format("Testing group-%s, created at: %s", i, new Date().toString()) + String.format("Testing group-%s, created at: %s", j, new Date().toString()) ); } logger.debugf("Done creating %s LDAP groups!", groupsPerIteration);