Fix LDAP group resolution for users with limited admin permissions by martin-kanis · Pull Request #54074 · keycloak/keycloak

martin-kanis · 2026-10-16T18:32:18Z

The fix also covers inherited LDAP groups, automatic synchronization of newly discovered groups, and hardcoded group mappers. These paths could cause duplicate-group errors, missing group-derived roles in tokens, or a stack overflow. Regression tests cover login, token issuance and refresh with cold and populated group caches, while verifying that administrative group access remains restricted.

Copilot

🔵 Needs a closer look

The shared authorization behavior change warrants human security review and integration-test validation before approval.

0 open findings

What changed in this PR

Fixes #54054 by separating internal LDAP group resolution from administrative permission filtering, preventing login failures for users with limited admin roles.

Changes:

Removes authorization filtering from group-name lookups and bypasses it during inherited LDAP group resolution and automatic synchronization.
Adds regression tests for authentication, tokens, cache states, and restricted administrative access.
Documents callers’ responsibility to enforce administrative permissions.

File	Description
tests/base/src/test/java/org/keycloak/tests/suites/Base2TestSuite.java	Includes federation tests in the suite.
tests/base/src/test/java/org/keycloak/tests/federation/ldap/LDAPGroupMapperFgapTest.java	Adds LDAP group-resolution regression tests.
tests/base/pom.xml	Adds the embedded LDAP test dependency.
server-spi/src/main/java/org/keycloak/storage/group/GroupLookupProvider.java	Clarifies lookup authorization responsibilities.
server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java	Documents group-path lookup permission requirements.
model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java	Removes administrative filtering from group-name resolution.
federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java	Bypasses filtering for inherited resolution and automatic synchronization.
docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc	Explains the authorization behavior change for extensions.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

pedroigor

LGTM

pedroigor

@martin-kanis I think the main problem here is that the request is going though FGAP but it should not as we are not in a request to the Admin API.

We should only run FGAP filters when processing requests to the Admin API.

It should be safer to do this and better isolate admin authz checks from other non-admin code paths.

#54074 (review)

Copilot

🔵 Needs a closer look

The shared authorization changes across Admin, SCIM, LDAP, and background sessions warrant security-focused human review and integration-suite validation.

No open issues 🎉

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

The shared authorization-boundary changes across Admin, SCIM, and background sessions require security-focused human review and integration-test confirmation.

No open issues 🎉

_{Review effort: Balanced}

martin-kanis · 2026-10-26T12:59:20Z

@pedroigor Thanks, I tried to implement your suggestion in the second commit. Please let me know what do you think.

pedroigor · 2026-10-27T20:15:08Z

@martin-kanis Can you please check the last commit:

A session-aware AdminAuth constructor registers the admin and enables query filtering for Admin and SCIM requests. SCIM reuses this instance for permission checks and admin events.
The admin user is wrapped in an FGAPAdminUserModel proxy, so resolving its roles, groups, and attributes is unfiltered regardless of the user storage provider. Returned streams remain lazy and preserve resource cleanup.
The existing operation-level scopes remain necessary for code that reloads users independently of the proxy, including policy evaluation and token-role resolution.
Single-group lookup by name/path is no longer filtered, consistently with lookup by ID; callers still enforce access explicitly.
Removed the additional runWithAuthorizationFilters API to keep the scope focused, and added coverage for LDAP-inherited admin roles and proxy stream handling.

Copilot

🟡 Changes recommended

A critical authorization flaw causes role checks to use the supplied user's roles even when ignoreTokenRoles is false, potentially granting roles omitted from the token.

1 open issue

Prevent tokenUser from bypassing token role checks · New

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Changes to FGAP authorization boundaries, unfiltered lazy user operations, and session-wide policy-evaluation suppression require maintainer validation of delegated-admin and cross-realm isolation across LDAP, Admin REST, and SCIM.

No open issues 🎉

1 resolved since last review

Prevent tokenUser from bypassing token role checks

_{Review effort: Balanced}

martin-kanis · 2026-10-30T12:10:56Z

@pedroigor Your design change makes sense to me. However, there might be two issues that should be considered for fixing:

Lazy-source initialization can still run with filtering enabled - The proxy disables filtering when reading stream elements, but some lazy streams start their database work while the proxy is setting up the stream.
EDIT after an internal discussion: we don't need to bother with this as this is just an imaginary provider asking a lazy stream for metadata that might initialize its underlying iterator. And if this init is touching a DB before the filtering is disabled. We don't have such provider in Keycloak so there is nothing to harden.
Self-equality breaks for some custom providers. Delegating equals() directly means proxy.equals(proxy) returns false when the underlying user uses identity-based equality.

Copilot

🔵 Needs a closer look

The changed FGAP boundaries and unfiltered user proxies require human review and integration validation of cross-realm authorization isolation, unauthorized list queries, and LDAP cold-cache behavior.

No open issues 🎉

_{Review effort: Balanced}

Closes keycloak#54054 Signed-off-by: Martin Kanis <mkanis@ibm.com>

…lookups Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

Copilot

🔵 Needs a closer look

Changes to FGAP activation across Admin REST and SCIM, including filtering suppression during lazy stream consumption, require human security review and LDAP/SCIM regression testing to validate delegated-admin isolation.

