Deleting a User or User Group might cause that all users suddenly get the permissions of the deleted user.

Closes #24651
Signed-off-by: Douglas Palmer <dpalmer@redhat.com>

authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java

@@ -166,7 +166,7 @@ public class ClientPolicyProviderFactory implements PolicyProviderFactory<Client
     private void updateClients(Policy policy, Set<String> clients, AuthorizationProvider authorization) {
         RealmModel realm = authorization.getRealm();
 
-        if (clients == null || clients.isEmpty()) {
+        if (clients == null) {
             throw new RuntimeException("No client provided.");
         }
 

authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProviderFactory.java

@@ -154,7 +154,7 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory<GroupPo
     }
 
     private void updatePolicy(Policy policy, String groupsClaim, Set<GroupPolicyRepresentation.GroupDefinition> groups, AuthorizationProvider authorization) {
-        if (groups == null || groups.isEmpty()) {
+        if (groups == null) {
             throw new RuntimeException("You must provide at least one group");
         }
 

docs/documentation/release_notes/topics/24_0_0.adoc

@@ -124,3 +124,7 @@ When enabling metrics for {project_name}'s embedded caches, the metrics now use
 
 For more details, check the
 link:{upgradingguide_link}[{upgradingguide_name}].
+
+= Authorization Policy
+
+In previous versions of Keycloak when the last member of a User, Group or Client policy was deleted then that policy would also be deleted. Unfortunately this could lead to an escalation of privileges if the policy was used in an aggregate policy. To avoid privilege escalation the effect policies are no longer deleted and an administrator will need to update those policies.

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

@@ -72,12 +72,7 @@ public class ClientApplicationSynchronizer implements Synchronizer<ClientRemoved
 
             clients.remove(event.getClient().getId());
 
-            if (clients.isEmpty()) {
-                policyFactory.onRemove(policy, authorizationProvider);
-                authorizationProvider.getStoreFactory().getPolicyStore().delete(realm, policy.getId());
-            } else {
             policyFactory.onUpdate(policy, representation, authorizationProvider);
         }
     }
 }
-}

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

@@ -62,12 +62,7 @@ public class GroupSynchronizer implements Synchronizer<GroupModel.GroupRemovedEv
 
             groups.removeIf(groupDefinition -> groupDefinition.getId().equals(group.getId()));
 
-            if (groups.isEmpty()) {
-                policyFactory.onRemove(policy, authorizationProvider);
-                policyStore.delete(realm, policy.getId());
-            } else {
             policyFactory.onUpdate(policy, representation, authorizationProvider);
         }
     }
 }
-}

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

@@ -72,14 +72,9 @@ public class UserSynchronizer implements Synchronizer<UserRemovedEvent> {
 
             users.remove(userModel.getId());
 
-            if (users.isEmpty()) {
-                policyFactory.onRemove(policy, authorizationProvider);
-                policyStore.delete(realm, policy.getId());
-            } else {
             policyFactory.onUpdate(policy, representation, authorizationProvider);
         }
     }
-    }
 
     private void removeUserResources(UserRemovedEvent event, AuthorizationProvider authorizationProvider) {
         StoreFactory storeFactory = authorizationProvider.getStoreFactory();

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/UserManagedPermissionServiceTest.java

@@ -488,44 +488,6 @@ public class UserManagedPermissionServiceTest extends AbstractResourceServerTest
 
     }
 
-    @Test
-    public void testRemoveUserPolicyWhenUserDeleted() {
-        ResourceRepresentation resource = new ResourceRepresentation();
-
-        resource.setName("Resource A");
-        resource.setOwnerManagedAccess(true);
-        resource.setOwner("marta");
-        resource.addScope("Scope A", "Scope B", "Scope C");
-
-        resource = getAuthzClient().protection().resource().create(resource);
-
-        UmaPermissionRepresentation permission = new UmaPermissionRepresentation();
-
-        permission.setName("Custom User-Managed Permission");
-        permission.addUser("kolo");
-
-        ProtectionResource protection = getAuthzClient().protection("marta", "password");
-
-        protection.policy(resource.getId()).create(permission);
-
-        AuthorizationResource authorization = getAuthzClient().authorization("kolo", "password");
-
-        AuthorizationRequest request = new AuthorizationRequest();
-
-        request.addPermission(resource.getId(), "Scope A");
-
-        AuthorizationResponse authzResponse = authorization.authorize(request);
-
-        assertNotNull(authzResponse);
-
-        UsersResource users = realmsResouce().realm(REALM_NAME).users();
-        UserRepresentation kolo = users.search("kolo").get(0);
-
-        users.delete(kolo.getId());
-
-        assertTrue(protection.policy(resource.getId()).find(null, null, null, null).isEmpty());
-    }
-
     @Test
     public void testRemovePolicyWhenOwnerDeleted() {
         ResourceRepresentation resource = new ResourceRepresentation();
@@ -1021,116 +983,6 @@ public class UserManagedPermissionServiceTest extends AbstractResourceServerTest
         assertTrue(policies.isEmpty());
     }
 
-    @Test
-    public void testRemovePoliciesOnGroupDelete() {
-        ResourceRepresentation resource = new ResourceRepresentation();
-
-        resource.setName("Resource A");
-        resource.setOwnerManagedAccess(true);
-        resource.setOwner("marta");
-        resource.addScope("Scope A", "Scope B", "Scope C");
-
-        resource = getAuthzClient().protection().resource().create(resource);
-
-        UmaPermissionRepresentation newPermission = new UmaPermissionRepresentation();
-
-        newPermission.setName("Custom User-Managed Permission");
-        newPermission.addGroup("/group_remove");
-
-        ProtectionResource protection = getAuthzClient().protection("marta", "password");
-
-        protection.policy(resource.getId()).create(newPermission);
-
-        getTestingClient().server().run((RunOnServer) UserManagedPermissionServiceTest::testRemovePoliciesOnGroupDelete);
-    }
-
-    private static void testRemovePoliciesOnGroupDelete(KeycloakSession session) {
-        RealmModel realm = session.realms().getRealmByName("authz-test");
-        ClientModel client = realm.getClientByClientId("resource-server-test");
-        AuthorizationProvider provider = session.getProvider(AuthorizationProvider.class);
-        UserModel user = session.users().getUserByUsername(realm, "marta");
-        ResourceServer resourceServer = provider.getStoreFactory().getResourceServerStore().findByClient(client);
-        Map<Policy.FilterOption, String[]> filters = new HashMap<>();
-
-        filters.put(Policy.FilterOption.TYPE, new String[] {"uma"});
-        filters.put(OWNER, new String[] {user.getId()});
-
-        List<Policy> policies = provider.getStoreFactory().getPolicyStore()
-                .find(realm, resourceServer, filters, null, null);
-        assertEquals(1, policies.size());
-
-        Policy policy = policies.get(0);
-        assertFalse(policy.getResources().isEmpty());
-
-        Resource resource = policy.getResources().iterator().next();
-        assertEquals("Resource A", resource.getName());
-
-        realm.removeGroup(session.groups().searchForGroupByNameStream(realm, "group_remove", false, null, null).findAny().get());
-
-        filters = new HashMap<>();
-
-        filters.put(OWNER, new String[] {user.getId()});
-
-        policies = provider.getStoreFactory().getPolicyStore()
-                .find(realm, resourceServer, filters, null, null);
-        assertTrue(policies.isEmpty());
-    }
-
-    @Test
-    public void testRemovePoliciesOnClientDelete() {
-        ResourceRepresentation resource = new ResourceRepresentation();
-
-        resource.setName("Resource A");
-        resource.setOwnerManagedAccess(true);
-        resource.setOwner("marta");
-        resource.addScope("Scope A", "Scope B", "Scope C");
-
-        resource = getAuthzClient().protection().resource().create(resource);
-
-        UmaPermissionRepresentation newPermission = new UmaPermissionRepresentation();
-
-        newPermission.setName("Custom User-Managed Permission");
-        newPermission.addClient("client-remove");
-
-        ProtectionResource protection = getAuthzClient().protection("marta", "password");
-
-        protection.policy(resource.getId()).create(newPermission);
-
-        getTestingClient().server().run((RunOnServer) UserManagedPermissionServiceTest::testRemovePoliciesOnClientDelete);
-    }
-
-    private static void testRemovePoliciesOnClientDelete(KeycloakSession session) {
-        RealmModel realm = session.realms().getRealmByName("authz-test");
-        ClientModel client = realm.getClientByClientId("resource-server-test");
-        AuthorizationProvider provider = session.getProvider(AuthorizationProvider.class);
-        UserModel user = session.users().getUserByUsername(realm, "marta");
-        ResourceServer resourceServer = provider.getStoreFactory().getResourceServerStore().findByClient(client);
-        Map<Policy.FilterOption, String[]> filters = new HashMap<>();
-
-        filters.put(Policy.FilterOption.TYPE, new String[] {"uma"});
-        filters.put(OWNER, new String[] {user.getId()});
-
-        List<Policy> policies = provider.getStoreFactory().getPolicyStore()
-            .find(realm, resourceServer, filters, null, null);
-        assertEquals(1, policies.size());
-
-        Policy policy = policies.get(0);
-        assertFalse(policy.getResources().isEmpty());
-
-        Resource resource = policy.getResources().iterator().next();
-        assertEquals("Resource A", resource.getName());
-
-        realm.removeClient(realm.getClientByClientId("client-remove").getId());
-
-        filters = new HashMap<>();
-
-        filters.put(OWNER, new String[] {user.getId()});
-
-        policies = provider.getStoreFactory().getPolicyStore()
-            .find(realm, resourceServer, filters, null, null);
-        assertTrue(policies.isEmpty());
-    }
-
     private List<PolicyRepresentation> getAssociatedPolicies(UmaPermissionRepresentation permission) {
         return getClient(getRealm()).authorization().policies().policy(permission.getId()).associatedPolicies();
     }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/AggregatePolicyManagementTest.java

