Add Client resource type and scopes to authorization schema#5
Add Client resource type and scopes to authorization schema#5ShashankFC wants to merge 1 commit into
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <mkanis@redhat.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce support for fine-grained client permissions in Keycloak's authorization system. A new CLIENTS resource type is added to AdminPermissionsSchema with associated scopes. ClientPermissionsV2, a new V2-tier permission evaluation class, is introduced to check client resource permissions. ClientPermissionEvaluator expands with new public permission check methods. Test utilities are refactored to accept client parameters for permission operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Admin Client
participant CPV2 as ClientPermissionsV2
participant AuthZ as AuthorizationProvider
participant RS as ResourceServer
participant PE as Policy Evaluator
Client->>CPV2: canManage(ClientModel)
activate CPV2
CPV2->>CPV2: hasPermission(ClientModel, "manage")
activate CPV2
CPV2->>AuthZ: getEvaluationContext(ClientModel)
activate AuthZ
AuthZ-->>CPV2: EvaluationContext
deactivate AuthZ
CPV2->>RS: getResources(CLIENTS_RESOURCE_TYPE)
activate RS
RS-->>CPV2: Resource list
deactivate RS
CPV2->>PE: evaluate(EvaluationContext, Policy)
activate PE
PE-->>CPV2: boolean result
deactivate PE
CPV2-->>CPV2: hasGrantedPermission check
deactivate CPV2
CPV2-->>Client: permission result
deactivate CPV2
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java:
- Around line 77-96: The event cleanup block in AdminPermissions (handling
RoleContainerModel.RoleRemovedEvent, ClientModel.ClientRemovedEvent,
GroupModel.GroupRemovedEvent) is currently guarded only by
Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ so it won't run when only
ADMIN_FINE_GRAINED_AUTHZ_V2 is enabled; update the feature check to require
either flag (i.e., check
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) ||
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) so the
management(...) .roles().setPermissionsEnabled(...),
.clients().setPermissionsEnabled(...), and .groups().setPermissionsEnabled(...)
cleanup handlers always run for V1 or V2.
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java:
- Around line 95-98: The Javadoc for requireView() is incorrect: it claims a
return value but the method is void and throws ForbiddenException on failure;
update the Javadoc for requireView() in ClientPermissionEvaluator to describe
that it enforces view permission, throws ForbiddenException when canView() is
false, and replace the incorrect "Returns ..." line with a short description and
a @throws ForbiddenException tag referencing canView().
- Around line 100-103: The Javadoc for requireViewClientScopes() incorrectly
uses "Returns" for a void method; update the Javadoc to describe the throwing
behavior instead (e.g., state that the method throws an appropriate exception if
canViewClientScopes() is false) similar to the fix for requireView(), and ensure
the comment references the canViewClientScopes() check and the exception
type/condition thrown by requireViewClientScopes().
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java:
- Around line 262-272: The private method getEvaluationContext (which constructs
a ClientModelIdentity and anonymous DefaultEvaluationContext overriding
getBaseAttributes) is unused; remove the getEvaluationContext method entirely
or, if it's intended for future use, add a clear TODO comment above it
explaining expected usage. Update references to ClientModelIdentity and
DefaultEvaluationContext only if you remove related imports or fields; ensure no
remaining unused imports or variables (e.g., authorizedClient, session) remain
after deletion.
In
@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java:
- Around line 330-341: The test creates client roles in the variables role and
subRole but never cleans them up; after creating each role (or immediately after
fetching their representations) register cleanup to delete them via the client
roles resource (use
realm.admin().clients().get(myclient.getId()).roles().delete(roleName) or delete
by id using role.getId()/subRole.getId()) so both "myclient-role" and
"myclient-subRole" are removed in test teardown.
- Around line 319-323: The test passes null to
realmAdminClient.realm(realm.getName()).clients().create(null), which can
trigger server-side validation errors instead of an authorization check; replace
the null with a minimal, valid ClientRepresentation (e.g., set clientId and
required fields such as protocol/enabled) and call clients().create(validClient)
so the request reaches the authorization layer and then assert the response
status is Status.FORBIDDEN; update the test case around PermissionClientTest to
construct and use that valid representation before calling create().
🧹 Nitpick comments (5)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
304-326: Missing CLIENTS handling ingetResourceName.The
getResourceNamemethod handlesUSERSresource type to provide human-readable names (e.g., username instead of ID), but there's no corresponding handling forCLIENTS. Consider adding a similar branch to return the client'sclientIdfor better readability in permission displays.♻️ Suggested addition for CLIENTS handling
if (USERS.getType().equals(resourceType)) { if (resource.getName().equals(USERS_RESOURCE_TYPE)) { return "All users"; } UserModel user = session.users().getUserById(session.getContext().getRealm(), resource.getName()); if (user == null) { throw new ModelIllegalStateException("User not found for resource [" + resource.getId() + "]"); } return user.getUsername(); + } else if (CLIENTS.getType().equals(resourceType)) { + if (resource.getName().equals(CLIENTS_RESOURCE_TYPE)) { + return "All clients"; + } + + ClientModel client = session.clients().getClientById(session.getContext().getRealm(), resource.getName()); + + if (client == null) { + throw new ModelIllegalStateException("Client not found for resource [" + resource.getId() + "]"); + } + + return client.getClientId(); }tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java (1)
163-179: Minor inconsistency: instance methods vs static pattern.
createPermissionat lines 163-179 are instance methods while the othercreatePermissionoverloads (lines 66-74) andcreateAllPermission(line 151) are static. Consider making these static as well for consistency, or document why these need instance access.services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (2)
46-46: Unused import.
TOKEN_EXCHANGEis imported but not used in this class.♻️ Remove unused import
-import static org.keycloak.services.resources.admin.permissions.AdminPermissionManagement.TOKEN_EXCHANGE;
207-260: Duplicate code inhasPermissionoverloads.The scope evaluation logic (lines 224-234 and 249-257) is nearly identical. Consider extracting a shared helper method to reduce duplication.
♻️ Extract shared scope-matching logic
+ private boolean hasScopeInPermissions(Collection<Permission> permissions, String scope) { + for (Permission permission : permissions) { + for (String s : permission.getScopes()) { + if (scope.equals(s)) { + return true; + } + } + } + return false; + } + private boolean hasPermission(ClientModel client, String scope) { if (!root.isAdminSameRealm()) { return false; } ResourceServer server = root.realmResourceServer(); if (server == null) return false; Resource resource = resourceStore.findByName(server, client.getId(), server.getId()); if (resource == null) { resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE); if (authz.getStoreFactory().getPolicyStore().findByResource(server, resource).isEmpty()) { return false; } } Collection<Permission> permissions = root.evaluatePermission(new ResourcePermission(resource, resource.getScopes(), server), server); - List<String> expectedScopes = Arrays.asList(scope); - for (Permission permission : permissions) { - for (String s : permission.getScopes()) { - if (expectedScopes.contains(s)) { - return true; - } - } - } - - return false; + return hasScopeInPermissions(permissions, scope); }tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (1)
147-153: Consider adding a guard for index access.Accessing
getDefaultClientScopes().get(1)directly may throwIndexOutOfBoundsExceptionif the realm has fewer than 2 default client scopes. While this is unlikely in typical Keycloak setups, consider adding an assertion or check.♻️ Suggested improvement
+ List<ClientScopeRepresentation> scopes = clientResource.getDefaultClientScopes(); + assertThat(scopes.size(), org.hamcrest.Matchers.greaterThanOrEqualTo(2)); try { - ClientScopeRepresentation clientScopeRep = clientResource.getDefaultClientScopes().get(1); + ClientScopeRepresentation clientScopeRep = scopes.get(1); clientResource.removeDefaultClientScope(clientScopeRep.getId()); fail("Expected exception wasn't thrown.");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.javaservices/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.javatests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypePermissionTest.java
🧰 Additional context used
🧬 Code graph analysis (4)
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java (1)
core/src/main/java/org/keycloak/representations/idm/authorization/ScopePermissionRepresentation.java (1)
ScopePermissionRepresentation(22-28)
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (2)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
AdminPermissionsSchema(52-327)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java (1)
PermissionBuilder(76-115)
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java (2)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
AdminPermissionsSchema(52-327)test-framework/core/src/main/java/org/keycloak/testframework/realm/UserConfigBuilder.java (1)
UserConfigBuilder(10-89)
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (3)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
AdminPermissionsSchema(52-327)server-spi-private/src/main/java/org/keycloak/authorization/AuthorizationProvider.java (1)
AuthorizationProvider(80-555)js/apps/account-ui/src/api/request.ts (1)
token(65-74)
🪛 Gitleaks (8.30.0)
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java
[high] 53-53: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (32)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (3)
53-68: LGTM! CLIENTS resource type definition follows established patterns.The new
CLIENTSresource type and its scopes (CONFIGURE,MANAGE,MAP_ROLES,MAP_ROLES_CLIENT_SCOPE,MAP_ROLES_COMPOSITE,VIEW) are well-defined and consistent with the existingUSERSresource type pattern.
93-94: LGTM! Client resolution branch added correctly.The resolution logic for
CLIENTSresources mirrors the existingUSERSpattern appropriately.
175-184: LGTM!resolveClientmethod follows the established pattern.The method correctly mirrors
resolveUserby first attempting lookup by ID, then falling back toclientId.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java (2)
54-64: LGTM! Refactored to static methods with explicit client parameter.Converting these to static utilities with explicit
ManagedClientparameter improves testability and makes dependencies explicit.
117-149: LGTM! Policy factory methods with proper cleanup registration.The cleanup callbacks properly remove policies after test execution, preventing test pollution. The pattern of registering cleanup during creation is a good practice.
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (2)
49-55: LGTM! Constructor properly extends ClientPermissions.The static analysis hint about an API key at line 53 is a false positive -
rootis simply a constructor parameter name, not a secret.
57-123: LGTM! Permission check methods correctly delegate to V2 evaluation logic.The override methods properly combine admin role checks with fine-grained permission evaluation.
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.java (2)
129-133: LGTM! Updated to use client-scoped permission creation.The change to pass
clientas the first argument aligns with the refactored test utilities inAbstractPermissionTest.
160-173: LGTM! Consistent client parameter propagation in error case tests.All permission creation calls in the error validation tests correctly pass the client context.
services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java (2)
29-29: LGTM! New field for ClientPermissionsV2.Follows the same pattern as the existing
userPermissionsfield.
62-68: LGTM! Lazy initialization of ClientPermissionsV2.The
clients()method correctly mirrors the existingusers()pattern with proper lazy initialization and caching.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypePermissionTest.java (3)
67-71: LGTM! Cleanup method updated to use client-scoped resource.The
@AfterEachcleanup correctly uses the updatedgetScopePermissionsResource(client)signature.
77-97: LGTM! Test methods updated for client-scoped permission access.Both
testCreateResourceTypePermissionandtestCreateResourceObjectPermissioncorrectly propagate the client context.
186-198: LGTM! Helper methods updated consistently.Both
createUserPermissionandcreateAllUserPermissioncorrectly pass the client tocreatePermission.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java (8)
66-67: Good addition of type constant.Using
AdminPermissionsSchema.USERS.getType()instead of a hardcoded string improves maintainability and ensures consistency with the schema definition.
70-79: LGTM!The cleanup method correctly uses the client-scoped permission resource retrieval and properly removes all scope permissions after each test.
81-90: LGTM!The test correctly uses the updated client-parameterized helpers (
createUserPolicy,createPermission) while maintaining the original test logic.
92-133: LGTM!The impersonation test properly exercises both positive and negative permission scenarios with the refactored helper methods.
135-186: LGTM!The manage permission tests correctly exercise the full permission lifecycle using the updated client-scoped helpers.
188-227: LGTM!The view permission tests properly validate both allow and deny scenarios using the refactored permission helpers.
229-330: LGTM!The map-roles and group-membership tests correctly use the updated helper method signatures and maintain proper test coverage for the permission evaluation logic.
332-367: LGTM!The transitive permissions test properly validates that MANAGE scope grants transitive access to VIEW, MAP_ROLES, and MANAGE_GROUP_MEMBERSHIP, while correctly asserting that IMPERSONATE is not included.
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (5)
70-83: LGTM!The test setup correctly injects the required managed realm client and adds proper cleanup to remove all scope permissions after each test, preventing test pollution.
85-124: LGTM!The policy type support tests correctly use the updated client-parameterized permission resource helpers while maintaining the original validation logic.
191-247: LGTM!The test properly validates that CONFIGURE permission grants secret management and view access while correctly denying client scope modifications, demonstrating the distinction from MANAGE permission.
249-281: LGTM!The test comprehensively validates that all-clients MANAGE permission enables client creation, deletion, and listing operations.
357-365: LGTM!The permission setup correctly grants both user-level MAP_ROLES permission and client-level permissions (MAP_ROLES, MAP_ROLES_COMPOSITE, CONFIGURE) to enable role assignment operations.
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java (5)
35-38: Javadoc inaccuracy: method returns void, not boolean.The Javadoc for
requireListClientScopes()says "Throws ForbiddenException" but doesn't indicate the return type isvoid. The method signature at line 38 shows it returnsvoid, which is correct, but the documentation could be clearer.
40-104: Well-documented interface methods.The Javadoc clearly explains the dual behavior for V1 (role-based) and V2 (fine-grained permission) authorization, providing good guidance for implementers and consumers of this API.
105-145: LGTM!The Javadoc for client-specific permission methods (
canManage(ClientModel),canConfigure(ClientModel),canView(ClientModel)) accurately documents the permission evaluation hierarchy and V1/V2 behavior.
147-190: LGTM!The new
ClientScopeModelpermission methods and role mapping permission methods (canMapRoles,canMapCompositeRoles,canMapClientScopeRoles) are well-documented and align with theAdminPermissionsSchemascopes.
192-195: LGTM!The existing
getAccessandgetClientsWithPermissionmethods are retained, maintaining backward compatibility.
| if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) { | ||
| if (event instanceof RoleContainerModel.RoleRemovedEvent) { | ||
| RoleContainerModel.RoleRemovedEvent cast = (RoleContainerModel.RoleRemovedEvent) event; | ||
| RoleModel role = cast.getRole(); | ||
| RealmModel realm; | ||
| if (role.getContainer() instanceof ClientModel) { | ||
| realm = ((ClientModel) role.getContainer()).getRealm(); | ||
|
|
||
| } else { | ||
| realm = (RealmModel)role.getContainer(); | ||
| } else { | ||
| realm = (RealmModel) role.getContainer(); | ||
| } | ||
| management(cast.getKeycloakSession(), realm).roles().setPermissionsEnabled(role, false); | ||
| } else if (event instanceof ClientModel.ClientRemovedEvent) { | ||
| ClientModel.ClientRemovedEvent cast = (ClientModel.ClientRemovedEvent) event; | ||
| management(cast.getKeycloakSession(), cast.getClient().getRealm()).clients().setPermissionsEnabled(cast.getClient(), false); | ||
| } else if (event instanceof GroupModel.GroupRemovedEvent) { | ||
| GroupModel.GroupRemovedEvent cast = (GroupModel.GroupRemovedEvent) event; | ||
| management(cast.getKeycloakSession(), cast.getRealm()).groups().setPermissionsEnabled(cast.getGroup(), false); | ||
| } | ||
| management(cast.getKeycloakSession(), realm).roles().setPermissionsEnabled(role, false); | ||
| } else if (event instanceof ClientModel.ClientRemovedEvent) { | ||
| ClientModel.ClientRemovedEvent cast = (ClientModel.ClientRemovedEvent)event; | ||
| management(cast.getKeycloakSession(), cast.getClient().getRealm()).clients().setPermissionsEnabled(cast.getClient(), false); | ||
| } else if (event instanceof GroupModel.GroupRemovedEvent) { | ||
| GroupModel.GroupRemovedEvent cast = (GroupModel.GroupRemovedEvent)event; | ||
| management(cast.getKeycloakSession(), cast.getRealm()).groups().setPermissionsEnabled(cast.getGroup(), false); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for both feature flags usage patterns
echo "=== ADMIN_FINE_GRAINED_AUTHZ usage ==="
rg -n "ADMIN_FINE_GRAINED_AUTHZ" --type java | head -150
echo ""
echo "=== Files containing both V1 and V2 flags ==="
rg -l "ADMIN_FINE_GRAINED_AUTHZ" --type java | xargs -I {} sh -c 'if grep -q "ADMIN_FINE_GRAINED_AUTHZ_V2" "$1"; then echo "$1"; fi' _ {}Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 12484
🏁 Script executed:
#!/bin/bash
# Get the AdminPermissions.java file and check surrounding context
fd "AdminPermissions.java" --type f -x cat -n {} | head -150Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 5594
🏁 Script executed:
#!/bin/bash
# Examine MgmtPermissions.java line 85 for context
sed -n '80,90p' services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.javaRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 685
🏁 Script executed:
#!/bin/bash
# Check what MgmtPermissionsV2 does - verify if it also relies on cleanup handlers
fd "MgmtPermissionsV2.java" --type f -x wc -l {}
fd "MgmtPermissionsV2.java" --type f -x head -100 {}Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 2554
Fix event listener feature flag check to support both V1 and V2.
The cleanup handlers for RoleRemovedEvent, ClientRemovedEvent, and GroupRemovedEvent at line 77 are gated behind only ADMIN_FINE_GRAINED_AUTHZ (V1). If V2 is enabled without V1, these cleanup handlers won't execute, leaving orphaned permission resources. Change the condition to check for either flag, matching the pattern used in MgmtPermissions.java line 85:
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) || Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) {🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java
around lines 77 - 96, The event cleanup block in AdminPermissions (handling
RoleContainerModel.RoleRemovedEvent, ClientModel.ClientRemovedEvent,
GroupModel.GroupRemovedEvent) is currently guarded only by
Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ so it won't run when only
ADMIN_FINE_GRAINED_AUTHZ_V2 is enabled; update the feature check to require
either flag (i.e., check
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) ||
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) so the
management(...) .roles().setPermissionsEnabled(...),
.clients().setPermissionsEnabled(...), and .groups().setPermissionsEnabled(...)
cleanup handlers always run for V1 or V2.
| /** | ||
| * Returns {@code true} if {@link #canView()} returns {@code true}. | ||
| */ | ||
| void requireView(); |
There was a problem hiding this comment.
Javadoc incorrectly states "Returns" for a void method.
The Javadoc says "Returns {@code true} if {@link #canView()} returns {@code true}." but requireView() returns void and throws ForbiddenException on failure.
📝 Proposed fix
- /**
- * Returns {@code true} if {@link #canView()} returns {@code true}.
- */
+ /**
+ * Throws ForbiddenException if {@link #canView()} returns {@code false}.
+ */
void requireView();🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java
around lines 95 - 98, The Javadoc for requireView() is incorrect: it claims a
return value but the method is void and throws ForbiddenException on failure;
update the Javadoc for requireView() in ClientPermissionEvaluator to describe
that it enforces view permission, throws ForbiddenException when canView() is
false, and replace the incorrect "Returns ..." line with a short description and
a @throws ForbiddenException tag referencing canView().
| /** | ||
| * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}. | ||
| */ | ||
| void requireViewClientScopes(); |
There was a problem hiding this comment.
Javadoc incorrectly states "Returns" for a void method.
Same issue as requireView() - the Javadoc should describe throwing behavior, not return value.
📝 Proposed fix
- /**
- * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}.
- */
+ /**
+ * Throws ForbiddenException if {@link #canViewClientScopes()} returns {@code false}.
+ */
void requireViewClientScopes();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}. | |
| */ | |
| void requireViewClientScopes(); | |
| /** | |
| * Throws ForbiddenException if {@link #canViewClientScopes()} returns {@code false}. | |
| */ | |
| void requireViewClientScopes(); |
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java
around lines 100 - 103, The Javadoc for requireViewClientScopes() incorrectly
uses "Returns" for a void method; update the Javadoc to describe the throwing
behavior instead (e.g., state that the method throws an appropriate exception if
canViewClientScopes() is false) similar to the fix for requireView(), and ensure
the comment references the canViewClientScopes() check and the exception
type/condition thrown by requireViewClientScopes().
| private EvaluationContext getEvaluationContext(ClientModel authorizedClient, AccessToken token) { | ||
| ClientModelIdentity identity = new ClientModelIdentity(session, authorizedClient, token); | ||
| return new DefaultEvaluationContext(identity, session) { | ||
| @Override | ||
| public Map<String, Collection<String>> getBaseAttributes() { | ||
| Map<String, Collection<String>> attributes = super.getBaseAttributes(); | ||
| attributes.put("kc.client.id", List.of(authorizedClient.getClientId())); | ||
| return attributes; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of getEvaluationContext across the entire codebase
rg -n "getEvaluationContext" --type javaRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 287
Remove unused getEvaluationContext method.
This method is never called within the codebase. Either remove it or add a TODO comment if it's intended for future use.
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java
around lines 262 - 272, The private method getEvaluationContext (which
constructs a ClientModelIdentity and anonymous DefaultEvaluationContext
overriding getBaseAttributes) is unused; remove the getEvaluationContext method
entirely or, if it's intended for future use, add a clear TODO comment above it
explaining expected usage. Update references to ClientModelIdentity and
DefaultEvaluationContext only if you remove related imports or fields; ensure no
remaining unused imports or variables (e.g., authorizedClient, session) remain
after deletion.
| // can't create a new client | ||
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) { | ||
| Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Passing null to clients().create() may not reliably test authorization.
The null parameter could cause a server-side error (e.g., NullPointerException or 400 Bad Request) before the authorization check is evaluated. Consider using a valid ClientRepresentation to ensure the 403 Forbidden response is genuinely from the authorization layer.
🐛 Proposed fix
// can't create a new client
- try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) {
+ ClientRepresentation newClient = new ClientRepresentation();
+ newClient.setClientId("forbidden-client");
+ newClient.setProtocol("openid-connect");
+ try (Response response = realmAdminClient.realm(realm.getName()).clients().create(newClient)) {
Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // can't create a new client | |
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) { | |
| Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); | |
| } | |
| } | |
| // can't create a new client | |
| ClientRepresentation newClient = new ClientRepresentation(); | |
| newClient.setClientId("forbidden-client"); | |
| newClient.setProtocol("openid-connect"); | |
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(newClient)) { | |
| Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java
around lines 319 - 323, The test passes null to
realmAdminClient.realm(realm.getName()).clients().create(null), which can
trigger server-side validation errors instead of an authorization check; replace
the null with a minimal, valid ClientRepresentation (e.g., set clientId and
required fields such as protocol/enabled) and call clients().create(validClient)
so the request reaches the authorization layer and then assert the response
status is Status.FORBIDDEN; update the test case around PermissionClientTest to
construct and use that valid representation before calling create().
| // create a role and sub-role | ||
| RoleRepresentation role = new RoleRepresentation(); | ||
| role.setName("myclient-role"); | ||
| role.setClientRole(true); | ||
| realm.admin().clients().get(myclient.getId()).roles().create(role); | ||
| role = realm.admin().clients().get(myclient.getId()).roles().get("myclient-role").toRepresentation(); | ||
|
|
||
| RoleRepresentation subRole = new RoleRepresentation(); | ||
| subRole.setName("myclient-subRole"); | ||
| subRole.setClientRole(true); | ||
| realm.admin().clients().get(myclient.getId()).roles().create(subRole); | ||
| subRole = realm.admin().clients().get(myclient.getId()).roles().get("myclient-subRole").toRepresentation(); |
There was a problem hiding this comment.
Missing cleanup for created client roles.
The test creates myclient-role and myclient-subRole but doesn't register cleanup for them. While the test uses an existing client (myclient), the roles may persist after the test, potentially affecting other tests.
🧹 Suggested cleanup registration
realm.admin().clients().get(myclient.getId()).roles().create(role);
role = realm.admin().clients().get(myclient.getId()).roles().get("myclient-role").toRepresentation();
+ realm.cleanup().add(r -> r.clients().get(myclient.getId()).roles().deleteRole("myclient-role"));
RoleRepresentation subRole = new RoleRepresentation();
subRole.setName("myclient-subRole");
subRole.setClientRole(true);
realm.admin().clients().get(myclient.getId()).roles().create(subRole);
subRole = realm.admin().clients().get(myclient.getId()).roles().get("myclient-subRole").toRepresentation();
+ realm.cleanup().add(r -> r.clients().get(myclient.getId()).roles().deleteRole("myclient-subRole"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // create a role and sub-role | |
| RoleRepresentation role = new RoleRepresentation(); | |
| role.setName("myclient-role"); | |
| role.setClientRole(true); | |
| realm.admin().clients().get(myclient.getId()).roles().create(role); | |
| role = realm.admin().clients().get(myclient.getId()).roles().get("myclient-role").toRepresentation(); | |
| RoleRepresentation subRole = new RoleRepresentation(); | |
| subRole.setName("myclient-subRole"); | |
| subRole.setClientRole(true); | |
| realm.admin().clients().get(myclient.getId()).roles().create(subRole); | |
| subRole = realm.admin().clients().get(myclient.getId()).roles().get("myclient-subRole").toRepresentation(); | |
| // create a role and sub-role | |
| RoleRepresentation role = new RoleRepresentation(); | |
| role.setName("myclient-role"); | |
| role.setClientRole(true); | |
| realm.admin().clients().get(myclient.getId()).roles().create(role); | |
| role = realm.admin().clients().get(myclient.getId()).roles().get("myclient-role").toRepresentation(); | |
| realm.cleanup().add(r -> r.clients().get(myclient.getId()).roles().deleteRole("myclient-role")); | |
| RoleRepresentation subRole = new RoleRepresentation(); | |
| subRole.setName("myclient-subRole"); | |
| subRole.setClientRole(true); | |
| realm.admin().clients().get(myclient.getId()).roles().create(subRole); | |
| subRole = realm.admin().clients().get(myclient.getId()).roles().get("myclient-subRole").toRepresentation(); | |
| realm.cleanup().add(r -> r.clients().get(myclient.getId()).roles().deleteRole("myclient-subRole")); |
🤖 Prompt for AI Agents
In
@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java
around lines 330 - 341, The test creates client roles in the variables role and
subRole but never cleans them up; after creating each role (or immediately after
fetching their representations) register cleanup to delete them via the client
roles resource (use
realm.admin().clients().get(myclient.getId()).roles().delete(roleName) or delete
by id using role.getId()/subRole.getId()) so both "myclient-role" and
"myclient-subRole" are removed in test teardown.
2 participants
Test 5
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#5