Enable modification of sub-attributes in complex OIDC claims & fix org switch default resolution in pre-issue id token action#3205
Open
Lashen1227 wants to merge 13 commits intowso2-extensions:masterfrom
Conversation
…ing to support nested map structures
…D token processing
… claims in OIDC ID token
📝 WalkthroughWalkthroughThe changes enhance OAuth/OpenID Connect token processing by refactoring stream-based operations to explicit iteration, adding support for nested and complex claim objects including Maps and Lists, implementing nested claim removal/replacement logic, and improving organization resolution for org-switch flows. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks 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}
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 | ||
| #### Log Improvement Suggestion No: 3 | ||
| #### Log Improvement Suggestion No: 4 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`:
- Around line 499-506: The code rebuilds rootValue from requestIDToken
(requestIDToken.getClaim(rootClaimName)) which ignores prior modifications;
change it to fetch the current response claim state (the claim value already
stored in the operation/response object) before calling removeFromNestedMap so
nested operations apply against the latest response, not the original request.
In other words, locate where requestIDToken.getClaim(rootClaimName) is used in
PreIssueIDTokenResponseProcessor, replace it with the response's current claim
retrieval (the claim object/value maintained on the Operation or response token)
and then call removeFromNestedMap(rootValue, nestedPath, 0) on that existing
map; keep the same null/type checks and return OperationExecutionResult on
failure. Ensure the same fix is applied to the other occurrence around lines
677-679 (same method/logic).
- Around line 522-535: removeFromNestedMap currently only deletes the leaf key
and leaves empty parent maps; update removeFromNestedMap to, after recursing
into the child map (the call in removeFromNestedMap((Map<String, Object>) next,
path, index + 1)), check if the child map is now empty and if so remove the
child key from the current map and return true; preserve the existing behavior
for the leaf removal (current.remove(key) != null) and ensure you only attempt
the empty-check when next is a Map and the recursive call returned true or the
child map size == 0 so parent maps get pruned up the unwind.
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java`:
- Around line 851-852: The test mutates a shared requestIDTokenBuilder created
in `@BeforeClass` which leaks state; fix by constructing a fresh IDToken.Builder
within each test that needs extra claims (replace uses of the shared
requestIDTokenBuilder.addClaim("rootClaim", ...) with a new local
IDToken.Builder copy or new builder instance), or move reset logic into a
`@BeforeMethod` to reinitialize requestIDTokenBuilder before each test; update the
tests that call executeProcessSuccessResponseForToken (e.g., the test using
requestIDTokenBuilder and the one at lines ~1130-1131) to pass the new per-test
builder so state is not shared across tests.
🪄 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: 48bb7270-f9f3-4230-a572-bca6297d7252
📒 Files selected for processing (4)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenRequestBuilder.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenRequestBuilderTest.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java
Comment on lines
+499
to
+506
| IDToken.Claim rootClaim = requestIDToken.getClaim(rootClaimName); | ||
| if (rootClaim == null || !(rootClaim.getValue() instanceof Map)) { | ||
| return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE, | ||
| "Root claim is not a complex object."); | ||
| } | ||
|
|
||
| Map<String, Object> rootValue = new HashMap<>((Map<String, Object>) rootClaim.getValue()); | ||
| boolean removed = removeFromNestedMap(rootValue, nestedPath, 0); |
There was a problem hiding this comment.
Apply nested edits against the current response state.
Both nested helpers rebuild rootValue from requestIDToken, so a second nested operation on the same root claim can resurrect data that a previous operation already removed or changed.
Suggested fix
- IDToken.Claim rootClaim = requestIDToken.getClaim(rootClaimName);
- if (rootClaim == null || !(rootClaim.getValue() instanceof Map)) {
+ Object currentRoot = responseIDTokenDTO.getCustomOIDCClaims().get(rootClaimName);
+ if (currentRoot == null) {
+ IDToken.Claim rootClaim = requestIDToken.getClaim(rootClaimName);
+ currentRoot = rootClaim != null ? rootClaim.getValue() : null;
+ }
+ if (!(currentRoot instanceof Map)) {
return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE,
"Root claim is not a complex object.");
}
- Map<String, Object> rootValue = new HashMap<>((Map<String, Object>) rootClaim.getValue());
+ Map<String, Object> rootValue = new HashMap<>((Map<String, Object>) currentRoot);Also applies to: 677-679
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`
around lines 499 - 506, The code rebuilds rootValue from requestIDToken
(requestIDToken.getClaim(rootClaimName)) which ignores prior modifications;
change it to fetch the current response claim state (the claim value already
stored in the operation/response object) before calling removeFromNestedMap so
nested operations apply against the latest response, not the original request.
In other words, locate where requestIDToken.getClaim(rootClaimName) is used in
PreIssueIDTokenResponseProcessor, replace it with the response's current claim
retrieval (the claim object/value maintained on the Operation or response token)
and then call removeFromNestedMap(rootValue, nestedPath, 0) on that existing
map; keep the same null/type checks and return OperationExecutionResult on
failure. Ensure the same fix is applied to the other occurrence around lines
677-679 (same method/logic).
Comment on lines
+522
to
+535
| private boolean removeFromNestedMap(Map<String, Object> current, List<String> path, int index) { | ||
|
|
||
| String key = path.get(index); | ||
| if (index == path.size() - 1) { | ||
| return current.remove(key) != null; | ||
| } | ||
|
|
||
| Object next = current.get(key); | ||
| if (!(next instanceof Map)) { | ||
| return false; | ||
| } | ||
|
|
||
| return removeFromNestedMap((Map<String, Object>) next, path, index + 1); | ||
| } |
There was a problem hiding this comment.
Prune empty parent maps after nested removals.
removeFromNestedMap deletes only the leaf. Removing /root/a/b from {a:{b:"v"}} leaves {a:{}}, so the response still contains empty objects even though replaceInNestedMap already cleans them up on unwind.
Suggested fix
private boolean removeFromNestedMap(Map<String, Object> current, List<String> path, int index) {
String key = path.get(index);
if (index == path.size() - 1) {
return current.remove(key) != null;
}
Object next = current.get(key);
if (!(next instanceof Map)) {
return false;
}
- return removeFromNestedMap((Map<String, Object>) next, path, index + 1);
+ boolean removed = removeFromNestedMap((Map<String, Object>) next, path, index + 1);
+ Map<String, Object> nextMap = (Map<String, Object>) next;
+ if (removed && nextMap.isEmpty()) {
+ current.remove(key);
+ }
+ return removed;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private boolean removeFromNestedMap(Map<String, Object> current, List<String> path, int index) { | |
| String key = path.get(index); | |
| if (index == path.size() - 1) { | |
| return current.remove(key) != null; | |
| } | |
| Object next = current.get(key); | |
| if (!(next instanceof Map)) { | |
| return false; | |
| } | |
| return removeFromNestedMap((Map<String, Object>) next, path, index + 1); | |
| } | |
| private boolean removeFromNestedMap(Map<String, Object> current, List<String> path, int index) { | |
| String key = path.get(index); | |
| if (index == path.size() - 1) { | |
| return current.remove(key) != null; | |
| } | |
| Object next = current.get(key); | |
| if (!(next instanceof Map)) { | |
| return false; | |
| } | |
| boolean removed = removeFromNestedMap((Map<String, Object>) next, path, index + 1); | |
| Map<String, Object> nextMap = (Map<String, Object>) next; | |
| if (removed && nextMap.isEmpty()) { | |
| current.remove(key); | |
| } | |
| return removed; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`
around lines 522 - 535, removeFromNestedMap currently only deletes the leaf key
and leaves empty parent maps; update removeFromNestedMap to, after recursing
into the child map (the call in removeFromNestedMap((Map<String, Object>) next,
path, index + 1)), check if the child map is now empty and if so remove the
child key from the current map and return true; preserve the existing behavior
for the leaf removal (current.remove(key) != null) and ensure you only attempt
the empty-check when next is a Map and the recursive call returned true or the
child map size == 0 so parent maps get pruned up the unwind.
Comment on lines
+851
to
+852
| requestIDTokenBuilder.addClaim("rootClaim", nestedClaim); | ||
| IDTokenDTO idTokenDTO = executeProcessSuccessResponseForToken(operationsToPerform, existingClaims); |
There was a problem hiding this comment.
Avoid mutating the shared requestIDTokenBuilder across tests.
This builder is created once in @BeforeClass, so adding rootClaim/complexClaim here leaks state into later tests and makes outcomes depend on execution order. Build a fresh IDToken.Builder in each test that needs extra claims, or reset the fixture in @BeforeMethod.
Also applies to: 1130-1131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java`
around lines 851 - 852, The test mutates a shared requestIDTokenBuilder created
in `@BeforeClass` which leaks state; fix by constructing a fresh IDToken.Builder
within each test that needs extra claims (replace uses of the shared
requestIDTokenBuilder.addClaim("rootClaim", ...) with a new local
IDToken.Builder copy or new builder instance), or move reset logic into a
`@BeforeMethod` to reinitialize requestIDTokenBuilder before each test; update the
tests that call executeProcessSuccessResponseForToken (e.g., the test using
requestIDTokenBuilder and the one at lines ~1130-1131) to pass the new per-test
builder so state is not shared across tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3205 +/- ##
============================================
+ Coverage 59.19% 59.25% +0.05%
- Complexity 10342 10351 +9
============================================
Files 711 712 +1
Lines 56756 58570 +1814
Branches 13685 14096 +411
============================================
+ Hits 33598 34703 +1105
- Misses 18673 19255 +582
- Partials 4485 4612 +127
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Lashen1227
changed the title
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
1 participant
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.
This pull request improves the handling of nested claims in the pre-issue id token action. It introduces support for remove and replace operations on nested claim structures, and enables add custom object claims to the id token
Nested Claim Support and Path Handling:
PreIssueIDTokenRequestBuilderto generate paths for nested claims, enabling precise targeting of sub-claims within complex objects.PreIssueIDTokenResponseProcessor.Remove and Replace Operations for Nested Claims:
removeNestedClaimandreplaceNestedClaimmethods to recursively remove or update values within nested claim maps, ensuring correct updates and cleanup of empty parent claims.Validation and Primitive Type Handling:
isValidMapValueto allow maps (objects) as valid claim values, extending support for complex claim types in addition to primitives and lists.Other Improvements