Enable modification of sub-attributes in complex OIDC claims & fix org switch default resolution in pre-issue id token action#3026
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...n/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenRequestBuilder.java
Show resolved
Hide resolved
...n/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenRequestBuilder.java
Show resolved
Hide resolved
...dentity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java
Show resolved
Hide resolved
...dentity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java
Show resolved
Hide resolved
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 |
…ing to support nested map structures
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3026 +/- ##
============================================
- Coverage 58.65% 55.58% -3.07%
- Complexity 9875 10530 +655
============================================
Files 677 704 +27
Lines 54181 61982 +7801
Branches 12957 14533 +1576
============================================
+ Hits 31778 34453 +2675
- Misses 18151 22795 +4644
- Partials 4252 4734 +482
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:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces stream-based claim-path generation with explicit nested traversal in the request builder; updates organization-resolution to prefer an accessingOrganization with tenant/root fallback; adds deep ADD/REMOVE/REPLACE handlers for map/array-valued custom claims in the response processor; and extends tests for nested and array claim scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestBuilder as PreIssueIDTokenRequestBuilder
participant TokenService
participant ResponseProcessor as PreIssueIDTokenResponseProcessor
participant IDToken
Client->>RequestBuilder: build request (includes custom claims)
RequestBuilder->>RequestBuilder: collectNestedClaimPaths()\nresolve accessingOrganization
RequestBuilder->>TokenService: send pre-issue request
TokenService->>ResponseProcessor: deliver claims + operations
ResponseProcessor->>ResponseProcessor: for each operation (ADD/REMOVE/REPLACE)\ninspect path segments
alt nested path (map/array)
ResponseProcessor->>ResponseProcessor: traverse nested maps/arrays\nremove/replace/add at deeper path
else root-level
ResponseProcessor->>ResponseProcessor: apply operation at root claim key
end
ResponseProcessor->>IDToken: construct final ID token with updated claims
IDToken->>Client: return token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java (1)
466-490:⚠️ Potential issue | 🟠 MajorArray-indexed claim operations are now unreachable dead code.
The new nested-path check at lines 470–475 intercepts all multi-segment paths (e.g.,
arrayClaim/0) beforeparseOperationPathis called at line 477. SinceparseOperationPathonly runs whenpathSegments.size() == 1,claimPathInfo.getIndex()will always be-1, making the array-removal branch at line 484–486 (andremoveClaimValueAtIndexFromArrayTypeClaim) unreachable fromremoveOtherClaims. The same applies toreplaceClaimValueAtIndexFromArrayTypeClaiminreplaceOtherClaims(lines 657–658).This is a breaking behavioral change: List-valued claims with index-based paths (e.g.,
/idToken/claims/arrayClaim/0wherearrayClaimis aList) will now fail with "Root claim is not a complex object" instead of performing index-based removal/replacement.If this is intentional, the dead code (
removeClaimValueAtIndexFromArrayTypeClaim,replaceClaimValueAtIndexFromArrayTypeClaim, andparseOperationPath) should be removed to avoid confusion.components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java (1)
958-973:⚠️ Potential issue | 🟡 MinorThis test still passes, but now for a different reason than its name implies.
testProcessSuccessResponse_ReplaceClaim_InArrayWithDuplicateValueuses aList-valuedarrayClaim. With the new nested-path routing, patharrayClaim/0is routed toreplaceNestedClaim, which fails immediately because the root claim is aList, not aMap. The test passes because the claim stays unmodified (size 2), but not because the duplicate-value check was triggered — that code path is now unreachable.Consider renaming or updating this test to reflect the actual behavior being verified.
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenRequestBuilder.java`:
- Around line 233-248: collectNestedClaimPaths currently only adds childPath for
Map entries and never generates the trailing-slash path variant needed for
array-indexed operations; update collectNestedClaimPaths to detect when
entry.getValue() is a List or a String[] and, in that branch, add childPath +
"/" to paths (in addition to the existing childPath) so nested lists produce the
same trailing-slash allowed-path variant as top-level handling; keep existing
recursion for Map values and do not change other behavior.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`:
- Around line 492-520: The root maps in removeNestedClaim and replaceNestedClaim
are shallow-copied which allows removeFromNestedMap / replaceInNestedMap to
mutate nested maps inside the original IDToken; change both methods to perform a
deep copy of the root claim before modifying it (replace new
HashMap<>((Map<String,Object>) rootClaim.getValue()) with a deep copy), add a
helper like deepCopyMap(Map<String,Object>) that recursively copies nested Map
values, and ensure subsequent logic updates responseIDTokenDTO using the deep
copy so the original IDToken remains unmodified (refer to removeNestedClaim,
replaceNestedClaim, removeFromNestedMap and replaceInNestedMap to locate the
changes).
- Around line 695-719: The replaceInNestedMap method currently upserts at the
leaf: replaceInNestedMap uses current.put(key, newValue) unconditionally, which
can add keys instead of strictly replacing; change the leaf logic in
replaceInNestedMap(String key, int index) to first check whether
current.containsKey(key) (or key presence) and return false if the key does not
exist, and only perform put(key, newValue) or remove(key) when the key is
present; keep the existing behavior for cleaning empty parent maps and return
the updated boolean as before.
- Around line 537-541: The extractNestedClaimPath method can produce empty
segments from trailing or duplicate slashes; update extractNestedClaimPath to
filter out empty strings after splitting (e.g., drop "" segments returned by
relativePath.split("/")) so it returns only valid path parts, ensuring
downstream callers like removeFromNestedMap and replaceInNestedMap never receive
an empty key.
🧹 Nitpick comments (3)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java (1)
408-415:isValidMapValuehas an unused variable and performs no actual validation.The
mapvariable on line 413 is assigned but never used. More importantly, the method accepts anyMap<?, ?>without validating its contents (e.g., that keys areStringand values are valid claim types like primitives, lists, or nested maps). This could allow arbitrary objects to be added as claim values.Proposed fix
private boolean isValidMapValue(Object value) { - if (!(value instanceof Map<?, ?>)) { - return false; - } - Map<?, ?> map = (Map<?, ?>) value; - return true; + return value instanceof Map; }components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenRequestBuilderTest.java (1)
1683-1684: Accessing allowed operations by hardcoded index is brittle.Lines 1683–1684 access
get(1)andget(2)assuming a fixed[ADD, REMOVE, REPLACE]order. Consider filtering by operation type (as done intestGetRemoveOrReplacePathsWithDeeplyNestedClaimsat lines 1652–1664) for consistency and resilience.Proposed fix
- List<String> removePaths = request.getAllowedOperations().get(1).getPaths(); - List<String> replacePaths = request.getAllowedOperations().get(2).getPaths(); + List<String> removePaths = request.getAllowedOperations().stream() + .filter(op -> op.getOp() == Operation.REMOVE) + .findFirst() + .orElseThrow(() -> new AssertionError("REMOVE operation not found")) + .getPaths(); + List<String> replacePaths = request.getAllowedOperations().stream() + .filter(op -> op.getOp() == Operation.REPLACE) + .findFirst() + .orElseThrow(() -> new AssertionError("REPLACE operation not found")) + .getPaths();components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java (1)
757-810: Tests now validate nested map removal, but actual List-based array-indexed removal is no longer covered.The change from
ListtoMapwith string keys"0"and"1"correctly tests the new nested-map traversal behavior. However, this means there's no remaining test for the scenario where a claim value is a genuineListand an index-based remove is attempted. With the current code, such an operation would fail with "Root claim is not a complex object."If List-valued array-indexed operations are intentionally no longer supported, consider adding a test that explicitly asserts this failure case to document the new contract.
| private void collectNestedClaimPaths(String basePath, Object value, List<String> paths) { | ||
|
|
||
| if (value instanceof Map) { | ||
| Map<?, ?> map = (Map<?, ?>) value; | ||
|
|
||
| String basePath = CLAIMS_PATH_PREFIX + entry.getKey(); | ||
| if (entry.getValue() instanceof List || entry.getValue() instanceof String[]) { | ||
| basePath += "/"; | ||
| for (Map.Entry<?, ?> entry : map.entrySet()) { | ||
| if (!(entry.getKey() instanceof String)) { | ||
| continue; | ||
| } | ||
| String childPath = basePath + "/" + entry.getKey(); | ||
| paths.add(childPath); | ||
|
|
||
| collectNestedClaimPaths(childPath, entry.getValue(), paths); | ||
| } | ||
| } | ||
| return basePath; | ||
| } |
There was a problem hiding this comment.
Nested lists within map claims won't have array-indexed allowed paths.
collectNestedClaimPaths only recurses into Map values and adds child paths. If a nested value is a List or String[], no trailing-slash path variant (e.g., /idToken/claims/root/nested_list/) is generated, unlike the top-level handling in getRemoveOrReplacePaths (lines 219–222). This means array-indexed operations on lists nested inside object claims won't appear in AllowedOperation paths.
If nested list operations should be supported, add the trailing-slash variant here:
Proposed fix
private void collectNestedClaimPaths(String basePath, Object value, List<String> paths) {
if (value instanceof Map) {
Map<?, ?> map = (Map<?, ?>) value;
for (Map.Entry<?, ?> entry : map.entrySet()) {
if (!(entry.getKey() instanceof String)) {
continue;
}
String childPath = basePath + "/" + entry.getKey();
paths.add(childPath);
+ Object childValue = entry.getValue();
+ if (childValue instanceof List || childValue instanceof String[]) {
+ paths.add(childPath + "/");
+ }
collectNestedClaimPaths(childPath, entry.getValue(), paths);
}
}
}🤖 Prompt for AI Agents
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenRequestBuilder.java`
around lines 233 - 248, collectNestedClaimPaths currently only adds childPath
for Map entries and never generates the trailing-slash path variant needed for
array-indexed operations; update collectNestedClaimPaths to detect when
entry.getValue() is a List or a String[] and, in that branch, add childPath +
"/" to paths (in addition to the existing childPath) so nested lists produce the
same trailing-slash allowed-path variant as top-level handling; keep existing
recursion for Map values and do not change other behavior.
| private OperationExecutionResult removeNestedClaim(List<String> pathSegments, IDToken requestIDToken, | ||
| IDTokenDTO responseIDTokenDTO, | ||
| PerformableOperation operation) { | ||
|
|
||
| String rootClaimName = pathSegments.get(0); | ||
| List<String> nestedPath = pathSegments.subList(1, pathSegments.size()); | ||
|
|
||
| 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); | ||
| if (!removed) { | ||
| return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE, | ||
| "Nested claim not found."); | ||
| } | ||
|
|
||
| if (rootValue.isEmpty()) { | ||
| responseIDTokenDTO.getCustomOIDCClaims().remove(rootClaimName); | ||
| } else { | ||
| responseIDTokenDTO.getCustomOIDCClaims().put(rootClaimName, rootValue); | ||
| } | ||
|
|
||
| return new OperationExecutionResult(operation, OperationExecutionResult.Status.SUCCESS, | ||
| "Nested claim removed."); | ||
| } |
There was a problem hiding this comment.
Shallow copy of root map means nested maps are mutated in-place.
Both removeNestedClaim (line 505) and replaceNestedClaim (line 677) create a shallow copy of the root map via new HashMap<>(...), but the nested child maps are still shared references with the original requestIDToken claim values. When removeFromNestedMap / replaceInNestedMap modifies these nested maps, it mutates the original request token's data. If the request token is referenced elsewhere (e.g., for logging, auditing, or retries), this could cause subtle data corruption.
Consider performing a deep copy instead:
Example deep copy helper
`@SuppressWarnings`("unchecked")
private Map<String, Object> deepCopyMap(Map<String, Object> original) {
Map<String, Object> copy = new HashMap<>();
for (Map.Entry<String, Object> entry : original.entrySet()) {
if (entry.getValue() instanceof Map) {
copy.put(entry.getKey(), deepCopyMap((Map<String, Object>) entry.getValue()));
} else {
copy.put(entry.getKey(), entry.getValue());
}
}
return copy;
}Also applies to: 664-693
🤖 Prompt for AI Agents
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`
around lines 492 - 520, The root maps in removeNestedClaim and
replaceNestedClaim are shallow-copied which allows removeFromNestedMap /
replaceInNestedMap to mutate nested maps inside the original IDToken; change
both methods to perform a deep copy of the root claim before modifying it
(replace new HashMap<>((Map<String,Object>) rootClaim.getValue()) with a deep
copy), add a helper like deepCopyMap(Map<String,Object>) that recursively copies
nested Map values, and ensure subsequent logic updates responseIDTokenDTO using
the deep copy so the original IDToken remains unmodified (refer to
removeNestedClaim, replaceNestedClaim, removeFromNestedMap and
replaceInNestedMap to locate the changes).
| private List<String> extractNestedClaimPath(String operationPath) { | ||
|
|
||
| String relativePath = operationPath.substring(CLAIMS_PATH_PREFIX.length()); | ||
| return List.of(relativePath.split("/")); | ||
| } |
There was a problem hiding this comment.
extractNestedClaimPath doesn't guard against trailing slashes producing empty segments.
A path like /idToken/claims/customClaim/ would produce ["customClaim", ""] after the split, and the empty string segment would propagate into removeFromNestedMap/replaceInNestedMap as a map key lookup for "". Consider filtering out empty segments or documenting the expected path contract.
Proposed fix
private List<String> extractNestedClaimPath(String operationPath) {
String relativePath = operationPath.substring(CLAIMS_PATH_PREFIX.length());
- return List.of(relativePath.split("/"));
+ return Arrays.stream(relativePath.split("/"))
+ .filter(s -> !s.isEmpty())
+ .collect(Collectors.toList());
}🤖 Prompt for AI Agents
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`
around lines 537 - 541, The extractNestedClaimPath method can produce empty
segments from trailing or duplicate slashes; update extractNestedClaimPath to
filter out empty strings after splitting (e.g., drop "" segments returned by
relativePath.split("/")) so it returns only valid path parts, ensuring
downstream callers like removeFromNestedMap and replaceInNestedMap never receive
an empty key.
| private boolean replaceInNestedMap(Map<String, Object> current, List<String> path, int index, Object newValue) { | ||
|
|
||
| String key = path.get(index); | ||
|
|
||
| if (index == path.size() - 1) { | ||
| if (newValue == null) { | ||
| return current.remove(key) != null; | ||
| } | ||
| current.put(key, newValue); | ||
| return true; | ||
| } | ||
|
|
||
| Object next = current.get(key); | ||
| if (!(next instanceof Map)) { | ||
| return false; | ||
| } | ||
| boolean updated = replaceInNestedMap((Map<String, Object>) next, path, index + 1, newValue); | ||
|
|
||
| Map<String, Object> nextMap = (Map<String, Object>) next; | ||
| if (updated && nextMap.isEmpty()) { | ||
| current.remove(key); | ||
| } | ||
|
|
||
| return updated; | ||
| } |
There was a problem hiding this comment.
replaceInNestedMap performs an upsert instead of a strict replace.
At line 703, when at the leaf level (index == path.size() - 1), current.put(key, newValue) is called and true is returned regardless of whether key already exists in the map. This means a REPLACE operation can silently add new keys to nested maps that didn't exist before, which contradicts the expected semantics of a replace operation.
Proposed fix: check key existence before replacing
if (index == path.size() - 1) {
if (newValue == null) {
return current.remove(key) != null;
}
+ if (!current.containsKey(key)) {
+ return false;
+ }
current.put(key, newValue);
return true;
}📝 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.
| private boolean replaceInNestedMap(Map<String, Object> current, List<String> path, int index, Object newValue) { | |
| String key = path.get(index); | |
| if (index == path.size() - 1) { | |
| if (newValue == null) { | |
| return current.remove(key) != null; | |
| } | |
| current.put(key, newValue); | |
| return true; | |
| } | |
| Object next = current.get(key); | |
| if (!(next instanceof Map)) { | |
| return false; | |
| } | |
| boolean updated = replaceInNestedMap((Map<String, Object>) next, path, index + 1, newValue); | |
| Map<String, Object> nextMap = (Map<String, Object>) next; | |
| if (updated && nextMap.isEmpty()) { | |
| current.remove(key); | |
| } | |
| return updated; | |
| } | |
| private boolean replaceInNestedMap(Map<String, Object> current, List<String> path, int index, Object newValue) { | |
| String key = path.get(index); | |
| if (index == path.size() - 1) { | |
| if (newValue == null) { | |
| return current.remove(key) != null; | |
| } | |
| if (!current.containsKey(key)) { | |
| return false; | |
| } | |
| current.put(key, newValue); | |
| return true; | |
| } | |
| Object next = current.get(key); | |
| if (!(next instanceof Map)) { | |
| return false; | |
| } | |
| boolean updated = replaceInNestedMap((Map<String, Object>) next, path, index + 1, newValue); | |
| Map<String, Object> nextMap = (Map<String, Object>) next; | |
| if (updated && nextMap.isEmpty()) { | |
| current.remove(key); | |
| } | |
| return updated; | |
| } |
🤖 Prompt for AI Agents
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`
around lines 695 - 719, The replaceInNestedMap method currently upserts at the
leaf: replaceInNestedMap uses current.put(key, newValue) unconditionally, which
can add keys instead of strictly replacing; change the leaf logic in
replaceInNestedMap(String key, int index) to first check whether
current.containsKey(key) (or key presence) and return false if the key does not
exist, and only perform put(key, newValue) or remove(key) when the key is
present; keep the existing behavior for cleaning empty parent maps and return
the updated boolean as before.
Lashen1227
changed the title
...dentity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java (1)
408-415:isValidMapValue: dead variable and simplifiable body.
map(line 413) is assigned but never used — it can be dropped entirely, collapsing the method to a single expression.♻️ Proposed simplification
private boolean isValidMapValue(Object value) { - if (!(value instanceof Map<?, ?>)) { - return false; - } - Map<?, ?> map = (Map<?, ?>) value; - return true; + return value instanceof Map<?, ?>; }🤖 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 408 - 415, The isValidMapValue method currently assigns a local variable map that is never used; simplify the method by removing the unused Map<?,?> map assignment and return the boolean expression directly (i.e., replace the body with a single return of value instanceof Map<?,?>) in the isValidMapValue method to eliminate the dead variable and unnecessary lines.
🤖 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 522-535: The removeFromNestedMap method currently removes the leaf
but leaves empty intermediate maps; modify it so after recursing into a child
map you check if that child map became empty and, if so, remove its key from the
current map (propagating cleanup upward). Concretely, in removeFromNestedMap:
when next is a Map, cast it to a variable (e.g., Map<String,Object> child), call
removeFromNestedMap(child, path, index+1), then if child.isEmpty() do
current.remove(key); return the boolean that indicates whether a removal
actually occurred. Keep the leaf case behavior (current.remove(key) != null) but
ensure parents remove empty child maps after recursion.
- Around line 816-833: The isArrayIndexPath method only returns true for exactly
two segments which misclassifies deeper paths (e.g., ["address","phones","0"]);
update isArrayIndexPath so it accepts any path with size >= 2 and returns true
when the final segment equals LAST_ELEMENT_CHARACTER or parses as an integer;
this will ensure callers like removeNestedClaim and replaceNestedClaim route to
the array-aware removal/replacement logic that ultimately uses
removeFromNestedMap/replaceInNestedMap.
---
Duplicate comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`:
- Around line 695-719: The method replaceInNestedMap is currently doing an
upsert at the leaf (it calls current.put(key, newValue) without checking
existence) which breaks REPLACE semantics; modify replaceInNestedMap so that at
the leaf (when index == path.size() - 1) it first checks whether current
contains the key and only performs the update or removal if the key already
existed, returning false if the leaf key was absent; preserve the existing
behavior for removal when newValue == null (only remove if key existed) and keep
the parent-empty cleanup logic after recursive calls (the cast/recursion on next
and the removal of empty nextMap should remain intact).
- Around line 537-541: The extractNestedClaimPath method currently returns empty
segments when operationPath has trailing or duplicate slashes which then become
invalid keys for removeFromNestedMap/replaceInNestedMap; change
extractNestedClaimPath (the method that computes relativePath from
CLAiMS_PATH_PREFIX and splits by "/") to normalize/filter the segments: after
computing relativePath, split on "/" then drop any empty strings (or trim
leading/trailing slashes first) and return only non-empty segments (e.g., via a
stream filter or equivalent) so callers never receive empty path elements.
- Around line 492-520: removeNestedClaim is making only a shallow copy of
rootClaim.getValue() so removeFromNestedMap mutates nested maps inside the
original requestIDToken; change the code to deep-clone the root map before
calling removeFromNestedMap (e.g., implement and call a helper like
deepCopyMap(Map<String,Object>) or use a safe serialization-based clone) so all
nested Map/List structures are duplicated, then proceed to modify the cloned
rootValue and update responseIDTokenDTO without mutating requestIDToken.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java`:
- Around line 408-415: The isValidMapValue method currently assigns a local
variable map that is never used; simplify the method by removing the unused
Map<?,?> map assignment and return the boolean expression directly (i.e.,
replace the body with a single return of value instanceof Map<?,?>) in the
isValidMapValue method to eliminate the dead variable and unnecessary lines.
| 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.
removeFromNestedMap doesn't clean up empty intermediate maps, unlike replaceInNestedMap.
After removing a leaf key, any intermediate Map that becomes empty is left in place. For example, removing foo/bar/baz when baz is the only key in bar leaves { foo: { bar: {} } } instead of { foo: {} }. This is inconsistent with replaceInNestedMap (lines 714–715), which explicitly removes empty intermediate maps after an update, and contradicts the PR's stated goal of "clean up empty parent claims."
🐛 Proposed fix — propagate empty-map cleanup on the way back up
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;
}🤖 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, The removeFromNestedMap method currently removes the
leaf but leaves empty intermediate maps; modify it so after recursing into a
child map you check if that child map became empty and, if so, remove its key
from the current map (propagating cleanup upward). Concretely, in
removeFromNestedMap: when next is a Map, cast it to a variable (e.g.,
Map<String,Object> child), call removeFromNestedMap(child, path, index+1), then
if child.isEmpty() do current.remove(key); return the boolean that indicates
whether a removal actually occurred. Keep the leaf case behavior
(current.remove(key) != null) but ensure parents remove empty child maps after
recursion.
| private boolean isArrayIndexPath(List<String> pathSegments) { | ||
|
|
||
| if (pathSegments.size() != 2) { | ||
| return false; | ||
| } | ||
|
|
||
| String lastSegment = pathSegments.get(1); | ||
| if (LAST_ELEMENT_CHARACTER.equals(lastSegment)) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| Integer.parseInt(lastSegment); | ||
| return true; | ||
| } catch (NumberFormatException e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the file mentioned in the review
find . -type f -name "PreIssueIDTokenResponseProcessor.java" | head -5Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 254
🏁 Script executed:
# Look for the methods mentioned in the review
rg -n "removeNestedClaim|replaceNestedClaim|removeFromNestedMap|replaceInNestedMap" --type java | head -20Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2766
🏁 Script executed:
# Search for test files related to this processor
fd -t f ".*PreIssueIDTokenResponseProcessor.*Test.*\.java$"Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 256
🏁 Script executed:
# Search for any test that might exercise nested array paths
rg -n "address.*phones.*[0-9]|claims.*\[\d+\]" --type java | head -20Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Let's examine the broader context around isArrayIndexPath to understand how it's used
sed -n '750,900p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 4583
🏁 Script executed:
# Let's look at the beginning of the file to understand the path construction
head -100 ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 5439
🏁 Script executed:
# Let's examine the test file to see what test cases exist
wc -l ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 263
🏁 Script executed:
# Check test coverage for nested paths and arrays
rg -n "pathSegments|isArrayIndexPath" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java | head -30Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Search for where pathSegments is constructed
rg -n "pathSegments.*split|split.*pathSegments" --type java -B 2 -A 2Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2133
🏁 Script executed:
#!/bin/bash
# Find where isArrayIndexPath is called
rg -n "isArrayIndexPath\|handleArrayIndexPath" ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java -B 5 -A 5Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Look for the method that constructs pathSegments to understand the flow
rg -n "toList\(\)|Arrays.asList\|new.*List.*path" ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java -B 2 -A 2 | head -50Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Look at specific line ranges around the method calls (470-520 and 640-700)
sed -n '450,530p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 3648
🏁 Script executed:
# Check how the nested path is processed in removeNestedClaim
sed -n '492,520p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1543
🏁 Script executed:
# Search for tests involving nested paths or array indices
rg -n "phones|address.*\[|nestedPath|pathSegment" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java | head -30Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Find the extractNestedClaimPath method
rg -n "extractNestedClaimPath" ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.java -B 2 -A 15Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2921
🏁 Script executed:
# Now let's look at the removeFromNestedMap method to see what happens with List
sed -n '522,550p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1411
🏁 Script executed:
# Look at the replaceNestedClaim method as well
sed -n '664,715p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 2306
🏁 Script executed:
# Check the test file for scenarios with array elements in nested claims
grep -n "phones\|address\|0\|-" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java | head -40Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 3289
🏁 Script executed:
#!/bin/bash
# Let's search for tests with 3+ segment paths
rg -n "CLAIMS_PATH_PREFIX.*/.*/.*/" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 929
🏁 Script executed:
# Let's look at test cases around nested claims with complex structures
sed -n '750,830p' ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 4074
🏁 Script executed:
# Check if there are any tests that set up nested List structures (like address/phones)
rg -n "List.*new ArrayList\|Arrays.asList" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java -B 2 -A 5 | head -60Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Let's look more carefully at the logic flow.
# First, let's see the complete condition in removeOtherClaims
sed -n '465,490p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1284
🏁 Script executed:
# Let's check replaceOtherClaims to see if it has the same logic
sed -n '638,660p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1254
🏁 Script executed:
# Now let's understand: if a path like "address/phones/0" comes in:
# extractNestedClaimPath removes "/idToken/claims/" and splits by "/"
# So we need to trace through what the actual issue is.
# Let me look at a complete example in the tests for nested structures
sed -n '900,970p' ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 3852
🏁 Script executed:
# Let's see if there are any tests with nested Maps containing Lists
grep -n "phones\|address\|List" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java | grep -i "map\|put" | head -20Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 577
🏁 Script executed:
# Check what the actual nested claim tests look like
sed -n '900,1000p' ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 5234
🏁 Script executed:
#!/bin/bash
# Let me trace through the actual code flow with a concrete example.
# If path = "/idToken/claims/address/phones/0"
# extractNestedClaimPath gives ["address", "phones", "0"]
# isArrayIndexPath(["address", "phones", "0"]) checks size != 2, so returns false
# So it goes to removeNestedClaim with pathSegments=["address", "phones", "0"]
# In removeNestedClaim, nestedPath = pathSegments.subList(1, 3) = ["phones", "0"]
# Then removeFromNestedMap is called with path=["phones", "0"]
# Let's verify what happens in removeFromNestedMap with path ["phones", "0"]
# Looking at the code, on first iteration:
# key = path.get(0) = "phones"
# next = current.get("phones")
# If "phones" is a List (array), then !(next instanceof Map) is true, so return false
# Let's create a test scenario to confirm this behavior
cat > /tmp/test_analysis.txt << 'EOF'
Scenario: Path "/idToken/claims/address/phones/0"
address claim structure: {
"address": {
"phones": ["555-1234", "555-5678"] // LIST, not MAP
}
}
Flow:
1. extractNestedClaimPath("/idToken/claims/address/phones/0")
-> removes prefix, splits by "/"
-> pathSegments = ["address", "phones", "0"]
2. isArrayIndexPath(["address", "phones", "0"])
-> checks size != 2
-> returns false (size is 3, not 2)
3. removeOtherClaims condition: pathSegments.size() > 1 && !isArrayIndexPath(pathSegments)
-> true && true = true
-> calls removeNestedClaim(["address", "phones", "0"], ...)
4. removeNestedClaim:
- rootClaimName = "address"
- nestedPath = ["phones", "0"]
- rootValue = {"phones": ["555-1234", "555-5678"]}
- calls removeFromNestedMap(rootValue, ["phones", "0"], 0)
5. removeFromNestedMap(current, ["phones", "0"], 0):
- key = "phones"
- next = current.get("phones") = ["555-1234", "555-5678"] // LIST
- !(next instanceof Map) = true
- returns false // FAILS HERE
Result: "Nested claim not found" error instead of removing the array element
The reviewer's concern is VALID - isArrayIndexPath doesn't recognize this pattern.
EOF
cat /tmp/test_analysis.txtRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1265
🏁 Script executed:
# Let's check if there are any tests with nested Maps containing Lists
rg -n "nestedValue\|innerMap" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java -B 2 -A 5 | head -80Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Let me check what happens with the current test structure
# The test "testProcessSuccessResponse_RemoveClaim_FromArrayWithValidIndex" uses:
# Map<String, Object> nestedValue = new HashMap<>();
# nestedValue.put("0", "value1");
# This is a Map with string keys "0", "1", NOT a List
# Let's verify the distinction by looking at how "arrayClaim" is used in tests
grep -n "customClaims.put.*arrayClaim" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Let's look more carefully at what cases are tested.
# Check if there's any test that puts a List inside a nested Map structure
sed -n '750,820p' ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 3696
🏁 Script executed:
# Check what the complete structure looks like for the nested claim test
sed -n '725,760p' ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1607
🏁 Script executed:
# Now let's verify the logic one more time by examining what conditions lead to each code path
sed -n '468,485p' ./components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessor.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 952
🏁 Script executed:
# Let me check the actual commit to see what was intended
git log --oneline --all | grep -i "array\|nested.*claim" | head -5Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 492
🏁 Script executed:
#!/bin/bash
# Let's examine the commit to understand the intent
git show e21385896 --stat | head -20Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 1145
🏁 Script executed:
# Let's look at the key insight: what does isArrayIndexPath actually check?
# It checks if pathSegments.size() == 2 AND the last element is numeric or "-"
# But for a path like /idToken/claims/address/phones/0
# pathSegments = ["address", "phones", "0"] (size 3)
# So isArrayIndexPath returns false, sending it to removeNestedClaim
# Which expects all intermediate values to be Maps, but "phones" is a List
# The reviewer's claim: there's no test covering nested Lists
# Current tests only have:
# 1. Root level arrays: customClaims.put("arrayClaim", Arrays.asList(...))
# 2. Maps with numeric string keys: nestedValue.put("0", ...)
# But NO: {"address": {"phones": [...]}} where phones is a List
# Let me verify by checking if any test has a nested structure with a List
rg -n "new HashMap\|new ArrayList" ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java | grep -A 5 -B 5 "phones\|address" | head -30Repository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 69
🏁 Script executed:
# Final check: Are there tests with 3+ segment paths?
rg -n 'CLAIMS_PATH_PREFIX.*".*/.*/.*' ./components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.javaRepository: wso2-extensions/identity-inbound-auth-oauth
Length of output: 929
isArrayIndexPath does not recognize deeply nested array-indexed paths; 3+ segment paths are silently misrouted.
The hard size() != 2 check at line 818 means a path like /idToken/claims/address/phones/0 produces pathSegments = ["address", "phones", "0"] (size 3). isArrayIndexPath returns false, routing the call to removeNestedClaim/replaceNestedClaim. Those methods then call removeFromNestedMap/replaceInNestedMap where phones (a List) is treated as a Map, causing the operation to fail with "Nested claim not found" rather than performing the intended array-element removal or replacement.
The commit message ("fix: handle array index paths in nested claim removal and replace") suggests this case should be handled. No tests currently exercise 3+ segment paths ending in array indices.
🤖 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 816 - 833, The isArrayIndexPath method only returns true for
exactly two segments which misclassifies deeper paths (e.g.,
["address","phones","0"]); update isArrayIndexPath so it accepts any path with
size >= 2 and returns true when the final segment equals LAST_ELEMENT_CHARACTER
or parses as an integer; this will ensure callers like removeNestedClaim and
replaceNestedClaim route to the array-aware removal/replacement logic that
ultimately uses removeFromNestedMap/replaceInNestedMap.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java (1)
643-662: Near-duplicate test, misleading name, and redundant assertion.Three issues in this new test:
Duplication with
testProcessSuccessResponse_AddClaim_WithValidListValue(lines 571–584): Both tests add anArrays.asList("value1","value2","value3")value as a custom claim and assert equality — they exercise the exact same production code path. The only material addition here is theinstanceof Listcheck on line 657, which could simply be added to the existing test.Misleading name — "Array" vs "List":
Arrays.asList(...)returns ajava.util.List, not a Java array. Naming the testWithValidArrayValuewhile the variable isList<String> arrayValuewill confuse readers. ConsidertestProcessSuccessResponse_AddClaim_WithValidListValue_TypeCheckor folding theinstanceofassertion back into the existing test.Redundant size assertion (line 660):
assertEquals(resultedList.size(), 3)is a no-op given the followingassertEquals(resultedList, arrayValue)on line 661 already validates full equality (including size).🛠️ Suggested consolidation — fold the new assertion into the existing test
`@Test` public void testProcessSuccessResponse_AddClaim_WithValidListValue() throws ActionExecutionResponseProcessorException { List<PerformableOperation> operationsToPerform = new ArrayList<>(); List<String> listValue = Arrays.asList("value1", "value2", "value3"); operationsToPerform.add(createPerformableOperation(Operation.ADD, CLAIMS_PATH_PREFIX + TAIL_CHARACTER, new IDToken.Claim("listClaim", listValue))); IDTokenDTO idTokenDTO = executeProcessSuccessResponseForToken(operationsToPerform, new HashMap<>()); assertNotNull(idTokenDTO.getCustomOIDCClaims()); assertNotNull(idTokenDTO.getCustomOIDCClaims().get("listClaim")); + assertTrue(idTokenDTO.getCustomOIDCClaims().get("listClaim") instanceof List, + "The added claim value should be processed as a List."); assertEquals(idTokenDTO.getCustomOIDCClaims().get("listClaim"), listValue); }And remove the new
testProcessSuccessResponse_AddClaim_WithValidArrayValuetest entirely.🤖 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 643 - 662, The new test testProcessSuccessResponse_AddClaim_WithValidArrayValue duplicates testProcessSuccessResponse_AddClaim_WithValidListValue, has a misleading name (it uses a List, not an array) and contains a redundant size check; delete the duplicate test method testProcessSuccessResponse_AddClaim_WithValidArrayValue (or if you prefer to keep it, rename it to reflect a List and remove redundant size assertion), and instead add the instanceof List assertion into testProcessSuccessResponse_AddClaim_WithValidListValue alongside the existing equality check, removing the redundant assertEquals(resultedList.size(), 3) since assertEquals(resultedList, arrayValue) already covers size and content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java`:
- Around line 643-662: The new test
testProcessSuccessResponse_AddClaim_WithValidArrayValue duplicates
testProcessSuccessResponse_AddClaim_WithValidListValue, has a misleading name
(it uses a List, not an array) and contains a redundant size check; delete the
duplicate test method testProcessSuccessResponse_AddClaim_WithValidArrayValue
(or if you prefer to keep it, rename it to reflect a List and remove redundant
size assertion), and instead add the instanceof List assertion into
testProcessSuccessResponse_AddClaim_WithValidListValue alongside the existing
equality check, removing the redundant assertEquals(resultedList.size(), 3)
since assertEquals(resultedList, arrayValue) already covers size and content.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java (2)
643-662: Near-duplicate oftestProcessSuccessResponse_AddClaim_WithValidListValue.This test exercises the same code path as
testProcessSuccessResponse_AddClaim_WithValidListValue(line 572) — both add aList<String>claim viaOperation.ADD. The only differences are the claim name and more granular assertions here (instanceof,size()). Consider consolidating into one test or differentiating the scenarios (e.g., nested lists, mixed-type arrays) to increase coverage breadth.🤖 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 643 - 662, The test testProcessSuccessResponse_AddClaim_WithValidArrayValue duplicates the behavior exercised by testProcessSuccessResponse_AddClaim_WithValidListValue (both add a List<String> via Operation.ADD using createPerformableOperation and executeProcessSuccessResponseForToken); either remove this duplicate and consolidate the extra assertions (instanceof, size) into the existing test, or change this test to cover a distinct scenario (e.g., nested lists, mixed-type list values, or non-String list elements) so it exercises a different code path in the IDToken claim processing; update assertions accordingly and keep references to Operation.ADD, createPerformableOperation, executeProcessSuccessResponseForToken, and IDTokenDTO.getCustomOIDCClaims to locate and modify the test.
1079-1131: The processor does not useidTokenClaimsSet, so its omission from inline tests causes no behavioral differences.While
testProcessSuccessResponse_ReplaceGroupomitsidTokenDTO.setIdTokenClaimsSet(...)(unlike helper-based tests), thePreIssueIDTokenResponseProcessornever reads this field during operation processing. The processor works exclusively withgetCustomOIDCClaims()andgetAudience(), making the missing setup a pattern inconsistency rather than a functional gap. If consistency across test patterns is desired, consider consolidating group-operation tests into a helper method, but it is not required for correctness.🤖 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 1079 - 1131, The test testProcessSuccessResponse_ReplaceGroup omits setting idTokenDTO.setIdTokenClaimsSet(...) which is inconsistent with other tests even though PreIssueIDTokenResponseProcessor only reads getCustomOIDCClaims() and getAudience(); update the test to populate idTokenDTO.setIdTokenClaimsSet(...) with the same claims (or refactor group-related tests into a shared helper) so patterns match the helper-based tests and make intent explicit without changing PreIssueIDTokenResponseProcessor logic (references: testProcessSuccessResponse_ReplaceGroup, idTokenDTO.setIdTokenClaimsSet, PreIssueIDTokenResponseProcessor, getCustomOIDCClaims, getAudience).
🤖 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/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java`:
- Around line 837-885: In testProcessSuccessResponse_RemoveGroup add an
assertion to verify the third group ("value3") is still present and set up
idTokenClaimsSet on the idTokenDTO (same way
executeProcessSuccessResponseForToken does) before calling
PreIssueIDTokenResponseProcessor.processSuccessResponse so the processor reads
the same claim source; specifically, update the idTokenDTO by calling
setIdTokenClaimsSet(...) with a claims set containing the expected iss/sub/aud
and then assert updatedGroups contains "value3" in addition to the existing
assertions.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/openidconnect/action/preissueidtoken/execution/PreIssueIDTokenResponseProcessorTest.java`:
- Around line 643-662: The test
testProcessSuccessResponse_AddClaim_WithValidArrayValue duplicates the behavior
exercised by testProcessSuccessResponse_AddClaim_WithValidListValue (both add a
List<String> via Operation.ADD using createPerformableOperation and
executeProcessSuccessResponseForToken); either remove this duplicate and
consolidate the extra assertions (instanceof, size) into the existing test, or
change this test to cover a distinct scenario (e.g., nested lists, mixed-type
list values, or non-String list elements) so it exercises a different code path
in the IDToken claim processing; update assertions accordingly and keep
references to Operation.ADD, createPerformableOperation,
executeProcessSuccessResponseForToken, and IDTokenDTO.getCustomOIDCClaims to
locate and modify the test.
- Around line 1079-1131: The test testProcessSuccessResponse_ReplaceGroup omits
setting idTokenDTO.setIdTokenClaimsSet(...) which is inconsistent with other
tests even though PreIssueIDTokenResponseProcessor only reads
getCustomOIDCClaims() and getAudience(); update the test to populate
idTokenDTO.setIdTokenClaimsSet(...) with the same claims (or refactor
group-related tests into a shared helper) so patterns match the helper-based
tests and make intent explicit without changing PreIssueIDTokenResponseProcessor
logic (references: testProcessSuccessResponse_ReplaceGroup,
idTokenDTO.setIdTokenClaimsSet, PreIssueIDTokenResponseProcessor,
getCustomOIDCClaims, getAudience).
| @Test | ||
| public void testProcessSuccessResponse_RemoveGroup() throws ActionExecutionResponseProcessorException { | ||
|
|
||
| List<String> initialGroupList = new ArrayList<>(Arrays.asList("value1", "value2", "value3")); | ||
| int removalIndex = 0; | ||
|
|
||
| IDToken.Builder idTokenEventBuilder = new IDToken.Builder() | ||
| .addClaim(IDToken.ClaimNames.ISS.getName(), ORIGINAL_ISS) | ||
| .addClaim(IDToken.ClaimNames.SUB.getName(), ORIGINAL_SUB) | ||
| .addClaim(IDToken.ClaimNames.AUD.getName(), Arrays.asList(ORIGINAL_AUD)) | ||
| .addClaim("groups", initialGroupList); | ||
|
|
||
| List<PerformableOperation> executionOperations = new ArrayList<>(); | ||
| executionOperations.add(createPerformableOperation(Operation.REMOVE, | ||
| CLAIMS_PATH_PREFIX + "groups/" + removalIndex, null)); | ||
|
|
||
| ActionInvocationSuccessResponse successResponse = new ActionInvocationSuccessResponse.Builder() | ||
| .actionStatus(ActionInvocationResponse.Status.SUCCESS) | ||
| .operations(executionOperations) | ||
| .responseData(mock(ResponseData.class)) | ||
| .build(); | ||
| ActionExecutionResponseContext<ActionInvocationSuccessResponse> responseContext = | ||
| ActionExecutionResponseContext.create(new PreIssueIDTokenEvent.Builder() | ||
| .idToken(idTokenEventBuilder.build()).build(), successResponse); | ||
|
|
||
| PreIssueIDTokenResponseProcessor processor = new PreIssueIDTokenResponseProcessor(); | ||
| FlowContext flowContext = FlowContext.create(); | ||
| OAuthTokenReqMessageContext tokenMessageContext = | ||
| new OAuthTokenReqMessageContext(new OAuth2AccessTokenReqDTO()); | ||
|
|
||
| IDTokenDTO idTokenDTO = new IDTokenDTO(); | ||
| Map<String, Object> currentClaimsMap = new HashMap<>(); | ||
| currentClaimsMap.put("groups", new ArrayList<>(initialGroupList)); | ||
| idTokenDTO.setCustomOIDCClaims(currentClaimsMap); | ||
| idTokenDTO.setAudience(new ArrayList<>(Arrays.asList(ORIGINAL_AUD))); | ||
|
|
||
| flowContext.add("tokenReqMessageContext", tokenMessageContext); | ||
| flowContext.add("idTokenDTO", idTokenDTO); | ||
| flowContext.add("requestType", "token"); | ||
| processor.processSuccessResponse(flowContext, responseContext); | ||
|
|
||
| IDTokenDTO resultDTO = tokenMessageContext.getPreIssueIDTokenActionDTO(); | ||
| List<String> updatedGroups = (List<String>) resultDTO.getCustomOIDCClaims().get("groups"); | ||
|
|
||
| assertNotNull(updatedGroups); | ||
| assertEquals(updatedGroups.size(), 2); | ||
| assertFalse(updatedGroups.contains("value1"), "Removed group should no longer be present."); | ||
| assertTrue(updatedGroups.contains("value2"), "Remaining groups should still be present."); | ||
| } |
There was a problem hiding this comment.
Missing assertion for third element and missing idTokenClaimsSet setup.
Two minor gaps:
- The test asserts
value1is removed andvalue2is present, but does not assert thatvalue3is still retained. - Unlike the shared helper
executeProcessSuccessResponseForToken(line 505), this inline setup does not callidTokenDTO.setIdTokenClaimsSet(...). If the processor reads fromidTokenClaimsSet, this test may behave differently than the helper-based tests.
Suggested assertion addition
assertFalse(updatedGroups.contains("value1"), "Removed group should no longer be present.");
assertTrue(updatedGroups.contains("value2"), "Remaining groups should still be present.");
+ assertTrue(updatedGroups.contains("value3"), "Remaining groups should still be present.");🤖 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 837 - 885, In testProcessSuccessResponse_RemoveGroup add an
assertion to verify the third group ("value3") is still present and set up
idTokenClaimsSet on the idTokenDTO (same way
executeProcessSuccessResponseForToken does) before calling
PreIssueIDTokenResponseProcessor.processSuccessResponse so the processor reads
the same claim source; specifically, update the idTokenDTO by calling
setIdTokenClaimsSet(...) with a claims set containing the expected iss/sub/aud
and then assert updatedGroups contains "value3" in addition to the existing
assertions.
2 participants
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
Summary by CodeRabbit
New Features
Improvements
Tests