From ce2c7d9fc072e4eaf7ee0a3e7be31f45c959dbd3 Mon Sep 17 00:00:00 2001 From: vramik Date: Mon, 13 Jul 2026 14:53:52 +0200 Subject: [PATCH] Incorrect authorization in admin role-composite deletion - delegated admin removes privileged child roles it cannot map Closes #51112 Signed-off-by: vramik --- .../resources/admin/RoleByIdResource.java | 3 +- .../admin/RoleContainerResource.java | 4 +- .../resources/admin/RoleResource.java | 3 +- .../fgap/RoleResourceTypeEvaluationTest.java | 77 +++++++++++++++++++ 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java index bb095c7cfbc9..74774035ec4f 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java @@ -197,6 +197,7 @@ public void updateRole(final @Parameter(description = "id of role") @PathParam(" @APIResponse(responseCode = "403", description = "Forbidden") }) public void addComposites(final @PathParam("role-id") String id, List roles) { + // any role by ID RoleModel role = getRoleModel(id); auth.roles().requireManage(role); addComposites(auth, adminEvent, session.getContext().getUri(), roles, role); @@ -308,7 +309,7 @@ public void deleteComposites(final @Parameter(description = "Role id") @PathPara @Parameter(description = "A set of roles to be removed") List roles) { RoleModel role = getRoleModel(id); auth.roles().requireManage(role); - deleteComposites(adminEvent, session.getContext().getUri(), roles, role); + deleteComposites(auth, adminEvent, session.getContext().getUri(), roles, role); } /** diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java index 81fd41a8c324..eda3e6fc369d 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java @@ -481,13 +481,13 @@ public Stream getClientRoleComposites(final @Parameter(descr public void deleteComposites( final @Parameter(description = "role's name (not id!)") @PathParam("role-name") String roleName, @Parameter(description = "roles to remove") List roles) { - + // realm/client roles by name auth.roles().requireManage(roleContainer); RoleModel role = roleContainer.getRole(roleName); if (role == null) { throw new NotFoundException("Could not find role"); } - deleteComposites(adminEvent, uriInfo, roles, role); + deleteComposites(auth, adminEvent, uriInfo, roles, role); } /** diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java index 0f7f7bca3d8a..f89d14803587 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java @@ -152,12 +152,13 @@ protected Stream getClientRoleComposites(ClientModel app, Ro .map(ModelToRepresentation::toBriefRepresentation); } - protected void deleteComposites(AdminEventBuilder adminEvent, UriInfo uriInfo, List roles, RoleModel role) { + protected void deleteComposites(AdminPermissionEvaluator auth, AdminEventBuilder adminEvent, UriInfo uriInfo, List roles, RoleModel role) { for (RoleRepresentation rep : roles) { RoleModel composite = realm.getRoleById(rep.getId()); if (composite == null) { throw new NotFoundException("Could not find composite role"); } + auth.roles().requireMapComposite(composite); role.removeCompositeRole(composite); } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RoleResourceTypeEvaluationTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RoleResourceTypeEvaluationTest.java index 3fca92201ccd..a274045648a5 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RoleResourceTypeEvaluationTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/RoleResourceTypeEvaluationTest.java @@ -266,6 +266,83 @@ public void testMapCompositeRoleAllRoles() { realmAdminClient.realm(realm.getName()).roles().get("myRole").addComposites(List.of(subRole)); } + @Test + public void testDeleteCompositeRoleRequiresMapCompositePermission() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + ClientRepresentation myclient = realm.admin().clients().findByClientId("myclient").get(0); + String myclientId = myclient.getId(); + + // create client sub-roles on myclient (using client roles avoids manage-realm bypassing canMapComposite) + RoleRepresentation allowedSubRole = new RoleRepresentation(); + allowedSubRole.setName("allowedSubRole"); + realm.admin().clients().get(myclientId).roles().create(allowedSubRole); + allowedSubRole = realm.admin().clients().get(myclientId).roles().get("allowedSubRole").toRepresentation(); + + RoleRepresentation restrictedSubRole = new RoleRepresentation(); + restrictedSubRole.setName("restrictedSubRole"); + realm.admin().clients().get(myclientId).roles().create(restrictedSubRole); + restrictedSubRole = realm.admin().clients().get(myclientId).roles().get("restrictedSubRole").toRepresentation(); + + // create parent realm role (for endpoint 1) and parent client role (for endpoints 2 & 3) + RoleRepresentation parentRealmRole = new RoleRepresentation(); + parentRealmRole.setName("parentRealmRole"); + realm.admin().roles().create(parentRealmRole); + parentRealmRole = realm.admin().roles().get("parentRealmRole").toRepresentation(); + realm.cleanup().add(r -> r.roles().get("parentRealmRole").remove()); + + RoleRepresentation parentClientRole = new RoleRepresentation(); + parentClientRole.setName("parentClientRole"); + realm.admin().clients().get(myclientId).roles().create(parentClientRole); + parentClientRole = realm.admin().clients().get(myclientId).roles().get("parentClientRole").toRepresentation(); + + // grant myadmin manage-realm (required for realm role endpoint's requireManage(RealmModel)) + String realmMgmtId = realm.admin().clients().findByClientId("realm-management").get(0).getId(); + RoleRepresentation manageRealmRole = realm.admin().clients().get(realmMgmtId).roles().get("manage-realm").toRepresentation(); + realm.admin().users().get(myadmin.getId()).roles().clientLevel(realmMgmtId).add(List.of(manageRealmRole)); + realmAdminClient.tokenManager().grantToken(); + + // grant FGAP MANAGE on myclient (for client role endpoints' requireManage) + UserPolicyRepresentation policy = createUserPolicy(realm, adminPermissionsClient, "Only My Admin User Policy", myadmin.getId()); + createPermission(adminPermissionsClient, myclientId, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE, Set.of(MANAGE), policy); + + // grant MAP_ROLE_COMPOSITE only on allowedSubRole + createPermission(adminPermissionsClient, allowedSubRole.getId(), rolesType, Set.of(MAP_ROLE_COMPOSITE), policy); + + // as full admin, add both sub-roles as composites of both parent roles + realm.admin().roles().get("parentRealmRole").addComposites(List.of(allowedSubRole, restrictedSubRole)); + realm.admin().clients().get(myclientId).roles().get("parentClientRole").addComposites(List.of(allowedSubRole, restrictedSubRole)); + + // --- Endpoint 1: DELETE /admin/realms/{realm}/roles/{role-name}/composites --- + try { + realmAdminClient.realm(realm.getName()).roles().get("parentRealmRole").deleteComposites(List.of(restrictedSubRole)); + fail("Should not be able to delete composite without MAP_ROLE_COMPOSITE permission"); + } catch (Exception ex) { + assertThat(ex, instanceOf(ForbiddenException.class)); + } + realmAdminClient.realm(realm.getName()).roles().get("parentRealmRole").deleteComposites(List.of(allowedSubRole)); + + // --- Endpoint 3: DELETE /admin/realms/{realm}/clients/{id}/roles/{role-name}/composites --- + try { + realmAdminClient.realm(realm.getName()).clients().get(myclientId).roles().get("parentClientRole").deleteComposites(List.of(restrictedSubRole)); + fail("Should not be able to delete composite without MAP_ROLE_COMPOSITE permission"); + } catch (Exception ex) { + assertThat(ex, instanceOf(ForbiddenException.class)); + } + realmAdminClient.realm(realm.getName()).clients().get(myclientId).roles().get("parentClientRole").deleteComposites(List.of(allowedSubRole)); + + // --- Endpoint 2: DELETE /admin/realms/{realm}/roles-by-id/{role-id}/composites --- + // re-add allowedSubRole as composite (was removed above) + realm.admin().clients().get(myclientId).roles().get("parentClientRole").addComposites(List.of(allowedSubRole)); + + try { + realmAdminClient.realm(realm.getName()).rolesById().deleteComposites(parentClientRole.getId(), List.of(restrictedSubRole)); + fail("Should not be able to delete composite without MAP_ROLE_COMPOSITE permission"); + } catch (Exception ex) { + assertThat(ex, instanceOf(ForbiddenException.class)); + } + realmAdminClient.realm(realm.getName()).rolesById().deleteComposites(parentClientRole.getId(), List.of(allowedSubRole)); + } + @Test public void testMapRoleOnlySpecificRole() { UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0);