From f78c54fa42c09f76870e093b5496cc99da5f0f3b Mon Sep 17 00:00:00 2001 From: Ricardo Martin Date: Fri, 8 Dec 2023 17:55:17 +0100 Subject: [PATCH] Fixes for LDAP group membership and search in chunks Closes #23966 --- .../org/keycloak/storage/ldap/LDAPConfig.java | 14 +++ .../storage/ldap/LDAPStorageProvider.java | 100 +++++++++++++++--- .../idm/store/ldap/LDAPIdentityStore.java | 7 ++ .../mappers/membership/MembershipType.java | 92 ++-------------- .../org/keycloak/models/LDAPConstants.java | 2 + .../java/org/keycloak/utils/StreamsUtil.java | 55 ++++++++++ .../federation/ldap/LDAPGroupMapperTest.java | 19 ++++ 7 files changed, 189 insertions(+), 100 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPConfig.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPConfig.java index b2704ba3606..a1c59975486 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPConfig.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPConfig.java @@ -203,6 +203,20 @@ public class LDAPConfig { return Boolean.parseBoolean(pagination); } + public int getMaxConditions() { + String string = config.getFirst(LDAPConstants.MAX_CONDITIONS); + if (string != null) { + try { + int max = Integer.parseInt(string); + if (max > 0) { + return max; + } + } catch (NumberFormatException e) { + } + } + return LDAPConstants.DEFAULT_MAX_CONDITIONS; + } + public int getBatchSizeForSync() { String pageSizeConfig = config.getFirst(LDAPConstants.BATCH_SIZE_FOR_SYNC); return pageSizeConfig!=null ? Integer.parseInt(pageSizeConfig) : LDAPConstants.DEFAULT_BATCH_SIZE_FOR_SYNC; diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java index 915d81cdede..558b463644f 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java @@ -18,6 +18,7 @@ package org.keycloak.storage.ldap; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -27,11 +28,13 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Stream; import javax.naming.AuthenticationException; import javax.naming.NamingException; +import javax.naming.directory.SearchControls; import org.jboss.logging.Logger; import org.keycloak.common.constants.KerberosConstants; @@ -72,6 +75,7 @@ import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.UserStorageUtil; import org.keycloak.storage.adapter.InMemoryUserAdapter; import org.keycloak.storage.adapter.UpdateOnlyChangeUserModelDelegate; +import org.keycloak.storage.ldap.idm.model.LDAPDn; import org.keycloak.storage.ldap.idm.model.LDAPObject; import org.keycloak.storage.ldap.idm.query.Condition; import org.keycloak.storage.ldap.idm.query.EscapeStrategy; @@ -96,7 +100,7 @@ import org.keycloak.userprofile.UserProfileDecorator; import org.keycloak.userprofile.UserProfileMetadata; import org.keycloak.userprofile.UserProfileUtil; -import static org.keycloak.utils.StreamsUtil.paginatedStream; +import org.keycloak.utils.StreamsUtil; /** * @author Marek Posolda @@ -272,24 +276,27 @@ public class LDAPStorageProvider implements UserStorageProvider, @Override public Stream searchForUserByUserAttributeStream(RealmModel realm, String attrName, String attrValue) { - try (LDAPQuery ldapQuery = LDAPUtils.createQueryForUserSearch(this, realm)) { - LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); + List ldapObjects; + if (LDAPConstants.LDAP_ID.equals(attrName)) { + // search by UUID attribute + LDAPObject ldapObject = loadLDAPUserByUuid(realm, attrValue); + ldapObjects = ldapObject == null? Collections.emptyList() : Collections.singletonList(ldapObject); + } else if (LDAPConstants.LDAP_ENTRY_DN.equals(attrName)) { + // search by DN attribute + LDAPObject ldapObject = loadLDAPUserByDN(realm, LDAPDn.fromString(attrValue)); + ldapObjects = ldapObject == null? Collections.emptyList() : Collections.singletonList(ldapObject); + } else { + try (LDAPQuery ldapQuery = LDAPUtils.createQueryForUserSearch(this, realm)) { + LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); - Condition attrCondition = conditionsBuilder.equal(attrName, attrValue, EscapeStrategy.DEFAULT); - ldapQuery.addWhereCondition(attrCondition); + Condition attrCondition = conditionsBuilder.equal(attrName, attrValue, EscapeStrategy.DEFAULT); + ldapQuery.addWhereCondition(attrCondition); - List ldapObjects = ldapQuery.getResultList(); + ldapObjects = ldapQuery.getResultList(); + } + } - return ldapObjects.stream().map(ldapUser -> { - String ldapUsername = LDAPUtils.getUsername(ldapUser, this.ldapIdentityStore.getConfig()); - UserModel localUser = UserStoragePrivateUtil.userLocalStorage(session).getUserByUsername(realm, ldapUsername); - if (localUser == null) { - return importUserFromLDAP(session, realm, ldapUser); - } else { - return proxy(realm, localUser, ldapUser, false); - } - }); - } + return ldapObjects.stream().map(ldapUser -> importUserFromLDAP(session, realm, ldapUser)); } public boolean synchronizeRegistrations() { @@ -376,7 +383,7 @@ public class LDAPStorageProvider implements UserStorageProvider, searchLDAP(realm, search, firstResult, maxResults) : searchLDAPByAttributes(realm, params, firstResult, maxResults); - return paginatedStream(result.filter(filterLocalUsers(realm)), firstResult, maxResults) + return StreamsUtil.paginatedStream(result.filter(filterLocalUsers(realm)), firstResult, maxResults) .map(ldapObject -> importUserFromLDAP(session, realm, ldapObject)); } @@ -422,6 +429,53 @@ public class LDAPStorageProvider implements UserStorageProvider, return result; } + private Stream loadUsersByDNsChunk(RealmModel realm, String rdnAttr, Collection dns) { + try (LDAPQuery ldapQuery = LDAPUtils.createQueryForUserSearch(this, realm)) { + final LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); + final Set dnSet = new HashSet<>(dns); + final Condition[] conditions = dns.stream() + .map(dn -> conditionsBuilder.equal(rdnAttr, dn.getFirstRdn().getAttrValue(rdnAttr))) + .toArray(Condition[]::new); + ldapQuery.addWhereCondition(conditionsBuilder.orCondition(conditions)); + return ldapQuery.getResultList().stream().filter(ldapUser -> dnSet.contains(ldapUser.getDn())); + } + } + + public Stream loadUsersByDNs(RealmModel realm, Collection dns, int firstResult, int maxResults) { + final String rdnAttr = ldapIdentityStore.getConfig().getRdnLdapAttribute(); + final LDAPDn usersDn = LDAPDn.fromString(ldapIdentityStore.getConfig().getUsersDn()); + final int chunkSize = ldapIdentityStore.getConfig().getMaxConditions(); + return StreamsUtil.chunkedStream( + dns.stream().filter(dn -> dn.getFirstRdn().getAttrValue(rdnAttr) != null && dn.isDescendantOf(usersDn)), + chunkSize) + .map(chunk -> loadUsersByDNsChunk(realm, rdnAttr, chunk)) + .flatMap(Function.identity()) + .skip(firstResult) + .limit(maxResults) + .map(ldapUser -> importUserFromLDAP(session, realm, ldapUser)); + } + + private Stream loadUsersByUniqueAttributeChunk(RealmModel realm, String uidName, Collection uids) { + try (LDAPQuery ldapQuery = LDAPUtils.createQueryForUserSearch(this, realm)) { + LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); + Condition[] conditions = uids.stream() + .map(uid -> conditionsBuilder.equal(uidName, uid)) + .toArray(Condition[]::new); + ldapQuery.addWhereCondition(conditionsBuilder.orCondition(conditions)); + return ldapQuery.getResultList().stream(); + } + } + + public Stream loadUsersByUniqueAttribute(RealmModel realm, String uidName, Collection uids, int firstResult, int maxResults) { + final int chunkSize = ldapIdentityStore.getConfig().getMaxConditions(); + return StreamsUtil.chunkedStream(uids.stream(), chunkSize) + .map(chunk -> loadUsersByUniqueAttributeChunk(realm, uidName, chunk)) + .flatMap(Function.identity()) + .skip(firstResult) + .limit(maxResults) + .map(ldapUser -> importUserFromLDAP(session, realm, ldapUser)); + } + /** * Searches LDAP using logical conjunction of params. It supports *
    @@ -909,6 +963,18 @@ public class LDAPStorageProvider implements UserStorageProvider, } } + public LDAPObject loadLDAPUserByDN(RealmModel realm, LDAPDn dn) { + if (dn == null || !dn.isDescendantOf(LDAPDn.fromString(ldapIdentityStore.getConfig().getUsersDn()))) { + // no need to search + return null; + } + try (LDAPQuery ldapQuery = LDAPUtils.createQueryForUserSearch(this, realm)) { + ldapQuery.setSearchDn(dn.getLdapName()); + ldapQuery.setSearchScope(SearchControls.OBJECT_SCOPE); + return ldapQuery.getFirstResult(); + } + } + private Predicate filterLocalUsers(RealmModel realm) { return ldapObject -> UserStoragePrivateUtil.userLocalStorage(session).getUserByUsername(realm, LDAPUtils.getUsername(ldapObject, LDAPStorageProvider.this.ldapIdentityStore.getConfig())) == null; } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java index 96fd281c12c..7d11e8eb11d 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java @@ -61,6 +61,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; +import javax.naming.NameNotFoundException; import javax.naming.directory.AttributeInUseException; import javax.naming.directory.NoSuchAttributeException; import javax.naming.directory.SchemaViolationException; @@ -284,6 +285,12 @@ public class LDAPIdentityStore implements IdentityStore { results.add(populateAttributedType(result, identityQuery)); } } + } catch (NameNotFoundException e) { + if (identityQuery.getSearchScope() == SearchControls.OBJECT_SCOPE) { + // if searching in base (dn search) return empty as entry does not exist + return Collections.emptyList(); + } + throw new ModelException("Querying of LDAP failed " + identityQuery, e); } catch (Exception e) { throw new ModelException("Querying of LDAP failed " + identityQuery, e); } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/MembershipType.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/MembershipType.java index e3e1194e3c2..f8715769deb 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/MembershipType.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/MembershipType.java @@ -17,7 +17,6 @@ package org.keycloak.storage.ldap.mappers.membership; -import org.keycloak.models.ModelException; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.storage.ldap.LDAPConfig; @@ -25,17 +24,12 @@ import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.LDAPUtils; import org.keycloak.storage.ldap.idm.model.LDAPDn; import org.keycloak.storage.ldap.idm.model.LDAPObject; -import org.keycloak.storage.ldap.idm.query.Condition; -import org.keycloak.storage.ldap.idm.query.EscapeStrategy; -import org.keycloak.storage.ldap.idm.query.internal.LDAPQuery; -import org.keycloak.storage.ldap.idm.query.internal.LDAPQueryConditionsBuilder; -import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; -import java.util.LinkedList; +import java.util.LinkedHashSet; import java.util.List; import java.util.Set; +import java.util.stream.Collectors; /** * @author Marek Posolda @@ -59,8 +53,8 @@ public enum MembershipType { String membershipLdapAttribute, LDAPDn requiredParentDn, String rdnAttr) { Set allMemberships = LDAPUtils.getExistingMemberships(ldapProvider, membershipLdapAttribute, ldapGroup); - // Filter and keep just descendants of requiredParentDn - Set result = new HashSet<>(); + // Filter and keep just descendants of requiredParentDn and with the correct RDN + Set result = new LinkedHashSet<>(); for (String membership : allMemberships) { LDAPDn childDn = LDAPDn.fromString(membership); if (childDn.getFirstRdn().getAttrValue(rdnAttr) != null && childDn.isDescendantOf(requiredParentDn)) { @@ -79,54 +73,15 @@ public enum MembershipType { LDAPDn usersDn = LDAPDn.fromString(ldapProvider.getLdapIdentityStore().getConfig().getUsersDn()); Set userDns = getLDAPMembersWithParent(ldapProvider, ldapGroup, config.getMembershipLdapAttribute(), usersDn, ldapConfig.getRdnLdapAttribute()); - if (userDns == null) { + if (userDns == null || userDns.size() <= firstResult) { return Collections.emptyList(); } - if (userDns.size() <= firstResult) { - return Collections.emptyList(); - } - - List dns = new ArrayList<>(userDns); - int max = Math.min(dns.size(), firstResult + maxResults); - dns = dns.subList(firstResult, max); - - // If usernameAttrName is same like DN, we can just retrieve usernames from DNs - List usernames = new LinkedList<>(); - if (ldapConfig.getUsernameLdapAttribute().equals(ldapConfig.getRdnLdapAttribute())) { - for (LDAPDn userDn : dns) { - String username = userDn.getFirstRdn().getAttrValue(ldapConfig.getRdnLdapAttribute()); - usernames.add(username); - } - } else { - LDAPQuery query = LDAPUtils.createQueryForUserSearch(ldapProvider, realm); - LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); - List orSubconditions = new ArrayList<>(); - for (LDAPDn userDn : dns) { - String firstRdnAttrValue = userDn.getFirstRdn().getAttrValue(ldapConfig.getRdnLdapAttribute()); - if (firstRdnAttrValue != null) { - Condition condition = conditionsBuilder.equal(ldapConfig.getRdnLdapAttribute(), firstRdnAttrValue, EscapeStrategy.DEFAULT); - orSubconditions.add(condition); - } - } - Condition orCondition = conditionsBuilder.orCondition(orSubconditions.toArray(new Condition[] {})); - query.addWhereCondition(orCondition); - List ldapUsers = query.getResultList(); - for (LDAPObject ldapUser : ldapUsers) { - if (dns.contains(ldapUser.getDn())) { - String username = LDAPUtils.getUsername(ldapUser, ldapConfig); - usernames.add(username); - } - } - } - - // We have dns of users, who are members of our group. Load them now - return ldapProvider.loadUsersByUsernames(usernames, realm); + return ldapProvider.loadUsersByDNs(realm, userDns, firstResult, maxResults) + .collect(Collectors.toList()); } - }, - /** * Used if LDAP role has it's members declared in form of pure user uids. For example ( "memberUid: john" ) */ @@ -150,40 +105,11 @@ public enum MembershipType { return Collections.emptyList(); } - List uids = new ArrayList<>(memberUids); - int max = Math.min(memberUids.size(), firstResult + maxResults); - uids = uids.subList(firstResult, max); - String membershipUserAttrName = groupMapper.getConfig().getMembershipUserLdapAttribute(ldapConfig); - List usernames; - if (membershipUserAttrName.equals(ldapConfig.getUsernameLdapAttribute())) { - usernames = uids; // Optimized version. No need to - } else { - usernames = new LinkedList<>(); - - LDAPQuery query = LDAPUtils.createQueryForUserSearch(ldapProvider, realm); - LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); - - Condition[] orSubconditions = new Condition[uids.size()]; - int index = 0; - for (String memberUid : uids) { - Condition condition = conditionsBuilder.equal(membershipUserAttrName, memberUid, EscapeStrategy.DEFAULT); - orSubconditions[index] = condition; - index++; - } - Condition orCondition = conditionsBuilder.orCondition(orSubconditions); - query.addWhereCondition(orCondition); - List ldapUsers = query.getResultList(); - for (LDAPObject ldapUser : ldapUsers) { - String username = LDAPUtils.getUsername(ldapUser, ldapConfig); - usernames.add(username); - } - } - - return groupMapper.getLdapProvider().loadUsersByUsernames(usernames, realm); + return ldapProvider.loadUsersByUniqueAttribute(realm, membershipUserAttrName, memberUids, firstResult, maxResults) + .collect(Collectors.toList()); } - }; public abstract Set getLDAPSubgroups(CommonLDAPGroupMapper groupMapper, LDAPObject ldapGroup); diff --git a/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java b/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java index 11528a1ff8b..9dfd1478b73 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java +++ b/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java @@ -79,6 +79,8 @@ public class LDAPConstants { public static final String READ_TIMEOUT = "readTimeout"; // Could be discovered by rootDse supportedControl: 1.2.840.113556.1.4.319 public static final String PAGINATION = "pagination"; + public static final String MAX_CONDITIONS = "maxConditions"; + public static final int DEFAULT_MAX_CONDITIONS = 64; public static final String EDIT_MODE = "editMode"; diff --git a/server-spi-private/src/main/java/org/keycloak/utils/StreamsUtil.java b/server-spi-private/src/main/java/org/keycloak/utils/StreamsUtil.java index f216b435530..96eeeeeb524 100644 --- a/server-spi-private/src/main/java/org/keycloak/utils/StreamsUtil.java +++ b/server-spi-private/src/main/java/org/keycloak/utils/StreamsUtil.java @@ -17,10 +17,14 @@ package org.keycloak.utils; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; import java.util.Iterator; import java.util.Set; +import java.util.Spliterator; import java.util.Spliterators; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Stream; @@ -87,4 +91,55 @@ public class StreamsUtil { Set seen = new HashSet<>(); return t -> seen.add(keyExtractor.apply(t)); } + + /** + * A Java stream utility that splits a stream into chunks of a fixed size. Last chunk in + * the stream might be smaller than the desired size. Ordering guarantees + * depend on underlying stream. + * + * @param The type of the stream + * @param originalStream The original stream + * @param chunkSize The chunk size + * @return The stream in chunks + */ + public static Stream> chunkedStream(Stream originalStream, int chunkSize) { + Spliterator source = originalStream.spliterator(); + return StreamSupport.stream(new Spliterator>() { + final ArrayList buf = new ArrayList<>(); + + @Override + public boolean tryAdvance(Consumer> action) { + while (buf.size() < chunkSize) { + if (!source.tryAdvance(buf::add)) { + if (!buf.isEmpty()) { + action.accept((Collection) buf.clone()); + buf.clear(); + return true; + } else { + return false; + } + } + } + action.accept((Collection) buf.clone()); + buf.clear(); + return true; + } + + @Override + public Spliterator> trySplit() { + return null; + } + + @Override + public long estimateSize() { + long sourceSize = source.estimateSize(); + return sourceSize / chunkSize + (sourceSize % chunkSize != 0? 1 : 0); + } + + @Override + public int characteristics() { + return NONNULL | ORDERED; + } + }, false); + } } 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 0f105100269..47cd2dc3a1e 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 @@ -721,10 +721,17 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { if (ldapConfig.isActiveDirectory() || LDAPConstants.VENDOR_RHDS.equals(ldapConfig.getVendor())) { return; } + ctx.getLdapModel().getConfig().putSingle(LDAPConstants.MAX_CONDITIONS, "15"); // create big grups that use ranged search String descriptionAttrName = getGroupDescriptionLDAPAttrName(ctx.getLdapProvider()); LDAPObject bigGroup = LDAPTestUtils.createLDAPGroup(session, appRealm, ctx.getLdapModel(), "biggroup", descriptionAttrName, "biggroup - description"); + // create a non-exitent group member first to check pagination is OK + LDAPDn nonExistentDn = LDAPDn.fromString(ctx.getLdapProvider().getLdapIdentityStore().getConfig().getUsersDn()); + nonExistentDn.addFirst(ctx.getLdapProvider().getLdapIdentityStore().getConfig().getRdnLdapAttribute(), "nonexistent"); + LDAPObject nonExistentLdapUser = new LDAPObject(); + nonExistentLdapUser.setDn(nonExistentDn); + LDAPUtils.addMember(ctx.getLdapProvider(), MembershipType.DN, LDAPConstants.MEMBER, "not-used", bigGroup, nonExistentLdapUser); // create the users to use range search and add them to the group for (int i = 0; i < membersToTest; i++) { String username = String.format("user%02d", i); @@ -759,6 +766,18 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { for (int i = 0; i < membersToTest; i++) { Assert.assertTrue("Group contains user " + i, usernames.contains(String.format("user%02d", i))); } + // check group members are paginated OK using page size 10 + usernames.clear(); + for (int i = 0; i < membersToTest; i += 10) { + groupMembers = session.users().getGroupMembersStream(appRealm, kcBigGroup, i, 10) + .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()); + } + for (int i = 0; i < membersToTest; i++) { + Assert.assertTrue("Group contains user after pagination " + i, usernames.contains(String.format("user%02d", i))); + } + ctx.getLdapModel().getConfig().remove(LDAPConstants.MAX_CONDITIONS); }); }