Add Groups resource type and scopes to authorization schema

[akshayutture-augment]

Test 6

---

Replicated from ai-code-review-evaluation/keycloak-greptile#6

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

PR Review Complete

Tasks

• Read repository guidelines (CLAUDE.md)
• Examine PR diff and understand scope of changes
• Review authorization schema changes
• Review new GroupPermissionsV2 implementation
• Review permission evaluator changes
• Review resource usage and remaining files
• Check for security issues
• Provide comprehensive feedback

---

Summary

This PR adds comprehensive support for Groups as a first-class resource type in the Fine-Grained Admin Permissions (FGAP) V2 authorization system. The implementation introduces group-specific scopes (manage-membership, manage-members, view-members) and integrates them with the existing user permission evaluation to enable group-based user access control.

The implementation is well-structured and follows established patterns in the codebase. The test coverage is extensive with 320 lines of new integration tests covering the main use cases.

Key Changes

1. Authorization Schema Extension (AdminPermissionsSchema.java)

• ✅ Adds GROUPS_RESOURCE_TYPE with 5 scopes: manage, view, manage-membership, manage-members, view-members
• ✅ Properly handles group resolution by ID in resolveGroup()
• ✅ Extends getOrCreateResource() to handle Groups resource type
• ✅ Includes resource cleanup logic in removeResource() and removeOrphanResources()

2. New GroupPermissionsV2 Implementation (GroupPermissionsV2.java)

• ✅ Extends GroupPermissions and overrides permission evaluation methods
• ✅ Properly delegates to admin roles first, then evaluates FGAP permissions
• ✅ Implements fallback to "all-groups" resource when specific group resource not found
• ✅ Correctly throws UnsupportedOperationException for V1-specific methods

3. User Permission Integration (UserPermissionsV2.java, UserPermissions.java)

• ✅ Integrates group-based permissions with user permissions via canViewByGroup() and canManageByGroup()
• ✅ Allows viewing/managing users through group membership permissions
• ✅ Properly traverses group hierarchy in evaluateHierarchy()

4. Test Coverage (GroupResourceTypeEvaluationTest.java)

• ✅ Comprehensive test scenarios covering all major use cases
• ✅ Tests permission inheritance and fallback to all-groups
• ✅ Validates integration with user permissions

---

Issues & Recommendations

🔴 Critical Issues

1. Inconsistent Permission Logic in GroupPermissionsV2.canManage() (Line 70)

Location: GroupPermissionsV2.java:70

public boolean canManage() {
    if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) {
        return true;
    }
    
    return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);
}

Problem: The method checks for both VIEW and MANAGE scopes when determining if a user can manage all groups. This is incorrect - VIEW permission should not grant MANAGE capabilities.

Expected: Should only check for MANAGE scope:

return hasPermission(null, AdminPermissionsSchema.MANAGE);

Impact: This could allow users with only VIEW permissions on all-groups to perform management operations, which is a privilege escalation vulnerability.

---

🟡 High Priority Issues

2. Missing Resource Existence Check in GroupPermissionsV2.getGroupIdsWithViewPermission()

Location: GroupPermissionsV2.java:121-124

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

Problem: The method uses groupResource.getId() as the group ID, but resources in FGAP V2 use the resource name, not ID. According to AdminPermissionsSchema.resolveGroup() line 185, the resource name is set to group.getId().

Expected: Should use groupResource.getName() instead:

granted.add(groupResource.getName());

Impact: This would return authorization resource IDs instead of actual group IDs, breaking functionality for clients expecting group IDs.

---

3. Potential NPE in Resource Type Resource Lookup

Location: GroupPermissionsV2.java:144

resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.GROUPS_RESOURCE_TYPE);

// check if there is a permission for "all-groups". If so, proceed with the evaluation to check scopes
if (policyStore.findByResource(server, resource).isEmpty()) {
    return false;
}

Problem: If getResourceTypeResource() returns null, the subsequent policyStore.findByResource(server, resource) will throw a NullPointerException.

Expected: Add null check:

if (resource == null || policyStore.findByResource(server, resource).isEmpty()) {
    return false;
}

Impact: Could cause application crashes when evaluating permissions for groups without proper resource setup.

---

🟢 Medium Priority Issues

4. Inconsistent Resource Naming Convention

Location: AdminPermissionsSchema.java:185

private String resolveGroup(KeycloakSession session, String id) {
    RealmModel realm = session.getContext().getRealm();
    GroupModel group = session.groups().getGroupById(realm, id);
    
    return group == null ? null : group.getId();
}

