From cbf7f208fb59797d19da4dda0ba6b28494bfd3c8 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Mon, 8 Jul 2024 08:57:20 -0300 Subject: [PATCH] Avoid iterating and updating all group policies when removing groups (#31057) Closes #31056 Signed-off-by: Pedro Igor --- .../provider/group/GroupPolicyProvider.java | 6 +- .../group/GroupPolicyProviderFactory.java | 91 +++++++++++-------- .../store/AuthorizationStoreFactory.java | 3 - .../syncronization/GroupSynchronizer.java | 66 -------------- .../admin/GroupPolicyManagementTest.java | 2 +- 5 files changed, 60 insertions(+), 108 deletions(-) delete mode 100644 server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/GroupSynchronizer.java diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java index a9c0ee445d..6969159ec0 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java @@ -59,6 +59,10 @@ public class GroupPolicyProvider implements PolicyProvider { for (GroupPolicyRepresentation.GroupDefinition definition : policy.getGroups()) { GroupModel allowedGroup = realm.getGroupById(definition.getId()); + if (allowedGroup == null) { + continue; + } + for (int i = 0; i < groupsClaim.size(); i++) { String group = groupsClaim.asString(i); @@ -84,4 +88,4 @@ public class GroupPolicyProvider implements PolicyProvider { public void close() { } -} \ No newline at end of file +} diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProviderFactory.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProviderFactory.java index e14d501fe7..30fd4f9acb 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProviderFactory.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProviderFactory.java @@ -33,10 +33,13 @@ import org.keycloak.authorization.model.Policy; import org.keycloak.authorization.policy.provider.PolicyProvider; import org.keycloak.authorization.policy.provider.PolicyProviderFactory; import org.keycloak.models.GroupModel; +import org.keycloak.models.GroupProvider; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.models.RealmModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.authorization.GroupPolicyRepresentation; +import org.keycloak.representations.idm.authorization.GroupPolicyRepresentation.GroupDefinition; import org.keycloak.representations.idm.authorization.PolicyRepresentation; import org.keycloak.util.JsonSerialization; @@ -79,7 +82,7 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory { - }); } @Override @@ -164,41 +170,11 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory topLevelGroups = authorization.getKeycloakSession().groups().getTopLevelGroupsStream(authorization.getRealm()).collect(Collectors.toList()); - for (GroupPolicyRepresentation.GroupDefinition definition : groups) { - GroupModel group = null; - - if (definition.getId() != null) { - group = authorization.getRealm().getGroupById(definition.getId()); - } - - String path = definition.getPath(); - - if (group == null && path != null) { - String canonicalPath = path.startsWith("/") ? path.substring(1, path.length()) : path; - - if (canonicalPath != null) { - String[] parts = canonicalPath.split("/"); - GroupModel parent = null; - - for (String part : parts) { - if (parent == null) { - parent = topLevelGroups.stream().filter(groupModel -> groupModel.getName().equals(part)).findFirst().orElseThrow(() -> new RuntimeException("Top level group with name [" + part + "] not found")); - } else { - group = parent.getSubGroupsStream().filter(groupModel -> groupModel.getName().equals(part)).findFirst().orElseThrow(() -> new RuntimeException("Group with name [" + part + "] not found")); - parent = group; - } - } - - if (parts.length == 1) { - group = parent; - } - } - } + GroupModel group = getGroup(authorization, definition); if (group == null) { - throw new RuntimeException("Group with id [" + definition.getId() + "] not found"); + continue; } definition.setId(group.getId()); @@ -214,7 +190,47 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory getGroupsDefinition(Map config) throws IOException { + private GroupModel getGroup(AuthorizationProvider authorization, GroupDefinition definition) { + RealmModel realm = authorization.getRealm(); + KeycloakSession session = authorization.getKeycloakSession(); + GroupProvider groups = session.groups(); + + if (definition.getId() != null) { + return realm.getGroupById(definition.getId()); + } + + GroupModel group = null; + String path = definition.getPath(); + + if (path != null) { + String canonicalPath = path.startsWith("/") ? path.substring(1) : path; + String[] parts = canonicalPath.split("/"); + GroupModel parent = null; + + for (String part : parts) { + if (parent == null) { + parent = groups.getGroupByName(realm, null, part); + if (parent == null) { + return null; + } + } else { + group = groups.getGroupByName(realm, parent, part); + if (group == null) { + return null; + } + parent = group; + } + } + + if (parts.length == 1) { + group = parent; + } + } + + return group; + } + + private Set getGroupsDefinition(Map config, AuthorizationProvider authorization) throws IOException { String groups = config.get("groups"); if (groups == null) { @@ -222,6 +238,7 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory getGroup(authorization, d) != null) .sorted() .collect(Collectors.toCollection(LinkedHashSet::new)); } diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/store/AuthorizationStoreFactory.java b/server-spi-private/src/main/java/org/keycloak/authorization/store/AuthorizationStoreFactory.java index b624b94a36..1de8db6fd3 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/store/AuthorizationStoreFactory.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/store/AuthorizationStoreFactory.java @@ -22,11 +22,9 @@ import java.util.HashMap; import java.util.Map; import org.keycloak.authorization.store.syncronization.ClientApplicationSynchronizer; -import org.keycloak.authorization.store.syncronization.GroupSynchronizer; import org.keycloak.authorization.store.syncronization.RealmSynchronizer; import org.keycloak.authorization.store.syncronization.Synchronizer; import org.keycloak.authorization.store.syncronization.UserSynchronizer; -import org.keycloak.models.GroupModel; import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.models.ClientModel.ClientRemovedEvent; import org.keycloak.models.RealmModel.RealmRemovedEvent; @@ -50,7 +48,6 @@ public interface AuthorizationStoreFactory extends ProviderFactory synchronizers.put(ClientRemovedEvent.class, new ClientApplicationSynchronizer()); synchronizers.put(RealmRemovedEvent.class, new RealmSynchronizer()); synchronizers.put(UserRemovedEvent.class, new UserSynchronizer()); - synchronizers.put(GroupModel.GroupRemovedEvent.class, new GroupSynchronizer()); factory.register(event -> { try { diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/GroupSynchronizer.java b/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/GroupSynchronizer.java deleted file mode 100644 index d6db5300c7..0000000000 --- a/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/GroupSynchronizer.java +++ /dev/null @@ -1,66 +0,0 @@ -/* - * 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.authorization.store.syncronization; - -import java.util.EnumMap; -import java.util.List; -import java.util.Map; -import java.util.Set; - -import org.keycloak.authorization.AuthorizationProvider; -import org.keycloak.authorization.model.Policy; -import org.keycloak.authorization.policy.provider.PolicyProviderFactory; -import org.keycloak.authorization.store.PolicyStore; -import org.keycloak.authorization.store.StoreFactory; -import org.keycloak.models.GroupModel; -import org.keycloak.models.KeycloakSessionFactory; -import org.keycloak.provider.ProviderFactory; -import org.keycloak.representations.idm.authorization.GroupPolicyRepresentation; - -/** - * @author Pedro Igor - */ -public class GroupSynchronizer implements Synchronizer { - - @Override - public void synchronize(GroupModel.GroupRemovedEvent event, KeycloakSessionFactory factory) { - ProviderFactory providerFactory = factory.getProviderFactory(AuthorizationProvider.class); - AuthorizationProvider authorizationProvider = providerFactory.create(event.getKeycloakSession()); - - StoreFactory storeFactory = authorizationProvider.getStoreFactory(); - PolicyStore policyStore = storeFactory.getPolicyStore(); - GroupModel group = event.getGroup(); - Map attributes = new EnumMap<>(Policy.FilterOption.class); - - attributes.put(Policy.FilterOption.TYPE, new String[] {"group"}); - attributes.put(Policy.FilterOption.CONFIG, new String[] {"groups", group.getId()}); - attributes.put(Policy.FilterOption.ANY_OWNER, Policy.FilterOption.EMPTY_FILTER); - - List search = policyStore.find(null, attributes, null, null); - - for (Policy policy : search) { - PolicyProviderFactory policyFactory = authorizationProvider.getProviderFactory(policy.getType()); - GroupPolicyRepresentation representation = GroupPolicyRepresentation.class.cast(policyFactory.toRepresentation(policy, authorizationProvider)); - Set groups = representation.getGroups(); - - groups.removeIf(groupDefinition -> groupDefinition.getId().equals(group.getId())); - - policyFactory.onUpdate(policy, representation, authorizationProvider); - } - } -} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/GroupPolicyManagementTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/GroupPolicyManagementTest.java index fccf630295..9b20ca8007 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/GroupPolicyManagementTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/GroupPolicyManagementTest.java @@ -228,7 +228,7 @@ public class GroupPolicyManagementTest extends AbstractPolicyManagementTest { GroupsResource groups = getRealm().groups(); GroupRepresentation group = groups.groups("Group G", null, null).get(0); - + groups.group(group.getId()).remove(); GroupPolicyRepresentation actual = getClient().authorization().policies().group().findByName(representation.getName());