From bb2f59df87572d28d2b7986a602ee25d9667d9a8 Mon Sep 17 00:00:00 2001 From: Bernd Bohmann Date: Wed, 20 Sep 2023 17:20:43 +0200 Subject: [PATCH] Calling getTopLevelGroups is slow inside GroupLDAPStorageMapper#getLDAPGroupMappingsConverted (#8430) Closes #14820 --------- Co-authored-by: Michal Hajas --- .../group/GroupLDAPStorageMapper.java | 53 ++++---- .../group/GroupLDAPStorageMapperFactory.java | 2 +- .../cache/infinispan/RealmCacheManager.java | 5 + .../cache/infinispan/RealmCacheSession.java | 36 +++++ .../infinispan/entities/GroupNameQuery.java | 49 +++++++ .../infinispan/events/GroupMovedEvent.java | 1 + .../infinispan/events/GroupRemovedEvent.java | 1 + .../infinispan/events/GroupUpdatedEvent.java | 2 +- .../infinispan/stream/InGroupPredicate.java | 83 ++++++++++++ .../InGroupPredicateWFExternalizer.java | 27 ++++ ...ildfly.clustering.marshalling.Externalizer | 1 + .../keycloak/models/jpa/JpaRealmProvider.java | 14 ++ .../models/jpa/entities/GroupEntity.java | 3 +- .../keycloak/storage/GroupStorageManager.java | 5 + .../models/map/group/MapGroupProvider.java | 22 ++++ .../models/utils/KeycloakModelUtils.java | 38 ++++++ .../storage/group/GroupLookupProvider.java | 15 ++- .../HardcodedGroupStorageProvider.java | 6 + .../federation/ldap/LDAPGroupMapperTest.java | 26 ++-- .../testsuite/model/group/GroupModelTest.java | 124 ++++++++++++++++++ 20 files changed, 471 insertions(+), 42 deletions(-) create mode 100644 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/GroupNameQuery.java create mode 100644 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/InGroupPredicate.java create mode 100644 model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/wildfly/InGroupPredicateWFExternalizer.java 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 b0748bb18f..1d78019a64 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 @@ -54,7 +54,6 @@ import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -338,7 +337,9 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements private void dropNonExistingKcGroups(RealmModel realm, SynchronizationResult syncResult, Set visitedGroupIds) { // Remove keycloak groups, which don't exist in LDAP - getAllKcGroups(realm) + GroupModel parent = getKcGroupsPathGroup(realm); + + getAllKcGroups(realm, parent) .filter(kcGroup -> !visitedGroupIds.contains(kcGroup.getId())) .forEach(kcGroup -> { logger.debugf("Removing Keycloak group '%s', which doesn't exist in LDAP", kcGroup.getName()); @@ -361,22 +362,22 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements } - protected GroupModel findKcGroupByLDAPGroup(RealmModel realm, LDAPObject ldapGroup) { + protected GroupModel findKcGroupByLDAPGroup(RealmModel realm, GroupModel parent, LDAPObject ldapGroup) { String groupNameAttr = config.getGroupNameLdapAttribute(); String groupName = ldapGroup.getAttributeAsString(groupNameAttr); if (config.isPreserveGroupsInheritance()) { // Override if better effectivity or different algorithm is needed - return getAllKcGroups(realm) + return getAllKcGroups(realm, parent) .filter(group -> Objects.equals(group.getName(), groupName)).findFirst().orElse(null); } else { // Without preserved inheritance, it's always at groups path - return KeycloakModelUtils.findGroupByPath(realm, getKcGroupPathFromLDAPGroupName(groupName)); + return session.groups().getGroupByName(realm, parent, groupName); } } - protected GroupModel findKcGroupOrSyncFromLDAP(RealmModel realm, LDAPObject ldapGroup, UserModel user) { - GroupModel kcGroup = findKcGroupByLDAPGroup(realm, ldapGroup); + protected GroupModel findKcGroupOrSyncFromLDAP(RealmModel realm, GroupModel parent, LDAPObject ldapGroup, UserModel user) { + GroupModel kcGroup = findKcGroupByLDAPGroup(realm, parent, ldapGroup); if (kcGroup == null) { @@ -385,7 +386,7 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements // Better to sync all groups from LDAP with preserved inheritance if (!syncFromLDAPPerformedInThisTransaction) { syncDataFromFederationProviderToKeycloak(realm); - kcGroup = findKcGroupByLDAPGroup(realm, ldapGroup); + kcGroup = findKcGroupByLDAPGroup(realm, parent, ldapGroup); } } else { String groupNameAttr = config.getGroupNameLdapAttribute(); @@ -656,14 +657,16 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements if (mode == LDAPGroupMapperMode.IMPORT && isCreate) { List ldapGroups = getLDAPGroupMappings(ldapUser); + if (!ldapGroups.isEmpty()) { + GroupModel parent = getKcGroupsPathGroup(realm); + // Import role mappings from LDAP into Keycloak DB + for (LDAPObject ldapGroup : ldapGroups) { - // Import role mappings from LDAP into Keycloak DB - for (LDAPObject ldapGroup : ldapGroups) { - - GroupModel kcGroup = findKcGroupOrSyncFromLDAP(realm, ldapGroup, user); - if (kcGroup != null) { - logger.debugf("User '%s' joins group '%s' during import from LDAP", user.getUsername(), kcGroup.getName()); - user.joinGroup(kcGroup); + GroupModel kcGroup = findKcGroupOrSyncFromLDAP(realm, parent, ldapGroup, user); + if (kcGroup != null) { + logger.debugf("User '%s' joins group '%s' during import from LDAP", user.getUsername(), kcGroup.getName()); + user.joinGroup(kcGroup); + } } } } @@ -760,13 +763,17 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements } List ldapGroups = getLDAPGroupMappings(ldapUser); + if (!ldapGroups.isEmpty()) { + GroupModel parent = getKcGroupsPathGroup(realm); - cachedLDAPGroupMappings = ldapGroups.stream() - .map(ldapGroup -> findKcGroupOrSyncFromLDAP(realm, ldapGroup, this)) - .filter(Objects::nonNull) - .collect(Collectors.toSet()); + cachedLDAPGroupMappings = ldapGroups.stream() + .map(ldapGroup -> findKcGroupOrSyncFromLDAP(realm, parent, ldapGroup, this)) + .filter(Objects::nonNull) + .collect(Collectors.toSet()); - return cachedLDAPGroupMappings.stream(); + return cachedLDAPGroupMappings.stream(); + } + return Stream.empty(); } } @@ -783,7 +790,7 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements * Provides KC group defined as groups path or null (top-level group) if corresponding group is not available. */ protected GroupModel getKcGroupsPathGroup(RealmModel realm) { - return config.isTopLevelGroupsPath() ? null : KeycloakModelUtils.findGroupByPath(realm, config.getGroupsPath()); + return config.isTopLevelGroupsPath() ? null : KeycloakModelUtils.findGroupByPath(session.groups(), realm, config.getGroupsPath()); } /** @@ -813,9 +820,7 @@ public class GroupLDAPStorageMapper extends AbstractLDAPStorageMapper implements /** * Provides a stream of all KC groups (with their sub groups) from groups path configured by the "Groups Path" configuration property. */ - protected Stream getAllKcGroups(RealmModel realm) { - GroupModel topParentGroup = getKcGroupsPathGroup(realm); - + protected Stream getAllKcGroups(RealmModel realm, GroupModel topParentGroup) { Stream allGroups = realm.getGroupsStream(); if (topParentGroup == null) return allGroups; diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java index dfa6251ca7..bf73d6c124 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapperFactory.java @@ -304,7 +304,7 @@ public class GroupLDAPStorageMapperFactory extends AbstractLDAPStorageMapperFact LDAPUtils.validateCustomLdapFilter(config.getConfig().getFirst(GroupMapperConfig.GROUPS_LDAP_FILTER)); String group = new GroupMapperConfig(config).getGroupsPath(); - if (!GroupMapperConfig.DEFAULT_LDAP_GROUPS_PATH.equals(group) && KeycloakModelUtils.findGroupByPath(realm, group) == null) { + if (!GroupMapperConfig.DEFAULT_LDAP_GROUPS_PATH.equals(group) && KeycloakModelUtils.findGroupByPath(session.groups(), realm, group) == null) { throw new ComponentValidationException("ldapErrorMissingGroupsPathGroup"); } } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheManager.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheManager.java index 311e1a3dc0..aeb4077318 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheManager.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheManager.java @@ -26,6 +26,7 @@ import org.keycloak.models.cache.infinispan.events.RealmCacheInvalidationEvent; import org.keycloak.models.cache.infinispan.stream.GroupListPredicate; import org.keycloak.models.cache.infinispan.stream.HasRolePredicate; import org.keycloak.models.cache.infinispan.stream.InClientPredicate; +import org.keycloak.models.cache.infinispan.stream.InGroupPredicate; import org.keycloak.models.cache.infinispan.stream.InRealmPredicate; import java.util.Set; @@ -96,6 +97,10 @@ public class RealmCacheManager extends CacheManager { addInvalidations(GroupListPredicate.create().realm(realmId), invalidations); } + public void groupNameInvalidations(String groupId, Set invalidations) { + addInvalidations(InGroupPredicate.create().group(groupId), invalidations); + } + public void clientAdded(String realmId, String clientUUID, String clientId, Set invalidations) { invalidations.add(RealmCacheSession.getRealmClientsQueryCacheKey(realmId)); } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java index 18461f43e4..0ac87e02fe 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java @@ -277,6 +277,7 @@ public class RealmCacheSession implements CacheRealmProvider { private void invalidateGroup(String id, String realmId, boolean invalidateQueries) { invalidateGroup(id); + cache.groupNameInvalidations(id, invalidations); if (invalidateQueries) { cache.groupQueriesInvalidations(realmId, invalidations); } @@ -564,6 +565,14 @@ public class RealmCacheSession implements CacheRealmProvider { return realm + ".top.groups"; } + static String getGroupByNameCacheKey(String realm, String parentId, String name) { + if (parentId != null) { + return realm + ".group." + parentId + "." + name; + } else { + return realm + ".group.top." + name; + } + } + static String getRolesCacheKey(String container) { return container + ROLES_QUERY_SUFFIX; } @@ -886,6 +895,33 @@ public class RealmCacheSession implements CacheRealmProvider { return adapter; } + @Override + public GroupModel getGroupByName(RealmModel realm, GroupModel parent, String name) { + String cacheKey = getGroupByNameCacheKey(realm.getId(), parent != null? parent.getId(): null, name); + GroupNameQuery query = cache.get(cacheKey, GroupNameQuery.class); + if (query != null) { + logger.tracev("Group by name cache hit: {0}", name); + } + if (query == null) { + Long loaded = cache.getCurrentRevision(cacheKey); + GroupModel model = getGroupDelegate().getGroupByName(realm, parent, name); + if (model == null) return null; + if (invalidations.contains(model.getId())) return model; + query = new GroupNameQuery(loaded, cacheKey, model.getId(), realm); + cache.addRevisioned(query, startupRevision); + return model; + } else if (invalidations.contains(cacheKey)) { + return getGroupDelegate().getGroupByName(realm, parent, name); + } else { + String groupId = query.getGroupId(); + if (invalidations.contains(groupId)) { + return getGroupDelegate().getGroupByName(realm, parent, name); + } + return getGroupById(realm, groupId); + } + + } + @Override public void moveGroup(RealmModel realm, GroupModel group, GroupModel toParent) { invalidateGroup(group.getId(), realm.getId(), true); diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/GroupNameQuery.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/GroupNameQuery.java new file mode 100644 index 0000000000..f40035634b --- /dev/null +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/GroupNameQuery.java @@ -0,0 +1,49 @@ +/* + * Copyright 2022 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.models.cache.infinispan.entities; + +import org.keycloak.models.RealmModel; + +public class GroupNameQuery extends AbstractRevisioned implements InRealm { + private final String realm; + private final String realmName; + private final String groupId; + + public GroupNameQuery(Long revisioned, String id, String groupId, RealmModel realm) { + super(revisioned, id); + this.realm = realm.getId(); + this.realmName = realm.getName(); + this.groupId = groupId; + } + + public String getGroupId() { + return groupId; + } + + public String getRealm() { + return realm; + } + + @Override + public String toString() { + return "GroupNameQuery{" + + "id='" + getId() + "'" + + "realmName='" + realmName + '\'' + + '}'; + } +} diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupMovedEvent.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupMovedEvent.java index 8ebc5c555c..f2e1ac10fa 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupMovedEvent.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupMovedEvent.java @@ -62,6 +62,7 @@ public class GroupMovedEvent extends InvalidationEvent implements RealmCacheInva @Override public void addInvalidations(RealmCacheManager realmCache, Set invalidations) { realmCache.groupQueriesInvalidations(realmId, invalidations); + realmCache.groupNameInvalidations(groupId, invalidations); if (newParentId != null) { invalidations.add(newParentId); } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupRemovedEvent.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupRemovedEvent.java index b25cb2a24f..76e25ff154 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupRemovedEvent.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupRemovedEvent.java @@ -60,6 +60,7 @@ public class GroupRemovedEvent extends InvalidationEvent implements RealmCacheIn @Override public void addInvalidations(RealmCacheManager realmCache, Set invalidations) { realmCache.groupQueriesInvalidations(realmId, invalidations); + realmCache.groupNameInvalidations(groupId, invalidations); if (parentId != null) { invalidations.add(parentId); } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupUpdatedEvent.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupUpdatedEvent.java index a7ec69e27c..eced20a473 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupUpdatedEvent.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/events/GroupUpdatedEvent.java @@ -54,7 +54,7 @@ public class GroupUpdatedEvent extends InvalidationEvent implements RealmCacheIn @Override public void addInvalidations(RealmCacheManager realmCache, Set invalidations) { - // Nothing. ID already invalidated + realmCache.groupNameInvalidations(groupId, invalidations); } public static class ExternalizerImpl implements Externalizer { diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/InGroupPredicate.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/InGroupPredicate.java new file mode 100644 index 0000000000..eae8f18442 --- /dev/null +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/InGroupPredicate.java @@ -0,0 +1,83 @@ +/* + * Copyright 2022 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.models.cache.infinispan.stream; + +import org.keycloak.models.cache.infinispan.entities.GroupNameQuery; +import org.keycloak.models.cache.infinispan.entities.Revisioned; + +import java.io.IOException; +import java.io.ObjectInput; +import java.io.ObjectOutput; +import java.io.Serializable; +import java.util.Map; +import java.util.function.Predicate; +import org.infinispan.commons.marshall.Externalizer; +import org.infinispan.commons.marshall.MarshallUtil; +import org.infinispan.commons.marshall.SerializeWith; + +@SerializeWith(InGroupPredicate.ExternalizerImpl.class) +public class InGroupPredicate implements Predicate>, Serializable { + private String group; + + public static InGroupPredicate create() { + return new InGroupPredicate(); + } + + public InGroupPredicate group(String id) { + group = id; + return this; + } + + @Override + public boolean test(Map.Entry entry) { + Object value = entry.getValue(); + if (value == null) return false; + if (!(value instanceof GroupNameQuery)) return false; + + return group.equals(((GroupNameQuery)value).getGroupId()); + } + + public static class ExternalizerImpl implements Externalizer { + + private static final int VERSION_1 = 1; + + @Override + public void writeObject(ObjectOutput output, InGroupPredicate obj) throws IOException { + output.writeByte(VERSION_1); + + MarshallUtil.marshallString(obj.group, output); + } + + @Override + public InGroupPredicate readObject(ObjectInput input) throws IOException, ClassNotFoundException { + switch (input.readByte()) { + case VERSION_1: + return readObjectVersion1(input); + default: + throw new IOException("Unknown version"); + } + } + + public InGroupPredicate readObjectVersion1(ObjectInput input) throws IOException, ClassNotFoundException { + InGroupPredicate res = new InGroupPredicate(); + res.group = MarshallUtil.unmarshallString(input); + + return res; + } + } +} \ No newline at end of file diff --git a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/wildfly/InGroupPredicateWFExternalizer.java b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/wildfly/InGroupPredicateWFExternalizer.java new file mode 100644 index 0000000000..019f658822 --- /dev/null +++ b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/wildfly/InGroupPredicateWFExternalizer.java @@ -0,0 +1,27 @@ +/* + * Copyright 2022 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.models.sessions.infinispan.entities.wildfly; + +import org.keycloak.models.cache.infinispan.stream.InGroupPredicate; + +public class InGroupPredicateWFExternalizer extends InfinispanExternalizerAdapter { + + public InGroupPredicateWFExternalizer() { + super(InGroupPredicate.class, new InGroupPredicate.ExternalizerImpl()); + } +} diff --git a/model/infinispan/src/main/resources/META-INF/services/org.wildfly.clustering.marshalling.Externalizer b/model/infinispan/src/main/resources/META-INF/services/org.wildfly.clustering.marshalling.Externalizer index 644eca4f6a..a1fa93ccd2 100644 --- a/model/infinispan/src/main/resources/META-INF/services/org.wildfly.clustering.marshalling.Externalizer +++ b/model/infinispan/src/main/resources/META-INF/services/org.wildfly.clustering.marshalling.Externalizer @@ -55,6 +55,7 @@ org.keycloak.models.sessions.infinispan.entities.wildfly.LockEntryWFExternalizer org.keycloak.models.sessions.infinispan.entities.wildfly.ActionTokenValueEntityWFExternalizer org.keycloak.models.sessions.infinispan.entities.wildfly.RoleAddedEventWFExternalizer org.keycloak.models.sessions.infinispan.entities.wildfly.InClientPredicateWFExternalizer +org.keycloak.models.sessions.infinispan.entities.wildfly.InGroupPredicateWFExternalizer org.keycloak.models.sessions.infinispan.entities.wildfly.UserFullInvalidationEventWFExternalizer org.keycloak.models.sessions.infinispan.entities.wildfly.ClientRemovedSessionEventWFExternalizer org.keycloak.models.sessions.infinispan.entities.wildfly.SessionPredicateWFExternalizer diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java index 8b25b396fc..56d32acf0b 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java @@ -435,6 +435,20 @@ public class JpaRealmProvider implements RealmProvider, ClientProvider, ClientSc return adapter; } + @Override + public GroupModel getGroupByName(RealmModel realm, GroupModel parent, String name) { + TypedQuery query = em.createNamedQuery("getGroupIdByNameAndParent", String.class); + query.setParameter("name", name); + query.setParameter("realm", realm.getId()); + query.setParameter("parent", parent != null ? parent.getId() : GroupEntity.TOP_PARENT_ID); + List entities = query.getResultList(); + if (entities.isEmpty()) return null; + if (entities.size() > 1) throw new IllegalStateException("Should not be more than one Group with same name"); + String id = query.getResultList().get(0); + + return session.groups().getGroupById(realm, id); + } + @Override public void moveGroup(RealmModel realm, GroupModel group, GroupModel toParent) { if (toParent != null && group.getId().equals(toParent.getId())) { diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java index bca70dd020..59f81266ac 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/GroupEntity.java @@ -37,7 +37,8 @@ import java.util.LinkedList; @NamedQuery(name="getGroupCountByNameContainingFromIdList", query="select count(u) from GroupEntity u where u.realm = :realm and lower(u.name) like lower(concat('%',:search,'%')) and u.id in :ids"), @NamedQuery(name="getTopLevelGroupIds", query="select u.id from GroupEntity u where u.parentId = :parent and u.realm = :realm order by u.name ASC"), @NamedQuery(name="getGroupCount", query="select count(u) from GroupEntity u where u.realm = :realm"), - @NamedQuery(name="getTopLevelGroupCount", query="select count(u) from GroupEntity u where u.realm = :realm and u.parentId = :parent") + @NamedQuery(name="getTopLevelGroupCount", query="select count(u) from GroupEntity u where u.realm = :realm and u.parentId = :parent"), + @NamedQuery(name="getGroupIdByNameAndParent", query="select u.id from GroupEntity u where u.realm = :realm and u.parentId = :parent and u.name = :name") }) @Entity @Table(name="KEYCLOAK_GROUP", diff --git a/model/legacy-private/src/main/java/org/keycloak/storage/GroupStorageManager.java b/model/legacy-private/src/main/java/org/keycloak/storage/GroupStorageManager.java index cd7ddf6eb9..98d9d75570 100644 --- a/model/legacy-private/src/main/java/org/keycloak/storage/GroupStorageManager.java +++ b/model/legacy-private/src/main/java/org/keycloak/storage/GroupStorageManager.java @@ -55,6 +55,11 @@ public class GroupStorageManager extends AbstractStorageManager searchGroupsByAttributes(RealmModel realm, Map attributes, Integer firstResult, Integer maxResults) { Stream local = localStorage().searchGroupsByAttributes(realm, attributes, firstResult, maxResults); diff --git a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java index 7bf9c7db7a..056a7f4b70 100644 --- a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java @@ -93,6 +93,28 @@ public class MapGroupProvider implements GroupProvider { : entityToAdapterFunc(realm).apply(entity); } + @Override + public GroupModel getGroupByName(RealmModel realm, GroupModel parent, String name) { + if (name == null) { + return null; + } + + LOG.tracef("getGroupByName(%s, %s)%s", realm, name, getShortStackTrace()); + + DefaultModelCriteria mcb = criteria(); + mcb = mcb.compare(SearchableFields.NAME, Operator.EQ, name) + .compare(SearchableFields.REALM_ID, Operator.EQ, realm.getId()); + if (parent != null) { + mcb = mcb.compare(SearchableFields.PARENT_ID, Operator.EQ, parent.getId()); + } else { + mcb = mcb.compare(SearchableFields.PARENT_ID, Operator.NOT_EXISTS); + } + QueryParameters queryParameters = withCriteria(mcb); + String groupId = storeWithRealm(realm).read(queryParameters).findFirst().map(MapGroupEntity::getId) + .orElse(null); + return groupId == null ? null : session.groups().getGroupById(realm, groupId); + } + @Override public Stream getGroupsStream(RealmModel realm) { return getGroupsStreamInternal(realm, null, null); diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index 96dfd64fdf..072d17da2c 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -36,6 +36,7 @@ import org.keycloak.models.ClientScopeModel; import org.keycloak.models.ClientSecretConstants; import org.keycloak.models.Constants; import org.keycloak.models.GroupModel; +import org.keycloak.models.GroupProvider; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.KeycloakContext; import org.keycloak.models.KeycloakSession; @@ -763,6 +764,43 @@ public final class KeycloakModelUtils { return segments; } + public static GroupModel findGroupByPath(GroupProvider groupProvider, RealmModel realm, String path) { + if (path == null) { + return null; + } + if (path.startsWith(GROUP_PATH_SEPARATOR)) { + path = path.substring(1); + } + if (path.endsWith(GROUP_PATH_SEPARATOR)) { + path = path.substring(0, path.length() - 1); + } + String[] split = path.split(GROUP_PATH_SEPARATOR); + if (split.length == 0) return null; + return getGroupModel(groupProvider, realm, null, split, 0); + } + + private static GroupModel getGroupModel(GroupProvider groupProvider, RealmModel realm, GroupModel parent, String[] split, int index) { + StringBuilder nameBuilder = new StringBuilder(); + for (int i = index; i < split.length; i++) { + nameBuilder.append(split[i]); + GroupModel group = groupProvider.getGroupByName(realm, parent, nameBuilder.toString()); + if (group != null) { + if (i < split.length-1) { + return getGroupModel(groupProvider, realm, group, split, i+1); + } else { + return group; + } + } + nameBuilder.append(GROUP_PATH_SEPARATOR); + } + return null; + } + + /** + * + * @deprecated please use {@link #findGroupByPath(GroupProvider, RealmModel, String)} instead + */ + @Deprecated public static GroupModel findGroupByPath(RealmModel realm, String path) { if (path == null) { return null; diff --git a/server-spi/src/main/java/org/keycloak/storage/group/GroupLookupProvider.java b/server-spi/src/main/java/org/keycloak/storage/group/GroupLookupProvider.java index 73c515f8f8..e95642713e 100644 --- a/server-spi/src/main/java/org/keycloak/storage/group/GroupLookupProvider.java +++ b/server-spi/src/main/java/org/keycloak/storage/group/GroupLookupProvider.java @@ -19,9 +19,7 @@ package org.keycloak.storage.group; import org.keycloak.models.GroupModel; import org.keycloak.models.RealmModel; -import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import java.util.stream.Stream; public interface GroupLookupProvider { @@ -35,6 +33,19 @@ public interface GroupLookupProvider { */ GroupModel getGroupById(RealmModel realm, String id); + /** + * Returns a group from the given realm with the corresponding name and parent + * + * @param realm Realm. + * @param parent parent Group. If {@code null} top level groups are searched + * @param name name. + * @return GroupModel with the corresponding name. + */ + default GroupModel getGroupByName(RealmModel realm, GroupModel parent, String name) { + return (parent == null ? realm.getTopLevelGroupsStream() : parent.getSubGroupsStream()) + .filter(groupModel -> groupModel.getName().equals(name)).findFirst().orElse(null); + } + /** * Returns the group hierarchy with the given string in name for the given realm. * diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/HardcodedGroupStorageProvider.java b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/HardcodedGroupStorageProvider.java index ceef6ce6fa..c1470d6310 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/HardcodedGroupStorageProvider.java +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/HardcodedGroupStorageProvider.java @@ -52,6 +52,12 @@ public class HardcodedGroupStorageProvider implements GroupStorageProvider { return null; } + @Override + public GroupModel getGroupByName(RealmModel realm, GroupModel parent, String name) { + if (this.groupName.equals(name)) return new HardcodedGroupAdapter(realm); + return null; + } + @Override public Stream searchForGroupByNameStream(RealmModel realm, String search, Boolean exact, Integer firstResult, Integer maxResults) { if (Boolean.parseBoolean(component.getConfig().getFirst(HardcodedGroupStorageProviderFactory.DELAYED_SEARCH))) try { 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 ec40fae48a..6b25880a85 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 @@ -94,31 +94,31 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { // 1 - Grant some groups in LDAP // This group should already exists as it was imported from LDAP - GroupModel group1 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1"); + GroupModel group1 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "/group1"); john.joinGroup(group1); // This group should already exists as it was imported from LDAP - GroupModel group11 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group11"); + GroupModel group11 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "/group1/group11"); mary.joinGroup(group11); // This group should already exists as it was imported from LDAP - GroupModel group12 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group12"); + GroupModel group12 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "/group1/group12"); john.joinGroup(group12); mary.joinGroup(group12); // This group should already exists as it was imported from LDAP - GroupModel groupWithSlashesInName = KeycloakModelUtils.findGroupByPath(appRealm, "Team 2016/2017"); + GroupModel groupWithSlashesInName = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "Team 2016/2017"); john.joinGroup(groupWithSlashesInName); mary.joinGroup(groupWithSlashesInName); // This group should already exists as it was imported from LDAP - GroupModel groupChildWithSlashesInName = KeycloakModelUtils.findGroupByPath(appRealm, "defaultGroup1/Team Child 2018/2019"); + GroupModel groupChildWithSlashesInName = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "defaultGroup1/Team Child 2018/2019"); john.joinGroup(groupChildWithSlashesInName); mary.joinGroup(groupChildWithSlashesInName); - Assert.assertEquals("Team SubChild 2020/2021", KeycloakModelUtils.findGroupByPath(appRealm, "defaultGroup1/Team Child 2018/2019/Team SubChild 2020/2021").getName()); - Assert.assertEquals("defaultGroup14", KeycloakModelUtils.findGroupByPath(appRealm, "defaultGroup13/Team SubChild 2022/2023/A/B/C/D/E/defaultGroup14").getName()); - Assert.assertEquals("Team SubChild 2026/2027", KeycloakModelUtils.findGroupByPath(appRealm, "Team Root 2024/2025/A/B/C/D/defaultGroup15/Team SubChild 2026/2027").getName()); + Assert.assertEquals("Team SubChild 2020/2021", KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "defaultGroup1/Team Child 2018/2019/Team SubChild 2020/2021").getName()); + Assert.assertEquals("defaultGroup14", KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "defaultGroup13/Team SubChild 2022/2023/A/B/C/D/E/defaultGroup14").getName()); + Assert.assertEquals("Team SubChild 2026/2027", KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "Team Root 2024/2025/A/B/C/D/defaultGroup15/Team SubChild 2026/2027").getName()); }); @@ -146,11 +146,11 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { LDAPTestContext ctx = LDAPTestContext.init(session); RealmModel appRealm = ctx.getRealm(); - GroupModel group1 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1"); - GroupModel group11 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group11"); - GroupModel group12 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group12"); - GroupModel groupTeam20162017 = KeycloakModelUtils.findGroupByPath(appRealm, "Team 2016/2017"); - GroupModel groupTeamChild20182019 = KeycloakModelUtils.findGroupByPath(appRealm, "defaultGroup1/Team Child 2018/2019"); + GroupModel group1 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "/group1"); + GroupModel group11 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "/group1/group11"); + GroupModel group12 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "/group1/group12"); + GroupModel groupTeam20162017 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "Team 2016/2017"); + GroupModel groupTeamChild20182019 = KeycloakModelUtils.findGroupByPath(session.groups(), appRealm, "defaultGroup1/Team Child 2018/2019"); UserModel john = session.users().getUserByUsername(appRealm, "johnkeycloak"); UserModel mary = session.users().getUserByUsername(appRealm, "marykeycloak"); diff --git a/testsuite/model/src/test/java/org/keycloak/testsuite/model/group/GroupModelTest.java b/testsuite/model/src/test/java/org/keycloak/testsuite/model/group/GroupModelTest.java index bcaa8795fa..7e8dd1feba 100644 --- a/testsuite/model/src/test/java/org/keycloak/testsuite/model/group/GroupModelTest.java +++ b/testsuite/model/src/test/java/org/keycloak/testsuite/model/group/GroupModelTest.java @@ -22,6 +22,7 @@ import org.keycloak.models.Constants; import org.keycloak.models.GroupModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; +import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.testsuite.model.KeycloakModelTest; import java.util.Arrays; @@ -29,9 +30,11 @@ import java.util.Collections; import java.util.List; import java.util.stream.Collectors; +import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; public class GroupModelTest extends KeycloakModelTest { @@ -99,4 +102,125 @@ public class GroupModelTest extends KeycloakModelTest { }); } + @Test + public void testGroupByName() { + String subGroupId1 = withRealm(realmId, (session, realm) -> { + GroupModel group = session.groups().createGroup(realm, "parent-1"); + GroupModel subGroup = session.groups().createGroup(realm, "sub-group-1", group); + return subGroup.getId(); + }); + + String subGroupId2 = withRealm(realmId, (session, realm) -> { + GroupModel group = session.groups().createGroup(realm, "parent-2"); + GroupModel subGroup = session.groups().createGroup(realm, "sub-group-1", group); + return subGroup.getId(); + }); + withRealm(realmId, (session, realm) -> { + GroupModel group1 = session.groups().getGroupByName(realm, null,"parent-1"); + GroupModel group2 = session.groups().getGroupByName(realm, null,"parent-2"); + + GroupModel subGroup1 = session.groups().getGroupByName(realm, group1,"sub-group-1"); + GroupModel subGroup2 = session.groups().getGroupByName(realm, group2,"sub-group-1"); + + assertThat(subGroup1.getId(), equalTo(subGroupId1)); + assertThat(subGroup1.getName(), equalTo("sub-group-1")); + assertThat(subGroup2.getId(), equalTo(subGroupId2)); + assertThat(subGroup2.getName(), equalTo("sub-group-1")); + return null; + }); + } + + @Test + public void testConflictingNames() { + final String conflictingGroupName = "conflicting-group-name"; + + String parentGroupWithChildId = withRealm(realmId, (session, realm) -> { + GroupModel parentGroupWithChild = session.groups().createGroup(realm, "parent-1"); + GroupModel subGroup1 = session.groups().createGroup(realm, conflictingGroupName, parentGroupWithChild); + return parentGroupWithChild.getId(); + }); + + String parentGroupWithConflictingNameId = withRealm(realmId, (session, realm) -> session.groups().createGroup(realm, conflictingGroupName).getId()); + String parentGroupWithoutChildrenId = withRealm(realmId, (session, realm) -> session.groups().createGroup(realm, "parent-2").getId()); + + withRealm(realmId, (session, realm) -> { + GroupModel searchedGroup = session.groups().getGroupByName(realm, null, conflictingGroupName); + assertThat(searchedGroup, notNullValue()); + assertThat(searchedGroup.getId(), equalTo(parentGroupWithConflictingNameId)); + return null; + }); + + withRealm(realmId, (session, realm) -> { + GroupModel parentGroupWithChild = session.groups().getGroupById(realm, parentGroupWithChildId); + GroupModel searchedGroup = session.groups().getGroupByName(realm, parentGroupWithChild, conflictingGroupName); + assertThat(searchedGroup, notNullValue()); + assertThat(searchedGroup.getParentId(), equalTo(parentGroupWithChildId)); + return null; + }); + + withRealm(realmId, (session, realm) -> { + GroupModel parentGroupWithoutChildren = session.groups().getGroupById(realm, parentGroupWithoutChildrenId); + GroupModel searchedGroup = session.groups().getGroupByName(realm, parentGroupWithoutChildren, conflictingGroupName); + assertThat(searchedGroup, nullValue()); + return null; + }); + } + + @Test + public void testGroupByNameCacheInvalidation() { + String subGroupId1 = withRealm(realmId, (session, realm) -> { + GroupModel group = session.groups().createGroup(realm, "parent-1"); + GroupModel subGroup = session.groups().createGroup(realm, "sub-group-1", group); + return subGroup.getId(); + }); + + withRealm(realmId, (session, realm) -> { + GroupModel group1 = session.groups().getGroupByName(realm, null, "parent-1"); + GroupModel subGroup1 = session.groups().getGroupByName(realm, group1, "sub-group-1"); + assertThat(subGroup1.getId(), equalTo(subGroupId1)); + return null; + }); + + withRealm(realmId, (session, realm) -> { + GroupModel group1 = session.groups().getGroupByName(realm, null, "parent-1"); + GroupModel subGroup1 = session.groups().getGroupByName(realm, group1, "sub-group-1"); + session.groups().removeGroup(realm, subGroup1); + return null; + }); + + withRealm(realmId, (session, realm) -> { + GroupModel group1 = session.groups().getGroupByName(realm, null, "parent-1"); + GroupModel subGroup1 = session.groups().getGroupByName(realm, group1, "sub-group-1"); + assertThat(subGroup1, nullValue()); + return null; + }); + + } + @Test + public void testFindGroupByPath() { + String subGroupId1 = withRealm(realmId, (session, realm) -> { + GroupModel group = session.groups().createGroup(realm, "parent-1"); + GroupModel subGroup = session.groups().createGroup(realm, "sub-group-1", group); + return subGroup.getId(); + }); + + String subGroupIdWithSlash = withRealm(realmId, (session, realm) -> { + GroupModel group = session.groups().createGroup(realm, "parent-2"); + GroupModel subGroup = session.groups().createGroup(realm, "sub-group/1", group); + return subGroup.getId(); + }); + + withRealm(realmId, (session, realm) -> { + GroupModel group1 = KeycloakModelUtils.findGroupByPath(session.groups(), realm, "/parent-1"); + GroupModel group2 = KeycloakModelUtils.findGroupByPath(session.groups(), realm, "/parent-2"); + assertThat(group1.getName(), equalTo("parent-1")); + assertThat(group2.getName(), equalTo("parent-2")); + + GroupModel subGroup1 = KeycloakModelUtils.findGroupByPath(session.groups(), realm, "/parent-1/sub-group-1"); + GroupModel subGroup2 = KeycloakModelUtils.findGroupByPath(session.groups(), realm, "/parent-2/sub-group/1"); + assertThat(subGroup1.getId(), equalTo(subGroupId1)); + assertThat(subGroup2.getId(), equalTo(subGroupIdWithSlash)); + return null; + }); + } }