From 8aa5019efec30172edd6398edeb70e458446b6a4 Mon Sep 17 00:00:00 2001 From: Phy Date: Sun, 23 Feb 2020 21:25:21 -0500 Subject: [PATCH] KEYCLOAK-13074 Don't return LDAP group members if under IMPORT mode If GroupLDAPStorageMapper is running under IMPORT mode, getGroupMembers should not return users in LDAP, which, according to how UserStorageManager.query works (getting both user federation and Keycloak storage), will cause duplicate users in the list. A test has been added as well, which will fail before the fix in the mapper. --- .../group/GroupLDAPStorageMapper.java | 5 +++++ .../federation/ldap/LDAPGroupMapperTest.java | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+) 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 700acd5f3a..f5f062082a 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 @@ -579,6 +579,11 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements @Override public List getGroupMembers(RealmModel realm, GroupModel kcGroup, int firstResult, int maxResults) { + if (config.getMode() == LDAPGroupMapperMode.IMPORT) { + // only results from Keycloak should be returned, or imported LDAP and KC items will duplicate + 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) { 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 cd34bf4dbe..4540eccfdb 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 @@ -417,6 +417,17 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { long dbGroupCount = rob.getGroupsCount(); Assert.assertEquals(4, dbGroupCount); + // Check getGroupMembers + List group1Members = session.users().getGroupMembers(appRealm, group1, 0, 10); + List group11Members = session.users().getGroupMembers(appRealm, group11, 0, 10); + List group12Members = session.users().getGroupMembers(appRealm, group12, 0, 10); + + Assert.assertEquals(0, group1Members.size()); + Assert.assertEquals(1, group11Members.size()); + Assert.assertEquals("robkeycloak", group11Members.get(0).getUsername()); + Assert.assertEquals(1, group12Members.size()); + Assert.assertEquals("robkeycloak", group12Members.get(0).getUsername()); + // Delete some group mappings in LDAP and check that it doesn't have any effect and user still has groups LDAPObject ldapGroup = groupMapper.loadLDAPGroupByName("group11"); groupMapper.deleteGroupMappingInLDAP(robLdap, ldapGroup); @@ -428,6 +439,17 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { Assert.assertTrue(robGroups.contains(group11)); Assert.assertTrue(robGroups.contains(group12)); + // Check getGroupMembers + group1Members = session.users().getGroupMembers(appRealm, group1, 0, 10); + group11Members = session.users().getGroupMembers(appRealm, group11, 0, 10); + group12Members = session.users().getGroupMembers(appRealm, group12, 0, 10); + + Assert.assertEquals(0, group1Members.size()); + Assert.assertEquals(1, group11Members.size()); + Assert.assertEquals("robkeycloak", group11Members.get(0).getUsername()); + Assert.assertEquals(1, group12Members.size()); + Assert.assertEquals("robkeycloak", group12Members.get(0).getUsername()); + // Delete group mappings through model and verifies that user doesn't have them anymore rob.leaveGroup(group11); rob.leaveGroup(group12);