Skip to content

Conversation

@akshayutture-augment
Copy link

…valuation implementation for ClientsPermissionsV2

Closes #35564

Signed-off-by: Martin Kanis <[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: 3/5

  • Safe to merge after fixing the missing CLIENTS handling in getResourceName method
  • The implementation follows established patterns and includes comprehensive test coverage, but contains a critical bug where the getResourceName method doesn't handle the new CLIENTS resource type. This will cause incorrect display names in the admin UI when viewing client permissions. Once fixed, the PR should be safe to merge.
  • server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java - Missing CLIENTS handling in getResourceName method (lines 310-322)

Important Files Changed

File Analysis

Filename Score Overview
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java 2/5 Added CLIENTS resource type with scopes (configure, manage, map-roles, etc.) and resolveClient method, but missing CLIENTS handling in getResourceName method which will cause display name issues
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java 4/5 New V2 implementation with Client resource type authorization support, implementing permission checks for configure, manage, view, and map-roles scopes
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java 4/5 Added comprehensive tests for client permissions including manage, configure, view, and map-roles scopes with positive and negative test cases

Sequence Diagram

sequenceDiagram
    participant Admin as Admin User
    participant API as Admin REST API
    participant CPV2 as ClientPermissionsV2
    participant APS as AdminPermissionsSchema
    participant RS as ResourceStore
    participant AuthZ as AuthorizationProvider
    participant Client as ClientModel

    Admin->>API: Request client operation (view/manage/configure)
    API->>CPV2: Check permission (canView/canManage/canConfigure)
    
    alt Check admin role first
        CPV2->>CPV2: Check if user has MANAGE_CLIENTS or VIEW_CLIENTS role
        CPV2-->>API: Return true if role exists
    else Check V2 permissions
        CPV2->>RS: findByName(server, clientId)
        alt Client-specific resource exists
            RS-->>CPV2: Return resource for client
        else Fallback to "all-clients" resource
            CPV2->>APS: getResourceTypeResource(CLIENTS_RESOURCE_TYPE)
            APS->>RS: findByName(server, "Clients")
            RS-->>APS: Return "all-clients" resource
            APS-->>CPV2: Return resource
        end
        
        CPV2->>AuthZ: evaluatePermission(resourcePermission)
        AuthZ-->>CPV2: Return granted permissions
        
        CPV2->>CPV2: Check if requested scope (VIEW/MANAGE/CONFIGURE) is granted
        CPV2-->>API: Return permission result
    end
    
    API->>Client: Execute operation if authorized
    Client-->>API: Return result
    API-->>Admin: Return response
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.

Additional Comments (1)

  1. server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java, line 310-322 (link)

    logic: Missing handling for CLIENTS resource type in getResourceName method

    The method only handles USERS resource type but doesn't handle the newly added CLIENTS resource type. This will cause the display name to fall back to resource.getDisplayName() for client resources, which may not provide proper display names in the admin UI.

    Add a similar block for CLIENTS:

10 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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