From d65649d5c0ac65b8ab705a1e07c8dc51e5321a35 Mon Sep 17 00:00:00 2001 From: vramik Date: Tue, 23 Apr 2024 15:12:04 +0200 Subject: [PATCH] Make sure organization are only manageable by the admin users with the manage-realm role Closes #28733 Signed-off-by: vramik --- .../jpa/entities/OrganizationEntity.java | 5 - .../organization/jpa/OrganizationAdapter.java | 18 +- .../models/OrganizationDomainModel.java | 14 +- .../OrganizationIdentityProviderResource.java | 5 + .../resource/OrganizationMemberResource.java | 6 + .../admin/resource/OrganizationResource.java | 23 ++- .../admin/AbstractOrganizationTest.java | 20 ++- .../organization/admin/OrganizationTest.java | 164 +++++++++++++++++- 8 files changed, 211 insertions(+), 44 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/OrganizationEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/OrganizationEntity.java index 5eed20202d..3cf1440e00 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/OrganizationEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/OrganizationEntity.java @@ -31,9 +31,6 @@ import jakarta.persistence.NamedQueries; import jakarta.persistence.NamedQuery; import jakarta.persistence.OneToMany; import jakarta.persistence.Table; -import org.hibernate.annotations.BatchSize; -import org.hibernate.annotations.Fetch; -import org.hibernate.annotations.FetchMode; @Table(name="ORG") @Entity @@ -60,8 +57,6 @@ public class OrganizationEntity { private String idpAlias; @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, mappedBy="organization") - @Fetch(FetchMode.SELECT) - @BatchSize(size = 20) protected Set domains = new HashSet<>(); public String getId() { diff --git a/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java b/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java index 1103c5d7cc..e4d118aeff 100644 --- a/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java @@ -17,16 +17,16 @@ package org.keycloak.organization.jpa; -import org.keycloak.models.GroupModel; - import java.util.HashSet; import java.util.Map; +import java.util.List; import java.util.Set; import java.util.Objects; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; +import org.keycloak.models.GroupModel; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.ModelValidationException; import org.keycloak.models.OrganizationDomainModel; @@ -37,8 +37,7 @@ import org.keycloak.models.jpa.entities.OrganizationDomainEntity; import org.keycloak.models.jpa.entities.OrganizationEntity; import org.keycloak.organization.OrganizationProvider; import org.keycloak.utils.EmailValidationUtil; - -import java.util.List; +import org.keycloak.utils.StringUtil; public final class OrganizationAdapter implements OrganizationModel, JpaModel { @@ -104,7 +103,7 @@ public final class OrganizationAdapter implements OrganizationModel, JpaModel modelMap = domains.stream() - .peek(this::validateDomainRepresentation) + .map(this::validateDomain) .collect(Collectors.toMap(OrganizationDomainModel::getName, Function.identity())); for (OrganizationDomainEntity domainEntity : new HashSet<>(this.entity.getDomains())) { @@ -122,7 +121,7 @@ public final class OrganizationAdapter implements OrganizationModel, JpaModel getMembers() { + auth.realm().requireManageRealm(); return provider.getMembersStream(organization).map(this::toRepresentation); } @@ -116,6 +118,7 @@ public class OrganizationMemberResource { @GET @Produces(MediaType.APPLICATION_JSON) public UserRepresentation get(@PathParam("id") String id) { + auth.realm().requireManageRealm(); if (StringUtil.isBlank(id)) { throw ErrorResponse.error("id cannot be null", Status.BAD_REQUEST); } @@ -126,6 +129,7 @@ public class OrganizationMemberResource { @Path("{id}") @DELETE public Response delete(@PathParam("id") String id) { + auth.realm().requireManageRealm(); if (StringUtil.isBlank(id)) { throw ErrorResponse.error("id cannot be null", Status.BAD_REQUEST); } @@ -139,6 +143,7 @@ public class OrganizationMemberResource { @PUT @Consumes(MediaType.APPLICATION_JSON) public Response update(@PathParam("id") String id, UserRepresentation user) { + auth.realm().requireManageRealm(); return new UserResource(session, getMember(id), auth, adminEvent).updateUser(user); } @@ -146,6 +151,7 @@ public class OrganizationMemberResource { @GET @Produces(MediaType.APPLICATION_JSON) public OrganizationRepresentation getOrganization(@PathParam("id") String id) { + auth.realm().requireManageRealm(); if (StringUtil.isBlank(id)) { throw ErrorResponse.error("id cannot be null", Status.BAD_REQUEST); } diff --git a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java b/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java index 9ce267f4a7..4ab681507b 100644 --- a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java +++ b/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java @@ -23,11 +23,9 @@ import java.util.Objects; import java.util.stream.Collectors; import java.util.stream.Stream; -import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.Consumes; import jakarta.ws.rs.DELETE; import jakarta.ws.rs.GET; -import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.POST; import jakarta.ws.rs.PUT; import jakarta.ws.rs.Path; @@ -44,6 +42,7 @@ import org.keycloak.models.OrganizationModel; import org.keycloak.organization.OrganizationProvider; import org.keycloak.representations.idm.OrganizationDomainRepresentation; import org.keycloak.representations.idm.OrganizationRepresentation; +import org.keycloak.services.ErrorResponse; import org.keycloak.services.resources.admin.AdminEventBuilder; import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator; import org.keycloak.utils.StringUtil; @@ -71,8 +70,9 @@ public class OrganizationResource { @POST @Consumes(MediaType.APPLICATION_JSON) public Response create(OrganizationRepresentation organization) { + auth.realm().requireManageRealm(); if (organization == null) { - throw new BadRequestException(); + throw ErrorResponse.error("Organization cannot be null.", Response.Status.BAD_REQUEST); } Set domains = organization.getDomains().stream().map(OrganizationDomainRepresentation::getName).collect(Collectors.toSet()); @@ -88,7 +88,8 @@ public class OrganizationResource { public Stream search( @Parameter(description = "A String representing an organization internet domain") @QueryParam("domain-name") String domainName ) { - if (domainName == null || domainName.trim().isEmpty()) { + auth.realm().requireManageRealm(); + if (StringUtil.isBlank(domainName)) { return provider.getAllStream().map(this::toRepresentation); } else { // search for the organization associated with the given domain @@ -101,8 +102,9 @@ public class OrganizationResource { @GET @Produces(MediaType.APPLICATION_JSON) public OrganizationRepresentation get(@PathParam("id") String id) { + auth.realm().requireManageRealm(); if (StringUtil.isBlank(id)) { - throw new BadRequestException(); + throw ErrorResponse.error("Id cannot be null.", Response.Status.BAD_REQUEST); } return toRepresentation(getOrganization(id)); @@ -111,8 +113,9 @@ public class OrganizationResource { @Path("{id}") @DELETE public Response delete(@PathParam("id") String id) { + auth.realm().requireManageRealm(); if (StringUtil.isBlank(id)) { - throw new BadRequestException(); + throw ErrorResponse.error("Id cannot be null.", Response.Status.BAD_REQUEST); } provider.remove(getOrganization(id)); @@ -124,6 +127,7 @@ public class OrganizationResource { @PUT @Consumes(MediaType.APPLICATION_JSON) public Response update(@PathParam("id") String id, OrganizationRepresentation organization) { + auth.realm().requireManageRealm(); OrganizationModel model = getOrganization(id); toModel(organization, model); @@ -143,14 +147,17 @@ public class OrganizationResource { } private OrganizationModel getOrganization(String id) { + //checking permissions before trying to find organization to be able to return 403 (instead of 400 or 404) + auth.realm().requireManageRealm(); + if (id == null) { - throw new BadRequestException(); + throw ErrorResponse.error("Id cannot be null.", Response.Status.BAD_REQUEST); } OrganizationModel model = provider.getById(id); if (model == null) { - throw new NotFoundException(); + throw ErrorResponse.error("Organization not found.", Response.Status.NOT_FOUND); } return model; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java index ea5b486624..e2dff35819 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java @@ -99,16 +99,10 @@ public abstract class AbstractOrganizationTest extends AbstractAdminTest { } protected OrganizationRepresentation createOrganization(String name, String orgDomain) { - OrganizationRepresentation org = new OrganizationRepresentation(); - - org.setName(name); + OrganizationRepresentation org = createRepresentation(name, orgDomain); String id; - OrganizationDomainRepresentation domainRep = new OrganizationDomainRepresentation(); - domainRep.setName(orgDomain); - org.addDomain(domainRep); - try (Response response = testRealm().organizations().create(org)) { assertEquals(Status.CREATED.getStatusCode(), response.getStatus()); id = ApiUtil.getCreatedId(response); @@ -123,6 +117,18 @@ public abstract class AbstractOrganizationTest extends AbstractAdminTest { return org; } + protected OrganizationRepresentation createRepresentation(String name, String orgDomain) { + OrganizationRepresentation org = new OrganizationRepresentation(); + + org.setName(name); + + OrganizationDomainRepresentation domainRep = new OrganizationDomainRepresentation(); + domainRep.setName(orgDomain); + org.addDomain(domainRep); + + return org; + } + protected UserRepresentation addMember(OrganizationResource organization) { return addMember(organization, memberEmail); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java index 1074609df6..12c6020bd6 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationTest.java @@ -19,29 +19,50 @@ package org.keycloak.testsuite.organization.admin; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; - +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import org.hamcrest.Matchers; import org.junit.Test; +import org.keycloak.admin.client.Keycloak; import org.keycloak.admin.client.resource.OrganizationResource; +import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.common.Profile.Feature; +import org.keycloak.models.AdminRoles; +import org.keycloak.models.Constants; +import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.idm.OrganizationDomainRepresentation; import org.keycloak.representations.idm.OrganizationRepresentation; +import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.arquillian.annotation.EnableFeature; +import org.keycloak.testsuite.util.AdminClientUtil; +import org.keycloak.testsuite.util.UserBuilder; @EnableFeature(Feature.ORGANIZATION) public class OrganizationTest extends AbstractOrganizationTest { + @Override + public void configureTestRealm(RealmRepresentation testRealm) { + testRealm.getUsers().add(UserBuilder.create().username("realmAdmin").password("password") + .role(Constants.REALM_MANAGEMENT_CLIENT_ID, AdminRoles.MANAGE_REALM) + .role(Constants.REALM_MANAGEMENT_CLIENT_ID, AdminRoles.MANAGE_IDENTITY_PROVIDERS) + .role(Constants.REALM_MANAGEMENT_CLIENT_ID, AdminRoles.MANAGE_USERS) + .build()); + } + @Test public void testUpdate() { OrganizationRepresentation expected = createOrganization(); @@ -237,4 +258,139 @@ public class OrganizationTest extends AbstractOrganizationTest { assertEquals(1, existing.getDomains().size()); assertNotNull(existing.getDomain("acme.com")); } + + @Test + public void permissionsTest() throws Exception { + try ( + Keycloak manageRealmAdminClient = AdminClientUtil.createAdminClient(suiteContext.isAdapterCompatTesting(), + TEST_REALM_NAME, "realmAdmin", "password", Constants.ADMIN_CLI_CLIENT_ID, null); + Keycloak userAdminClient = AdminClientUtil.createAdminClient(suiteContext.isAdapterCompatTesting(), + TEST_REALM_NAME, "test-user@localhost", "password", Constants.ADMIN_CLI_CLIENT_ID, null) + ) { + RealmResource realmAdminResource = manageRealmAdminClient.realm(TEST_REALM_NAME); + RealmResource realmUserResource = userAdminClient.realm(TEST_REALM_NAME); + + /* Org */ + //create org + OrganizationRepresentation orgRep = createRepresentation("testOrg", "testOrg.org"); + String orgId; + try ( + Response userResponse = realmUserResource.organizations().create(orgRep); + Response adminResponse = realmAdminResource.organizations().create(orgRep) + ) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + assertThat(adminResponse.getStatus(), equalTo(Status.CREATED.getStatusCode())); + orgId = ApiUtil.getCreatedId(adminResponse); + getCleanup().addCleanup(() -> testRealm().organizations().get(orgId).delete().close()); + } + + //search for org + try { + realmUserResource.organizations().getAll("testOrg.org"); + fail("Expected ForbiddenException"); + } catch (ForbiddenException expected) {} + assertThat(realmAdminResource.organizations().getAll("testOrg.org"), Matchers.notNullValue()); + + //get org + try { + realmUserResource.organizations().get(orgId).toRepresentation(); + fail("Expected ForbiddenException"); + } catch (ForbiddenException expected) {} + assertThat(realmAdminResource.organizations().get(orgId).toRepresentation(), Matchers.notNullValue()); + + //update org + try (Response userResponse = realmUserResource.organizations().get(orgId).update(orgRep)) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + } + + //delete org + try (Response userResponse = realmUserResource.organizations().get(orgId).delete()) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + } + + /* IdP */ + IdentityProviderRepresentation idpRep = new IdentityProviderRepresentation(); + idpRep.setAlias("dummy"); + idpRep.setProviderId("oidc"); + //create IdP + try ( + Response userResponse = realmUserResource.organizations().get(orgId).identityProvider().create(idpRep); + Response adminResponse = realmAdminResource.organizations().get(orgId).identityProvider().create(idpRep) + ) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + assertThat(adminResponse.getStatus(), equalTo(Status.CREATED.getStatusCode())); + getCleanup().addCleanup(() -> testRealm().organizations().get(orgId).identityProvider().delete().close()); + } + + //get IdP + try { + //we should get 403, not 400 or 404 etc. + realmUserResource.organizations().get("non-existing").identityProvider().toRepresentation(); + fail("Expected ForbiddenException"); + } catch (ForbiddenException expected) {} + try { + realmUserResource.organizations().get(orgId).identityProvider().toRepresentation(); + fail("Expected ForbiddenException"); + } catch (ForbiddenException expected) {} + assertThat(realmAdminResource.organizations().get(orgId).identityProvider().toRepresentation(), Matchers.notNullValue()); + + //update IdP + try (Response userResponse = realmUserResource.organizations().get(orgId).identityProvider().update(idpRep)) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + } + + //delete IdP + try (Response userResponse = realmUserResource.organizations().get(orgId).identityProvider().delete()) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + } + + /* Members */ + UserRepresentation userRep = UserBuilder.create() + .username("user@testOrg.org") + .email("user@testOrg.org") + .build(); + String userId; + + //create member + try ( + Response userResponse = realmUserResource.organizations().get(orgId).members().addMember(userRep); + Response adminResponse = realmAdminResource.organizations().get(orgId).members().addMember(userRep) + ) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + assertThat(adminResponse.getStatus(), equalTo(Status.CREATED.getStatusCode())); + userId = ApiUtil.getCreatedId(adminResponse); + assertThat(userId, Matchers.notNullValue()); + getCleanup().addCleanup(() -> testRealm().organizations().get(orgId).members().member(userId).delete().close()); + } + + //get members + try { + //we should get 403, not 400 or 404 etc. + realmUserResource.organizations().get("non-existing").members().getAll(); + fail("Expected ForbiddenException"); + } catch (ForbiddenException expected) {} + try { + realmUserResource.organizations().get(orgId).members().getAll(); + fail("Expected ForbiddenException"); + } catch (ForbiddenException expected) {} + assertThat(realmAdminResource.organizations().get(orgId).members().getAll(), Matchers.notNullValue()); + + //get member + try { + realmUserResource.organizations().get(orgId).members().member(userId).toRepresentation(); + fail("Expected ForbiddenException"); + } catch (ForbiddenException expected) {} + assertThat(realmAdminResource.organizations().get(orgId).members().member(userId).toRepresentation(), Matchers.notNullValue()); + + //update member + try (Response userResponse = realmUserResource.organizations().get(orgId).members().member(userId).update(userRep)) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + } + + //delete member + try (Response userResponse = realmUserResource.organizations().get(orgId).members().member(userId).delete()) { + assertThat(userResponse.getStatus(), equalTo(Status.FORBIDDEN.getStatusCode())); + } + } + } }