Hi there 👋
| 🌥️ Tech |
|---|
| 3 high, 2 medium, 2 low |
This review identifies a critical bug in GroupPermissionsV2.java where getGroupIdsWithViewPermission() uses groupResource.getId() (the authorization resource's database UUID) instead of groupResource.getName() (the group model ID). This causes individual group permission checks to always fall back to the type-level "all-groups" resource, effectively ignoring per-group VIEW_MEMBERS/MANAGE_MEMBERS permissions. Additionally, the returned set contains resource DB UUIDs instead of group model IDs, breaking downstream user queries such as UsersResource.searchForUser() and getUsersCount(). Another critical issue is that the method does not filter out the type-level resource (named "Groups"), which would be added to the result set when a user has all-groups permission, causing downstream failures. A security concern is noted in GroupPermissionsV2.canManage() where VIEW scope is included in the type-level check, potentially granting manage capabilities to users with only view permission. Minor issues include a Javadoc mismatch in GroupPermissionEvaluator.java, a double semicolon in test code, and incomplete getResourceName() handling for Groups/Clients in AdminPermissionsSchema.java. The test infrastructure changes are clean and safe.
These might need a close look
- 🔴 services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (L121)
CRITICAL BUG: groupResource.getId() returns the resource's database UUID, not the group model ID. Inside hasPermission(), this value is used as a name lookup key via resourceStore.findByName(). Since individual group resources are named with the group model ID, the lookup always fails, causing every check to fall through to the type-level 'all-groups' resource. Fix: use groupResource.getName() instead.
- 🔴 services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (L122)
CRITICAL BUG: The granted set adds groupResource.getId() (resource DB UUID) instead of the actual group model ID. Downstream callers (e.g., UsersResource.searchForUser, session.users().getUsersCount()) expect group model IDs. This will cause silent failures in group-filtered user lookups. Fix: use granted.add(groupResource.getName()).
- 🔴 services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (L120)
CRITICAL BUG: findByType(...) iterates the type-level resource (named "Groups") in addition to individual group resources. When a user has all-groups permission, the string "Groups" would be added to the result set, which is not a valid group ID and would break downstream queries. Fix: filter out the type-level resource by checking the resource name.
Worth checking
- 🟡 services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java (L62)
SECURITY CONCERN: canManage() includes AdminPermissionsSchema.VIEW in the type-level scope check, meaning a user with only VIEW permission on all groups would pass canManage(). This grants the ability to create top-level groups. Consider checking only MANAGE scope here, or add a clarifying comment.
- 🟡 server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (L330)
INCOMPLETE: getResourceName() only handles USERS_RESOURCE_TYPE. For GROUPS and CLIENTS resource types, it falls through to resource.getDisplayName(), which may return null or raw internal names. This affects the FGAP UI display for group and client resources. Consider adding handling for GROUPS and CLIENTS resource types.
Small things (take or leave)
- 🔵 services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java (L146)
MINOR: Javadoc @return tag says "Stream of IDs" but the return type is Set<String>. Update the description to say "Set of IDs".
- 🔵 tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java (L53)
MINOR: Double semicolon ;; at the end of a field declaration. Remove the extra semicolon.
FriendlyReviewer detected both golden bugs and, in the case of the
getId()vsgetName()bug (#2), decomposed it into 3 separate findings at different locations in the same file (L120, L121, L122) — more precise than the single golden comment that described the same issue.The
canManage()bug (#1) was correctly identified but classified as MEDIUM by FriendlyReviewer. The golden severity (HIGH) is more accurate here: allowing VIEW-only users to create groups constitutes privilege escalation.Only 3 supplementary findings — the lowest count among all Keycloak PRs — but that is consistent with a PR where the golden comments already covered the two critical bugs. The incomplete
getResourceName()handling for GROUPS and CLIENTS is a real UI bug.This is a clean sweep: all golden bugs found, none missed, and the supplementary findings are all legitimate.