From b66d8d151b7505e65cb481858d3ac75b9593301b Mon Sep 17 00:00:00 2001 From: kvfi Date: Sun, 17 May 2026 21:15:19 +0200 Subject: [PATCH] fix: include nested group members for recursive retrieve strategy When an LDAP group mapper uses the LOAD_GROUPS_BY_MEMBER_ATTRIBUTE_RECURSIVELY retrieve strategy, a user's group memberships - and therefore the role mappings granted through those groups - are resolved transitively through nested groups. Listing the members of a group, however, only returned its direct members, so a user that belonged to a group solely through a nested sub group was invisible on that group's members endpoint, even though it effectively belonged to it. This left role evaluation and member listing inconsistent. GroupLDAPStorageMapper.getGroupMembers now resolves members recursively when the recursive strategy is selected: it walks the transitive closure of the group's nested sub groups via the 'member' attribute and returns the de-duplicated union of all their members, with pagination applied to the combined result. Closes #26699 Signed-off-by: kvfi --- .../group/GroupLDAPStorageMapper.java | 46 +++++++++++++++++++ .../group/GroupLDAPStorageMapperFactory.java | 2 +- .../federation/ldap/LDAPGroupMapperTest.java | 44 ++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-) 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 3601fffe70f..e536ed477f8 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 @@ -17,11 +17,14 @@ package org.keycloak.storage.ldap.mappers.membership.group; +import java.util.ArrayDeque; import java.util.Collection; import java.util.Collections; +import java.util.Deque; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -582,10 +585,53 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements } String strategyKey = config.getUserGroupsRetrieveStrategy(); + + if (GroupMapperConfig.LOAD_GROUPS_BY_MEMBER_ATTRIBUTE_RECURSIVELY.equals(strategyKey)) { + return getGroupMembersRecursively(realm, ldapGroup, firstResult, maxResults); + } + UserRolesRetrieveStrategy strategy = factory.getUserGroupsRetrieveStrategy(strategyKey); return strategy.getLDAPRoleMembers(realm, this, ldapGroup, firstResult, maxResults); } + private List getGroupMembersRecursively(RealmModel realm, LDAPObject ldapGroup, int firstResult, int maxResults) { + MembershipType membershipType = config.getMembershipTypeLdapAttribute(); + + Map groupClosure = new LinkedHashMap<>(); + Deque pending = new ArrayDeque<>(); + groupClosure.put(ldapGroup.getDn().toString(), ldapGroup); + pending.add(ldapGroup); + + while (!pending.isEmpty()) { + LDAPObject currentGroup = pending.poll(); + for (LDAPDn subGroupDn : getLDAPSubgroups(currentGroup)) { + String subGroupDnString = subGroupDn.toString(); + if (groupClosure.containsKey(subGroupDnString)) { + continue; + } + String subGroupName = subGroupDn.getFirstRdn().getAttrValue(config.getGroupNameLdapAttribute()); + LDAPObject subGroup = subGroupName == null ? null : loadLDAPGroupByName(subGroupName); + if (subGroup == null) { + continue; + } + groupClosure.put(subGroupDnString, subGroup); + pending.add(subGroup); + } + } + + Map members = new LinkedHashMap<>(); + for (LDAPObject group : groupClosure.values()) { + for (UserModel member : membershipType.getGroupMembers(realm, this, group, 0, Integer.MAX_VALUE)) { + members.putIfAbsent(member.getId(), member); + } + } + + return members.values().stream() + .skip(Math.max(firstResult, 0)) + .limit(maxResults < 0 ? Long.MAX_VALUE : maxResults) + .collect(Collectors.toList()); + } + public void addGroupMappingInLDAP(RealmModel realm, GroupModel kcGroup, LDAPObject ldapUser) { String groupName = kcGroup.getName(); LDAPObject ldapGroup = loadLDAPGroupByName(groupName); diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java index d3a6cc67c9d..80e0dc3cb38 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java @@ -183,7 +183,7 @@ public class GroupLDAPStorageMapperFactory extends AbstractLDAPStorageMapperFact String groupRetrieversHelpText = "Specify how to retrieve groups of user. LOAD_GROUPS_BY_MEMBER_ATTRIBUTE means that roles of user will be retrieved by sending LDAP query to retrieve all groups where 'member' is our user. " + "GET_GROUPS_FROM_USER_MEMBEROF_ATTRIBUTE means that groups of user will be retrieved from 'memberOf' attribute of our user. Or from the other attribute specified by 'Member-Of LDAP Attribute' . "; if (isActiveDirectory) { - groupRetrieversHelpText = groupRetrieversHelpText + "LOAD_GROUPS_BY_MEMBER_ATTRIBUTE_RECURSIVELY is applicable just in Active Directory and it means that groups of user will be retrieved recursively with usage of LDAP_MATCHING_RULE_IN_CHAIN Ldap extension."; + groupRetrieversHelpText = groupRetrieversHelpText + "LOAD_GROUPS_BY_MEMBER_ATTRIBUTE_RECURSIVELY is applicable just in Active Directory and it means that groups of user will be retrieved recursively with usage of LDAP_MATCHING_RULE_IN_CHAIN Ldap extension. When this strategy is selected, listing the members of a group also returns the members of its nested groups."; } else { // Option should be available just for the Active Directory groupRetrievers.remove(GroupMapperConfig.LOAD_GROUPS_BY_MEMBER_ATTRIBUTE_RECURSIVELY); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java index fa5bde19f17..b14b9b4d15f 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java @@ -1130,4 +1130,48 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { appRealm.updateComponent(mapperModel); }); } + + @Test + public void test12_recursiveGroupMemberRetrieval() { + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(appRealm, ctx.getLdapModel(), "groupsMapper"); + GroupLDAPStorageMapper groupMapper = LDAPTestUtils.getGroupMapper(mapperModel, ctx.getLdapProvider(), appRealm); + + LDAPObject group1 = groupMapper.loadLDAPGroupByName("group1"); + LDAPObject group11 = groupMapper.loadLDAPGroupByName("group11"); + LDAPObject group12 = groupMapper.loadLDAPGroupByName("group12"); + + LDAPObject user1 = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "recuser1", "Rec", "One", "recuser1@email.org", null, "1"); + LDAPObject user2 = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "recuser2", "Rec", "Two", "recuser2@email.org", null, "2"); + LDAPObject user3 = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "recuser3", "Rec", "Three", "recuser3@email.org", null, "3"); + + LDAPUtils.addMember(ctx.getLdapProvider(), MembershipType.DN, LDAPConstants.MEMBER, "not-used", group1, user1); + LDAPUtils.addMember(ctx.getLdapProvider(), MembershipType.DN, LDAPConstants.MEMBER, "not-used", group11, user2); + LDAPUtils.addMember(ctx.getLdapProvider(), MembershipType.DN, LDAPConstants.MEMBER, "not-used", group12, user3); + + LDAPTestUtils.updateConfigOptions(mapperModel, + GroupMapperConfig.MODE, LDAPGroupMapperMode.LDAP_ONLY.toString(), + GroupMapperConfig.USER_ROLES_RETRIEVE_STRATEGY, GroupMapperConfig.LOAD_GROUPS_BY_MEMBER_ATTRIBUTE_RECURSIVELY); + appRealm.updateComponent(mapperModel); + }); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + GroupModel group1 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group1"); + GroupModel group11 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group1/group11"); + + List group11Members = session.users().getGroupMembersStream(appRealm, group11, 0, 10) + .map(UserModel::getUsername).collect(Collectors.toList()); + assertThat(group11Members, containsInAnyOrder("recuser2")); + + List group1Members = session.users().getGroupMembersStream(appRealm, group1, 0, 10) + .map(UserModel::getUsername).collect(Collectors.toList()); + assertThat(group1Members, containsInAnyOrder("recuser1", "recuser2", "recuser3")); + }); + } }