Add Client resource type and scopes to authorization schema#6
Open
ShashankFC wants to merge 1 commit into
Open
Add Client resource type and scopes to authorization schema#6ShashankFC wants to merge 1 commit into
ShashankFC wants to merge 1 commit into
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <mkanis@redhat.com>
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:XL
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| RealmModel realm; | ||
| if (role.getContainer() instanceof ClientModel) { | ||
| realm = ((ClientModel)role.getContainer()).getRealm(); | ||
| if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) { |
There was a problem hiding this comment.
Suggestion: The provider event listener that disables permissions on role, client, or group removal is now gated only by the legacy ADMIN_FINE_GRAINED_AUTHZ feature flag, so when only the new ADMIN_FINE_GRAINED_AUTHZ_V2 flag is enabled the cleanup never runs, leaving stale permissions and roles enabled in V2 deployments. [logic error]
Severity Level: Critical 🚨
- ❌ V2-only fine-grained admin realms leave orphan admin permissions.
- ❌ Deleting groups/clients/roles under v2 never disables their permissions.
- ⚠️ Authorization schema (`AdminPermissionsSchema`) accumulates stale resources.
- ⚠️ ManagementPermissions behavior diverges between v1 and v2 configurations.
Suggested change
| if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) { | |
| // Run cleanup when either fine-grained admin permissions v1 or v2 is enabled | |
| if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) | |
| || Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) { |
Steps of Reproduction ✅
1. Start a Keycloak server configured to enable only the v2 fine-grained admin permissions
feature, for example using the test server config in
`tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/KeycloakAdminPermissionsServerConfig.java:24-29`,
where `configure()` returns `config.features(Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)` and
does NOT enable `ADMIN_FINE_GRAINED_AUTHZ` v1.
2. On server startup, the session factory (e.g.
`quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/QuarkusKeycloakSessionFactory.java:88`
and `services/src/main/java/org/keycloak/services/DefaultKeycloakSessionFactory.java:122`)
calls `AdminPermissions.registerListener(this)`, registering the listener implemented in
`services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java:73-99`,
which currently guards all cleanup logic with `if
(Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ))`.
3. In a realm where admin permissions are enabled under v2 (created as in
`RealmManager.importRealm()` at
`services/src/main/java/org/keycloak/services/managers/RealmManager.java:566-573`, which
calls `AdminPermissionsSchema.SCHEMA.init(session, realm)` when
`ADMIN_FINE_GRAINED_AUTHZ_V2` is enabled and `rep.isAdminPermissionsEnabled()` is true),
use the admin REST API as in
`tests/base/src/test/java/org/keycloak/tests/admin/ManagementPermissionsTest.java` to: (a)
create a group/client/role, then (b) turn on management permissions via
`setPermissions(new ManagementPermissionRepresentation(true))`, which internally uses
`AdminPermissions.management(...)` (see `AdminPermissions.management()` at
`services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java:66-70`
and `MgmtPermissionsV2` at
`services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissionsV2.java:31-48`)
to create the necessary authorization resources and mark them enabled.
4. Still in the same v2-only configuration, delete the group/client/role you just enabled
permissions for (e.g. using the same admin client patterns as in
`ManagementPermissionsTest` for `GroupResource`, `ClientResource`, or `RoleResource`);
this triggers a `GroupModel.GroupRemovedEvent`, `ClientModel.ClientRemovedEvent`, or
`RoleContainerModel.RoleRemovedEvent` that is dispatched through the
`ProviderEventManager` to the registered listener at `AdminPermissions.java:73-99`, but
because only `ADMIN_FINE_GRAINED_AUTHZ_V2` is enabled, the condition
`Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)` evaluates to false,
so none of the cleanup calls `management(...).roles().setPermissionsEnabled(...)`,
`management(...).clients().setPermissionsEnabled(...)`, or
`management(...).groups().setPermissionsEnabled(...)` are executed—leaving the
corresponding admin-permission authorization resources created by
`MgmtPermissions`/`MgmtPermissionsV2` enabled and orphaned in the realm.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java
**Line:** 77:77
**Comment:**
*Logic Error: The provider event listener that disables permissions on role, client, or group removal is now gated only by the legacy ADMIN_FINE_GRAINED_AUTHZ feature flag, so when only the new ADMIN_FINE_GRAINED_AUTHZ_V2 flag is enabled the cleanup never runs, leaving stale permissions and roles enabled in V2 deployments.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+214
to
+218
| 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); | ||
|
|
There was a problem hiding this comment.
Suggestion: The per-client permission check looks up the resource using the resource server's internal ID as the owner, but resources created by the admin permissions schema use the realm-management client's clientId as the owner, so per-client permissions will never be found and only "all-clients" permissions will be considered, causing client-specific permissions to be ignored. [logic error]
Severity Level: Critical 🚨
- ❌ Per-client manage/configure grants are never recognized.
- ❌ Per-client role-mapping scopes MAP_ROLES* effectively unusable.
- ⚠️ Only global "all-clients" policies affect client operations.
- ⚠️ Fine-grained delegation per client cannot be configured.
Suggested change
| 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); | |
| // Look up the resource using the default owner (resource server clientId), which matches how resources are created | |
| Resource resource = resourceStore.findByName(server, client.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 (resource == null || authz.getStoreFactory().getPolicyStore().findByResource(server, resource).isEmpty()) { |
Steps of Reproduction ✅
1. Enable admin fine-grained permissions v2 so that the AdminPermissionsSchema is active
and the admin-permissions client is created and initialized via
`AdminPermissionsSchema.init(...)` in
`server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java:222-255`.
2. Observe how the schema creates per-client resources: in `getOrCreateResource(...)` at
`AdminPermissionsSchema.java:76-114`, when a client resource is created it calls
`resourceStore.create(resourceServer, name, resourceServer.getClientId())`, meaning the
resource owner is the admin-permissions client **clientId**, and later lookups for that
resource use `resourceStore.findByName(resourceServer, name)` without an explicit owner
(`AdminPermissionsSchema.java:101-103`).
3. In the v2 permissions evaluator, per-client checks route through
`ClientPermissionsV2.canManage(ClientModel)` at
`services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java:64-69`,
`canConfigure(ClientModel)` at `57-62`, and the various `canMap*` methods at `90-103`, all
of which delegate to `hasPermission(ClientModel,String)` at `207-234`.
4. When `hasPermission(ClientModel,String)` executes (e.g., during any client management
or configuration check), it performs `resourceStore.findByName(server, client.getId(),
server.getId())` at `ClientPermissionsV2.java:214`, passing the resource server **ID** as
the owner. Since the resources created by `AdminPermissionsSchema` are owned by
`resourceServer.getClientId()` (clientId) and are normally looked up without specifying an
owner, this lookup never finds the per-client resource, causing the method to fall back to
the "all-clients" resource (`getResourceTypeResource(...)` at
`ClientPermissionsV2.java:217`) and completely ignore any client-specific permissions.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java
**Line:** 214:218
**Comment:**
*Logic Error: The per-client permission check looks up the resource using the resource server's internal ID as the owner, but resources created by the admin permissions schema use the realm-management client's clientId as the owner, so per-client permissions will never be found and only "all-clients" permissions will be considered, causing client-specific permissions to be ignored.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| ResourceServer server = root.realmResourceServer(); | ||
| if (server == null) return false; | ||
|
|
||
| Resource resource = resourceStore.findByName(server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE, server.getId()); |
There was a problem hiding this comment.
Suggestion: The global client permission check looks up the "Clients" resource using the resource server's internal ID as owner, but the schema creates that resource owned by the admin-permissions clientId, so the lookup always fails and realm-wide client permissions (manage/view/etc.) are never recognized. [logic error]
Severity Level: Critical 🚨
- ❌ Global "Clients" manage/view scopes never grant permissions.
- ❌ canManage() ignores FGAP v2 realm-wide client policies.
- ⚠️ Admins must rely on broad legacy AdminRoles instead.
- ⚠️ Fine-grained v2 schema ineffective for global client control.
Suggested change
| Resource resource = resourceStore.findByName(server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE, server.getId()); | |
| // Use the AdminPermissionsSchema helper to resolve the "Clients" resource with the correct owner | |
| Resource resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE); |
Steps of Reproduction ✅
1. With admin fine-grained permissions v2 enabled, the admin-permissions client and its
resource server are created and initialized through `AdminPermissionsSchema.init(...)` in
`server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java:222-255`,
which seeds an "all-clients" resource named `"Clients"` with type `"Clients"` via
`resourceServerRep.setResources(...)` at `AdminPermissionsSchema.java:246-252`.
2. The helper `AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session,
resourceServer, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE)` at
`AdminPermissionsSchema.java:116-134` looks up this global `"Clients"` resource using
`resourceStore.findByName(resourceServer, type.getType())` (no explicit owner), matching
how it was created.
3. In the v2 client permissions class, global manage/view checks call the parameterless
`hasPermission(String scope)` in
`services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java:237-259`
from `canManage()` at `71-74`, `canView()` at `85-88`, and `canManageClientScopes()` at
`105-110` to evaluate realm-wide client scopes such as `AdminPermissionsSchema.MANAGE` and
`AdminPermissionsSchema.VIEW`.
4. When `hasPermission(String scope)` runs, it attempts to locate the `"Clients"` resource
using `resourceStore.findByName(server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE,
server.getId())` at `ClientPermissionsV2.java:244`, passing the resource server **ID** as
owner. Since the `"Clients"` resource created by `AdminPermissionsSchema.init()` is owned
using the admin-permissions clientId and is looked up everywhere else via
`getResourceTypeResource(...)` without an owner filter, this lookup always returns null,
so the method returns `false` even when global client policies exist, and realm-wide
client permissions (manage/view/etc.) are never honored.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java
**Line:** 244:244
**Comment:**
*Logic Error: The global client permission check looks up the "Clients" resource using the resource server's internal ID as owner, but the schema creates that resource owned by the admin-permissions clientId, so the lookup always fails and realm-wide client permissions (manage/view/etc.) are never recognized.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
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
CodeAnt-AI Description
Add client resource type and scoped client administration permissions (V2)
What Changed
Impact
✅ Finer client admin delegation✅ Clearer forbidden errors for client operations✅ Increased test coverage for client authorization💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.