Skip to content

Commit

Permalink
Deleting a User or User Group might cause that all users suddenly get…
Browse files Browse the repository at this point in the history 
… the permissions of the deleted user.

Closes keycloak#24651
Signed-off-by: Douglas Palmer <[email protected]>
  • Loading branch information
@douglaspalmer @pedroigor
douglaspalmer authored and pedroigor committed Jan 8, 2024
1 parent badf3f4 commit 58d167f
Show file tree
Hide file tree
Showing 11 changed files with 96 additions and 199 deletions.
2 changes: 1 addition & 1 deletion ...n/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 166,7 @@ public String getId() {
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.");
}

Expand Down
2 changes: 1 addition & 1 deletion ...ain/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProviderFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 154,7 @@ public void close() {
}

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");
}

Expand Down
4 changes: 4 additions & 0 deletions docs/documentation/release_notes/topics/24_0_0.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
7 changes: 1 addition & 6 deletions ...n/java/org/keycloak/authorization/store/syncronization/ClientApplicationSynchronizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 72,7 @@ private void removeFromClientPolicies(ClientRemovedEvent event, AuthorizationPro

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);
}
policyFactory.onUpdate(policy, representation, authorizationProvider);
}
}
}
7 changes: 1 addition & 6 deletions ...vate/src/main/java/org/keycloak/authorization/store/syncronization/GroupSynchronizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 62,7 @@ public void synchronize(GroupModel.GroupRemovedEvent event, KeycloakSessionFacto

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);
}
policyFactory.onUpdate(policy, representation, authorizationProvider);
}
}
}
7 changes: 1 addition & 6 deletions ...ivate/src/main/java/org/keycloak/authorization/store/syncronization/UserSynchronizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 72,7 @@ private void removeFromUserPolicies(UserRemovedEvent event, AuthorizationProvide

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

Expand Down
148 changes: 0 additions & 148 deletions ...sts/base/src/test/java/org/keycloak/testsuite/authz/UserManagedPermissionServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,44 488,6 @@ public void testUserManagedPermission() {

}

@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();
Expand Down Expand Up @@ -1021,116 983,6 @@ private static void testRemovePoliciesOnResourceDelete(KeycloakSession session)
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();
}
Expand Down
77 changes: 77 additions & 0 deletions .../base/src/test/java/org/keycloak/testsuite/authz/admin/AggregatePolicyManagementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,15 28,29 @@
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:[email protected]">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();
Expand Down Expand Up @@ -103,6 118,47 @@ public void testDelete() {
}
}

//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)) {
Expand All @@ -112,8 168,29 @@ private void assertCreated(AuthorizationResource authorization, AggregatePolicyR
}
}

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

}
10 changes: 4 additions & 6 deletions ...sts/base/src/test/java/org/keycloak/testsuite/authz/admin/ClientPolicyManagementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 169,10 @@ public void testDeleteClient() {
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
Expand Down
21 changes: 2 additions & 19 deletions ...ests/base/src/test/java/org/keycloak/testsuite/authz/admin/GroupPolicyManagementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,25 231,8 @@ public void testDeleteGroupAndPolicy() {

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) {
Expand Down
Loading

0 comments on commit 58d167f

Please sign in to comment.