SCIM : Support type "Group" in "members" field for child groups - #51478
SCIM : Support type "Group" in "members" field for child groups#51478cgeorgilakis wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Authorization leaks, hierarchy cycles, unsafe detachments, and incorrect move semantics must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds SCIM nested-group membership support.
Changes:
- Returns child groups as
Groupmembers. - Supports adding/removing subgroup members through PATCH.
- Adds request-builder support and integration tests.
File summaries
| File | Description |
|---|---|
GroupCoreModelSchema.java |
Maps and manages subgroup members. |
PatchRequest.java |
Adds structured member PATCH values. |
GroupTest.java |
Tests mixed user/group membership. |
Review details
Suppressed comments (1)
scim/model/src/main/java/org/keycloak/scim/model/group/GroupCoreModelSchema.java:185
- The untyped removal path can likewise detach any visible group named by the filter, even when that group is not a member of
model. Guard the parent relationship and perform the change through the realm move API.
model.removeChild(subGroup);
- Files reviewed: 3/3 changed files
- Comments generated: 6
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if (permissions.hasPermission(model, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, AdminPermissionsSchema.VIEW_MEMBERS)) { | ||
| members = session.users().getGroupMembersStream(realm, model) | ||
| .filter(this::canViewUser); | ||
| members.addAll(session.users().getGroupMembersStream(realm, model) | ||
| .filter(this::canViewUser).toList()); | ||
| } | ||
| members.addAll(model.getSubGroupsStream().toList()); |
There was a problem hiding this comment.
Is it possible in Keycloak model, a user to have access to a parent group and not to its subgroup?
| throw new ModelValidationException("Group with id " + member.getValue() + " not found"); | ||
| } | ||
| checkGroupMembershipPermission(session.getContext().getPermissions(), subGroup); | ||
| model.removeChild(subGroup); |
| } | ||
| if (!model.getId().equals(subGroup.getParentId())) { | ||
| checkGroupMembershipPermission(session.getContext().getPermissions(), subGroup); | ||
| model.addChild(subGroup); |
| } else { | ||
| GroupModel subGroup = session.groups().getGroupById(realm, member.getValue()); | ||
| if (subGroup == null || !canViewGroup(subGroup)) { | ||
| throw new ModelValidationException("User or Group ` with id " + member.getValue() + " not found"); |
| if (model.getId().equals(member.getValue())) { | ||
| throw new ModelValidationException("A group cannot be a member of itself"); | ||
| } |
There was a problem hiding this comment.
This is correct. However, I don't see any such check in Admin REST API.
Do we need it? Generally, I believe that we should have the same check as Admin REST API.
| public Builder add(String path, Object objectValue) { | ||
| try { | ||
| String json = JsonSerialization.writeValueAsString(objectValue); | ||
| operation("add", path, json); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| return this; | ||
| } |
Closes keycloak#51078 Signed-off-by: cgeorgilakis-grnet <cgeorgilakis@admin.grnet.gr>
a0972b7 to
99c2ead
Compare
|
I have made changes based on Copilot comments except my comments in some of its proposals. |
There was a problem hiding this comment.
🟡 Changes recommended
Subgroup membership is not included in existing members.value filtering.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
scim/model/src/main/java/org/keycloak/scim/model/group/GroupCoreModelSchema.java:76
members.valuenow returns subgroup IDs, but filtering that same attribute still joins onlyUserGroupMembershipEntity(GroupResourceTypeProvider.java:205-210). As a result,GET /Groups?filter=members.value eq "<child-group-id>"omits the parent even though its returnedmemberscontains that child; extend the filter predicate to include groups whose child has that ID.
members.addAll(model.getSubGroupsStream().toList());
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
Group-member filtering remains user-only, and prior authorization and hierarchy-cycle concerns remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
scim/model/src/main/java/org/keycloak/scim/model/group/GroupCoreModelSchema.java:76
- Adding subgroups to
membersmakesmembers.valuecover both users and groups, butGroupResourceTypeProvider.getAttributeExpressionstill joins onlyUserGroupMembershipEntity; consequently, filtering Groups by a child-group ID omits the parent even though GET returns that child inmembers. Extend the query mapping to match child groups via theirparentIdand cover this alongside the existing user-member filter test.
members.addAll(model.getSubGroupsStream().toList());
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Closes #51078