Introduce selective multi-valued claim handling in JWT and UserInfo based on claim metadata#3255
Open
PasinduYeshan wants to merge 1 commit into
Conversation
…ased on claim metadata Add opt-in config ConvertOnlyMultiValuedClaimsToArray (default false) so that claim values containing commas are emitted as a JSON array only when the claim's local-claim metadata has multiValued=true. When the flag is off (default), legacy comma-split behavior is preserved unchanged. Special cases retained: groups always array, address never array. JWT/ID-token path: OAuthServerConfiguration parses the flag; DefaultOIDCClaimsCallbackHandler resolves the set of multi-valued OIDC claim URIs per tenant via the claim metadata service and gates the array/string decision on that set. JWTAccessTokenOIDCClaimsHandler (the access-token claims-separation path) applies the same selective logic so JWT access tokens honour the flag too. UserInfo path: ClaimUtil.isMultiValuedAttribute overloaded to accept the local-claim-URI-keyed set; UserInfoUserStoreClaimRetriever uses the same set on the cache-hit path. OAuth2Util holds the shared helper. All paths degrade gracefully to legacy comma-split on ClaimMetadataException or an unavailable claim metadata service. Tests: DefaultOIDCClaimsCallbackHandlerMultiValuedClaimTest and JWTAccessTokenOIDCClaimsHandlerMultiValuedClaimTest (JWT/ID-token + access-token paths), OAuth2UtilTest and ClaimUtilTest additions (UserInfo path) covering flag off/on, single vs multi-valued, groups/address special cases, and graceful fallback.
📝 WalkthroughWalkthroughA new ChangesSelective Multi-Valued Claim Handling
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/util/OAuth2UtilTest.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+46
to
+49
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { | ||
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | ||
| multiValuedLocalClaimUris = OAuth2Util.getMultiValuedLocalClaimUris(tenantDomain); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { | |
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | |
| multiValuedLocalClaimUris = OAuth2Util.getMultiValuedLocalClaimUris(tenantDomain); | |
| } | |
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { | |
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | |
| log.debug("ConvertOnlyMultiValuedClaimsToArray is enabled. Retrieving multi-valued claims for tenant: " + tenantDomain); | |
| multiValuedLocalClaimUris = OAuth2Util.getMultiValuedLocalClaimUris(tenantDomain); |
Comment on lines
61
to
65
| .getUserInfoMultiValueSupportEnabled(); | ||
| if (isMultiValueSupportEnabledForUserinfoResponse && | ||
| ClaimUtil.isMultiValuedAttribute(claimUri, claimValue)) { | ||
| ClaimUtil.isMultiValuedAttribute(claimUri, localClaimUri, claimValue, | ||
| multiValuedLocalClaimUris)) { | ||
| String[] attributeValues = ClaimUtil.processMultiValuedAttribute(claimValue); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| .getUserInfoMultiValueSupportEnabled(); | |
| if (isMultiValueSupportEnabledForUserinfoResponse && | |
| ClaimUtil.isMultiValuedAttribute(claimUri, claimValue)) { | |
| ClaimUtil.isMultiValuedAttribute(claimUri, localClaimUri, claimValue, | |
| multiValuedLocalClaimUris)) { | |
| String[] attributeValues = ClaimUtil.processMultiValuedAttribute(claimValue); | |
| if (isMultiValueSupportEnabledForUserinfoResponse && | |
| ClaimUtil.isMultiValuedAttribute(claimUri, localClaimUri, claimValue, | |
| multiValuedLocalClaimUris)) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Processing multi-valued claim: " + claimUri); | |
| } | |
| String[] attributeValues = ClaimUtil.processMultiValuedAttribute(claimValue); |
Comment on lines
+269
to
+273
| // multi-valued. A null set means the feature is off or the lookup failed, so legacy behaviour applies. | ||
| Set<String> multiValuedLocalClaimUris = null; | ||
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { | ||
| multiValuedLocalClaimUris = OAuth2Util.getMultiValuedLocalClaimUris(spTenantDomain); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| // multi-valued. A null set means the feature is off or the lookup failed, so legacy behaviour applies. | |
| Set<String> multiValuedLocalClaimUris = null; | |
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { | |
| multiValuedLocalClaimUris = OAuth2Util.getMultiValuedLocalClaimUris(spTenantDomain); | |
| } | |
| Set<String> multiValuedLocalClaimUris = null; | |
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("ConvertOnlyMultiValuedClaimsToArray is enabled. Resolving multi-valued local claim URIs for tenant: " + spTenantDomain); | |
| } | |
| multiValuedLocalClaimUris = OAuth2Util.getMultiValuedLocalClaimUris(spTenantDomain); | |
| } |
Comment on lines
+600
to
+612
| public static boolean isMultiValuedAttribute(String claimUri, String localClaimUri, String claimValue, | ||
| Set<String> multiValuedLocalClaimUris) { | ||
|
|
||
| /* To format the groups claim to always return as an array, we should consider single | ||
| group as multi value attribute. */ | ||
| if (GROUPS.equals(claimUri)) { | ||
| return true; | ||
| } | ||
| if (multiValuedLocalClaimUris != null) { | ||
| return multiValuedLocalClaimUris.contains(localClaimUri); | ||
| } | ||
| return StringUtils.contains(claimValue, FrameworkUtils.getMultiAttributeSeparator()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| public static boolean isMultiValuedAttribute(String claimUri, String localClaimUri, String claimValue, | |
| Set<String> multiValuedLocalClaimUris) { | |
| /* To format the groups claim to always return as an array, we should consider single | |
| group as multi value attribute. */ | |
| if (GROUPS.equals(claimUri)) { | |
| return true; | |
| } | |
| if (multiValuedLocalClaimUris != null) { | |
| return multiValuedLocalClaimUris.contains(localClaimUri); | |
| } | |
| return StringUtils.contains(claimValue, FrameworkUtils.getMultiAttributeSeparator()); | |
| } | |
| public static boolean isMultiValuedAttribute(String claimUri, String localClaimUri, String claimValue, | |
| Set<String> multiValuedLocalClaimUris) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Checking if claim is multi-valued. OIDC claim: " + claimUri + ", Local claim: " + localClaimUri); | |
| } | |
| /* To format the groups claim to always return as an array, we should consider single | |
| group as multi value attribute. */ | |
| if (GROUPS.equals(claimUri)) { | |
| return true; | |
| } | |
| if (multiValuedLocalClaimUris != null) { | |
| boolean isMultiValued = multiValuedLocalClaimUris.contains(localClaimUri); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Using claim metadata to determine multi-valued status. Local claim " + localClaimUri + " is multi-valued: " + isMultiValued); | |
| } | |
| return isMultiValued; | |
| } | |
| return StringUtils.contains(claimValue, FrameworkUtils.getMultiAttributeSeparator()); | |
| } |
Comment on lines
+3950
to
+3955
| OMElement convertOnlyMultiValuedClaimsToArrayElem = openIDConnectConfigElem.getFirstChildWithName( | ||
| getQNameWithIdentityNS(ConfigElements.OPENID_CONNECT_CONVERT_ONLY_MULTI_VALUED_CLAIMS_TO_ARRAY)); | ||
| if (convertOnlyMultiValuedClaimsToArrayElem != null) { | ||
| convertOnlyMultiValuedClaimsToArray = Boolean.parseBoolean( | ||
| convertOnlyMultiValuedClaimsToArrayElem.getText().trim()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| OMElement convertOnlyMultiValuedClaimsToArrayElem = openIDConnectConfigElem.getFirstChildWithName( | |
| getQNameWithIdentityNS(ConfigElements.OPENID_CONNECT_CONVERT_ONLY_MULTI_VALUED_CLAIMS_TO_ARRAY)); | |
| if (convertOnlyMultiValuedClaimsToArrayElem != null) { | |
| convertOnlyMultiValuedClaimsToArray = Boolean.parseBoolean( | |
| convertOnlyMultiValuedClaimsToArrayElem.getText().trim()); | |
| } | |
| if (convertOnlyMultiValuedClaimsToArrayElem != null) { | |
| convertOnlyMultiValuedClaimsToArray = Boolean.parseBoolean( | |
| convertOnlyMultiValuedClaimsToArrayElem.getText().trim()); | |
| log.info("ConvertOnlyMultiValuedClaimsToArray is set to: " + convertOnlyMultiValuedClaimsToArray); | |
| } |
Comment on lines
+2333
to
+2334
| */ | ||
| public static Set<String> getMultiValuedLocalClaimUris(String tenantDomain) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| */ | |
| public static Set<String> getMultiValuedLocalClaimUris(String tenantDomain) { | |
| public static Set<String> getMultiValuedLocalClaimUris(String tenantDomain) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Building multi-valued local claim URIs for tenant domain: " + tenantDomain); | |
| } |
Comment on lines
+2350
to
+2351
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| } | |
| } | |
| } | |
| if (log.isDebugEnabled()) { | |
| log.debug("Found " + multiValuedLocalClaimUris.size() + " multi-valued local claims for tenant " + | |
| "domain: " + tenantDomain); | |
| } | |
| return multiValuedLocalClaimUris; |
Comment on lines
106
to
111
| try { | ||
| Map<String, Object> userClaimsInOIDCDialect = getUserClaimsInOIDCDialect(tokenReqMessageContext); | ||
| tokenReqMessageContext.addProperty(ID_TOKEN_USER_CLAIMS_PROP_KEY, userClaimsInOIDCDialect.keySet()); | ||
| return setClaimsToJwtClaimSet(jwtClaimsSetBuilder, userClaimsInOIDCDialect); | ||
| return setClaimsToJwtClaimSet(jwtClaimsSetBuilder, userClaimsInOIDCDialect, | ||
| getServiceProviderTenantDomain(tokenReqMessageContext)); | ||
| } catch (OAuthSystemException e) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| try { | |
| Map<String, Object> userClaimsInOIDCDialect = getUserClaimsInOIDCDialect(tokenReqMessageContext); | |
| tokenReqMessageContext.addProperty(ID_TOKEN_USER_CLAIMS_PROP_KEY, userClaimsInOIDCDialect.keySet()); | |
| return setClaimsToJwtClaimSet(jwtClaimsSetBuilder, userClaimsInOIDCDialect); | |
| return setClaimsToJwtClaimSet(jwtClaimsSetBuilder, userClaimsInOIDCDialect, | |
| getServiceProviderTenantDomain(tokenReqMessageContext)); | |
| } catch (OAuthSystemException e) { | |
| try { | |
| Map<String, Object> userClaimsInOIDCDialect = getUserClaimsInOIDCDialect(tokenReqMessageContext); | |
| tokenReqMessageContext.addProperty(ID_TOKEN_USER_CLAIMS_PROP_KEY, userClaimsInOIDCDialect.keySet()); | |
| log.info("Successfully retrieved user claims for ID token generation"); | |
| return setClaimsToJwtClaimSet(jwtClaimsSetBuilder, userClaimsInOIDCDialect, | |
| getServiceProviderTenantDomain(tokenReqMessageContext)); |
Comment on lines
+967
to
+970
| private Set<String> getMultiValuedClaimUris(String tenantDomain) { | ||
|
|
||
| try { | ||
| ClaimMetadataManagementService claimMetadataManagementService = |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| private Set<String> getMultiValuedClaimUris(String tenantDomain) { | |
| try { | |
| ClaimMetadataManagementService claimMetadataManagementService = | |
| private Set<String> getMultiValuedClaimUris(String tenantDomain) { | |
| log.debug("Retrieving multi-valued claim URIs for tenant domain: " + tenantDomain); | |
| try { |
Comment on lines
+954
to
+956
| private Set<String> getMultiValuedClaimUris(String tenantDomain) { | ||
|
|
||
| try { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
Suggested change
| private Set<String> getMultiValuedClaimUris(String tenantDomain) { | |
| try { | |
| private Set<String> getMultiValuedClaimUris(String tenantDomain) { | |
| log.info("Retrieving multi-valued claim URIs for tenant domain: " + tenantDomain); |
Comment on lines
+979
to
+983
| } | ||
| return multiValuedClaimUris; | ||
| } catch (ClaimMetadataException e) { | ||
| log.error("Error while retrieving claim metadata to determine multi-valued claims for tenant domain: " + | ||
| tenantDomain + ". Falling back to legacy multi-valued claim handling.", e); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| } | |
| return multiValuedClaimUris; | |
| } catch (ClaimMetadataException e) { | |
| log.error("Error while retrieving claim metadata to determine multi-valued claims for tenant domain: " + | |
| tenantDomain + ". Falling back to legacy multi-valued claim handling.", e); | |
| return multiValuedClaimUris; | |
| } catch (ClaimMetadataException e) { | |
| log.error("Error while retrieving claim metadata to determine multi-valued claims for tenant domain: " + | |
| tenantDomain + ". Falling back to legacy multi-valued claim handling."); |
Comment on lines
+3114
to
+3118
|
|
||
| try (MockedStatic<OAuth2ServiceComponentHolder> mockedHolder = | ||
| mockStatic(OAuth2ServiceComponentHolder.class)) { | ||
|
|
||
| OAuth2ServiceComponentHolder holder = mock(OAuth2ServiceComponentHolder.class); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| try (MockedStatic<OAuth2ServiceComponentHolder> mockedHolder = | |
| mockStatic(OAuth2ServiceComponentHolder.class)) { | |
| OAuth2ServiceComponentHolder holder = mock(OAuth2ServiceComponentHolder.class); | |
| try (MockedStatic<OAuth2ServiceComponentHolder> mockedHolder = | |
| mockStatic(OAuth2ServiceComponentHolder.class)) { | |
| log.debug("Testing getMultiValuedLocalClaimUris with proper claim metadata service"); | |
| OAuth2ServiceComponentHolder holder = mock(OAuth2ServiceComponentHolder.class); |
Comment on lines
+3134
to
+3137
|
|
||
| Set<String> result = OAuth2Util.getMultiValuedLocalClaimUris(SUPER_TENANT_DOMAIN_NAME); | ||
|
|
||
| assertNotNull(result, "Expected a non-null set when claim metadata is resolved."); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| Set<String> result = OAuth2Util.getMultiValuedLocalClaimUris(SUPER_TENANT_DOMAIN_NAME); | |
| assertNotNull(result, "Expected a non-null set when claim metadata is resolved."); | |
| Set<String> result = OAuth2Util.getMultiValuedLocalClaimUris(SUPER_TENANT_DOMAIN_NAME); | |
| log.info("Successfully retrieved multi-valued local claim URIs for tenant: {}", SUPER_TENANT_DOMAIN_NAME); | |
| assertNotNull(result, "Expected a non-null set when claim metadata is resolved."); |
Comment on lines
+202
to
+204
| .setClaimMetadataManagementService(claimMetadataManagementService); | ||
|
|
||
| assertNull(invokeGetMultiValuedClaimUris(TENANT_DOMAIN), |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
Suggested change
| .setClaimMetadataManagementService(claimMetadataManagementService); | |
| assertNull(invokeGetMultiValuedClaimUris(TENANT_DOMAIN), | |
| private boolean invokeIsMultiValued(String claimKey, String claimValue, Set<String> multiValuedClaimUris) | |
| throws Exception { | |
| log.debug("Checking if claim is multi-valued: claimKey={}, hasMultiValuedSet={}", | |
| claimKey, multiValuedClaimUris != null); |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java (1)
1831-1833: ⚡ Quick winUpdate the getter Javadoc to include UserInfo behavior.
Line 1831–1833 says this flag applies to JWT access tokens and ID tokens, but the same getter is also used on the UserInfo path. Please broaden the description to avoid future misconfiguration assumptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java` around lines 1831 - 1833, The Javadoc comment for the getter method on lines 1831-1833 only mentions JWT access tokens and ID tokens but the same getter is also used on the UserInfo path, which could lead to misconfiguration assumptions. Update the Javadoc description to broaden the scope and explicitly include that this flag also applies to the UserInfo endpoint behavior, ensuring developers understand the flag's complete applicability across all token types and endpoints.components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtilTest.java (1)
573-615: ⚡ Quick winAdd
addressinvariance cases to this selective test matrix.This provider/test verifies groups and metadata-driven selection, but it does not assert the PR contract that
addressmust never be treated as multi-valued (both withmultiValuedLocalClaimUris == nulland non-null). Adding those two rows would close a high-value regression gap on the UserInfo path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtilTest.java` around lines 573 - 615, The selectiveMultiValuedAttributeProvider method lacks test coverage for the address claim, which must never be treated as multi-valued according to the PR contract. Add two test rows to the return statement of selectiveMultiValuedAttributeProvider: one with multiValuedLocalClaimUris set to null and expected value false, and another with multiValuedLocalClaimUris set to the non-null multiValuedLocalClaimUris set and expected value false. Both rows should use an ADDRESS_CLAIM_URI constant (or similar identifier) as the claimUri and corresponding ADDRESS_LOCAL_URI as the localClaimUri, following the same pattern as the existing test rows for groups and other claims.components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandlerMultiValuedClaimTest.java (1)
149-176: 💤 Low valueMinor style inconsistency: use consistent import or fully qualified name.
Line 174 uses
java.util.Collections.singletonListwith a fully qualified name, butCollectionsis not imported. Consider importingjava.util.Collectionsfor consistency with other importedjava.utilclasses (Arrays, HashSet, Set).♻️ Suggested fix
Add the import:
import java.lang.reflect.Method; import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; import java.util.Set;Then simplify line 174:
- assertEquals(resolved, new HashSet<>(java.util.Collections.singletonList(MULTI_TEST)), + assertEquals(resolved, new HashSet<>(Collections.singletonList(MULTI_TEST)), "Only OIDC claims mapped to a multiValued=true local claim must be returned.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandlerMultiValuedClaimTest.java` around lines 149 - 176, The test method testGetMultiValuedClaimUrisResolvesFlaggedClaims uses the fully qualified name java.util.Collections.singletonList on line 174, while Collections is not imported despite other java.util classes (Arrays, HashSet, Set) being imported. Add an import statement for java.util.Collections at the top of the test class, then replace the fully qualified java.util.Collections.singletonList reference with just Collections.singletonList to maintain consistent import and naming style throughout the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/UserInfoUserStoreClaimRetriever.java`:
- Around line 43-49: The multi-valued local claim URIs lookup in the cache-hit
path (line 46 where OAuth2Util.getMultiValuedLocalClaimUris is called) uses the
thread-local tenant domain from PrivilegedCarbonContext, but the cache-miss path
in ClaimUtil uses spTenantDomain resolved from the app configuration (lines
195-202 of ClaimUtil.java). This mismatch causes inconsistent behavior in
cross-tenant scenarios. Replace the thread-local tenant domain retrieval with
the spTenantDomain that matches the cache-miss path, ensuring
OAuth2Util.getMultiValuedLocalClaimUris receives the same tenant domain as the
ClaimUtil cache-miss path uses, so both paths treat multi-valued claims
consistently regardless of the calling context.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java`:
- Around line 2347-2353: Add null checks before iterating over the result of
claimMetadataManagementService.getLocalClaims() in three methods to prevent
NullPointerException when the service returns null. In the
getMultiValuedLocalClaimUris method in OAuth2Util (where the loop iterates over
claimMetadataManagementService.getLocalClaims(tenantDomain)), in
JWTAccessTokenOIDCClaimsHandler.getMultiValuedClaimUris(), and in
DefaultOIDCClaimsCallbackHandler.getMultiValuedClaimUris(), verify that
getLocalClaims() does not return null before entering the for-each loop. If null
is encountered, allow the method to gracefully return null (consistent with its
documented contract) rather than throwing an unhandled NullPointerException that
bypasses the ClaimMetadataException catch block.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandler.java`:
- Around line 959-999: The getMultiValuedClaimUris() method in
DefaultOIDCClaimsCallbackHandler (lines 959-999) and the corresponding method in
JWTAccessTokenOIDCClaimsHandler (lines 946-986) only return OIDC dialect claim
URIs, but when local claim URIs are preserved in the claim keys, they won't
match this metadata set and multi-valued flags will be lost. Modify both methods
to include both the local claim URIs flagged as multi-valued and their mapped
OIDC claim URIs in the returned multiValuedClaimUris set. In
DefaultOIDCClaimsCallbackHandler, after the existing loop that adds OIDC
external claims, also iterate through the localClaimMultiValuedMap and add all
local claim URIs where the value is Boolean.TRUE to the multiValuedClaimUris
set. Apply the same logic to JWTAccessTokenOIDCClaimsHandler to keep JWT access
token claim handling consistent with ID token handling.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtilTest.java`:
- Around line 573-615: The selectiveMultiValuedAttributeProvider method lacks
test coverage for the address claim, which must never be treated as multi-valued
according to the PR contract. Add two test rows to the return statement of
selectiveMultiValuedAttributeProvider: one with multiValuedLocalClaimUris set to
null and expected value false, and another with multiValuedLocalClaimUris set to
the non-null multiValuedLocalClaimUris set and expected value false. Both rows
should use an ADDRESS_CLAIM_URI constant (or similar identifier) as the claimUri
and corresponding ADDRESS_LOCAL_URI as the localClaimUri, following the same
pattern as the existing test rows for groups and other claims.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java`:
- Around line 1831-1833: The Javadoc comment for the getter method on lines
1831-1833 only mentions JWT access tokens and ID tokens but the same getter is
also used on the UserInfo path, which could lead to misconfiguration
assumptions. Update the Javadoc description to broaden the scope and explicitly
include that this flag also applies to the UserInfo endpoint behavior, ensuring
developers understand the flag's complete applicability across all token types
and endpoints.
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandlerMultiValuedClaimTest.java`:
- Around line 149-176: The test method
testGetMultiValuedClaimUrisResolvesFlaggedClaims uses the fully qualified name
java.util.Collections.singletonList on line 174, while Collections is not
imported despite other java.util classes (Arrays, HashSet, Set) being imported.
Add an import statement for java.util.Collections at the top of the test class,
then replace the fully qualified java.util.Collections.singletonList reference
with just Collections.singletonList to maintain consistent import and naming
style throughout the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f2762853-1f6d-4289-ad32-303e6c18c14b
📒 Files selected for processing (11)
components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/UserInfoUserStoreClaimRetriever.javacomponents/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtil.javacomponents/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtilTest.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandler.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandler.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/util/OAuth2UtilTest.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandlerMultiValuedClaimTest.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandlerMultiValuedClaimTest.javacomponents/org.wso2.carbon.identity.oauth/src/test/resources/testng.xml
Comment on lines
+43
to
+49
| // When ConvertOnlyMultiValuedClaimsToArray is enabled, resolve local claim URIs flagged multi-valued. | ||
| // A null set means the feature is off or the lookup failed, so legacy behaviour applies. | ||
| Set<String> multiValuedLocalClaimUris = null; | ||
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { | ||
| String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | ||
| multiValuedLocalClaimUris = OAuth2Util.getMultiValuedLocalClaimUris(tenantDomain); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for UserInfo endpoint entry points and tenant context setup
# Find UserInfo endpoint implementations that set tenant context
rg -nP -C5 'getUserClaimsUsingTokenResponse|getClaimsMap' --type java
# Search for PrivilegedCarbonContext tenant domain setup in UserInfo paths
rg -nP -C3 'startTenantFlow|PrivilegedCarbonContext.*setTenantDomain' \
-g '*UserInfo*.java' -g '*ClaimUtil*.java'
# Look for spTenantDomain vs thread-local tenant usage patterns
rg -nP 'getThreadLocalCarbonContext\(\)\.getTenantDomain\(\)|spTenantDomain' \
--type java -C2 -g '*oauth*endpoint*'Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 33773
🏁 Script executed:
# Examine ClaimUtil to understand tenant domain handling
cd components/org.wso2.carbon.identity.oauth.endpoint
# Find the getClaimsFromUserStore method and surrounding code
rg -n 'getClaimsFromUserStore|spTenantDomain|userAccessingTenantDomain' src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtil.java -A 3 -B 1
# Look at the full method to understand tenant context
echo "=== Full getUserClaimsUsingTokenResponse method ==="
sed -n '95,120p' src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtil.java
echo "=== Full getClaimsFromUserStore method signature and start ==="
sed -n '130,180p' src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtil.java
echo "=== Tenant flow setup in getClaimsFromUserStore ==="
sed -n '230,270p' src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/ClaimUtil.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 10665
🏁 Script executed:
# Check UserInfoResponseBase to see how getClaimsMap is called and if tenant context is set
fd 'UserInfoResponseBase\.java' components/ --type f | head -1 | xargs wc -l
echo "=== Looking for calls to getUserClaimsUsingTokenResponse and retrieveUserClaims ==="
rg -n 'retrieveUserClaims|getClaimsMap' components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/UserInfo*.java -B 3 -A 3Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 4338
🏁 Script executed:
# Find where the tenant flow is started in the UserInfo endpoint flow
rg -n 'startTenantFlow|PrivilegedCarbonContext' components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/ --type java -g '*Response*.java' -B 2 -A 2Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Find UserInfoResponseBase and check how it calls ClaimUtil
fd 'UserInfoResponseBase\.java' components/org.wso2.carbon.identity.oauth.endpoint --type f
# Once found, examine it for tenant context setup
find components/org.wso2.carbon.identity.oauth.endpoint -name 'UserInfoResponseBase.java' -type f -exec wc -l {} \;Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Look at AbstractUserInfoResponseBuilder or UserInfoResponseBase
# to see the flow from endpoint to ClaimUtil
find components/org.wso2.carbon.identity.oauth.endpoint -name '*UserInfoResponse*.java' -type f | grep -E '(Base|Abstract)' | head -3
# Then check for tenant context setup in that file
for file in $(find components/org.wso2.carbon.identity.oauth.endpoint -name '*UserInfoResponse*.java' -type f | grep -E '(Base|Abstract)'); do
echo "=== $file ==="
wc -l "$file"
doneRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 534
🏁 Script executed:
# Look for tenant flow setup before retrieveUserClaims is called
rg -n 'startTenantFlow|endTenantFlow|retrieveUserClaims' \
components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/ \
--type java -B 2 -A 2 | head -80Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1908
🏁 Script executed:
# Find the actual UserInfoResponseBase (not test)
find components/org.wso2.carbon.identity.oauth.endpoint/src/main/java -name '*UserInfoResponse*.java' -type f
# Get details on these files
for file in $(find components/org.wso2.carbon.identity.oauth.endpoint/src/main/java -name '*UserInfoResponse*.java' -type f); do
echo "=== File: $file ==="
wc -l "$file"
doneRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Look at AbstractUserInfoResponseBuilder to see the flow
find components/org.wso2.carbon.identity.oauth.endpoint/src/main/java -name 'Abstract*UserInfo*.java' -type f -exec cat {} \; | head -200Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Search for where buildResponse or retrieveUserClaims is called and check tenant context
rg -n 'buildResponse|retrieveUserClaims|buildUserInfoResponse' \
components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/ \
--type java -B 5 -A 10 | head -150Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 8071
🏁 Script executed:
# Find and examine AbstractUserInfoResponseBuilder
find components/org.wso2.carbon.identity.oauth.endpoint/src/main/java -name 'AbstractUserInfoResponseBuilder.java' -type f
# Get its full content - check size first
find components/org.wso2.carbon.identity.oauth.endpoint/src/main/java -name 'AbstractUserInfoResponseBuilder.java' -type f -exec wc -l {} \;Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Read AbstractUserInfoResponseBuilder
find components/org.wso2.carbon.identity.oauth.endpoint/src/main/java -name 'AbstractUserInfoResponseBuilder.java' -type f -exec cat {} \; | head -300Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Look for where retrieveUserClaims is called in AbstractUserInfoResponseBuilder
rg -n 'retrieveUserClaims|FrameworkUtils.startTenantFlow|PrivilegedCarbonContext' \
components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/AbstractUserInfoResponseBuilder.java \
-B 3 -A 3Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 279
🏁 Script executed:
# List all files in the user/impl directory
ls -la components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 690
🏁 Script executed:
# Find all UserInfo related classes in the main source
find components/org.wso2.carbon.identity.oauth.endpoint/src/main/java -name '*UserInfo*.java' -type fRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 974
🏁 Script executed:
# Search for where retrieveUserClaims is defined (as an abstract method or interface)
rg -n 'retrieveUserClaims' components/org.wso2.carbon.identity.oauth.endpoint/src/main/java --type java -B 2 -A 5Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2970
🏁 Script executed:
# Search more broadly in the entire component for retrieveUserClaims definition
rg -n 'abstract.*retrieveUserClaims|protected.*retrieveUserClaims' components/org.wso2.carbon.identity.oauth.endpoint --type java -B 2 -A 5Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2970
🏁 Script executed:
# Search in the oauth module (not just endpoint)
find components/org.wso2.carbon.identity.oauth*/src/main/java -name '*.java' -type f -exec rg -l 'abstract.*retrieveUserClaims|protected.*retrieveUserClaims' {} \;Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 507
🏁 Script executed:
# Look at UserInfoJWTResponse more completely to find its parent class
cat components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/UserInfoJWTResponse.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 6591
🏁 Script executed:
# Now examine the AbstractUserInfoResponseBuilder in the oauth module
cat components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/AbstractUserInfoResponseBuilder.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 20808
Tenant domain mismatch between cache-hit and cache-miss paths.
Lines 43-49 look up multi-valued local claim URIs using the thread-local tenant domain (PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain()), which is not guaranteed to match the spTenantDomain used by the cache-miss path in ClaimUtil.getClaimsFromUserStore (line 272). The cache-miss path explicitly resolves spTenantDomain from the app configuration (lines 195-202 of ClaimUtil.java), while the cache-hit path relies on whatever tenant context the caller set in the thread-local context. In shared-user or cross-tenant scenarios, these could differ, causing the same claim to be treated as multi-valued or single-valued depending on the cache path taken, violating the PR's objective of "consistent selective logic" across all UserInfo code paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/user/impl/UserInfoUserStoreClaimRetriever.java`
around lines 43 - 49, The multi-valued local claim URIs lookup in the cache-hit
path (line 46 where OAuth2Util.getMultiValuedLocalClaimUris is called) uses the
thread-local tenant domain from PrivilegedCarbonContext, but the cache-miss path
in ClaimUtil uses spTenantDomain resolved from the app configuration (lines
195-202 of ClaimUtil.java). This mismatch causes inconsistent behavior in
cross-tenant scenarios. Replace the thread-local tenant domain retrieval with
the spTenantDomain that matches the cache-miss path, ensuring
OAuth2Util.getMultiValuedLocalClaimUris receives the same tenant domain as the
ClaimUtil cache-miss path uses, so both paths treat multi-valued claims
consistently regardless of the calling context.
Comment on lines
+2347
to
+2353
| for (LocalClaim localClaim : claimMetadataManagementService.getLocalClaims(tenantDomain)) { | ||
| if (Boolean.parseBoolean(localClaim.getClaimProperty(ClaimConstants.MULTI_VALUED_PROPERTY))) { | ||
| multiValuedLocalClaimUris.add(localClaim.getClaimURI()); | ||
| } | ||
| } | ||
| return multiValuedLocalClaimUris; | ||
| } catch (ClaimMetadataException e) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify getLocalClaims nullability contract and existing call-site assumptions (read-only).
# 1) Locate ClaimMetadataManagementService interface and getLocalClaims signature/docs.
fd -i "ClaimMetadataManagementService.java" --exec sed -n '1,240p' {}
# 2) Find implementations and inspect return behavior for getLocalClaims.
fd -i "*ClaimMetadataManagementService*.java" --exec rg -n -C3 "getLocalClaims\\s*\\(" {}
# 3) Check repository usage patterns to see whether callers assume non-null.
rg -n -C2 "getLocalClaims\\s*\\(" --type=javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 9955
🏁 Script executed:
# Find the ClaimMetadataManagementService interface definition
fd -type f -name "ClaimMetadataManagementService.java" -exec cat {} \;Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 258
🏁 Script executed:
# Examine the complete getMultiValuedLocalClaimUris method
sed -n '2330,2360p' components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1820
🏁 Script executed:
# Check other call sites at lines 968 and 981 for comparison
sed -n '960,975p' components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandler.java
sed -n '973,988p' components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandler.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2238
🏁 Script executed:
# Search for any null checks on getLocalClaims results in codebase
rg -n "getLocalClaims.*null" --type=java -A 2 -B 2Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Search for ClaimMetadataManagementService interface/class with getLocalClaims method
rg -n "getLocalClaims" -A 5 -B 5 | grep -A 10 -B 10 "interface\|class.*ClaimMetadataManagementService" | head -60Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Find and examine ClaimMetadataManagementService files
find . -name "*ClaimMetadataManagementService*" -type fRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 198
🏁 Script executed:
# Look for null return in getLocalClaims implementations
rg -n "getLocalClaims" -A 10 | grep -E "(return null|return Collections|return List)" | head -20Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 226
🏁 Script executed:
# Check the complete context of the other two call sites to see if they have try-catch
sed -n '950,1000p' components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandler.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2796
🏁 Script executed:
# Search for ClaimMetadataManagementService interface definition (likely in a separate library)
rg -n "interface ClaimMetadataManagementService" -A 20Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Look for any documentation or comments about getLocalClaims return contract
rg -n "getLocalClaims" -B 5 -A 2 | grep -E "(null|`@return`|javadoc|/**)" | head -40Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 7697
🏁 Script executed:
# Check if there are any defensive null checks elsewhere in the codebase for similar patterns
rg -n "if.*getLocalClaims.*null" --type javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Examine the complete context of both other call sites to confirm they also lack null checks
sed -n '950,1010p' components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandler.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 3460
🏁 Script executed:
# Search for all imports of LocalClaim and getLocalClaims to understand the return type
rg -n "import.*LocalClaim" --type java | head -10Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1311
🏁 Script executed:
# Look for the actual implementation of getLocalClaims to check if it documents null returns
rg -n "public.*List.*getLocalClaims" -A 10 | head -40Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Verify if there are tests covering null return scenario for getLocalClaims
rg -n "getLocalClaims.*thenReturn.*null" --type javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Search for any null documentation or defensive code around getLocalClaims across codebase
rg -n "getLocalClaims\|getExternalClaims" -B 3 -A 3 | grep -E "(null|`@return`|List<|throws)" | head -50Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Check if getLocalClaims can return empty list vs null by examining actual usage patterns
rg -n "getLocalClaims" -A 1 | grep -E "(size|isEmpty|== null|!= null)"Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Look at the test setup for getLocalClaims mocks more broadly
cat components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/util/OAuth2UtilTest.java | sed -n '3120,3180p'Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 3670
🏁 Script executed:
# Verify the complete method context and exception handling around both other call sites
sed -n '940,990p' components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandler.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2786
🏁 Script executed:
# Check what exception types are caught at those call sites
sed -n '940,1020p' components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandler.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 4029
Guard against null claim lists in three locations to preserve graceful fallback semantics.
At Line 2347, the code iterates directly over claimMetadataManagementService.getLocalClaims(tenantDomain) without null protection. If that API returns null (e.g., partial service failure), this throws NullPointerException, which bypasses the ClaimMetadataException catch block and can crash token/userinfo processing instead of degrading to legacy behavior.
This same vulnerability exists at two additional locations:
JWTAccessTokenOIDCClaimsHandler.getMultiValuedClaimUris()(line 968)DefaultOIDCClaimsCallbackHandler.getMultiValuedClaimUris()(line 981)
All three methods follow the same pattern: checking if the service itself is null (showing defensive intent), then calling getLocalClaims() directly without null protection, then catching only ClaimMetadataException. The method's documented contract states it returns null on metadata failure, indicating these methods should gracefully degrade rather than crash.
Proposed fix
public static Set<String> getMultiValuedLocalClaimUris(String tenantDomain) {
try {
ClaimMetadataManagementService claimMetadataManagementService =
OAuth2ServiceComponentHolder.getInstance().getClaimMetadataManagementService();
if (claimMetadataManagementService == null) {
if (log.isDebugEnabled()) {
log.debug("ClaimMetadataManagementService is not available. Falling back to legacy multi-valued " +
"claim handling for tenant domain: " + tenantDomain);
}
return null;
}
+ List<LocalClaim> localClaims = claimMetadataManagementService.getLocalClaims(tenantDomain);
+ if (localClaims == null) {
+ if (log.isDebugEnabled()) {
+ log.debug("Local claim metadata is null. Falling back to legacy multi-valued claim handling for " +
+ "tenant domain: " + tenantDomain);
+ }
+ return null;
+ }
Set<String> multiValuedLocalClaimUris = new HashSet<>();
- for (LocalClaim localClaim : claimMetadataManagementService.getLocalClaims(tenantDomain)) {
+ for (LocalClaim localClaim : localClaims) {
if (Boolean.parseBoolean(localClaim.getClaimProperty(ClaimConstants.MULTI_VALUED_PROPERTY))) {
multiValuedLocalClaimUris.add(localClaim.getClaimURI());
}
}
return multiValuedLocalClaimUris;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java`
around lines 2347 - 2353, Add null checks before iterating over the result of
claimMetadataManagementService.getLocalClaims() in three methods to prevent
NullPointerException when the service returns null. In the
getMultiValuedLocalClaimUris method in OAuth2Util (where the loop iterates over
claimMetadataManagementService.getLocalClaims(tenantDomain)), in
JWTAccessTokenOIDCClaimsHandler.getMultiValuedClaimUris(), and in
DefaultOIDCClaimsCallbackHandler.getMultiValuedClaimUris(), verify that
getLocalClaims() does not return null before entering the for-each loop. If null
is encountered, allow the method to gracefully return null (consistent with its
documented contract) rather than throwing an unhandled NullPointerException that
bypasses the ClaimMetadataException catch block.
Comment on lines
+959
to
+999
| /** | ||
| * Build the set of OIDC dialect claim URIs that map to local claims flagged as multi-valued in claim metadata | ||
| * for the given tenant. Used when the {@code ConvertOnlyMultiValuedClaimsToArray} configuration is enabled. | ||
| * | ||
| * @param tenantDomain Tenant domain of the service provider. | ||
| * @return Set of OIDC claim URIs flagged as multi-valued, or {@code null} if the metadata could not be resolved | ||
| * (so that the caller falls back to legacy multi-valued claim handling). | ||
| */ | ||
| private Set<String> getMultiValuedClaimUris(String tenantDomain) { | ||
|
|
||
| try { | ||
| ClaimMetadataManagementService claimMetadataManagementService = | ||
| OpenIDConnectServiceComponentHolder.getInstance().getClaimMetadataManagementService(); | ||
| if (claimMetadataManagementService == null) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("ClaimMetadataManagementService is not available. Falling back to legacy multi-valued " + | ||
| "claim handling for tenant domain: " + tenantDomain); | ||
| } | ||
| return null; | ||
| } | ||
| // Build a map of local claim URI -> multiValued flag. | ||
| Map<String, Boolean> localClaimMultiValuedMap = new HashMap<>(); | ||
| for (LocalClaim localClaim : claimMetadataManagementService.getLocalClaims(tenantDomain)) { | ||
| localClaimMultiValuedMap.put(localClaim.getClaimURI(), Boolean.parseBoolean( | ||
| localClaim.getClaimProperty(ClaimConstants.MULTI_VALUED_PROPERTY))); | ||
| } | ||
| // Map each OIDC dialect claim to its local claim and collect the ones flagged multi-valued. | ||
| Set<String> multiValuedClaimUris = new HashSet<>(); | ||
| for (ExternalClaim oidcClaim : claimMetadataManagementService.getExternalClaims( | ||
| OAuthConstants.OIDC_DIALECT, tenantDomain)) { | ||
| if (Boolean.TRUE.equals(localClaimMultiValuedMap.get(oidcClaim.getMappedLocalClaim()))) { | ||
| multiValuedClaimUris.add(oidcClaim.getClaimURI()); | ||
| } | ||
| } | ||
| return multiValuedClaimUris; | ||
| } catch (ClaimMetadataException e) { | ||
| log.error("Error while retrieving claim metadata to determine multi-valued claims for tenant domain: " + | ||
| tenantDomain + ". Falling back to legacy multi-valued claim handling.", e); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Include local claim URI keys in selective multi-valued detection.
Both helpers return only mapped OIDC external claim URIs. When preserved original/non-OIDC claim keys reach the JWT writer, a local claim marked multiValued=true will not match the non-null metadata set and will be emitted as a string instead of an array. Seed the returned set with metadata-flagged local claim URIs as well, or pass the local claim URI into the predicate like the UserInfo path does, while keeping the groups and address overrides intact.
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandler.java#L959-L999: return a claim-key set that includes both multi-valued local claim URIs and their mapped OIDC claim URIs.components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandler.java#L946-L986: apply the same local-plus-OIDC URI handling so JWT access tokens stay consistent with ID tokens.
📍 Affects 2 files
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandler.java#L959-L999(this comment)components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/JWTAccessTokenOIDCClaimsHandler.java#L946-L986
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/DefaultOIDCClaimsCallbackHandler.java`
around lines 959 - 999, The getMultiValuedClaimUris() method in
DefaultOIDCClaimsCallbackHandler (lines 959-999) and the corresponding method in
JWTAccessTokenOIDCClaimsHandler (lines 946-986) only return OIDC dialect claim
URIs, but when local claim URIs are preserved in the claim keys, they won't
match this metadata set and multi-valued flags will be lost. Modify both methods
to include both the local claim URIs flagged as multi-valued and their mapped
OIDC claim URIs in the returned multiValuedClaimUris set. In
DefaultOIDCClaimsCallbackHandler, after the existing loop that adds OIDC
external claims, also iterate through the localClaimMultiValuedMap and add all
local claim URIs where the value is Boolean.TRUE to the multiValuedClaimUris
set. Apply the same logic to JWTAccessTokenOIDCClaimsHandler to keep JWT access
token claim handling consistent with ID token handling.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3255 +/- ##
============================================
+ Coverage 59.11% 60.15% +1.04%
+ Complexity 12108 11271 -837
============================================
Files 726 726
Lines 67338 63537 -3801
Branches 15864 15122 -742
============================================
- Hits 39807 38221 -1586
+ Misses 22289 20327 -1962
+ Partials 5242 4989 -253
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // When ConvertOnlyMultiValuedClaimsToArray is enabled, resolve (once) the claims flagged multi-valued in | ||
| // metadata. A null set means the feature is off or the lookup failed, so legacy behaviour applies. | ||
| Set<String> multiValuedClaimUris = null; | ||
| if (OAuthServerConfiguration.getInstance().getConvertOnlyMultiValuedClaimsToArray()) { |
Contributor
There was a problem hiding this comment.
can we fix this in a common place?.
I am thinking whether userClaimsInOIDCDialect should have the multi valued claims as an array.
If we fix this in common place, we might be able to handle without touching all the places.
Also the config name can be more general term like honouring multi valued state of the attribute.
We will later bring bring honouring data type also.
int and integers, decimals as decimals, complex as objects.
Contributor
There was a problem hiding this comment.
After a common fix, it can be benifited at all places. Need to see whether it is possible or not.
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
2 participants
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.
Purpose
Resolves wso2/product-is#27652.
Currently any user-claim value containing the multi-attribute separator (
,) is unconditionally split into a JSON array when building JWT access tokens, ID tokens and the UserInfo response — corrupting genuinely single-valued claims that happen to contain commas. This PR makes the split selective and opt-in.Approach
New config
ConvertOnlyMultiValuedClaimsToArray(defaultfalse). When enabled, a claim is emitted as an array only if its local-claim metadata hasmultiValued=true. Special cases preserved:groupsalways array,addressnever. When disabled (default), legacy comma-split behaviour is retained — no change for existing deployments.Config key wiring: wso2/carbon-identity-framework#8157.
Changes
OAuthServerConfiguration— parse the new flag + getter.DefaultOIDCClaimsCallbackHandler— JWT/ID-token path: resolve per-tenant set of multi-valued OIDC claim URIs and gate the array/string decision on it.JWTAccessTokenOIDCClaimsHandler— same selective logic on the access-token claims-separation path, so JWT access tokens honour the flag too.OAuth2Util— shared helper resolving local claim URIs flaggedmultiValued.ClaimUtil+UserInfoUserStoreClaimRetriever— UserInfo path (primary + cache-hit), keyed on the local claim URI.ClaimMetadataException/ unavailable service.Tests
DefaultOIDCClaimsCallbackHandlerMultiValuedClaimTest,JWTAccessTokenOIDCClaimsHandlerMultiValuedClaimTest— JWT/ID-token + access-token decision logic.OAuth2UtilTest,ClaimUtilTest— UserInfo helper + path.groups/addressspecial cases, graceful fallback. All green.🤖 Generated with Claude Code