Repository navigation
Skip admin permission filters for LDAP group mapper group lookups - #54065
Closed
mohitduhan19 wants to merge 3 commits into
Closed
mohitduhan19 wants to merge 3 commits into
mohitduhan19 wants to merge 3 commits into
Conversation
When a user holding a query-* admin role, but neither view-users nor manage-users, is the user of the request (token refresh, account console, account REST API), the fine-grained admin permission filters are applied to group queries. The group lookups of GroupLDAPStorageMapper then miss existing groups, so the mapper tries to create them again and fails with a duplicate key on SIBLING_NAMES, which locks the user out. HardcodedLDAPGroupStorageMapper is affected the same way: the hardcoded group is not found, and the permission check calls back into the mapper until a StackOverflowError. These lookups resolve or sync the groups LDAP says a user belongs to, they are not admin queries. Run them without authorization, as GroupPolicyProvider already does for its group lookup, and collect the streams inside that scope so that the queries are not filtered later when the streams are consumed. Closes keycloak#54054 Signed-off-by: Mohit Kumar <91405114+mohitduhan19@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Subgroup lookups remain filtered during preserved-inheritance synchronization and can still cause duplicate-group failures.
1 open finding
What changed in this PR
Addresses #54054 by bypassing admin permission filters for internal LDAP group lookups, preventing failures for federated users with query-only admin roles.
Changes:
- Wraps group and hardcoded-group lookups in authorization-free scopes.
- Adds regression tests for token refresh and account group access.
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPHardcodedGroupMapperTest.java | Tests hardcoded-group access for query-only users. |
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java | Tests token refresh with flat and preserved-inheritance mappings. |
| federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/membership/group/GroupLDAPStorageMapper.java | Bypasses authorization for internal group lookups. |
| federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/HardcodedLDAPGroupStorageMapper.java | Bypasses authorization for hardcoded-group resolution. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
getKcSubGroups still applied the fine-grained admin permission filters when listing the sub groups of a parent group. During an on-demand sync with preserved inheritance, the parent group is updated first, so the cache delegates to the JPA query, existing child groups are hidden and the mapper tries to create them again. Signed-off-by: Mohit Kumar <91405114+mohitduhan19@users.noreply.github.com>
With preserved inheritance, a new LDAP group that the user belongs to makes the mapper sync the group tree while resolving the user's groups. The existing child group must be found, not created again. Signed-off-by: Mohit Kumar <91405114+mohitduhan19@users.noreply.github.com>
Member
|
Thank you for the PR, but closing in favor of #54074 |
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
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.
Closes #54054
A federated user holding a query-* admin role (but neither view-users nor manage-users) is locked out when the group-name cache is cold: the LDAP group mapper's internal group lookups get the FGAP partial-evaluation filters of that user applied (
ID = 'none'), an existing group is not found, and the mapper tries to create it again (ModelDuplicateException on SIBLING_NAMES). This happens for every request where the user is bound to the session context before the group mappings are resolved, such as a token refresh or the account console, and when the LDAP user cache is disabled.HardcodedLDAPGroupStorageMapper has the same problem. The hardcoded group lookup is filtered, and shouldSkipPartialEvaluation calls user.hasRole(), which goes back into the mapper, so the request ends in a StackOverflowError.
Changes:
AdminPermissionsSchema.runWithoutAuthorization. This followsGroupPolicyProvider. Streams are collected inside that scope, so that the queries are not filtered later when the streams are consumed.Tests (existing classes, as asked by testsuite/DEPRECATED.md):
test10c/test10d, flat and preserved-inheritance groups in READ_ONLY mode with NO_CACHE. A hardcoded role mapper gives the userquery-clients, so the tests also run in LDAPGroupMapperNoImportTest. They log in, clear the realm cache and refresh the token twice. Without the fix, the refresh fails with "conflict: Duplicate resource error" and the server logs the SIBLING_NAMES violation, in both import and no-import modes.testHardcodedGroupForUserWithQueryAdminRolecalls/account/groupswith the user's token. Without the fix, the server returns 500 (StackOverflowError).The existing LDAP group mapper, sync, hardcoded mapper, role mapping and no-import test classes still pass.
LDAPProvidersIntegrationTest#testSyncRegistrationEmailRDNNoDefaultfails locally for me, and it fails the same way on main without this change.An AI coding assistant helped with parts of this change; I reviewed and understand every line and can address review feedback.