From 2d188068c4d3b8dcd7c835c9fafcbf2084c8b12d Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 14 Mar 2016 09:01:57 +0100 Subject: [PATCH 1/2] KEYCLOAK-2644 Delete user with a READ_ONLY LDAP federation provider just from Keycloak DB --- .../federation/ldap/LDAPFederationProvider.java | 4 ++-- .../org/keycloak/models/UserFederationManager.java | 2 +- .../base/FederationProvidersIntegrationTest.java | 12 +++++++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java index 010870ee2a..4ddd142f23 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java @@ -179,8 +179,8 @@ public class LDAPFederationProvider implements UserFederationProvider { @Override public boolean removeUser(RealmModel realm, UserModel user) { if (editMode == EditMode.READ_ONLY || editMode == EditMode.UNSYNCED) { - logger.warnf("User '%s' can't be deleted in LDAP as editMode is '%s'", user.getUsername(), editMode.toString()); - return false; + logger.warnf("User '%s' can't be deleted in LDAP as editMode is '%s'. Deleting user just from Keycloak DB, but he will be re-imported from LDAP again once searched in Keycloak", user.getUsername(), editMode.toString()); + return true; } LDAPObject ldapObject = loadAndValidateUser(realm, user); diff --git a/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java b/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java index 8f809f3389..ef8d182534 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java +++ b/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java @@ -96,7 +96,7 @@ public class UserFederationManager implements UserProvider { boolean localRemoved = session.userStorage().removeUser(realm, user); managedUsers.remove(user.getId()); if (!localRemoved) { - logger.warn("User removed from federation provider, but failed to remove him from keycloak model"); + logger.warn("User possibly removed from federation provider, but failed to remove him from keycloak model"); } return localRemoved; } else { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/FederationProvidersIntegrationTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/FederationProvidersIntegrationTest.java index 3ee6b70dcf..be8c97bd8d 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/FederationProvidersIntegrationTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/ldap/base/FederationProvidersIntegrationTest.java @@ -500,6 +500,8 @@ public class FederationProvidersIntegrationTest { } } + + // TODO: Rather separate test for fullNameMapper to better test all the possibilities @Test public void testFullNameMapper() { KeycloakSession session = keycloakRule.startSession(); @@ -691,7 +693,7 @@ public class FederationProvidersIntegrationTest { } - Assert.assertFalse(session.users().removeUser(appRealm, user)); + Assert.assertTrue(session.users().removeUser(appRealm, user)); } finally { keycloakRule.stopSession(session, false); } @@ -826,8 +828,12 @@ public class FederationProvidersIntegrationTest { LDAPObject ldapUser = ldapProvider.loadLDAPUserByUsername(appRealm, "johnkeycloak"); ldapProvider.getLdapIdentityStore().validatePassword(ldapUser, "Password1"); - // ATM it's not permitted to delete user in unsynced mode. Should be user deleted just locally instead? - Assert.assertFalse(session.users().removeUser(appRealm, user)); + // User is deleted just locally + Assert.assertTrue(session.users().removeUser(appRealm, user)); + + // Assert user not available locally, but will be reimported from LDAP once searched + Assert.assertNull(session.userStorage().getUserByUsername("johnkeycloak", appRealm)); + Assert.assertNotNull(session.users().getUserByUsername("johnkeycloak", appRealm)); } finally { keycloakRule.stopSession(session, false); } From e24ce91e819c757ff8b94e8fda4fb7a1d1fc0223 Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 14 Mar 2016 09:37:31 +0100 Subject: [PATCH 2/2] KEYCLOAK-2659 Allow sync all roles even if there are more than 1000 --- .../keycloak/federation/ldap/LDAPUtils.java | 40 +++++++++++++++++++ .../group/GroupLDAPFederationMapper.java | 23 +---------- .../GroupLDAPFederationMapperFactory.java | 7 ++-- .../role/RoleLDAPFederationMapper.java | 6 +-- 4 files changed, 48 insertions(+), 28 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java index b075d19029..e406c11d3f 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java @@ -19,6 +19,7 @@ package org.keycloak.federation.ldap; import java.util.Collection; import java.util.HashSet; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; @@ -229,6 +230,45 @@ public class LDAPUtils { } + /** + * Load all LDAP objects corresponding to given query. We will load them paginated, so we allow to bypass the limitation of 1000 + * maximum loaded objects in single query in MSAD + * + * @param ldapQuery + * @param ldapProvider + * @return + */ + public static List loadAllLDAPObjects(LDAPQuery ldapQuery, LDAPFederationProvider ldapProvider) { + LDAPConfig ldapConfig = ldapProvider.getLdapIdentityStore().getConfig(); + boolean pagination = ldapConfig.isPagination(); + if (pagination) { + // For now reuse globally configured batch size in LDAP provider page + int pageSize = ldapConfig.getBatchSizeForSync(); + + List result = new LinkedList<>(); + boolean nextPage = true; + + while (nextPage) { + ldapQuery.setLimit(pageSize); + final List currentPageGroups = ldapQuery.getResultList(); + result.addAll(currentPageGroups); + nextPage = ldapQuery.getPaginationContext() != null; + } + + return result; + } else { + // LDAP pagination not available. Do everything in single transaction + return ldapQuery.getResultList(); + } + } + + + /** + * Validate configured customFilter matches the requested format + * + * @param customFilter + * @throws FederationConfigValidationException + */ public static void validateCustomLdapFilter(String customFilter) throws FederationConfigValidationException { if (customFilter != null) { diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java index 00108bc6be..cd220b292a 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapper.java @@ -343,28 +343,7 @@ public class GroupLDAPFederationMapper extends AbstractLDAPFederationMapper impl // Send LDAP query to retrieve all groups protected List getAllLDAPGroups() { LDAPQuery ldapGroupQuery = createGroupQuery(); - - LDAPConfig ldapConfig = ldapProvider.getLdapIdentityStore().getConfig(); - boolean pagination = ldapConfig.isPagination(); - if (pagination) { - // For now reuse globally configured batch size in LDAP provider page - int pageSize = ldapConfig.getBatchSizeForSync(); - - List result = new LinkedList<>(); - boolean nextPage = true; - - while (nextPage) { - ldapGroupQuery.setLimit(pageSize); - final List currentPageGroups = ldapGroupQuery.getResultList(); - result.addAll(currentPageGroups); - nextPage = ldapGroupQuery.getPaginationContext() != null; - } - - return result; - } else { - // LDAP pagination not available. Do everything in single transaction - return ldapGroupQuery.getResultList(); - } + return LDAPUtils.loadAllLDAPObjects(ldapGroupQuery, ldapProvider); } diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapperFactory.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapperFactory.java index fb6056b743..169875d0a6 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapperFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/group/GroupLDAPFederationMapperFactory.java @@ -88,9 +88,9 @@ public class GroupLDAPFederationMapperFactory extends AbstractLDAPFederationMapp for (MembershipType membershipType : MembershipType.values()) { membershipTypes.add(membershipType.toString()); } - ProviderConfigProperty membershipType = createConfigProperty(RoleMapperConfig.MEMBERSHIP_ATTRIBUTE_TYPE, "Membership Attribute Type", - "DN means that LDAP role has it's members declared in form of their full DN. For example 'member: uid=john,ou=users,dc=example,dc=com' . " + - "UID means that LDAP role has it's members declared in form of pure user uids. For example 'memberUid: john' .", + ProviderConfigProperty membershipType = createConfigProperty(GroupMapperConfig.MEMBERSHIP_ATTRIBUTE_TYPE, "Membership Attribute Type", + "DN means that LDAP group has it's members declared in form of their full DN. For example 'member: uid=john,ou=users,dc=example,dc=com' . " + + "UID means that LDAP group has it's members declared in form of pure user uids. For example 'memberUid: john' .", ProviderConfigProperty.LIST_TYPE, membershipTypes); configProperties.add(membershipType); @@ -165,6 +165,7 @@ public class GroupLDAPFederationMapperFactory extends AbstractLDAPFederationMapp defaultValues.put(GroupMapperConfig.PRESERVE_GROUP_INHERITANCE, "true"); defaultValues.put(GroupMapperConfig.MEMBERSHIP_LDAP_ATTRIBUTE, LDAPConstants.MEMBER); + defaultValues.put(GroupMapperConfig.MEMBERSHIP_ATTRIBUTE_TYPE, MembershipType.DN.toString()); String mode = config.getEditMode() == UserFederationProvider.EditMode.WRITABLE ? LDAPGroupMapperMode.LDAP_ONLY.toString() : LDAPGroupMapperMode.READ_ONLY.toString(); defaultValues.put(GroupMapperConfig.MODE, mode); diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/role/RoleLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/role/RoleLDAPFederationMapper.java index cbafcc4cee..9dbeec95cc 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/role/RoleLDAPFederationMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/membership/role/RoleLDAPFederationMapper.java @@ -122,9 +122,9 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper imple logger.debugf("Syncing roles from LDAP into Keycloak DB. Mapper is [%s], LDAP provider is [%s]", mapperModel.getName(), ldapProvider.getModel().getDisplayName()); - // Send LDAP query - LDAPQuery ldapQuery = createRoleQuery(); - List ldapRoles = ldapQuery.getResultList(); + // Send LDAP query to load all roles + LDAPQuery ldapRoleQuery = createRoleQuery(); + List ldapRoles = LDAPUtils.loadAllLDAPObjects(ldapRoleQuery, ldapProvider); RoleContainerModel roleContainer = getTargetRoleContainer(); String rolesRdnAttr = config.getRoleNameLdapAttribute();