Skip to content

Add Groups resource type and scopes to authorization schema#5

Open
zaibkhan wants to merge 1 commit into
feature-groups-authz-baselinefrom
feature-groups-authz-implementation
Open

Add Groups resource type and scopes to authorization schema#5
zaibkhan wants to merge 1 commit into
feature-groups-authz-baselinefrom
feature-groups-authz-implementation

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

…ation implementation

Closes #35562

Signed-off-by: vramik <vramik@redhat.com>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Simplify group filtering, remove unused delegator
What’s good: Removal of redundant canViewGlobal checks in group listing simplifies logic without changing behavior; delegating directly to session.groups().searchGroupsByAttributes removes an unnecessary wrapper.
Review Status: ❌ Requires changes
Overall Priority: Critical
Review Update:
• Coverage: Reviewed all 19 files across 2 batches

Issues (Critical & High only)

Severity Issue Why it matters
Critical Security — Over-permissive canManage() allows VIEW to grant manage r... …/permissions/GroupPermissionsV2.java
Granting manage via VIEW scope is over-permissive and allows manage operations (create/update groups) with only view permissions. Restrict to MANAGE scope to avoid privilege escalation and align intent.

Showing top 1 issues. Critical: 1, High: 0. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Ensure consistency of permission checks across user search paths; other list endpoints retain explicit canView filtering while UsersResource.searchForUser now relies on downstream logic.
  • Testing: Consider adding/adjusting tests to verify that user search results are still filtered by per-user view permissions (not only by group scoping), especially for evaluators with partial permissions.
  • Documentation: If the intent is to centralize permission enforcement in toRepresentation or the user store via session attributes, document this to avoid future double-filtering or regressions.
  • Compatibility: Switching from getGroupsWithViewPermission() to getGroupIdsWithViewPermission() implies downstream APIs now expect IDs; confirm all consumers support IDs to avoid runtime mismatches across storage providers.
  • Performance: Minor micro-optimizations (method references, fewer local vars) are fine; no notable performance regressions observed.
  • Security: Verify that user search results remain permission-filtered after removing usersEvaluator::canView; if not centrally enforced, this can leak users outside allowed scopes.
  • Open questions: After removing usersEvaluator::canView from UsersResource.searchForUser, does toRepresentation(...) or the user store reliably enforce per-user view permissions (beyond group scoping)? If not, we should reintroduce filtering to prevent overexposure.

Confidence: 2/5 — Not ready to merge (1 critical · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

return true;
}

return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Critical: Granting manage via VIEW scope is over-permissive and allows manage operations (create/update groups) with only view permissions. Restrict to MANAGE scope to avoid privilege escalation and align intent.

Suggested change
return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);
return hasPermission(null, AdminPermissionsSchema.MANAGE);

Set<String> granted = new HashSet<>();

resourceStore.findByType(server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE, groupResource -> {
if (hasPermission(groupResource.getId(), AdminPermissionsSchema.VIEW_MEMBERS, AdminPermissionsSchema.MANAGE_MEMBERS)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Resource.getId() is the internal resource store ID, but hasPermission expects the resource name (group ID). This prevents correct per-group permission checks and causes full-realm searches instead of group-filtered queries. Use resource.getName() for both the check and the returned IDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants