Fix addManagedMember adding user as unmanaged when user is cache-wrapped#51161
Open
simonbleher wants to merge 1 commit into
Open
Fix addManagedMember adding user as unmanaged when user is cache-wrapped#51161simonbleher wants to merge 1 commit into
simonbleher wants to merge 1 commit into
Conversation
UserModel.joinGroup(GroupModel, MembershipMetadata) has a default implementation that drops the metadata and calls the single-arg joinGroup(GroupModel), which defaults the membership to UNMANAGED. The infinispan cache UserAdapter only overrode the single-arg variant, so any UserModel obtained through the standard cached user provider (e.g. session.users().getUserById(...), as used by custom admin resource providers and other real callers of addManagedMember) fell through to that default and silently lost the MANAGED metadata. Add the missing override so the metadata is passed through to the delegate, mirroring the existing joinGroup(GroupModel) override. Closes keycloak#43094 Signed-off-by: simonbleher <blehersimon69@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes #43094 by preserving managed-membership metadata for cache-wrapped users.
Changes:
- Adds metadata-aware
joinGroupdelegation to the Infinispan user adapter. - Adds regression coverage for managed organization membership with cached users.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserAdapter.java |
Forwards membership metadata to persistence. |
tests/base/src/test/java/org/keycloak/tests/organization/cache/OrganizationCacheTest.java |
Verifies cached users remain managed members. |
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.
Problem
OrganizationProvider.addManagedMember is supposed to add a user to an
organization with MembershipType.MANAGED, but the user ends up UNMANAGED
instead whenever the UserModel passed in came from the standard cached
user provider (e.g. session.users().getUserById(...), as used by custom
AdminRealmResourceProviders and other real callers of addManagedMember).
Root cause
JpaOrganizationProvider.addMember() calls user.joinGroup(group, metadata).
UserModel has a default implementation for that overload:
The infinispan cache UserAdapter only overrides the single-arg
joinGroup(GroupModel), not the metadata-aware overload. So when user is
a cache-wrapped UserAdapter, the call falls through to the default
above, silently drops the MANAGED metadata, and ends up at the JPA
layer's joinGroup(group) -> joinGroup(group, null), which defaults the
membership type to UNMANAGED.
Fix
Added the missing joinGroup(GroupModel, MembershipMetadata) override to
the infinispan UserAdapter, mirroring the existing joinGroup(GroupModel)
override exactly, so the metadata is passed through to the delegate
instead of being dropped.
Testing
Added OrganizationCacheTest#testAddManagedMemberWithCachedUser, which
fetches a user through the normal cached user provider, calls
addManagedMember, and asserts isManagedMember is true.
(expected: but was: ) and passes with the fix.
(OrganizationCacheTest, OrganizationMemberTest,
OrganizationMemberIdpLinkTest, OrganizationInternalGroupTest,
OrganizationGroupMembershipTest) - 60 tests, all passing, no
regressions.
Closes #43094