Skip to content

Filter role mappings by visibility in group lookup by path #54919

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

Closed
pedroigor wants to merge 1 commit into from
Closed

Filter role mappings by visibility in group lookup by path #54919

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
7 changes: 7 additions & 0 deletions docs/documentation/upgrading/topics/changes/changes-26_8_3.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ Updating an existing client scope does not revalidate its stored mappers.
Existing deployments continue to start, because mappers that are read from the database at startup are not validated.
If you provision realms from an exported JSON file, import the file once after upgrading to find out whether it contains a mapper that is now rejected.

=== Group lookup by path filters role mappings by visibility
// https://github.com/keycloak/keycloak/issues/54356

The Admin REST API endpoint `GET /admin/realms/\{realm}/group-by-path/\{path}` now omits the role mappings that the caller is not allowed to view or to map to users, as the `groups/\{id}` endpoint already does.
Previously, any caller allowed to view the group received all of its role mappings in `realmRoles` and `clientRoles`.
For example, an administrator with only the `view-users` role no longer receives them, while an administrator with the `manage-users` role still does.
If an integration reads group role mappings through this endpoint, make sure its account is allowed to view the roles, for example through the `view-realm` and `view-clients` roles or the corresponding fine-grained permissions.

// ------------------------ Notable changes ------------------------ //
== Notable changes
Expand Down
1 change: 1 addition & 0 deletions services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1319,6 +1319,7 @@ public GroupRepresentation getGroupByPath(@PathParam("path") String path) {
}
auth.groups().requireView(found);
GroupRepresentation groupRep = ModelToRepresentation.toRepresentation(found, true);
GroupUtils.filterRolesInRepresentation(groupRep, realm, session, auth);
return GroupUtils.populateSubGroupCount(found, groupRep);
}

Expand Down
24 changes: 15 additions & 9 deletions tests/base/src/test/java/org/keycloak/tests/admin/PermissionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,10 +1173,13 @@ public void realmRoleMappingsVisibleWithManageUsers() {
assertThat(roleNames(manageUsers.groups().group(groupUuid).roles().clientLevel(clientUuid).listEffective()), Matchers.hasItem(clientRoleName));

// the group representation follows the same rule
GroupRepresentation groupRep = manageUsers.groups().group(groupUuid).toRepresentation();
assertThat(groupRep.getRealmRoles(), Matchers.hasItem(roleName));
assertThat(groupRep.getClientRoles(), Matchers.hasKey("role-mapping-client"));
assertThat(groupRep.getClientRoles().get("role-mapping-client"), Matchers.hasItem(clientRoleName));
for (GroupRepresentation groupRep : List.of(
manageUsers.groups().group(groupUuid).toRepresentation(),
manageUsers.getGroupByPath("/role-mapping-group"))) {
assertThat(groupRep.getRealmRoles(), Matchers.hasItem(roleName));
assertThat(groupRep.getClientRoles(), Matchers.hasKey("role-mapping-client"));
assertThat(groupRep.getClientRoles().get("role-mapping-client"), Matchers.hasItem(clientRoleName));
}

RealmResource viewUsers = clients.get(AdminRoles.VIEW_USERS).realm(REALM_NAME);

Expand All @@ -1188,11 +1191,14 @@ public void realmRoleMappingsVisibleWithManageUsers() {
assertThat(viewUsers.users().get(userUuid).roles().clientLevel(clientUuid).listEffective(), Matchers.empty());
assertThat(viewUsers.groups().group(groupUuid).roles().clientLevel(clientUuid).listAll(), Matchers.empty());
assertThat(viewUsers.groups().group(groupUuid).roles().clientLevel(clientUuid).listEffective(), Matchers.empty());
GroupRepresentation viewUsersGroupRep = viewUsers.groups().group(groupUuid).toRepresentation();
assertThat(viewUsersGroupRep.getRealmRoles() == null ? List.<String>of() : viewUsersGroupRep.getRealmRoles(),
Matchers.not(Matchers.hasItem(roleName)));
assertThat(viewUsersGroupRep.getClientRoles() == null ? Map.<String, List<String>>of() : viewUsersGroupRep.getClientRoles(),
Matchers.not(Matchers.hasKey("role-mapping-client")));
for (GroupRepresentation viewUsersGroupRep : List.of(
viewUsers.groups().group(groupUuid).toRepresentation(),
viewUsers.getGroupByPath("/role-mapping-group"))) {
assertThat(viewUsersGroupRep.getRealmRoles() == null ? List.<String>of() : viewUsersGroupRep.getRealmRoles(),
Matchers.not(Matchers.hasItem(roleName)));
assertThat(viewUsersGroupRep.getClientRoles() == null ? Map.<String, List<String>>of() : viewUsersGroupRep.getClientRoles(),
Matchers.not(Matchers.hasKey("role-mapping-client")));
}

// being able to view (and map) the role does not grant management of the role itself
String roleId = createdRole.getId();
Expand Down
22 changes: 12 additions & 10 deletions ...e/src/test/java/org/keycloak/tests/admin/authz/fgap/ClientResourceTypeEvaluationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,15 +952,17 @@ public void testGroupRepresentationFiltersRolesByVisibility() {
createPermission(adminPermissionsClient, mappableRealmRole.getId(), AdminPermissionsSchema.ROLES.getType(), Set.of(MAP_ROLE), policy);
createPermission(adminPermissionsClient, mappableClientRole.getId(), AdminPermissionsSchema.ROLES.getType(), Set.of(MAP_ROLE), policy);

GroupRepresentation groupRep = realmAdminClient.realm(realm.getName()).groups().group(targetGroup.getId()).toRepresentation();

assertThat(groupRep.getRealmRoles(), notNullValue());
assertThat(groupRep.getRealmRoles(), hasItem("GROUP_REP_MAPPABLE_REALM_ROLE"));
assertThat(groupRep.getRealmRoles(), not(hasItem("GROUP_REP_HIDDEN_REALM_ROLE")));

assertThat(groupRep.getClientRoles(), notNullValue());
assertThat(groupRep.getClientRoles(), hasKey("group-rep-hidden-client"));
assertThat(groupRep.getClientRoles().get("group-rep-hidden-client"), hasItem("GROUP_REP_MAPPABLE_CLIENT_ROLE"));
assertThat(groupRep.getClientRoles().get("group-rep-hidden-client"), not(hasItem("GROUP_REP_HIDDEN_CLIENT_ROLE")));
for (GroupRepresentation groupRep : List.of(
realmAdminClient.realm(realm.getName()).groups().group(targetGroup.getId()).toRepresentation(),
realmAdminClient.realm(realm.getName()).getGroupByPath("/" + targetGroup.getName()))) {
assertThat(groupRep.getRealmRoles(), notNullValue());
assertThat(groupRep.getRealmRoles(), hasItem("GROUP_REP_MAPPABLE_REALM_ROLE"));
assertThat(groupRep.getRealmRoles(), not(hasItem("GROUP_REP_HIDDEN_REALM_ROLE")));

assertThat(groupRep.getClientRoles(), notNullValue());
assertThat(groupRep.getClientRoles(), hasKey("group-rep-hidden-client"));
assertThat(groupRep.getClientRoles().get("group-rep-hidden-client"), hasItem("GROUP_REP_MAPPABLE_CLIENT_ROLE"));
assertThat(groupRep.getClientRoles().get("group-rep-hidden-client"), not(hasItem("GROUP_REP_HIDDEN_CLIENT_ROLE")));
}
}
}
Loading