Repository navigation
Make OrganizationGroupMembershipMapper claim name configurable - #47852
Merged
Merged
Conversation
vramik
suggested changes
Apr 15, 2026
Contributor
There was a problem hiding this comment.
@sventorben Thanks for the contribution! The approach looks good overall — aligning OrganizationGroupMembershipMapper with OrganizationMembershipMapper makes sense, and the getEffectiveModel() + mapClaim refactoring is a nice improvement.
Two concerns:
- Nested claim paths seems to be broken due to flat read in
getOrInitializeOrganizationClaimAsMap
The issue description explicitly mentions support for nested claim paths (e.g., custom.org.groups). The write side now correctly uses mapClaim() which splits on dots and creates nested structure, but the read side at
https://github.com/keycloak/keycloak/blob/f61c6672d4/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java#L291 still does a flat Map.get():
Object existingClaim = token.getOtherClaims().get(effectiveModel.getConfig().get(TOKEN_CLAIM_NAME));
When OrganizationMembershipMapper writes to a dotted claim name like "foo.organization", it creates otherClaims["foo"]["organization"] via mapClaim. Then OrganizationGroupMembershipMapper tries to read it back with otherClaims.get("foo.organization") — finds nothing, creates an empty map, and overwrites the nested path, losing the membership data.
Before this PR both sides were flat (get + setOtherClaims), so they were at least consistent. Now the write side uses path-aware mapClaim while the read side remains flat. The read should traverse the nested map using splitClaimPath. wdyt?
- Missing tests for custom claim name
It seems that there are no tests verifying that a non-default claim name actually works — neither for OrganizationGroupMembershipMapper nor for the existing OrganizationMembershipMapper (which should have been done in the past - it'd be great to add it as well).
A test should:
- Configure both mappers with a custom claim name (e.g., "my_orgs")
- Verify the token contains the claim at "my_orgs" and not at "organization"
- Verify composition still works (groups are merged into the membership data)
- Ideally also a test with a dotted claim name to cover the nested path scenario
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.organization.mapper.OrganizationGroupMembershipOIDCMapperTest#testNestedGroupsWithRelativePathsorg.keycloak.testsuite.organization.mapper.OrganizationGroupMembershipOIDCMapperTest#testCompositionWithStringTypeOrgMapperorg.keycloak.testsuite.organization.mapper.OrganizationGroupMembershipOIDCMapperTest#testCompositionWithAttributesAndIdorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testMapDifferentOrganizationWhenReAuthenticatingorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationsClaimSingleValuedorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testPasswordGrantWithAllOrganizationsAndRefreshorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testUserInfoEndpointorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationScopeSpecifyDisabledOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenWithAllOrganizationsAskingForAnyorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testCustomOrganizationScopeNameorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationScopeSelectDisabledOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationScopeMapsSpecificOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationNotAddedByGroupMapperorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testCustomOrganizationScopeNameAllOrganizationsorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testIntrospectionEndpointorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationsClaimAsListorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenWithSingleOrganizationsAskingDifferentOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenScopeWithOrganizationSelectionAskingForDifferentOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testMultipleTabsTrackingDifferentOrganizationSelectionHoldAcrossTokenRefreshorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationScopeAnyMapsSingleOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenScopeWithSingleOrganizationAskingForAllorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testIncludeOrganizationAttributesorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testIncludeOrganizationIdorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testPasswordGrantTypeorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationsClaimMappedIfScopeInTokenDisabledorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenScopeWithOrganizationSelectionAskingForAllorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenWithAllOrganizationsAskingForSpecificOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testSelectDifferentOrganizationWhenReAuthenticatingorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationAttributeNamedIdIsOverriddenByOrganizationIdorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testSelectOrganizationMapDifferentOrganizationWhenReAuthenticatingorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenScopeWithOrganizationSelectionAskingForSameOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testRefreshTokenWithSingleOrganizationsAskingAllOrganizationsorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testMultipleTabsWithMixedScopeFormatsorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testMultipleOrganizationScopesorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationScopeAnyAskUserToSelectOrganizationorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testCustomOrganizationScopeNameMultipleSpecificorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testOrganizationScopeMapsAllOrganizationsorg.keycloak.testsuite.organization.mapper.OrganizationOIDCProtocolMapperTest#testForceSelectingOrganizationWhenReAuthenticatingUsingDifferentClient |
Contributor
|
@sventorben perfect, thank you for the update. From the bot report it seems it needs one last thing to make tests independent of each other. In Could you add setMapperConfig(OIDCAttributeMapperHelper.TOKEN_CLAIM_NAME, null);there? |
pedroigor
force-pushed
the
gh-47851
branch
2 times, most recently
from
April 28, 2026 16:11
c47393a to
822d453
Compare
Contributor
|
@vramik @sventorben Updated the PR to avoid the claim name config in the group mapper so that we just resolve the org claim name from the organization mapper mapping orgs to tokens. |
Contributor
|
One thing perhaps worth documenting is that there should not be more than one organization-membership mapper configured to a client. The last change I did assumes there is only one but we do not impose any validation on this constraint. |
pedroigor
force-pushed
the
gh-47851
branch
2 times, most recently
from
April 28, 2026 18:09
3341672 to
e97fd07
Compare
Contributor
+1 do you prefer to do it within this one or rather follow-up? |
The OrganizationGroupMembershipMapper introduced in 26.6.0 hardcoded the token claim name to "organization", unlike OrganizationMembershipMapper which already exposes the claim name as a configurable property. - Add TOKEN_CLAIM_NAME config property to OrganizationGroupMembershipMapper via OIDCAttributeMapperHelper.addTokenClaimNameConfig() - Override getEffectiveModel() to default the claim name to OAuth2Constants.ORGANIZATION when not set, preserving backward compatibility for existing mapper configurations - Set TOKEN_CLAIM_NAME default in the static create() factory method - Refactor OIDCAttributeMapperHelper.getOrInitializeOrganizationClaimAsMap() to accept a ProtocolMapperModel instead of a raw String, delegating to mapClaim() for correct claim placement (including nested path support) Closes keycloak#47851 Signed-off-by: Sven-Torben Janus <sven-torben.janus@conciso.de>
The read side of getOrInitializeOrganizationClaimAsMap was doing a flat
Map.get() on the dotted claim name, while the write side (mapClaim) already
creates a nested structure by splitting on dots. This caused the group mapper
to find nothing when the claim name contained a dot, overwriting the
membership data written by OrganizationMembershipMapper.
Fix by splitting the claim path via splitClaimPath() and traversing the
nested map with a new private getNestedClaimValue() helper in
OIDCAttributeMapperHelper. The helper belongs there rather than in JsonUtils
because it operates on Map<String,Object>, not JsonNode.
Also add integration tests covering:
- Custom flat claim name ("my_orgs") for both OrganizationMembershipMapper
and OrganizationGroupMembershipMapper, verifying the claim appears at the
configured name and not at "organization"
- Dotted claim name ("custom.org") for OrganizationGroupMembershipMapper,
verifying the token contains nested otherClaims["custom"]["org"] and that
group composition is preserved
Signed-off-by: Sven-Torben Janus <sven-torben.janus@conciso.de>
Contributor
|
@vramik Updated the docs with a "notice". |
vramik
approved these changes
Apr 29, 2026
pedroigor
approved these changes
May 4, 2026
Closed
sashyo
pushed a commit
to tide-foundation/keycloak-IGA
that referenced
this pull request
Jul 20, 2026
…oak#47852) * Make OrganizationGroupMembershipMapper claim name configurable The OrganizationGroupMembershipMapper introduced in 26.6.0 hardcoded the token claim name to "organization", unlike OrganizationMembershipMapper which already exposes the claim name as a configurable property. - Add TOKEN_CLAIM_NAME config property to OrganizationGroupMembershipMapper via OIDCAttributeMapperHelper.addTokenClaimNameConfig() - Override getEffectiveModel() to default the claim name to OAuth2Constants.ORGANIZATION when not set, preserving backward compatibility for existing mapper configurations - Set TOKEN_CLAIM_NAME default in the static create() factory method - Refactor OIDCAttributeMapperHelper.getOrInitializeOrganizationClaimAsMap() to accept a ProtocolMapperModel instead of a raw String, delegating to mapClaim() for correct claim placement (including nested path support) Closes keycloak#47851 Signed-off-by: Sven-Torben Janus <sven-torben.janus@conciso.de> * Fix nested claim path read and add custom claim name tests The read side of getOrInitializeOrganizationClaimAsMap was doing a flat Map.get() on the dotted claim name, while the write side (mapClaim) already creates a nested structure by splitting on dots. This caused the group mapper to find nothing when the claim name contained a dot, overwriting the membership data written by OrganizationMembershipMapper. Fix by splitting the claim path via splitClaimPath() and traversing the nested map with a new private getNestedClaimValue() helper in OIDCAttributeMapperHelper. The helper belongs there rather than in JsonUtils because it operates on Map<String,Object>, not JsonNode. Also add integration tests covering: - Custom flat claim name ("my_orgs") for both OrganizationMembershipMapper and OrganizationGroupMembershipMapper, verifying the claim appears at the configured name and not at "organization" - Dotted claim name ("custom.org") for OrganizationGroupMembershipMapper, verifying the token contains nested otherClaims["custom"]["org"] and that group composition is preserved Signed-off-by: Sven-Torben Janus <sven-torben.janus@conciso.de> --------- Signed-off-by: Sven-Torben Janus <sven-torben.janus@conciso.de>
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.
Summary
The OrganizationGroupMembershipMapper introduced in 26.6.0 hardcodes the token claim name to "organization". This PR brings it in line with OrganizationMembershipMapper, which already allows the claim name to be configured.
Changes
OrganizationGroupMembershipMapper
OIDCAttributeMapperHelper
Backward Compatibility
Fully backward compatible. Existing deployments without an explicit claim name in the mapper config will continue to emit the claim under "organization" via the getEffectiveModel() fallback.
Closes #47851