No open issues 🎉

_{Review effort: Balanced}

…kups Backport all three commits from PR keycloak#54074 for keycloak#54054 to release/26.8. Bind automatic FGAP query filtering to the authenticated Admin or SCIM identity. Keep internal role, group and policy lookups unfiltered to avoid LDAP group resolution failures and recursive authorization evaluation. Include LDAP login, token and delegated-admin regression coverage. Adapt the group visibility test to the 26.8 admin client using a local REST proxy while preserving the flat-list assertions. Closes keycloak#54054 (cherry picked from commit 8ae1053) (cherry picked from commit c612179) (cherry picked from commit 244ec89) Signed-off-by: Martin Kanis <mkanis@ibm.com> Co-authored-by: Pedro Igor <pigor.craveiro@gmail.com> Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

…kups Backport all three commits from PR #54074 for #54054 to release/26.8. Bind automatic FGAP query filtering to the authenticated Admin or SCIM identity. Keep internal role, group and policy lookups unfiltered to avoid LDAP group resolution failures and recursive authorization evaluation. Include LDAP login, token and delegated-admin regression coverage. Adapt the group visibility test to the 26.8 admin client using a local REST proxy while preserving the flat-list assertions. Closes #54054 (cherry picked from commit 8ae1053) (cherry picked from commit c612179) (cherry picked from commit 244ec89) Signed-off-by: Martin Kanis <mkanis@ibm.com> Co-authored-by: Pedro Igor <pigor.craveiro@gmail.com> Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

Copilot AI balanced review requested due to automatic review settings October 16, 2026 18:32

martin-kanis requested a review from a team as a code owner October 16, 2026 18:32

keycloak-github-bot Bot added the team/core-iam label Oct 16, 2026

Copilot started reviewing on behalf of martin-kanis October 16, 2026 18:32

View session

Copilot AI reviewed Oct 16, 2026

View reviewed changes

pedroigor previously approved these changes Oct 23, 2026

View reviewed changes

pedroigor reviewed Oct 23, 2026

View reviewed changes

pedroigor self-requested a review October 23, 2026 19:57

Copilot AI balanced review requested due to automatic review settings October 26, 2026 11:51

Copilot started reviewing on behalf of martin-kanis October 26, 2026 11:52

View session

Copilot AI reviewed Oct 26, 2026

View reviewed changes

martin-kanis force-pushed the issue-54054 branch from 5fb4bd6 to 41a1bd7 Compare October 26, 2026 11:56

Copilot AI balanced review requested due to automatic review settings October 26, 2026 11:56

Copilot started reviewing on behalf of martin-kanis October 26, 2026 11:57

View session

Copilot AI reviewed Oct 26, 2026

View reviewed changes

pedroigor force-pushed the issue-54054 branch from 41a1bd7 to ac38641 Compare October 27, 2026 20:10

Copilot AI balanced review requested due to automatic review settings October 27, 2026 20:10

Copilot started reviewing on behalf of martin-kanis October 27, 2026 20:11

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/authorization/common/KeycloakIdentity.java Resolved

pedroigor force-pushed the issue-54054 branch from ac38641 to 614dfd5 Compare October 27, 2026 20:25

Copilot AI balanced review requested due to automatic review settings October 27, 2026 20:25

Copilot started reviewing on behalf of martin-kanis October 27, 2026 20:26

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

pedroigor force-pushed the issue-54054 branch from 614dfd5 to e878c1f Compare October 30, 2026 20:13

Copilot AI balanced review requested due to automatic review settings October 30, 2026 20:13

Copilot started reviewing on behalf of martin-kanis October 30, 2026 20:13

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

martin-kanis and others added 3 commits October 30, 2026 18:06

Fix LDAP group resolution for users with limited admin permissions

b97e700

Closes keycloak#54054 Signed-off-by: Martin Kanis <mkanis@ibm.com>

Scope FGAP query filtering to management operations

10f2ca5

Closes keycloak#54054 Signed-off-by: Martin Kanis <mkanis@ibm.com>

Centralize FGAP admin identity and preserve unfiltered authorization …

a8acf56

…lookups Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

pedroigor force-pushed the issue-54054 branch from e878c1f to a8acf56 Compare October 30, 2026 21:06

Copilot AI balanced review requested due to automatic review settings October 30, 2026 21:06

Copilot started reviewing on behalf of martin-kanis October 30, 2026 21:06

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

pedroigor approved these changes Nov 3, 2026

View reviewed changes

pedroigor merged commit 244ec89 into keycloak:main Nov 3, 2026
100 checks passed

This was referenced Nov 3, 2026

Skip admin permission filters for LDAP group mapper group lookups #54065

Closed

Scope FGAP filtering to admin requests (26.8 backport) #54630

Merged

Conversation

martin-kanis commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

pedroigor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

martin-kanis commented Oct 26, 2026

Uh oh!

pedroigor commented Oct 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

martin-kanis commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martin-kanis commented Oct 16, 2026 •

edited

Loading

pedroigor left a comment •

edited

Loading

martin-kanis commented Oct 30, 2026 •

edited

Loading