Add Client resource type and scopes to authorization schema#6
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <mkanis@redhat.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces fine-grained authorization support for Keycloak admin clients by adding a new CLIENTS resource type with associated scopes and a V2 permission evaluation implementation, alongside event listener feature-flag guarding and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin Client
participant Schema as AdminPermissionsSchema
participant Evaluator as ClientPermissionEvaluator
participant V2 as ClientPermissionsV2
participant AuthzProvider as AuthorizationProvider
Admin->>Evaluator: canManage(ClientModel)
Evaluator->>V2: Delegate permission check
V2->>V2: hasPermission(client, "manage")
V2->>AuthzProvider: Evaluate scope "manage"
AuthzProvider-->>V2: Permission granted?
V2-->>Evaluator: boolean result
Evaluator-->>Admin: boolean result
sequenceDiagram
participant Event as Keycloak Event
participant Listener as AdminPermissions Listener
participant FeatureFlag as FGAP Feature Flag
participant Handler as Permission Handler
Event->>Listener: RoleRemovedEvent/ClientRemovedEvent
Listener->>FeatureFlag: Is Admin Fine-Grained Authz enabled?
alt Feature enabled
FeatureFlag-->>Listener: true
Listener->>Handler: Process event (resolve realm/update permissions)
Handler-->>Listener: Complete
else Feature disabled
FeatureFlag-->>Listener: false
Listener-->>Event: Skip processing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java (1)
45-50: V2 evaluator overload likely ignoring target realmIn
evaluator(KeycloakSession session, RealmModel realm, RealmModel adminsRealm, UserModel admin), the V2 branch currently calls:return new MgmtPermissionsV2(session, adminsRealm, admin);whereas the non-V2 branch calls:
return new MgmtPermissions(session, realm, adminsRealm, admin);Given
MgmtPermissionsV2defines a matching 4‑arg constructor(session, realm, adminsRealm, admin), this V2 path appears to drop therealmcontext and treatadminsRealmas the managed realm, which is likely incorrect when admin and managed realms differ.Consider switching to the 4‑arg constructor to maintain parity with the v1 behavior:
return new MgmtPermissionsV2(session, realm, adminsRealm, admin);
🧹 Nitpick comments (8)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
91-95: Client resolution logic mirrors user resolution; consider future-friendly naming
resolveClientcorrectly tries ID first and thenclientId, andgetOrCreateResourcenow handlesCLIENTSsymmetrically withUSERS, which fits the FGAP model.If you later need human-friendly labels for client resources (similar to
"All users"ingetResourceName), you might add aCLIENTSbranch there as well; not required for functionality now but could improve diagnostics/UI messages.Also applies to: 175-184
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (4)
126-189:testManageOnlyOneClientexercises per-client MANAGE semantics; minor test nitsThe test correctly verifies:
- All relevant client operations fail with
ForbiddenExceptionbefore granting MANAGE.- After granting MANAGE for
myclientonly, view/list/update/default-scope operations are allowed for that client.- Cross-client update (
realmClient) remains forbidden.Two minor test-quality suggestions:
- Using JUnit 5
assertThrows(ForbiddenException.class, ...)would simplify the try/catch +fail()pattern.- Accessing
getDefaultClientScopes().get(1)assumes at least two default scopes; consider asserting the size first to avoid brittle failures if realm config changes.
191-247:testConfigureOnlyOneClientcleanly separates CONFIGURE vs MANAGE responsibilitiesThis test nicely captures that CONFIGURE on a single client enables secrets and read-only scope operations, while scope membership changes still require MANAGE. Same small nits as above:
- Prefer
assertThrowsover manual try/catch where practical.- Guard
defaultClientScopes.get(0)with a size assertion to avoid hard index assumptions.
283-323:testViewAllClientscovers VIEW-all-clients vs write operations; watch thecreate(null)callThe test clearly distinguishes:
- No listing/mapper access before VIEW permission.
- After granting VIEW-all-clients, listing and reading mappers are allowed, but creating mappers and clients remains forbidden.
One potential fragility:
clients().create(null)relies on the admin client tolerating anullrepresentation. If the client implementation changes to validate input earlier, this might start returning a different status or error. Passing a minimalClientRepresentationinstead would be more robust.
325-365:testMapRolesAndCompositesOnlyOneClientcorrectly combines user+client MAP_ scopes*The test sensibly:
- Pre-creates client roles with an unrestricted admin.
- Asserts both client-level role assignment and composite addition are forbidden for
myadmininitially.- Grants
MAP_ROLESon the user plusMAP_ROLES,MAP_ROLES_COMPOSITE, andCONFIGUREon the specific client, then verifies both operations succeed.Optional: if the test framework ever reuses the same realm instance, registering cleanup for the created client roles (similar to what you do for policies) would keep state tidy.
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java (1)
117-161: User/client policy helpers with cleanup are useful; consider making cleanup more robustThe new
createUserPolicy,createClientPolicy, andcreateAllPermissionhelpers:
- Centralize policy construction.
- Assert on
201 Created.- Register realm cleanup to remove the created policy.
Two optional robustness tweaks:
- Capture the created policy ID from the
Locationheader or initial create response (if available) and store it, instead of re-resolving viafindByName(name)at cleanup time.- If names are ever reused, this avoids ambiguity around which policy instance gets removed.
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (2)
124-145:getClientsWithPermission/hasGrantedPermissionbehavior is coherent; ensure callers expect client IDs
getClientsWithPermissionwalks all resources of typeCLIENTS_RESOURCE_TYPEand useshasGrantedPermissionto filter those having the given scope, returningresource.getName()for each.Given how resources are created (per-client resources named by
client.getId()and an “all-clients” resource named"Clients"), this effectively returns:
- Specific client IDs with the requested permission.
- Potentially the
"Clients"resource name if a global permission is defined.That’s reasonable, but downstream code must treat these as resource names/client IDs, not display names. If any consumer expects
clientId(the human-readable identifier), you’ll need a translation step.Also applies to: 274-286
262-272:getEvaluationContextis currently unused; consider removing or documenting intended usage
getEvaluationContextbuilds a client-basedEvaluationContextwith akc.client.idattribute but is not referenced anywhere in this class. If there’s no planned use, you can drop it to avoid dead code; otherwise a brief comment about the intended consumer would help future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java(3 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java(1 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java(2 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java(1 hunks)services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java(2 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java(3 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java(3 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.java(2 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java(16 hunks)tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypePermissionTest.java(9 hunks)
🧰 Additional context used
🪛 Gitleaks (8.29.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 (14)
services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java (1)
29-68: Client permissions wiring matches existing patternThe new
clientPermissionsfield andclients()lazy initialization mirror the existingusers()implementation and correctly constructClientPermissionsV2(session, realm, authz, this). Looks consistent and safe.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.java (1)
128-174: Tests correctly migrated to client-aware createPermission helperAll updated usages of
createPermissionconsistently passclientand preserve the original permission shapes and expected statuses. No behavioral regressions apparent.services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java (1)
77-95: Confirm feature flag used to guard admin-permission cleanup listenerThe listener is now fully guarded by
Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, and the new RoleRemovedEvent handling correctly derives the realm from either aClientModelcontainer or aRealmModelcontainer before disabling permissions on the removed role.Given other entry points in this class switch to V2 based on
ADMIN_FINE_GRAINED_AUTHZ_V2, please confirm that gating this listener solely onADMIN_FINE_GRAINED_AUTHZ(and not also on the V2 flag) matches the intended feature-flag semantics and deployment scenarios.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypePermissionTest.java (1)
67-201: User permission tests correctly switched to client-scoped helpersAll uses of
getScopePermissionsResource(...)andcreatePermission(...)are consistently updated to passclient, and the permission definitions and assertions remain unchanged. The cleanup in@AfterEachalso correctly uses the client-scoped resource. Change set looks sound.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeEvaluationTest.java (1)
66-367: Evaluation tests cleanly migrated to client-scoped helpers with centralized usersTypeIntroducing
usersType = AdminPermissionsSchema.USERS.getType()removes repetition, and all permission/policy helper calls are consistently updated to passclientand, where needed,usersType. The assertions and overall scenarios remain intact, so behavior should be preserved while aligning with the new client-scoped API surface.server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
54-69: CLIENTS resource type and scopes are wired consistentlyThe new
CLIENTS_RESOURCE_TYPE, associated scopes, andCLIENTSResourceTyperegistration in the constructor look coherent and match the usages in V2 permissions and tests. No issues spotted with the schema wiring.Also applies to: 73-73
tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (3)
70-83: Realm admin client injection and per-test permission cleanup look soundThe
@InjectAdminClient/@InjectClientfixtures plusonAfter()cleanup againstgetScopePermissionsResource(client)provide a clear separation between the realm admin under test and the FGAP admin-permissions client. This should keep scope permissions isolated across tests.
85-105: Policy-type tests correctly exercise FGAP-aware permissions endpointRouting
testUnsupportedPolicyTypes/testSupportedPolicyTypesthroughgetPermissionsResource(client)ensures these assertions are made against the admin-permissions client’s authorization server, which is what the schema is defining. Thescopepolicy test usingUSERSresource type/scopes also matches the schema.
249-281:testManageAllClientsvalidates global MANAGE-all-clients permissionThe flow (create denied before permission, empty list, granting
createAllPermission(... MANAGE), then successful create/delete/list) aligns well with the intended “all-clients” MANAGE semantics and the newCLIENTSresource type.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java (2)
163-179: OverloadedcreatePermissionhelpers build correct scope permissionsThe overloaded
createPermissionmethods correctly:
- Set logic, resourceType, scopes, and
resourceswith the givenresourceId.- Attach all provided policies by name.
- Delegate persistence to the static
createPermissionhelper.The construction and wiring are consistent with how tests use them for both per-object and all-clients permissions.
54-72: All call sites correctly use new static helper signatures; verification passedThe shell script confirms that all usages of the static permission helpers have been properly migrated:
getPermissionsResource(client)andgetScopePermissionsResource(client)are consistently called with the new static signatures across all test classes- No legacy no-arg calls to
getPermissionsResource()orgetScopePermissionsResource()remain- All static
createPermissioncalls passManagedClient clientas the required first parameter- The overloaded
createPermissionmethods with resourceId and other parameters are appropriately defined as protected instance methods in the base test class and called correctly from subclassesservices/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (3)
57-123: V2 client and client-scope permission checks align with schema and evaluator docsThe implementations of:
canConfigure(ClientModel),canManage(ClientModel),canManage()canView(ClientModel),canView()canMapRoles,canMapCompositeRoles,canMapClientScopeRolescanManageClientScopes,canManage(ClientScopeModel),canView(ClientScopeModel)are consistent with:
- Role-based shortcuts (
AdminRoles.MANAGE_CLIENTS/VIEW_CLIENTS).- FGAP V2 scopes from
AdminPermissionsSchema(MANAGE/VIEW/CONFIGURE/MAP_*).- The documented behavior in
ClientPermissionEvaluator.The fallbacks to global “all-clients” permissions via
hasPermission(String scope)complement the per-client checks well.
237-260: GlobalhasPermission(String scope)implementation is straightforward and matches schemaThe global-scope checker correctly:
- Ensures same-realm admin and non-null
ResourceServer.- Resolves the
"Clients"resource by name/owner.- Evaluates permissions and checks for the requested scope.
The early
resource == nullguard already protects against missing type-resources here, so no changes needed.
147-205: V2 code paths are properly segregated via feature flag; confirm at runtime that unsupported methods are not invokedThe factory pattern correctly routes to V2 implementations when
ADMIN_FINE_GRAINED_AUTHZ_V2is enabled (AdminPermissions.javalines 40–70).ClientPermissionsV2extendsClientPermissionsand overrides all legacy methods to throwUnsupportedOperationException.The concern is valid: verify that when V2 is active, code paths don't accidentally trigger these exceptions. Static analysis shows proper segregation via feature flags and exception guards, but runtime execution flow (particularly around parent class initialization and inherited method chains) should be manually verified to confirm V2 flows never call
exchangeToPermission(),mapRolesPermission(),getPermissions(), etc.
| /** | ||
| * Returns {@code true} if {@link #canView()} returns {@code true}. | ||
| */ | ||
| void requireView(); | ||
|
|
||
| /** | ||
| * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}. | ||
| */ | ||
| void requireViewClientScopes(); |
There was a problem hiding this comment.
Fix misleading Javadoc on requireView/requireViewClientScopes
Both methods are void “require” guards but their Javadoc says “Returns true if …”, which is inconsistent with the rest of the API (and with how other require* methods are documented).
Suggested wording:
- /**
- * Returns {@code true} if {@link #canView()} returns {@code true}.
- */
+ /**
+ * Throws ForbiddenException if {@link #canView()} returns {@code false}.
+ */
void requireView();
@@
- /**
- * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}.
- */
+ /**
+ * Throws ForbiddenException if {@link #canViewClientScopes()} returns {@code false}.
+ */
void requireViewClientScopes();This matches the intent and the pattern used by the other require* methods.
📝 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 #canView()} returns {@code true}. | |
| */ | |
| void requireView(); | |
| /** | |
| * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}. | |
| */ | |
| void requireViewClientScopes(); | |
| /** | |
| * Throws ForbiddenException if {@link #canView()} returns {@code false}. | |
| */ | |
| void requireView(); | |
| /** | |
| * 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 95 to 103, the Javadoc for requireView and requireViewClientScopes
incorrectly says “Returns true if …”; update the comments to reflect that these
are void guard methods that enforce permission and throw an exception when the
permission is missing (e.g. "Requires view permission for the client; throws
ForbiddenException if not allowed.") and similarly for client scopes ("Requires
view permission for client scopes; throws ForbiddenException if not allowed."),
matching the pattern used by the other require* methods.
| 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) { | ||
| // check if there is permission for "all-clients". If so, load its resource and proceed with evaluation | ||
| 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; | ||
| } |
There was a problem hiding this comment.
Guard against missing type-resource to avoid potential NPE in hasPermission(ClientModel, ...)
If resourceStore.findByName(server, client.getId(), server.getId()) returns null, you fall back to:
resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE);
if (authz.getStoreFactory().getPolicyStore().findByResource(server, resource).isEmpty()) {
return false;
}However, getResourceTypeResource can legitimately return null (e.g., FGAP V2 disabled or the “Clients” resource missing), which would lead to a NullPointerException inside findByResource. Safer to bail out when the type-resource is absent.
Suggested fix:
- Resource resource = resourceStore.findByName(server, client.getId(), server.getId());
+ Resource resource = resourceStore.findByName(server, client.getId(), server.getId());
if (resource == null) {
// check if there is permission for "all-clients". If so, load its resource and proceed with evaluation
- resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE);
-
- if (authz.getStoreFactory().getPolicyStore().findByResource(server, resource).isEmpty()) {
+ resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE);
+ if (resource == null) {
+ return false;
+ }
+ if (authz.getStoreFactory().getPolicyStore().findByResource(server, resource).isEmpty()) {
return false;
}
}This keeps the semantics while avoiding a hard failure in misconfigured realms.
🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java
around lines 207 to 235, the fallback call to
AdminPermissionsSchema.SCHEMA.getResourceTypeResource(...) can return null which
leads to a possible NullPointerException when passing that null into
authz.getStoreFactory().getPolicyStore().findByResource(...); update the method
to check whether the returned type-resource is null and immediately return false
(or otherwise bail out) before calling findByResource, so findByResource is
never invoked with a null resource.
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