Skip to content

Conversation

@akshayutture-augment
Copy link

…ation implementation

Closes #35562

Signed-off-by: vramik <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@akshayutture-augment
Copy link
Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Nov 14, 2025

Greptile Overview

Confidence Score: 1/5

  • This PR contains a critical security vulnerability that enables privilege escalation
  • The canManage() method in GroupPermissionsV2 incorrectly validates permissions by accepting VIEW scope when only MANAGE scope should grant management access. This is a severe security flaw that would allow users with view-only permissions to perform privileged management operations on groups. The bug was not caught by existing tests.
  • Critical fix required in services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java at line 70

Important Files Changed

File Analysis

Filename Score Overview
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java 4/5 Added Groups resource type with 5 scopes (manage, view, manage-membership, manage-members, view-members) and resolved group resources by ID
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java 1/5 New V2 implementation for group permissions with CRITICAL SECURITY BUG: canManage() checks VIEW scope instead of MANAGE scope
services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java 4/5 Refactored methods to improve clarity, changed visibility of fields to protected for V2 subclass, and fixed canViewMembers() logic
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/GroupResourceTypeEvaluationTest.java 4/5 Comprehensive test suite for group permissions with view, manage, membership, and members scopes - does not test canManage() global bug

Sequence Diagram

sequenceDiagram
    participant Client as Admin Client
    participant GRes as GroupsResource
    participant GPerm as GroupPermissionsV2
    participant Schema as AdminPermissionsSchema
    participant AuthZ as AuthorizationProvider

    Client->>GRes: Request to view groups
    GRes->>GPerm: Check canView()
    
    alt Has admin role
        GPerm-->>GRes: Permitted by role
    else Check fine-grained authz
        GPerm->>Schema: Get Groups resource type
        GPerm->>AuthZ: Evaluate permission
        AuthZ-->>GPerm: Permission result
        GPerm-->>GRes: Permitted by policy
    end
    
    alt User authorized
        GRes->>GPerm: Filter groups - canView(group)
        loop Each group
            GPerm->>Schema: Get group resource
            GPerm->>AuthZ: Evaluate group permission
            AuthZ-->>GPerm: Allowed or denied
        end
        GRes-->>Client: Filtered group list
    else Not authorized
        GRes-->>Client: ForbiddenException
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

19 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

return true;
}

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

Choose a reason for hiding this comment

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

logic: canManage() incorrectly checks VIEW scope - should only check MANAGE scope

Suggested change
return hasPermission(null, AdminPermissionsSchema.VIEW, AdminPermissionsSchema.MANAGE);
return hasPermission(null, AdminPermissionsSchema.MANAGE);
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionsV2.java
Line: 70:70

Comment:
**logic:** `canManage()` incorrectly checks `VIEW` scope - should only check `MANAGE` scope

```suggestion
        return hasPermission(null, AdminPermissionsSchema.MANAGE);
```

How can I resolve this? If you propose a fix, please make it concise.

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.

3 participants