Avoid iterating and updating all group policies when removing groups (#31057)

Closes #31056

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

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() {
 
     }
-}
+}

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<GroupPo
         representation.setGroupsClaim(policy.getConfig().get("groupsClaim"));
 
         try {
-            representation.setGroups(getGroupsDefinition(policy.getConfig()));
+            representation.setGroups(getGroupsDefinition(policy.getConfig(), authorization));
         } catch (IOException cause) {
             throw new RuntimeException("Failed to deserialize groups", cause);
         }
@@ -104,7 +107,7 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory<GroupPo
     @Override
     public void onImport(Policy policy, PolicyRepresentation representation, AuthorizationProvider authorization) {
         try {
-            updatePolicy(policy, representation.getConfig().get("groupsClaim"), getGroupsDefinition(representation.getConfig()), authorization);
+            updatePolicy(policy, representation.getConfig().get("groupsClaim"), getGroupsDefinition(representation.getConfig(), authorization), authorization);
         } catch (IOException cause) {
             throw new RuntimeException("Failed to deserialize groups", cause);
         }
@@ -118,6 +121,11 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory<GroupPo
 
         for (GroupPolicyRepresentation.GroupDefinition definition: groups) {
             GroupModel group = authorization.getRealm().getGroupById(definition.getId());
+
+            if (group == null) {
+                continue;
+            }
+
             definition.setId(null);
             definition.setPath(ModelToRepresentation.buildGroupPath(group));
         }
@@ -144,8 +152,6 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory<GroupPo
 
     @Override
     public void postInit(KeycloakSessionFactory factory) {
-        factory.register(event -> {
-        });
     }
 
     @Override
@@ -164,41 +170,11 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory<GroupPo
             config.put("groupsClaim", groupsClaim);
         }
 
-        List<GroupModel> topLevelGroups = authorization.getKeycloakSession().groups().getTopLevelGroupsStream(authorization.getRealm()).collect(Collectors.toList());
-
         for (GroupPolicyRepresentation.GroupDefinition definition : groups) {
-            GroupModel group = null;
+            GroupModel group = getGroup(authorization, definition);
-
-            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;
-                    }
-                }
-            }
 
             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<GroupPo
         policy.setConfig(config);
     }
 
-    private Set<GroupPolicyRepresentation.GroupDefinition> getGroupsDefinition(Map<String, String> 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<GroupPolicyRepresentation.GroupDefinition> getGroupsDefinition(Map<String, String> config, AuthorizationProvider authorization) throws IOException {
         String groups = config.get("groups");
 
         if (groups == null) {
@@ -222,6 +238,7 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory<GroupPo
         }
 
         return Arrays.stream(JsonSerialization.readValue(groups, GroupPolicyRepresentation.GroupDefinition[].class))
+                .filter(d -> getGroup(authorization, d) != null)
                 .sorted()
                 .collect(Collectors.toCollection(LinkedHashSet::new));
     }

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<StoreFactory>
         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 {

server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/GroupSynchronizer.java

@@ -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 <a href="mailto:psilva@redhat.com">Pedro Igor</a>
- */
-public class GroupSynchronizer implements Synchronizer<GroupModel.GroupRemovedEvent> {
-
-    @Override
-    public void synchronize(GroupModel.GroupRemovedEvent event, KeycloakSessionFactory factory) {
-        ProviderFactory<AuthorizationProvider> providerFactory = factory.getProviderFactory(AuthorizationProvider.class);
-        AuthorizationProvider authorizationProvider = providerFactory.create(event.getKeycloakSession());
-
-        StoreFactory storeFactory = authorizationProvider.getStoreFactory();
-        PolicyStore policyStore = storeFactory.getPolicyStore();
-        GroupModel group = event.getGroup();
-        Map<Policy.FilterOption, String[]> 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<Policy> 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<GroupPolicyRepresentation.GroupDefinition> groups = representation.getGroups();
-
-            groups.removeIf(groupDefinition -> groupDefinition.getId().equals(group.getId()));
-
-            policyFactory.onUpdate(policy, representation, authorizationProvider);
-        }
-    }
-}

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());