From af434d6bc1ae904da2538f207bf5313098757114 Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Wed, 30 Oct 2024 09:14:25 -0300 Subject: [PATCH] Add checks to prevent GroupLDAPStorageMapper from performing operations on groups it does not manage Closes #11008 Closes #17593 Closes #19652 Signed-off-by: Stefan Guilhen --- .../group/GroupLDAPStorageMapper.java | 35 ++++- .../testsuite/util/LDAPTestUtils.java | 8 +- .../federation/ldap/LDAPGroupMapperTest.java | 134 ++++++++++++++++-- 3 files changed, 156 insertions(+), 21 deletions(-) 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 ccda382696..0202ae2a1a 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 @@ -569,6 +569,11 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements return Collections.emptyList(); } + if (!isGroupInGroupPath(realm, kcGroup)) { + // group being inspected is not managed by this mapper - return empty collection + return Collections.emptyList(); + } + // TODO: with ranged search in AD we can improve the search using the specific range (not done for the moment) LDAPObject ldapGroup = loadLDAPGroupByName(kcGroup.getName()); if (ldapGroup == null) { @@ -703,18 +708,18 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements @Override public Stream getGroupsStream() { Stream ldapGroupMappings = getLDAPGroupMappingsConverted(); - if (config.getMode() == LDAPGroupMapperMode.LDAP_ONLY) { + if (config.isTopLevelGroupsPath() && config.getMode() == LDAPGroupMapperMode.LDAP_ONLY) { // Use just group mappings from LDAP return ldapGroupMappings; } else { - // Merge mappings from both DB and LDAP + // Merge mappings from both DB and LDAP (including groups assigned from other group mappers) return Stream.concat(ldapGroupMappings, super.getGroupsStream()); } } @Override public void joinGroup(GroupModel group) { - if (config.getMode() == LDAPGroupMapperMode.LDAP_ONLY) { + if (config.getMode() == LDAPGroupMapperMode.LDAP_ONLY && isGroupInGroupPath(realm, group)) { // We need to create new role mappings in LDAP cachedLDAPGroupMappings = null; addGroupMappingInLDAP(realm, group, ldapUser); @@ -725,6 +730,11 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements @Override public void leaveGroup(GroupModel group) { + // if user is leaving group not managed by this mapper, let the call proceed to the next mapper or to the DB. + if (!isGroupInGroupPath(realm, group)) { + super.leaveGroup(group); + } + try (LDAPQuery ldapQuery = createGroupQuery(true)) { LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); Condition roleNameCondition = conditionsBuilder.equal(config.getGroupNameLdapAttribute(), group.getName()); @@ -756,7 +766,7 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements @Override public boolean isMemberOf(GroupModel group) { - return RoleUtils.isDirectMember(getGroupsStream(),group); + return isGroupInGroupPath(realm, group) && RoleUtils.isDirectMember(getGroupsStream(),group); } protected Stream getLDAPGroupMappingsConverted() { @@ -795,6 +805,23 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements return config.isTopLevelGroupsPath() ? null : KeycloakModelUtils.findGroupByPath(session, realm, config.getGroupsPath()); } + protected boolean isGroupInGroupPath(RealmModel realm, GroupModel group) { + if (config.isTopLevelGroupsPath()) { + return true; // any group is in the path of the top level path. + } + GroupModel groupPathGroup = KeycloakModelUtils.findGroupByPath(session, realm, config.getGroupsPath()); + if (groupPathGroup != null) { + while(!groupPathGroup.getId().equals(group.getId())) { + group = group.getParent(); + if (group == null) { + return false; // we checked every ancestor group, and none matches the group path group. + } + } + return true; + } + return false; + } + /** * Creates a new KC group from given LDAP group name in given KC parent group or the groups path. */ diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java index 1d603e68e5..e7333cac4a 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java @@ -299,14 +299,18 @@ public class LDAPTestUtils { } public static void addOrUpdateGroupMapper(RealmModel realm, ComponentModel providerModel, LDAPGroupMapperMode mode, String descriptionAttrName, String... otherConfigOptions) { - ComponentModel mapperModel = getSubcomponentByName(realm, providerModel, "groupsMapper"); + addOrUpdateGroupMapper("groupsMapper", realm, providerModel, mode, descriptionAttrName, otherConfigOptions); + } + + public static void addOrUpdateGroupMapper(String mapperName, RealmModel realm, ComponentModel providerModel, LDAPGroupMapperMode mode, String descriptionAttrName, String... otherConfigOptions) { + ComponentModel mapperModel = getSubcomponentByName(realm, providerModel, mapperName); if (mapperModel != null) { mapperModel.getConfig().putSingle(GroupMapperConfig.MODE, mode.toString()); updateGroupMapperConfigOptions(mapperModel, otherConfigOptions); realm.updateComponent(mapperModel); } else { String baseDn = providerModel.getConfig().getFirst(LDAPConstants.BASE_DN); - mapperModel = KeycloakModelUtils.createComponentModel("groupsMapper", providerModel.getId(), GroupLDAPStorageMapperFactory.PROVIDER_ID, LDAPStorageMapper.class.getName(), + mapperModel = KeycloakModelUtils.createComponentModel(mapperName, providerModel.getId(), GroupLDAPStorageMapperFactory.PROVIDER_ID, LDAPStorageMapper.class.getName(), GroupMapperConfig.GROUPS_DN, "ou=Groups," + baseDn, GroupMapperConfig.MAPPED_GROUP_ATTRIBUTES, descriptionAttrName, GroupMapperConfig.PRESERVE_GROUP_INHERITANCE, "true", 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 f58832d1e2..c41cea3d7c 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 @@ -40,6 +40,7 @@ import org.keycloak.storage.ldap.idm.query.internal.LDAPQuery; 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; +import org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapperFactory; import org.keycloak.storage.ldap.mappers.membership.group.GroupMapperConfig; import org.keycloak.testsuite.util.LDAPRule; import org.keycloak.testsuite.util.LDAPTestUtils; @@ -72,7 +73,6 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { } - @Test public void test01_ldapOnlyGroupMappings() { test01_ldapOnlyGroupMappings(true); @@ -202,7 +202,7 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { mary.leaveGroup(groupTeamChild20182019); Assert.assertEquals(0, john.getGroupsStream().count()); - + groupCount = john.getGroupsCount(); Assert.assertEquals(0, groupCount); }); @@ -214,7 +214,6 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { } - protected void test02_readOnlyGroupMappings(boolean importEnabled) { testingClient.server().run(session -> { LDAPTestContext ctx = LDAPTestContext.init(session); @@ -617,7 +616,7 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { UserModel john = session.users().getUserByUsername(appRealm, "johnkeycloak"); - GroupModel group4 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group4"); + GroupModel group4 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group4"); john.joinGroup(group4); GroupModel group31 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group3/group31"); @@ -637,11 +636,11 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { UserModel john = session.users().getUserByUsername(appRealm, "johnkeycloak"); - GroupModel group14 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group1/group14"); - GroupModel group3 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group3"); + GroupModel group14 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group1/group14"); + GroupModel group3 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group3"); GroupModel group31 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group3/group31"); GroupModel group32 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group3/group32"); - GroupModel group4 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group4"); + GroupModel group4 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group4"); Set groups = john.getGroupsStream().collect(Collectors.toSet()); Assert.assertTrue(groups.contains(group14)); @@ -676,17 +675,17 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { UserModel david = session.users().addUser(appRealm, "davidkeycloak"); - GroupModel defaultGroup11 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/defaultGroup1/defaultGroup11"); + GroupModel defaultGroup11 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/defaultGroup1/defaultGroup11"); Assert.assertNotNull(defaultGroup11); - GroupModel defaultGroup12 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/defaultGroup1/defaultGroup12"); + GroupModel defaultGroup12 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/defaultGroup1/defaultGroup12"); Assert.assertNotNull(defaultGroup12); GroupModel group31 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group3/group31"); Assert.assertNotNull(group31); GroupModel group32 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group3/group32"); Assert.assertNotNull(group32); - GroupModel group4 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group4"); + GroupModel group4 = KeycloakModelUtils.findGroupByPath(session, appRealm, "/group4"); Assert.assertNotNull(group4); Set groups = david.getGroupsStream().collect(Collectors.toSet()); @@ -701,10 +700,10 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { private static LDAPObject searchObjectInBase(LDAPStorageProvider ldapProvider, String dn, String... attrs) { LDAPQuery q = new LDAPQuery(ldapProvider) - .setSearchDn(dn) - .setSearchScope(SearchControls.OBJECT_SCOPE); + .setSearchDn(dn) + .setSearchScope(SearchControls.OBJECT_SCOPE); if (attrs != null) { - for (String attr: attrs) { + for (String attr : attrs) { q.addReturningLdapAttribute(attr); } } @@ -776,9 +775,9 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { usernames.clear(); for (int i = 0; i < membersToTest; i += 10) { groupMembers = session.users().getGroupMembersStream(appRealm, kcBigGroup, i, 10) - .collect(Collectors.toList()); + .collect(Collectors.toList()); usernames.addAll(groupMembers.stream().map(u -> u.getUsername()).collect(Collectors.toSet())); - Assert.assertEquals("Incorrect number of users after pagination " + i, membersToTest < i + 10? membersToTest : i + 10, usernames.size()); + Assert.assertEquals("Incorrect number of users after pagination " + i, membersToTest < i + 10 ? membersToTest : i + 10, usernames.size()); } for (int i = 0; i < membersToTest; i++) { Assert.assertTrue("Group contains user after pagination " + i, usernames.contains(String.format("user%02d", i))); @@ -839,4 +838,109 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { Assert.assertEquals(LDAPDn.fromString(LDAPConstants.EMPTY_MEMBER_ATTRIBUTE_VALUE), LDAPDn.fromString(deleteGroup.getAttributeAsString(LDAPConstants.MEMBER))); }); } + + @Test + public void test10_multipleGroupMappersInDifferentPaths() { + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel realm = ctx.getRealm(); + + // 1- Remove the group mapper already installed + ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(realm, ctx.getLdapModel(), "groupsMapper"); + realm.removeComponent(mapperModel); + }); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel realm = ctx.getRealm(); + + // 2- Add a couple of local groups + realm.createGroup("ldap-groups-1"); + realm.createGroup("ldap-groups-2"); + + // 3- Add a couple of group mappers that map groups from different OUs into different group paths in Keycloak. + ComponentModel ldapModel = LDAPTestUtils.getLdapProviderModel(realm); + LDAPStorageProvider ldapProvider = LDAPTestUtils.getLdapProvider(session, ldapModel); + String baseDn = ldapModel.getConfig().getFirst(LDAPConstants.BASE_DN); + String descriptionName = ldapProvider.getLdapIdentityStore().getConfig().isActiveDirectory() ? "displayName" : "description"; + LDAPTestUtils.addLdapOUinBaseDn(ldapProvider, "Groups1"); + LDAPTestUtils.addOrUpdateGroupMapper("group-mapper-1", realm, ldapModel, LDAPGroupMapperMode.LDAP_ONLY, descriptionName, + GroupMapperConfig.GROUPS_DN, "ou=Groups1," + baseDn, GroupMapperConfig.LDAP_GROUPS_PATH, "/ldap-groups-1"); + LDAPTestUtils.addLdapOUinBaseDn(ldapProvider, "Groups2"); + LDAPTestUtils.addOrUpdateGroupMapper("group-mapper-2", realm, ldapModel, LDAPGroupMapperMode.LDAP_ONLY, descriptionName, + GroupMapperConfig.GROUPS_DN, "ou=Groups2," + baseDn, GroupMapperConfig.LDAP_GROUPS_PATH, "/ldap-groups-2"); + + LDAPTestUtils.createLDAPGroup("group-mapper-1", session, realm, ldapModel, "GroupA", descriptionName, "GroupA Description"); + LDAPTestUtils.createLDAPGroup("group-mapper-1", session, realm, ldapModel, "GroupB", descriptionName, "GroupB Description"); + LDAPTestUtils.createLDAPGroup("group-mapper-2", session, realm, ldapModel, "GroupC", descriptionName, "GroupC Description"); + + // 4- Sync LDAP groups from both mappers to Keycloak DB + ComponentModel mapperModel = LDAPTestUtils.getSubcomponentByName(realm, ldapModel, "group-mapper-1"); + new GroupLDAPStorageMapperFactory().create(session, mapperModel).syncDataFromFederationProviderToKeycloak(realm); + mapperModel = LDAPTestUtils.getSubcomponentByName(realm, ldapModel, "group-mapper-2"); + new GroupLDAPStorageMapperFactory().create(session, mapperModel).syncDataFromFederationProviderToKeycloak(realm); + }); + + testingClient.server(TEST_REALM_NAME).run(session -> { + RealmModel realm = session.getContext().getRealm(); + + GroupModel groupA = KeycloakModelUtils.findGroupByPath(session, realm, "/ldap-groups-1/GroupA"); + GroupModel groupB = KeycloakModelUtils.findGroupByPath(session, realm, "/ldap-groups-1/GroupB"); + GroupModel groupC = KeycloakModelUtils.findGroupByPath(session, realm, "/ldap-groups-2/GroupC"); + + UserModel john = session.users().getUserByUsername(realm, "johnkeycloak"); + UserModel mary = session.users().getUserByUsername(realm, "marykeycloak"); + + // 5- Join some of the groups, and check that the correct mapper was applied (i.e. user was added as a member of the correct group) + john.joinGroup(groupA); + john.joinGroup(groupC); + mary.joinGroup(groupB); + mary.joinGroup(groupC); + + // 5.1- Check that group mappings are in LDAP and hence available through federation + Set johnGroups = john.getGroupsStream().collect(Collectors.toSet()); + Assert.assertEquals(4, johnGroups.size()); + Assert.assertTrue(johnGroups.contains(groupA)); + Assert.assertFalse(johnGroups.contains(groupB)); + Assert.assertTrue(johnGroups.contains(groupC)); + + Set maryGroups = mary.getGroupsStream().collect(Collectors.toSet()); + Assert.assertEquals(4, maryGroups.size()); + Assert.assertFalse(maryGroups.contains(groupA)); + Assert.assertTrue(maryGroups.contains(groupB)); + Assert.assertTrue(maryGroups.contains(groupC)); + + // 5.2- Check through userProvider + List groupAMembers = session.users().getGroupMembersStream(realm, groupA, 0, 10) + .map(UserModel::getUsername).toList(); + List groupBMembers = session.users().getGroupMembersStream(realm, groupB, 0, 10) + .map(UserModel::getUsername).toList(); + List groupCMembers = session.users().getGroupMembersStream(realm, groupC, 0, 10) + .map(UserModel::getUsername).toList(); + + Assert.assertEquals(1, groupAMembers.size()); + Assert.assertEquals("johnkeycloak", groupAMembers.get(0)); + Assert.assertEquals(1, groupBMembers.size()); + Assert.assertEquals("marykeycloak", groupBMembers.get(0)); + Assert.assertEquals(2, groupCMembers.size()); + Assert.assertTrue(groupCMembers.contains("johnkeycloak")); + Assert.assertTrue(groupCMembers.contains("marykeycloak")); + }); + + testingClient.server(TEST_REALM_NAME).run(session -> { + RealmModel realm = session.getContext().getRealm(); + + // 6- Create a local group with the same name of one of the LDAP groups, and add a local user to this group + GroupModel localGroup = realm.createGroup("GroupA"); + UserModel localUser = UserStoragePrivateUtil.userLocalStorage(session).addUser(realm, "localuser"); + localUser.joinGroup(localGroup); + + // 7- Check that the members of the local group are not mixed with members of the group with same name from LDAP + List localGroupMembers = session.users().getGroupMembersStream(realm, localGroup, 0, 10) + .map(UserModel::getUsername).toList(); + Assert.assertEquals(1, localGroupMembers.size()); + Assert.assertEquals("localuser", localGroupMembers.get(0)); + }); + } } +