Skip to content

Incorrect authorization in admin role-composite deletion - delegated … #51136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vramik wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Incorrect authorization in admin role-composite deletion - delegated … #51136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<RoleRepresentation> roles) {
// any role by ID
RoleModel role = getRoleModel(id);
auth.roles().requireManage(role);
addComposites(auth, adminEvent, session.getContext().getUri(), roles, role);
Expand Down Expand Up @@ -308,7 +309,7 @@ public void deleteComposites(final @Parameter(description = "Role id") @PathPara
@Parameter(description = "A set of roles to be removed") List<RoleRepresentation> roles) {
RoleModel role = getRoleModel(id);
auth.roles().requireManage(role);
deleteComposites(adminEvent, session.getContext().getUri(), roles, role);
deleteComposites(auth, adminEvent, session.getContext().getUri(), roles, role);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,13 +481,13 @@ public Stream<RoleRepresentation> 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<RoleRepresentation> 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);
}

/**
Expand Down
3 changes: 2 additions & 1 deletion services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,13 @@ protected Stream<RoleRepresentation> getClientRoleComposites(ClientModel app, Ro
.map(ModelToRepresentation::toBriefRepresentation);
}

protected void deleteComposites(AdminEventBuilder adminEvent, UriInfo uriInfo, List<RoleRepresentation> roles, RoleModel role) {
protected void deleteComposites(AdminPermissionEvaluator auth, AdminEventBuilder adminEvent, UriInfo uriInfo, List<RoleRepresentation> 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);
}

Expand Down
77 changes: 77 additions & 0 deletions ...ase/src/test/java/org/keycloak/tests/admin/authz/fgap/RoleResourceTypeEvaluationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading