Introduce dedicated delegation permission for token exchange delegation - #51520
Introduce dedicated delegation permission for token exchange delegation#51520mabartos wants to merge 3 commits into
Conversation
65316b5 to
14ef744
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testPostBadAssertionSignatureKeycloak CI - Adapter IT Strict Cookies |
There was a problem hiding this comment.
Pull request overview
Introduces FGAP V2-specific delegation permissions for token exchange, separating delegation from administrator impersonation.
Changes:
- Adds
delegateanddelegate-membersscopes with migration support. - Evaluates delegation through FGAP V2 and the feature flag.
- Updates delegation tests and documentation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/base/.../TokenExchangeDelegationTest.java |
Tests delegation permissions. |
tests/base/.../ParameterizedScopesOAuthGrantTest.java |
Updates implicit-flow delegation test. |
services/.../UserPermissionsV2.java |
Implements delegation evaluation. |
services/.../UserPermissions.java |
Rejects delegation under FGAP V1. |
services/.../UserPermissionEvaluator.java |
Adds delegation API methods. |
services/.../IdentityProviderPermissions.java |
Uses shared client attribute constant. |
services/.../ClientPermissions.java |
Uses shared client attribute constant. |
services/.../DelegationScopeType.java |
Switches to delegation permission. |
services/.../ClientScopeAuthorizationRequestParser.java |
Handles unsupported FGAP operations. |
services/.../DefaultEvaluationContext.java |
Uses shared attribute constants. |
server-spi-private/.../EvaluationContext.java |
Defines evaluation attribute constants. |
server-spi-private/.../AdminPermissionsSchema.java |
Defines new FGAP scopes. |
model/storage-private/.../DefaultMigrationManager.java |
Registers the migration. |
model/storage-private/.../MigrateTo26_8_0.java |
Migrates existing FGAP schemas. |
docs/guides/.../token-exchange.adoc |
Documents delegation configuration. |
docs/documentation/.../changes-26_8_0.adoc |
Adds upgrade guidance. |
authz/policy/common/.../ClientPolicyProvider.java |
Uses the shared client attribute. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
services/src/main/java/org/keycloak/protocol/oidc/scope/DelegationScopeType.java:72
- Client policies cannot authorize this path because this overload supplies no requester, so evaluation from the actor's
UserModelIdentityhas nokc.client.id;ClientPolicyProvideronly grants when that attribute is present. Pass the intended client into delegation evaluation while preserving the actor identity, or remove the documented Client-policy support.
if (!evaluator.users().canDelegate(currentUser)) {
services/src/main/java/org/keycloak/services/resources/admin/fgap/UserPermissionEvaluator.java:133
- The earlier
canImpersonate()Javadoc still links toImpersonationConstantseven though this change removes that import, leaving an unresolved Javadoc reference. Update that link toAdminRoles#IMPERSONATIONas well.
* Returns {@code true} if the caller has the {@link AdminRoles#IMPERSONATION} role.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
docs/guides/securing-apps/token-exchange.adoc:371
- A Client policy cannot authorize this flow:
canDelegateevaluates with aUserModelIdentityand nokc.client.idcontext attribute, whileClientPolicyProvideronly matches that attribute. Following this guidance silently drops the scope, so recommend a User policy for both administrators and service-account users unless the evaluator is changed to provide the actor client ID.
NOTE: To restrict delegation to specific users, select individual users when creating the permission. To restrict which administrators or service accounts can delegate, use a more specific policy (for example, a *User* or *Client* policy).
|
@rmartinc Updated based on the discussion. Should be ready for another review. External links failure is not related to this PR. |
rmartinc
left a comment
There was a problem hiding this comment.
One last comment. You can forget about it if you think that it's too pretentious.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/src/main/java/org/keycloak/services/resources/admin/fgap/UserPermissionsV2.java:144
- The new service-account branch that injects
CLIENT_ID_ATTRIBUTEis untested; all delegation tests use a regular administrator with a User policy. Add coverage with a service-account actor and a Client policy, including a nonmatching client, to verify this authorization path.
DefaultEvaluationContext context = Optional.ofNullable(root.admin())
.map(UserModel::getServiceAccountClientLink)
.map(root.realm::getClientById)
.map(client -> new DefaultEvaluationContext(root.identity(), Map.of(CLIENT_ID_ATTRIBUTE, List.of(client.getClientId())), session))
.orElse(null);
f87f80a to
4f3af66
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/src/main/java/org/keycloak/services/resources/admin/fgap/UserPermissionsV2.java:144
- The new service-account-specific evaluation path is not covered: all added delegation tests use User policies, so a regression in Client-policy matching would go unnoticed. Add a delegation test using a service-account actor and a Client policy, including a non-matching client case.
DefaultEvaluationContext context = Optional.ofNullable(root.admin())
.map(UserModel::getServiceAccountClientLink)
.map(root.realm::getClientById)
.map(client -> new DefaultEvaluationContext(root.identity(), Map.of(CLIENT_ID_ATTRIBUTE, List.of(client.getClientId())), session))
.orElse(null);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/src/main/java/org/keycloak/services/resources/admin/fgap/UserPermissionsV2.java:144
- The new service-account/Client-policy authorization branch is not exercised: all added delegation tests use
UserPolicyRepresentation, even though this path and the documentation promise Client policies for service accounts. Add an integration test with a service-account actor and a Client policy, including a non-matching client case, to prevent delegation authorization regressions in this security-sensitive path.
DefaultEvaluationContext context = Optional.ofNullable(root.admin())
.map(UserModel::getServiceAccountClientLink)
.map(root.realm::getClientById)
.map(client -> new DefaultEvaluationContext(root.identity(), Map.of(CLIENT_ID_ATTRIBUTE, List.of(client.getClientId())), session))
.orElse(null);
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessAuthAttachmentRegTest#authenticatorAttachmentPlatform |
Closes keycloak#51481 Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessAuthAttachmentRegTest#authenticatorAttachmentPlatform |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/src/main/java/org/keycloak/services/resources/admin/fgap/UserPermissionsV2.java:143
- This service-account/Client-policy evaluation path is not exercised by the updated delegation tests; they only use a regular user actor with
UserPolicyRepresentation. Add a delegation test using a service-account actor and an FGAP Client policy so the newCLIENT_ID_ATTRIBUTEbehavior is verified.
DefaultEvaluationContext context = Optional.ofNullable(root.admin())
.map(UserModel::getServiceAccountClientLink)
.map(root.realm::getClientById)
.map(client -> new DefaultEvaluationContext(root.identity(), Map.of(CLIENT_ID_ATTRIBUTE, List.of(client.getClientId())), session))
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessOtherSettingsTest#excludeCredentials |
|
@rmartinc Should be ready for the review. |
rmartinc
left a comment
There was a problem hiding this comment.
Thanks @mabartos! LGTM!
@pedroigor Do you want to take a look to this? You know much more than us.... 😄
delegatescope on USERS resource type anddelegate-memberson GROUPS (mirrors impersonate/impersonate-members pattern)