From 56da6c4b7d52259854460fdb5c361ca2a9a3bd41 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Thu, 14 Aug 2025 12:16:16 -0300 Subject: [PATCH] memberOf attribute empty or values with a DN that does not match the role base DN fetches all roles Closes #41842 Signed-off-by: Pedro Igor --- .../storage/ldap/idm/model/LDAPObject.java | 6 +- .../membership/UserRolesRetrieveStrategy.java | 24 +++--- .../federation/ldap/LDAPGroupMapperTest.java | 83 +++++++++++++++++++ 3 files changed, 100 insertions(+), 13 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LDAPObject.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LDAPObject.java index 6d0e2afdfb8..b6d0dd3786d 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LDAPObject.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/model/LDAPObject.java @@ -193,8 +193,12 @@ public class LDAPObject { // Case-insensitive. Return null if there is not value of attribute with given name or set with all values otherwise public Set getAttributeAsSet(String name) { + return getAttributeAsSetOrDefault(name, null); + } + + public Set getAttributeAsSetOrDefault(String name, Set defaultValue) { Map.Entry> entry = lowerCasedAttributes.get(name.toLowerCase()); - return (entry == null) ? null : new LinkedHashSet<>(entry.getValue()); + return (entry == null) ? defaultValue : new LinkedHashSet<>(entry.getValue()); } public boolean isRangeComplete(String name) { diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/UserRolesRetrieveStrategy.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/UserRolesRetrieveStrategy.java index dee4b03fe29..eca7413a457 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/UserRolesRetrieveStrategy.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/UserRolesRetrieveStrategy.java @@ -94,27 +94,27 @@ public interface UserRolesRetrieveStrategy { @Override public List getLDAPRoleMappings(CommonLDAPGroupMapper roleOrGroupMapper, LDAPObject ldapUser, LDAPConfig ldapConfig) { - Set memberOfValues = ldapUser.getAttributeAsSet(roleOrGroupMapper.getConfig().getMemberOfLdapAttribute()); - if (memberOfValues == null || memberOfValues.isEmpty()) { - return Collections.emptyList(); - } - try (LDAPQuery ldapQuery = roleOrGroupMapper.createLDAPGroupQuery()) { - - String rdnAttr = roleOrGroupMapper.getConfig().getLDAPGroupNameLdapAttribute(); + CommonLDAPGroupMapperConfig config = roleOrGroupMapper.getConfig(); + String rdnAttr = config.getLDAPGroupNameLdapAttribute(); LDAPQueryConditionsBuilder conditionBuilder = new LDAPQueryConditionsBuilder(); - + Set memberOfValues = ldapUser.getAttributeAsSetOrDefault(config.getMemberOfLdapAttribute(), Set.of()); // load only those groups/roles the user is memberOf // we do this by query to apply defined custom filters - final Condition[] conditions = memberOfValues.stream() + // and make sure the values of memberOf have the role/group base DN as its parent + Condition[] conditions = memberOfValues.stream() .map(LDAPDn::fromString) - .filter(roleDN -> roleDN.isDescendantOf(LDAPDn.fromString(roleOrGroupMapper.getConfig().getLDAPGroupsDn()))) + .filter(roleDN -> roleDN.isDescendantOf(LDAPDn.fromString(config.getLDAPGroupsDn()))) .map(roleDN -> conditionBuilder.equal(rdnAttr, roleDN.getFirstRdn().getAttrValue(rdnAttr))) .toArray(Condition[]::new); - if (conditions.length > 0) { - ldapQuery.addWhereCondition(conditionBuilder.orCondition(conditions)); + + if (conditions.length == 0) { + // no roles/groups to fetch based on the pre-filters applied to the memberOf values + return List.of(); } + ldapQuery.addWhereCondition(conditionBuilder.orCondition(conditions)); + return LDAPUtils.loadAllLDAPObjects(ldapQuery, ldapConfig); } } 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 11b5a0fd95b..74c836f5898 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 @@ -599,6 +599,89 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { testRealm().components().component(groupMapperRep.getId()).update(groupMapperRep); } + @Test + public void test05_DoNotResolveGroupsIfMemberOfNotChildOfGroupBaseDN() { + ComponentRepresentation groupMapperRep = findMapperRepByName("groupsMapper"); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + LDAPTestUtils.addUserAttributeMapper(appRealm, ctx.getLdapModel(), "streetMapper", "street", LDAPConstants.STREET); + ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(appRealm, ctx.getLdapModel(), "groupsMapper"); + GroupLDAPStorageMapper groupMapper = LDAPTestUtils.getGroupMapper(mapperModel, ctx.getLdapProvider(), appRealm); + String baseGroupDN = groupMapper.getConfig().getLDAPGroupsDn(); + String invalidBaseGroupDN = baseGroupDN.replace("ou=Groups", "ou=OtherGroupBaseDN"); + LDAPObject carlos = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "alice", "Alice", "Jack", "alice@email.org", "cn=mygroup," + invalidBaseGroupDN, "1234"); + + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), carlos, "Password1"); + LDAPTestUtils.updateConfigOptions(mapperModel, + GroupMapperConfig.USER_ROLES_RETRIEVE_STRATEGY, GroupMapperConfig.GET_GROUPS_FROM_USER_MEMBEROF_ATTRIBUTE, + GroupMapperConfig.MEMBEROF_LDAP_ATTRIBUTE, LDAPConstants.STREET); + appRealm.updateComponent(mapperModel); + }); + + ComponentRepresentation streetMapperRep = findMapperRepByName("streetMapper"); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + UserModel user = session.users().getUserByUsername(appRealm, "alice"); + Set groups = user.getGroupsStream().collect(Collectors.toSet()); + + Assert.assertTrue(groups.isEmpty()); + }); + + testRealm().components().component(streetMapperRep.getId()).remove(); + groupMapperRep.getConfig().putSingle(GroupMapperConfig.USER_ROLES_RETRIEVE_STRATEGY, GroupMapperConfig.LOAD_GROUPS_BY_MEMBER_ATTRIBUTE); + testRealm().components().component(groupMapperRep.getId()).update(groupMapperRep); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + LDAPStorageProvider ldapFedProvider = LDAPTestUtils.getLdapProvider(session, ctx.getLdapModel()); + LDAPTestUtils.removeLDAPUserByUsername(ctx.getLdapProvider(), ctx.getRealm(), ldapFedProvider.getLdapIdentityStore().getConfig(), "alice"); + }); + } + + @Test + public void test05_DoNotResolveGroupsIfMemberOfHasEmptyValue() { + ComponentRepresentation groupMapperRep = findMapperRepByName("groupsMapper"); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + LDAPTestUtils.addUserAttributeMapper(appRealm, ctx.getLdapModel(), "streetMapper", "street", LDAPConstants.STREET); + ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(appRealm, ctx.getLdapModel(), "groupsMapper"); + LDAPObject carlos = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "alice", "Alice", "Jack", "alice@email.org", "", "1234"); + + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), carlos, "Password1"); + LDAPTestUtils.updateConfigOptions(mapperModel, + GroupMapperConfig.USER_ROLES_RETRIEVE_STRATEGY, GroupMapperConfig.GET_GROUPS_FROM_USER_MEMBEROF_ATTRIBUTE, + GroupMapperConfig.MEMBEROF_LDAP_ATTRIBUTE, LDAPConstants.STREET); + appRealm.updateComponent(mapperModel); + }); + + ComponentRepresentation streetMapperRep = findMapperRepByName("streetMapper"); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + UserModel user = session.users().getUserByUsername(appRealm, "alice"); + Set groups = user.getGroupsStream().collect(Collectors.toSet()); + + Assert.assertTrue(groups.isEmpty()); + }); + + testRealm().components().component(streetMapperRep.getId()).remove(); + groupMapperRep.getConfig().putSingle(GroupMapperConfig.USER_ROLES_RETRIEVE_STRATEGY, GroupMapperConfig.LOAD_GROUPS_BY_MEMBER_ATTRIBUTE); + testRealm().components().component(groupMapperRep.getId()).update(groupMapperRep); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + LDAPStorageProvider ldapFedProvider = LDAPTestUtils.getLdapProvider(session, ctx.getLdapModel()); + LDAPTestUtils.removeLDAPUserByUsername(ctx.getLdapProvider(), ctx.getRealm(), ldapFedProvider.getLdapIdentityStore().getConfig(), "alice"); + }); + } // KEYCLOAK-5017 @Test