@@ -16,6 +16,7 @@
  */
 package org.keycloak.testsuite.authz.admin;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
 
 import java.util.Collections;
@@ -27,15 +28,29 @@ import org.junit.Test;
 import org.keycloak.admin.client.resource.AggregatePoliciesResource;
 import org.keycloak.admin.client.resource.AggregatePolicyResource;
 import org.keycloak.admin.client.resource.AuthorizationResource;
+import org.keycloak.admin.client.resource.UserPoliciesResource;
+import org.keycloak.admin.client.resource.UserPolicyResource;
+import org.keycloak.admin.client.resource.UsersResource;
+import org.keycloak.representations.idm.UserRepresentation;
 import org.keycloak.representations.idm.authorization.AggregatePolicyRepresentation;
 import org.keycloak.representations.idm.authorization.DecisionStrategy;
 import org.keycloak.representations.idm.authorization.Logic;
+import org.keycloak.representations.idm.authorization.TimePolicyRepresentation;
+import org.keycloak.representations.idm.authorization.UserPolicyRepresentation;
+import org.keycloak.testsuite.util.RealmBuilder;
+import org.keycloak.testsuite.util.UserBuilder;
 
 /**
  * @author <a href="mailto:psilva@redhat.com">Pedro Igor</a>
  */
 public class AggregatePolicyManagementTest extends AbstractPolicyManagementTest {
 
+    @Override
+    protected RealmBuilder createTestRealm() {
+        return super.createTestRealm()
+                .user(UserBuilder.create().username("AggregatePolicyManagementTestUser"));
+    }
+
     @Test
     public void testCreate() {
         AuthorizationResource authorization = getClient().authorization();
@@ -103,6 +118,47 @@ public class AggregatePolicyManagementTest extends AbstractPolicyManagementTest
         }
     }
 
+    //Issue #24651
+    @Test
+    public void testDeleteUser() {
+        AuthorizationResource authorization = getClient().authorization();
+
+        UsersResource users = getRealm().users();
+        UserRepresentation user = users.search("AggregatePolicyManagementTestUser").get(0);
+
+        UserPolicyRepresentation userPolicyRepresentation = new UserPolicyRepresentation();
+        userPolicyRepresentation.setName("AggregatePolicyManagementTestUser Only");
+        userPolicyRepresentation.setDescription("description");
+        userPolicyRepresentation.setDecisionStrategy(DecisionStrategy.CONSENSUS);
+        userPolicyRepresentation.setLogic(Logic.NEGATIVE);
+        userPolicyRepresentation.addUser(user.getId());
+        authorization.policies().user().create(userPolicyRepresentation);
+
+        TimePolicyRepresentation timePolicyRepresentation = new TimePolicyRepresentation();
+        timePolicyRepresentation.setDecisionStrategy(DecisionStrategy.CONSENSUS);
+        timePolicyRepresentation.setLogic(Logic.NEGATIVE);
+        timePolicyRepresentation.setName("Dayshift");
+        timePolicyRepresentation.setHour("8");
+        timePolicyRepresentation.setHourEnd("17");
+        authorization.policies().time().create(timePolicyRepresentation);
+
+        AggregatePolicyRepresentation representation = new AggregatePolicyRepresentation();
+        representation.setName("AggregatePolicyManagementTestUser Only during dayshift");
+        representation.setDescription("description");
+        representation.setDecisionStrategy(DecisionStrategy.CONSENSUS);
+        representation.setLogic(Logic.NEGATIVE);
+        representation.addPolicy("AggregatePolicyManagementTestUser Only", "Dayshift");
+        assertCreated(authorization, representation);
+
+        users.get(user.getId()).remove();
+
+        UserPolicyRepresentation actualUserPolicy = authorization.policies().user().findByName(userPolicyRepresentation.getName());
+        assertEquals(0, actualUserPolicy.getUsers().size());
+
+        AggregatePolicyResource actual = authorization.policies().aggregate().findById(representation.getId());
+        assertRepresentation(representation, actual);
+    }
+
     private void assertCreated(AuthorizationResource authorization, AggregatePolicyRepresentation representation) {
         AggregatePoliciesResource permissions = authorization.policies().aggregate();
         try (Response response = permissions.create(representation)) {
@@ -112,8 +168,29 @@ public class AggregatePolicyManagementTest extends AbstractPolicyManagementTest
         }
     }
 
+    private void assertCreated(AuthorizationResource authorization, UserPolicyRepresentation representation) {
+        UserPoliciesResource permissions = authorization.policies().user();
+
+        try (Response response = permissions.create(representation)) {
+            UserPolicyRepresentation created = response.readEntity(UserPolicyRepresentation.class);
+            UserPolicyResource permission = permissions.findById(created.getId());
+            assertRepresentation(representation, permission);
+        }
+    }
+
     private void assertRepresentation(AggregatePolicyRepresentation representation, AggregatePolicyResource policy) {
         AggregatePolicyRepresentation actual = policy.toRepresentation();
         assertRepresentation(representation, actual, () -> policy.resources(), () -> Collections.emptyList(), () -> policy.associatedPolicies());
     }
+
+    private void assertRepresentation(UserPolicyRepresentation representation, UserPolicyResource permission) {
+        UserPolicyRepresentation actual = permission.toRepresentation();
+        assertRepresentation(representation, actual, () -> permission.resources(), () -> Collections.emptyList(), () -> permission.associatedPolicies());
+        assertEquals(representation.getUsers().size(), actual.getUsers().size());
+        assertEquals(0, actual.getUsers().stream().filter(userId -> !representation.getUsers().stream()
+                        .filter(userName -> getRealm().users().get(userId).toRepresentation().getUsername().equalsIgnoreCase(userName))
+                        .findFirst().isPresent())
+                .count());
+    }
+
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/ClientPolicyManagementTest.java

@@ -169,12 +169,10 @@ public class ClientPolicyManagementTest extends AbstractPolicyManagementTest {
         client = clients.findByClientId("Client F").get(0);
         clients.get(client.getId()).remove();
 
-        try {
+        representation = authorization.policies().client().findById(representation.getId()).toRepresentation();
-            authorization.policies().client().findById(representation.getId()).toRepresentation();
+
-            fail("Client policy should be removed");
+        Assert.assertEquals(0, representation.getClients().size());
-        } catch (NotFoundException nfe) {
+        Assert.assertFalse(representation.getClients().contains(client.getId()));
-            // ignore
-        }
     }
 
     @Test

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/GroupPolicyManagementTest.java

@@ -231,25 +231,8 @@ public class GroupPolicyManagementTest extends AbstractPolicyManagementTest {
         
         groups.group(group.getId()).remove();
 
-        try {
+        GroupPolicyRepresentation actual = getClient().authorization().policies().group().findByName(representation.getName());
-            getClient().authorization().policies().group().findByName(representation.getName());
+        assertEquals(0, actual.getGroups().size());
-        } catch (NotFoundException e) {
-        }
-        
-        representation.getGroups().clear();
-        representation.addGroupPath("/Group H/Group I/Group K");
-        representation.addGroupPath("/Group F");
-        
-        assertCreated(authorization, representation);
-
-        group = groups.groups("Group K", null, null).get(0);
-
-        groups.group(group.getId()).remove();
-
-        GroupPolicyRepresentation policy = getClient().authorization().policies().group().findByName(representation.getName());
-
-        assertNotNull(policy);
-        assertEquals(1, policy.getGroups().size());
     }
 
     private void assertCreated(AuthorizationResource authorization, GroupPolicyRepresentation representation) {

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/UserPolicyManagementTest.java

@@ -171,12 +171,10 @@ public class UserPolicyManagementTest extends AbstractPolicyManagementTest {
         user = users.search("User F").get(0);
         users.get(user.getId()).remove();
 
-        try {
+        representation = authorization.policies().user().findById(representation.getId()).toRepresentation();
-            authorization.policies().user().findById(representation.getId()).toRepresentation();
+
-            fail("User policy should be removed");
+        Assert.assertEquals(0, representation.getUsers().size());
-        } catch (NotFoundException nfe) {
+        Assert.assertFalse(representation.getUsers().contains(user.getId()));
-            // ignore
-        }
     }
 
     @Test