Skip to content

Parameterized UserPropertyMapper exposes target user attributes without permission check#51063

Open
mabartos wants to merge 1 commit into
keycloak:mainfrom
mabartos:KC-51061
Open

Parameterized UserPropertyMapper exposes target user attributes without permission check#51063
mabartos wants to merge 1 commit into
keycloak:mainfrom
mabartos:KC-51061

Conversation

@mabartos

@mabartos mabartos commented Jul 21, 2026

Copy link
Copy Markdown
Member

@keycloak/core-authn Could you please check it? Thanks!

Screencast.From.2026-07-23.14-24-36.mp4

Copilot AI review requested due to automatic review settings July 21, 2026 11:53
@mabartos
mabartos requested a review from a team as a code owner July 21, 2026 11:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds authorization checks before exposing user attributes through parameterized scope mappers.

Changes:

  • Requires canView permission for resolved users.
  • Adds privileged and unprivileged integration tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
ParameterizedScopeUserPropertyMapper.java Adds user-view permission enforcement.
ParameterizedScopeMapperTest.java Tests permitted and suppressed claims.

@mabartos
mabartos marked this pull request as draft July 21, 2026 12:19
@mabartos
mabartos marked this pull request as ready for review July 21, 2026 12:26
Copilot AI review requested due to automatic review settings July 21, 2026 12:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

services/src/main/java/org/keycloak/protocol/oidc/mappers/ParameterizedScopeUserPropertyMapper.java:80

  • This reverses the authorization relationship for the built-in delegation scope. There, the parameter user is the administrator authorized to impersonate the logged-in user, so the logged-in user generally cannot view or impersonate the parameter user; consequently the built-in may_act.sub mapper is suppressed and existing delegation flows fail (for example, TokenExchangeDelegationTest.delegation() expects that claim). Preserve the delegation scope's already-validated impersonation semantics while applying this new check to ordinary username scopes.
            if (!userPermissions.canView(user) && !userPermissions.canImpersonate(user, client)) {
                continue;

Copilot AI review requested due to automatic review settings July 22, 2026 07:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

services/src/main/java/org/keycloak/protocol/oidc/mappers/ParameterizedScopeUserPropertyMapper.java:81

  • This impersonation fallback leaves a disclosure path on ordinary username scopes: an unprivileged requester can target any administrator who can impersonate them, and the mapper will expose that administrator's configured attributes even though the requester cannot view the administrator. Only permit this fallback when the mapper belongs to a validated delegation scope; all other scope types must require userPermissions.canView(user).
            // allow if the current user can view the target, or the target can impersonate the current user (delegation)
            UserPermissionEvaluator targetUserPermissions = AdminPermissions.evaluator(keycloakSession, realm, realm, user).users();
            if (!userPermissions.canView(user) && !targetUserPermissions.canImpersonate(currentUser, client)) {

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings July 22, 2026 08:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 22, 2026 13:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@mabartos mabartos self-assigned this Jul 23, 2026
@mabartos
mabartos marked this pull request as draft July 23, 2026 09:23

@rmartinc rmartinc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion. Think if it makes sense...

…ut permission check

Closes keycloak#51061

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

@rmartinc rmartinc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mabartos! I think it's OK.

@mabartos
mabartos marked this pull request as ready for review July 23, 2026 13:21
@mabartos
mabartos requested a review from a team as a code owner July 23, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parameterized UserPropertyMapper exposes target user attributes without permission check

3 participants