Skip to content

Fix GroupLDAPStorageMapper so it doesn't attempt to update a group fe… #30379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pedroigor merged 1 commit into from
Jun 12, 2024
Merged

Fix GroupLDAPStorageMapper so it doesn't attempt to update a group fe… #30379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions .../main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,13 +194,15 @@ public String getStatus() {
return syncResult;
}

private void syncExistingGroup(GroupModel kcExistingGroup, Map.Entry<String, LDAPObject> groupEntry,
private void syncExistingGroup(RealmModel realm, GroupModel kcExistingGroup, Map.Entry<String, LDAPObject> groupEntry,
SynchronizationResult syncResult, Set<String> visitedGroupIds, String groupName) {
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(), session -> {
updateAttributesOfKCGroup(kcExistingGroup, groupEntry.getValue());
RealmModel innerTransactionRealm = session.realms().getRealm(realm.getId());
GroupModel innerTransactionGroup = session.groups().getGroupById(innerTransactionRealm, kcExistingGroup.getId());
updateAttributesOfKCGroup(innerTransactionGroup, groupEntry.getValue());
syncResult.increaseUpdated();
visitedGroupIds.add(kcExistingGroup.getId());
});
Expand Down Expand Up @@ -278,9 +280,9 @@ private void syncFlatGroupStructure(RealmModel realm, SynchronizationResult sync
GroupModel kcExistingGroup = transactionGroupPathGroups.get(groupName);

if (kcExistingGroup != null) {
syncExistingGroup(kcExistingGroup, groupEntry, syncResult, visitedGroupIds, groupName);
syncExistingGroup(currentRealm, kcExistingGroup, groupEntry, syncResult, visitedGroupIds, groupName);
} else {
syncNonExistingGroup(realm, groupEntry, syncResult, visitedGroupIds, groupName);
syncNonExistingGroup(currentRealm, groupEntry, syncResult, visitedGroupIds, groupName);
}
}
});
Expand Down
60 changes: 59 additions & 1 deletion ...ts/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperSyncTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.keycloak.storage.ldap.LDAPUtils;
import org.keycloak.storage.ldap.idm.model.LDAPDn;
import org.keycloak.storage.ldap.idm.model.LDAPObject;
import org.keycloak.storage.ldap.mappers.LDAPStorageMapper;
import org.keycloak.storage.ldap.mappers.membership.LDAPGroupMapperMode;
import org.keycloak.storage.ldap.mappers.membership.MembershipType;
import org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper;
Expand All @@ -52,6 +53,7 @@
import java.util.Date;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.keycloak.testsuite.util.LDAPTestUtils.getGroupDescriptionLDAPAttrName;

Expand Down Expand Up @@ -114,7 +116,6 @@ private void testSyncNoPreserveGroupInheritance() throws Exception {
testingClient.server().run(session -> {
LDAPTestContext ctx = LDAPTestContext.init(session);
RealmModel realm = ctx.getRealm();
String descriptionAttrName = LDAPTestUtils.getGroupDescriptionLDAPAttrName(ctx.getLdapProvider());
ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(realm, ctx.getLdapModel(), "groupsMapper");
LDAPStorageProvider ldapProvider = LDAPTestUtils.getLdapProvider(session, ctx.getLdapModel());
GroupLDAPStorageMapper groupMapper = LDAPTestUtils.getGroupMapper(mapperModel, ldapProvider, realm);
Expand Down Expand Up @@ -500,4 +501,61 @@ public void test07_ldapGroupsSyncHasLinearTimeComplexity() throws Exception {

}

@Test
public void test08_flatSynchWithBatchSizeLessThanNumberOfGroups() {
// update the LDAP config to use pagination and a batch size that is less than the number of LDAP groups.
testingClient.server().run(session -> {
LDAPTestContext ctx = LDAPTestContext.init(session);
RealmModel realm = ctx.getRealm();
ComponentModel ldapModel = LDAPTestUtils.getLdapProviderModel(realm);
LDAPTestUtils.updateConfigOptions(ldapModel, LDAPConstants.PAGINATION, "true", LDAPConstants.BATCH_SIZE_FOR_SYNC, "1");
realm.updateComponent(ldapModel);

ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(ctx.getRealm(), ldapModel, "groupsMapper");
LDAPTestUtils.updateConfigOptions(mapperModel, GroupMapperConfig.PRESERVE_GROUP_INHERITANCE, "false");
realm.updateComponent(mapperModel);
});

testingClient.server().run(session -> {
LDAPTestContext ctx = LDAPTestContext.init(session);
RealmModel realm = ctx.getRealm();
ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(realm, ctx.getLdapModel(), "groupsMapper");

// check right config is in place.
Assert.assertEquals(ctx.getLdapModel().getConfig().getFirst(LDAPConstants.PAGINATION), "true");
Assert.assertEquals(ctx.getLdapModel().getConfig().getFirst(LDAPConstants.BATCH_SIZE_FOR_SYNC), "1");
Assert.assertEquals(mapperModel.getConfig().getFirst(GroupMapperConfig.PRESERVE_GROUP_INHERITANCE), "false");

// synch groups a first time - imports all new groups into keycloak.
LDAPStorageMapper groupMapper = new GroupLDAPStorageMapperFactory().create(session, mapperModel);
SynchronizationResult syncResult = groupMapper.syncDataFromFederationProviderToKeycloak(realm);
LDAPTestAsserts.assertSyncEquals(syncResult, 3, 0, 0, 0);

// check all groups were imported as top level groups with no subgroups.
Stream.of("/group1", "/group11", "/group12").forEach(path -> {
GroupModel kcGroup = KeycloakModelUtils.findGroupByPath(session, realm, path);
Assert.assertNotNull(kcGroup);
Assert.assertEquals(0, kcGroup.getSubGroupsStream().count());
});

// re-synch groups, updating previously imported groups.
syncResult = groupMapper.syncDataFromFederationProviderToKeycloak(realm);
LDAPTestAsserts.assertSyncEquals(syncResult, 0, 3, 0, 0);
});

// restore pagination, batch size and preserve inheritance configs.
testingClient.server().run(session -> {
LDAPTestContext ctx = LDAPTestContext.init(session);
RealmModel realm = ctx.getRealm();
ComponentModel ldapModel = LDAPTestUtils.getLdapProviderModel(realm);
ldapModel.getConfig().putSingle(LDAPConstants.PAGINATION, "false");
ldapModel.getConfig().remove(LDAPConstants.BATCH_SIZE_FOR_SYNC);
realm.updateComponent(ldapModel);

ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(ctx.getRealm(), ldapModel, "groupsMapper");
LDAPTestUtils.updateConfigOptions(mapperModel, GroupMapperConfig.PRESERVE_GROUP_INHERITANCE, "false");
realm.updateComponent(mapperModel);
});
}

}