Observation: Unlike resolveUser() and resolveClient() which can accept both ID and username/clientId, resolveGroup() only accepts ID. This is inconsistent, though groups may not have a separate "name" field like users/clients.

Recommendation: Add a comment explaining why groups only support ID resolution, or consider supporting group name resolution if applicable.

---

5. Missing Javadoc for New Methods

Location: GroupPermissionEvaluator.java:143-147

/**
 * If {@link UserPermissionEvaluator#canView()} evaluates to {@code true}, returns empty set.
 * 
 * @return Stream of IDs of groups with view permission.
 */
Set<String> getGroupIdsWithViewPermission();

Issue: The documentation says "returns Stream" but the actual return type is Set<String>.

Recommendation: Update Javadoc to match actual return type.

---

6. Code Duplication in Permission Checks

Location: GroupPermissionsV2.java - Multiple methods follow identical patterns

The hasPermission() method is called with nearly identical logic in multiple places. Consider extracting common patterns to reduce duplication.

---

🔵 Low Priority / Style Issues

7. Unused Empty Line

Location: GroupPermissionsV2.java:299

Extra blank line before method definition. Minor style issue.

---

8. Test Method Naming Could Be More Descriptive

Location: GroupResourceTypeEvaluationTest.java

Some test method names like testManageGroup() could be more specific, e.g., testManageSpecificGroupWithPermission().

---

Security Considerations

✅ Good Security Practices Observed:

1. Proper privilege escalation prevention: Admin roles are checked first before falling back to FGAP permissions
2. Defense in depth: Multiple layers of permission checks (role-based + fine-grained)
3. Comprehensive test coverage: Tests include negative test cases (403 Forbidden scenarios)
4. Proper authorization context: Uses ResourcePermission for evaluation with proper scopes

⚠️ Security Concerns:

1. Issue Bump snyk/actions from 28606799782bc8e809f4076e9f8293bc4212d05e to 9adf32b1121593767fc3c057af55b55db032dc04 #1 (Critical): The canManage() method checking for VIEW scope could lead to privilege escalation
2. Group hierarchy traversal: The evaluateHierarchy() method correctly prevents cycles with the visited set, which is good
3. Resource cleanup: Proper orphan resource cleanup is implemented to prevent stale permissions

---

Performance Considerations

1. Group hierarchy evaluation (UserPermissions.java:572-580) uses recursive traversal with cycle detection. For deep hierarchies, this could be expensive. Consider adding metrics/logging for deep traversals.

2. Multiple database lookups in permission evaluation: Each permission check may trigger multiple resource store lookups. Consider caching strategy for frequently accessed resources.

---

Testing Gaps

While the test coverage is extensive, consider adding:

1. Tests for group hierarchy: Verify that permissions on parent groups properly affect child groups
2. Tests for edge cases: Empty groups, groups with no members, deleted groups
3. Performance tests: Large numbers of groups/users with complex permission structures
4. Negative tests for Issue Bump snyk/actions from 28606799782bc8e809f4076e9f8293bc4212d05e to 9adf32b1121593767fc3c057af55b55db032dc04 #1: Verify that VIEW permission alone cannot grant MANAGE capabilities

---

Recommendations

Must Fix (Before Merge):

1. ✅ Fix Issue Bump snyk/actions from 28606799782bc8e809f4076e9f8293bc4212d05e to 9adf32b1121593767fc3c057af55b55db032dc04 #1 - Remove VIEW scope from canManage() method
2. ✅ Fix Issue Bump aquasecurity/trivy-action from 0.32.0 to 0.33.1 #2 - Use getName() instead of getId() in getGroupIdsWithViewPermission()
3. ✅ Fix Issue Bump actions/download-artifact from 4.3.0 to 6.0.0 #3 - Add null check for resource in fallback logic

Should Fix:

4. Update Javadoc for getGroupIdsWithViewPermission() to reflect actual return type
5. Add comment explaining group name resolution strategy
6. Add test cases for the critical permission boundary conditions

Nice to Have:

7. Refactor duplicated permission checking patterns
8. Add performance monitoring for deep group hierarchies
9. Improve test method naming for clarity

---

Conclusion

This is a well-designed implementation that properly extends the FGAP V2 system to support Groups. The code follows established patterns and includes comprehensive test coverage. However, there are 3 critical/high-priority issues that must be addressed before merging, particularly the privilege escalation issue in canManage().

Once these issues are fixed, this PR will be ready to merge. Great work on the comprehensive testing and documentation! 🎉

---