diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java index e4740349f16..6f11f929629 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java @@ -166,7 +166,7 @@ public class ClientPolicyProviderFactory implements PolicyProviderFactory clients, AuthorizationProvider authorization) { RealmModel realm = authorization.getRealm(); - if (clients == null || clients.isEmpty()) { + if (clients == null) { throw new RuntimeException("No client provided."); } 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 77e7e3247b5..efd7f157de4 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 @@ -154,7 +154,7 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory groups, AuthorizationProvider authorization) { - if (groups == null || groups.isEmpty()) { + if (groups == null) { throw new RuntimeException("You must provide at least one group"); } diff --git a/docs/documentation/release_notes/topics/24_0_0.adoc b/docs/documentation/release_notes/topics/24_0_0.adoc index f9fba887160..ca26efa2062 100644 --- a/docs/documentation/release_notes/topics/24_0_0.adoc +++ b/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. diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/ClientApplicationSynchronizer.java b/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/ClientApplicationSynchronizer.java index 1470add658d..c8ab487800e 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/ClientApplicationSynchronizer.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/store/syncronization/ClientApplicationSynchronizer.java @@ -72,12 +72,7 @@ public class ClientApplicationSynchronizer implements Synchronizer groupDefinition.getId().equals(group.getId())); - if (groups.isEmpty()) { - policyFactory.onRemove(policy, authorizationProvider); - policyStore.delete(realm, policy.getId()); - } else { - policyFactory.onUpdate(policy, representation, authorizationProvider); - } + policyFactory.onUpdate(policy, representation, authorizationProvider); } } } 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 0c8a621b8fe..62a363f0623 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 @@ -72,12 +72,7 @@ public class UserSynchronizer implements Synchronizer { users.remove(userModel.getId()); - if (users.isEmpty()) { - policyFactory.onRemove(policy, authorizationProvider); - policyStore.delete(realm, policy.getId()); - } else { - policyFactory.onUpdate(policy, representation, authorizationProvider); - } + policyFactory.onUpdate(policy, representation, authorizationProvider); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/UserManagedPermissionServiceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/UserManagedPermissionServiceTest.java index 536026106ac..f57abfec358 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/UserManagedPermissionServiceTest.java +++ b/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 filters = new HashMap<>(); - - filters.put(Policy.FilterOption.TYPE, new String[] {"uma"}); - filters.put(OWNER, new String[] {user.getId()}); - - List 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 filters = new HashMap<>(); - - filters.put(Policy.FilterOption.TYPE, new String[] {"uma"}); - filters.put(OWNER, new String[] {user.getId()}); - - List 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 getAssociatedPolicies(UmaPermissionRepresentation permission) { return getClient(getRealm()).authorization().policies().policy(permission.getId()).associatedPolicies(); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/AggregatePolicyManagementTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/AggregatePolicyManagementTest.java index d94e6609730..1a1163c614d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/AggregatePolicyManagementTest.java +++ b/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 Pedro Igor */ 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()); + } + } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/ClientPolicyManagementTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/ClientPolicyManagementTest.java index 18e4e1043ad..183d33dc8ca 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/admin/ClientPolicyManagementTest.java +++ b/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 { - authorization.policies().client().findById(representation.getId()).toRepresentation(); - fail("Client policy should be removed"); - } catch (NotFoundException nfe) { - // ignore - } + representation = authorization.policies().client().findById(representation.getId()).toRepresentation(); + + Assert.assertEquals(0, representation.getClients().size()); + Assert.assertFalse(representation.getClients().contains(client.getId())); } @Test 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 12ce2fadbb5..fccf630295f 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 @@ -231,25 +231,8 @@ public class GroupPolicyManagementTest extends AbstractPolicyManagementTest { groups.group(group.getId()).remove(); - try { - getClient().authorization().policies().group().findByName(representation.getName()); - } 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()); + GroupPolicyRepresentation actual = getClient().authorization().policies().group().findByName(representation.getName()); + assertEquals(0, actual.getGroups().size()); } private void assertCreated(AuthorizationResource authorization, GroupPolicyRepresentation representation) { 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 a20131873ed..c41cf708bfc 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 @@ -171,12 +171,10 @@ public class UserPolicyManagementTest extends AbstractPolicyManagementTest { user = users.search("User F").get(0); users.get(user.getId()).remove(); - try { - authorization.policies().user().findById(representation.getId()).toRepresentation(); - fail("User policy should be removed"); - } catch (NotFoundException nfe) { - // ignore - } + representation = authorization.policies().user().findById(representation.getId()).toRepresentation(); + + Assert.assertEquals(0, representation.getUsers().size()); + Assert.assertFalse(representation.getUsers().contains(user.getId())); } @Test