From 45e92430546c9c743b810894c687f6e4562651f5 Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Fri, 17 Dec 2021 14:45:56 +0100 Subject: [PATCH] Verify fine-grained admin permissions feature is enabled before checking fine-grained permissions when creating users (#9211) * Verify fine-grained admin permissions feature is enabled before checking fine-grained permissions when creating users Co-authored-by: stianst * fixing test Co-authored-by: Pedro Igor --- .../resources/admin/UsersResource.java | 50 +++++++++++++------ .../admin/FineGrainAdminUnitTest.java | 34 +++++++++++-- .../keycloak/testsuite/admin/UserTest.java | 43 ++++++++++++++++ 3 files changed, 107 insertions(+), 20 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java index a05599a0cb..8734cd4a9d 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java @@ -20,6 +20,7 @@ import org.jboss.logging.Logger; import org.jboss.resteasy.annotations.cache.NoCache; import org.jboss.resteasy.spi.ResteasyProviderFactory; import org.keycloak.common.ClientConnection; +import org.keycloak.common.Profile; import org.keycloak.common.util.ObjectUtil; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; @@ -57,10 +58,15 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.keycloak.models.utils.KeycloakModelUtils.findGroupByPath; import static org.keycloak.userprofile.UserProfileContext.USER_API; /** @@ -110,22 +116,8 @@ public class UsersResource { // first check if user has manage rights try { auth.users().requireManage(); - } - catch (ForbiddenException exception) { - // if user does not have manage rights, fallback to fine grain admin permissions per group - if (rep.getGroups() != null) { - // if groups is part of the user rep, check if admin has manage_members and manage_membership on each group - for (String groupPath : rep.getGroups()) { - GroupModel group = KeycloakModelUtils.findGroupByPath(realm, groupPath); - if (group != null) { - auth.groups().requireManageMembers(group); - auth.groups().requireManageMembership(group); - } else { - return ErrorResponse.error(String.format("Group %s not found", groupPath), Response.Status.BAD_REQUEST); - } - } - } else { - // propagate exception if no group specified + } catch (ForbiddenException exception) { + if (!canCreateGroupMembers(rep)) { throw exception; } } @@ -194,6 +186,32 @@ public class UsersResource { return ErrorResponse.error("Could not create user", Response.Status.BAD_REQUEST); } } + + private boolean canCreateGroupMembers(UserRepresentation rep) { + if (!Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) { + return false; + } + + List groups = Optional.ofNullable(rep.getGroups()) + .orElse(Collections.emptyList()) + .stream().map(path -> findGroupByPath(realm, path)) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + + if (groups.isEmpty()) { + return false; + } + + // if groups is part of the user rep, check if admin has manage_members and manage_membership on each group + // an exception is thrown in case the current user does not have permissions to manage any of the groups + for (GroupModel group : groups) { + auth.groups().requireManageMembers(group); + auth.groups().requireManageMembership(group); + } + + return true; + } + /** * Get representation of the user * diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/FineGrainAdminUnitTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/FineGrainAdminUnitTest.java index 51d6e318b3..4423d58f96 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/FineGrainAdminUnitTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/FineGrainAdminUnitTest.java @@ -40,6 +40,7 @@ import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.RepresentationToModel; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.ClientScopeRepresentation; +import org.keycloak.representations.idm.GroupRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.representations.idm.UserRepresentation; @@ -58,6 +59,7 @@ import org.keycloak.testsuite.arquillian.annotation.EnableFeature; import org.keycloak.testsuite.arquillian.annotation.UncaughtServerErrorExpected; import org.keycloak.testsuite.auth.page.AuthRealm; import org.keycloak.testsuite.util.AdminClientUtil; +import org.keycloak.testsuite.util.GroupBuilder; import org.keycloak.testsuite.utils.tls.TLSUtils; import javax.ws.rs.ClientErrorException; @@ -65,6 +67,7 @@ import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Response; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.stream.Collectors; @@ -81,6 +84,7 @@ import static org.keycloak.testsuite.util.ServerURLs.getAuthServerContextRoot; * @author Bill Burke * @version $Revision: 1 $ */ +@EnableFeature(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) public class FineGrainAdminUnitTest extends AbstractKeycloakTest { public static final String CLIENT_NAME = "application"; @@ -97,6 +101,7 @@ public class FineGrainAdminUnitTest extends AbstractKeycloakTest { testRealmRep.setRealm(TEST); testRealmRep.setEnabled(true); testRealms.add(testRealmRep); + testRealmRep.setGroups(Arrays.asList(GroupBuilder.create().name("restricted-group").build())); } public static void setupDemo(KeycloakSession session) { @@ -656,14 +661,35 @@ public class FineGrainAdminUnitTest extends AbstractKeycloakTest { Assert.assertEquals(403, e.getResponse().getStatus()); } - UserRepresentation newGroupMemberWithAWrongGroup = createUserRepresentation("new-group-member", + UserRepresentation newEmptyGroupList = createUserRepresentation("new-group-member", + "new-group-member@keycloak.org", "New", "Member", true); + newEmptyGroupList.setGroups(Collections.emptyList()); + + try { + ApiUtil.createUserWithAdminClient(realmClient.realm(TEST), newEmptyGroupList); + Assert.fail("should fail with HTTP response code 403 Forbidden"); + } catch (WebApplicationException e) { + Assert.assertEquals(403, e.getResponse().getStatus()); + } + + UserRepresentation newGroupMemberWithNonExistentGroup = createUserRepresentation("new-group-member", "new-group-member@keycloak.org", "New", "Member", Arrays.asList("wrong-group"),true); try { - ApiUtil.createUserWithAdminClient(realmClient.realm(TEST), newGroupMemberWithAWrongGroup); - Assert.fail("should fail with HTTP response code 400 Bad Request"); + ApiUtil.createUserWithAdminClient(realmClient.realm(TEST), newGroupMemberWithNonExistentGroup); + Assert.fail("should fail with HTTP response code 403 Forbidden"); } catch (WebApplicationException e) { - Assert.assertEquals(400, e.getResponse().getStatus()); + Assert.assertEquals(403, e.getResponse().getStatus()); + } + + UserRepresentation newGroupMemberOfNotManagedGroup = createUserRepresentation("new-group-member", + "new-group-member@keycloak.org", "New", "Member", + Arrays.asList("restricted-group"),true); + try { + ApiUtil.createUserWithAdminClient(realmClient.realm(TEST), newGroupMemberOfNotManagedGroup); + Assert.fail("should fail with HTTP response code 403 Forbidden"); + } catch (WebApplicationException e) { + Assert.assertEquals(403, e.getResponse().getStatus()); } UserRepresentation newGroupMember = createUserRepresentation("new-group-member", diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java index 6b9cc62e7e..2f39931ea8 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java @@ -26,6 +26,7 @@ import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.keycloak.TokenVerifier; +import org.keycloak.admin.client.Keycloak; import org.keycloak.admin.client.resource.GroupResource; import org.keycloak.admin.client.resource.IdentityProviderResource; import org.keycloak.admin.client.resource.RealmResource; @@ -39,6 +40,7 @@ import org.keycloak.common.util.ObjectUtil; import org.keycloak.credential.CredentialModel; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; +import org.keycloak.jose.jws.JWSInput; import org.keycloak.models.Constants; import org.keycloak.models.LDAPConstants; import org.keycloak.models.PasswordPolicy; @@ -73,6 +75,7 @@ import org.keycloak.testsuite.pages.PageUtils; import org.keycloak.testsuite.pages.ProceedPage; import org.keycloak.testsuite.runonserver.RunHelpers; import org.keycloak.testsuite.updaters.Creator; +import org.keycloak.testsuite.util.AdminClientUtil; import org.keycloak.testsuite.util.AdminEventPaths; import org.keycloak.testsuite.util.ClientBuilder; import org.keycloak.testsuite.util.GreenMailRule; @@ -90,6 +93,7 @@ import javax.mail.internet.MimeMessage; import javax.ws.rs.BadRequestException; import javax.ws.rs.ClientErrorException; import javax.ws.rs.NotFoundException; +import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Response; import javax.ws.rs.core.UriBuilder; import java.io.IOException; @@ -118,6 +122,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.keycloak.testsuite.Assert.assertNames; import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.REMOTE; +import static org.keycloak.testsuite.auth.page.AuthRealm.TEST; /** * @author Stian Thorgersen @@ -2646,4 +2651,42 @@ public class UserTest extends AbstractAdminTest { group.setId(groupId); return group; } + + @Test + public void failCreateUserUsingRegularUser() throws Exception { + String regularUserId = ApiUtil.getCreatedId( + testRealm().users().create(UserBuilder.create().username("regular-user").password("password").build())); + UserResource regularUser = testRealm().users().get(regularUserId); + UserRepresentation regularUserRep = regularUser.toRepresentation(); + + try (Keycloak adminClient = AdminClientUtil.createAdminClient(suiteContext.isAdapterCompatTesting(), + TEST, regularUserRep.getUsername(), "password", Constants.ADMIN_CLI_CLIENT_ID, null)) { + UserRepresentation invalidUser = UserBuilder.create().username("do-not-create-me").build(); + + Response response = adminClient.realm(TEST).users().create(invalidUser); + assertEquals(Response.Status.FORBIDDEN.getStatusCode(), response.getStatus()); + + invalidUser.setGroups(Collections.emptyList()); + response = adminClient.realm(TEST).users().create(invalidUser); + + assertEquals(Response.Status.FORBIDDEN.getStatusCode(), response.getStatus()); + } + } + + @Test + public void testCreateUserDoNotGrantRole() { + testRealm().roles().create(RoleBuilder.create().name("realm-role").build()); + + try { + UserRepresentation userRep = UserBuilder.create().username("alice").password("password").addRoles("realm-role") + .build(); + String userId = ApiUtil.getCreatedId(testRealm().users().create(userRep)); + UserResource user = testRealm().users().get(userId); + List realmMappings = user.roles().getAll().getRealmMappings(); + + assertFalse(realmMappings.stream().map(RoleRepresentation::getName).anyMatch("realm-role"::equals)); + } finally { + testRealm().roles().get("realm-role").remove(); + } + } }