From c2a369a6f270d8813b96db0028f5f373398994cd Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Thu, 16 Dec 2021 18:24:49 -0300 Subject: [PATCH] Verify fine-grained admin permissions feature is enabled before checking fine-grained permissions when creating users --- CHANGELOG.md | 18 +++--- .../resources/admin/UsersResource.java | 56 +++++++++++++------ .../admin/FineGrainAdminUnitTest.java | 34 +++++++++-- .../keycloak/testsuite/admin/UserTest.java | 38 +++++++++++++ 4 files changed, 117 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0a73d2b9697..ec8e181753d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 For Keycloak upstream changelog please see https://www.keycloak.org/docs/latest/release_notes/index.html. Full Keycloak upstream jira issue can be shown if filtered by Fix version. For example [Keycloak jira issue for 15.0.2 version](https://issues.redhat.com/browse/KEYCLOAK-19161?jql=project%20%3D%20keycloak%20and%20fixVersion%20%3D%2015.0.2) -## [v15.0.2-r1.0.6] - 10-12-2021 +## [v15.0.2-r1.0.7] - 2021-12-23 +### Fixed +- Verify fine-grained admin permissions feature is enabled before checking fine-grained permissions when creating users + +## [v15.0.2-r1.0.6] - 2021-12-10 ### Added - SAML Federation sync mode [EOSC-KC-144](https://github.com/eosc-kc/keycloak/issues/144) -###Changed +### Changed - Increase User Attribute Value length to 4000 [EOSC-KC-132](https://github.com/eosc-kc/keycloak/issues/132) - Improve implementation in attribute Identity Provider mapper taking into account emailVerified User field. [EOSC-KC-70](https://github.com/eosc-kc/keycloak/issues/70) - FreeMarkerLoginFormsProvider now has an additional common attribute passed to the ftl templates, the "uriInfo" @@ -19,26 +23,26 @@ Full Keycloak upstream jira issue can be shown if filtered by Fix version. For e ### Fixed - Fixed SAML principals tooltip -## [v15.0.2-r1.0.5] - 16-11-2021 +## [v15.0.2-r1.0.5] - 2021-11-16 ### Added -the idpLoginFullUrl common attribute passed to the ftl templates for any theme except from the default -## [v15.0.2-r1.0.4] - 4-11-2021 +## [v15.0.2-r1.0.4] - 2021-11-04 ### Added - Attribute Identity Provider mapper taking into account emailVerified User field. [EOSC-KC-70](https://github.com/eosc-kc/keycloak/issues/70) ### Fixed - Fix a bug for Identity Providers in ModelToRepresentation -## [v15.0.2-r1.0.3] - 18-10-2021 +## [v15.0.2-r1.0.3] - 2021-10-18 ### Added - new release created on tag -## [v15.0.2-r1.0.2] - 5-10-2021 +## [v15.0.2-r1.0.2] - 2021-10-05 ### Fixed - Brokered SAML logins fail due to wrong InResponseTo in 15.x. [KEYCLOAK-19143](https://issues.redhat.com/browse/KEYCLOAK-19143) -## [v15.0.2-r1.0.1] - 29-9-2021 +## [v15.0.2-r1.0.1] - 2021-09-29 ### Added - Support for SAML IdP Federation - User reaccepting Terms and Conditions. [EOSC-KC-48](https://github.com/eosc-kc/keycloak/issues/48) 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 06d2da903bf0..e7768b7eaeb9 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 @@ -16,12 +16,11 @@ */ package org.keycloak.services.resources.admin; -import static org.keycloak.userprofile.UserProfileContext.USER_API; - 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; @@ -56,11 +55,20 @@ import javax.ws.rs.core.HttpHeaders; 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; + /** * Base resource for managing users * @@ -108,22 +116,8 @@ public Response createUser(final UserRepresentation rep) { // 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; } } @@ -192,6 +186,32 @@ public Response createUser(final UserRepresentation rep) { 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 4f05f4ebf0ef..7168777e0245 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.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.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.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 @@ * @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 void addTestRealms(List testRealms) { 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 void testRestEvaluation() throws Exception { 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 ab1a8b5d8feb..cb4fab5291c5 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.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.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; @@ -72,6 +74,7 @@ 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; @@ -89,6 +92,7 @@ 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; @@ -116,6 +120,7 @@ import static org.keycloak.testsuite.Assert.assertNames; import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.QUARKUS; import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.REMOTE; +import static org.keycloak.testsuite.auth.page.AuthRealm.TEST; import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude; import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer; @@ -2495,4 +2500,37 @@ private GroupRepresentation createGroup(RealmResource realm, GroupRepresentation 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()); + + 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)); + } }