From b76f4f9c1b289653b80106bbc3973ccf40543b69 Mon Sep 17 00:00:00 2001
From: Pedro Igor <pigor.craveiro@gmail.com>
Date: Fri, 11 Oct 2024 14:35:21 -0300
Subject: [PATCH] Avoid iterating over user policies when removing users

Closes #19358

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
---
 .../user/UserPolicyProviderFactory.java       | 57 ++++++++-----------
 .../syncronization/UserSynchronizer.java      | 26 +--------
 .../authz/admin/UserPolicyManagementTest.java |  7 +++
 3 files changed, 31 insertions(+), 59 deletions(-)

diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java
index a973396608..3ddccce0f1 100644
--- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java
+++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java
@@ -78,7 +78,9 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory<UserPoli
             if (users == null) {
                 representation.setUsers(Collections.emptySet());
             } else {
-                representation.setUsers(JsonSerialization.readValue(users, Set.class));
+                representation.setUsers((Set<String>) JsonSerialization.readValue(users, Set.class).stream()
+                        .filter(id -> getUser((String) id, authorization) != null)
+                        .collect(Collectors.toSet()));
             }
         } catch (IOException cause) {
             throw new RuntimeException("Failed to deserialize roles", cause);
@@ -94,12 +96,12 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory<UserPoli
 
     @Override
     public void onCreate(Policy policy, UserPolicyRepresentation representation, AuthorizationProvider authorization) {
-        updateUsers(policy, representation, authorization);
+        updateUsers(policy, authorization, representation.getUsers());
     }
 
     @Override
     public void onUpdate(Policy policy, UserPolicyRepresentation representation, AuthorizationProvider authorization) {
-        updateUsers(policy, representation, authorization);
+        updateUsers(policy, authorization, representation.getUsers());
     }
 
     @Override
@@ -133,28 +135,12 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory<UserPoli
         representation.setConfig(config);
     }
 
-    private void updateUsers(Policy policy, UserPolicyRepresentation representation, AuthorizationProvider authorization) {
-        updateUsers(policy, authorization, representation.getUsers());
-    }
-
     private void updateUsers(Policy policy, AuthorizationProvider authorization, Set<String> users) {
-        KeycloakSession session = authorization.getKeycloakSession();
-        RealmModel realm = authorization.getRealm();
-        UserProvider userProvider = session.users();
         Set<String> updatedUsers = new HashSet<>();
 
         if (users != null) {
             for (String userId : users) {
-                UserModel user = null;
-
-                try {
-                    user = userProvider.getUserByUsername(realm, userId);
-                } catch (Exception ignore) {
-                }
-
-                if (user == null) {
-                    user = userProvider.getUserById(realm, userId);
-                }
+                UserModel user = getUser(userId, authorization);
 
                 if (user == null) {
                     throw new RuntimeException("Error while updating policy [" + policy.getName()  + "]. User [" + userId + "] could not be found.");
@@ -172,6 +158,23 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory<UserPoli
         }
     }
 
+    private static UserModel getUser(String userId, AuthorizationProvider authorization) {
+        if (userId == null) {
+            return null;
+        }
+
+        KeycloakSession session = authorization.getKeycloakSession();
+        RealmModel realm = authorization.getRealm();
+        UserProvider userProvider = session.users();
+        UserModel user = userProvider.getUserByUsername(realm, userId);
+
+        if (user == null) {
+            user = userProvider.getUserById(realm, userId);
+        }
+
+        return user;
+    }
+
     @Override
     public void init(Config.Scope config) {
 
@@ -191,18 +194,4 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory<UserPoli
     public String getId() {
         return "user";
     }
-
-    static String[] getUsers(Policy policy) {
-        String users = policy.getConfig().get("users");
-
-        if (users != null) {
-            try {
-                return JsonSerialization.readValue(users.getBytes(), String[].class);
-            } catch (IOException e) {
-                throw new RuntimeException("Could not parse users [" + users + "] from policy config [" + policy.getName() + ".", e);
-            }
-        }
-
-        return new String[0];
-    }
 }
diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/UserSynchronizer.java b/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/UserSynchronizer.java
index f43cf2f876..36b19e423d 100644
--- a/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/UserSynchronizer.java
+++ b/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/UserSynchronizer.java
@@ -48,30 +48,6 @@ public class UserSynchronizer implements Synchronizer<UserRemovedEvent> {
 
         removeFromUserPermissionTickets(event, authorizationProvider);
         removeUserResources(event, authorizationProvider);
-        removeFromUserPolicies(event, authorizationProvider);
-    }
-
-    private void removeFromUserPolicies(UserRemovedEvent event, AuthorizationProvider authorizationProvider) {
-        StoreFactory storeFactory = authorizationProvider.getStoreFactory();
-        PolicyStore policyStore = storeFactory.getPolicyStore();
-        UserModel userModel = event.getUser();
-        Map<Policy.FilterOption, String[]> attributes = new EnumMap<>(Policy.FilterOption.class);
-
-        attributes.put(Policy.FilterOption.TYPE, new String[] {"user"});
-        attributes.put(Policy.FilterOption.CONFIG, new String[] {"users", userModel.getId()});
-        attributes.put(Policy.FilterOption.ANY_OWNER, new String[] {Boolean.TRUE.toString()});
-
-        List<Policy> search = policyStore.find(null, attributes, null, null);
-
-        for (Policy policy : search) {
-            PolicyProviderFactory policyFactory = authorizationProvider.getProviderFactory(policy.getType());
-            UserPolicyRepresentation representation = UserPolicyRepresentation.class.cast(policyFactory.toRepresentation(policy, authorizationProvider));
-            Set<String> users = representation.getUsers();
-
-            users.remove(userModel.getId());
-
-            policyFactory.onUpdate(policy, representation, authorizationProvider);
-        }
     }
 
     private void removeUserResources(UserRemovedEvent event, AuthorizationProvider authorizationProvider) {
@@ -106,7 +82,7 @@ public class UserSynchronizer implements Synchronizer<UserRemovedEvent> {
         }
 
         attributes.clear();
-        
+
         attributes.put(PermissionTicket.FilterOption.REQUESTER, userModel.getId());
 
         for (PermissionTicket ticket : ticketStore.find(null, attributes, null, null)) {
diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/UserPolicyManagementTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/UserPolicyManagementTest.java
index b2a5ce03b4..e62cfef5a5 100644
--- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/UserPolicyManagementTest.java
+++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/UserPolicyManagementTest.java
@@ -107,6 +107,13 @@ public class UserPolicyManagementTest extends AbstractPolicyManagementTest {
 
         permission.update(representation);
         assertRepresentation(representation, permission);
+
+        String userName = representation.getUsers().iterator().next();
+        UserRepresentation user = getRealm().users().search(userName).get(0);
+        getRealm().users().delete(user.getId()).close();
+
+        representation.getUsers().clear();
+        assertRepresentation(representation, permission);
     }
 
     @Test