Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import javax.servlet.http.HttpServletRequestWrapper;

Expand Down Expand Up @@ -207,24 +206,45 @@ private List<AllowedOperation> getAllowedOperations(Map<String, Object> claimsTo

private List<String> getRemoveOrReplacePaths(Map<String, Object> oidcClaims) {

List<String> removeOrReplacePaths = oidcClaims.entrySet().stream()
.filter(entry -> entry.getValue() instanceof String ||
entry.getValue() instanceof Number ||
entry.getValue() instanceof Boolean || entry.getValue() instanceof List ||
entry.getValue() instanceof String[])
.map(this::generatePathForClaim)
.collect(Collectors.toList());
List<String> removeOrReplacePaths = new ArrayList<>();
for (Map.Entry<String, Object> entry : oidcClaims.entrySet()) {

String basePath = CLAIMS_PATH_PREFIX + entry.getKey();
Object value = entry.getValue();

removeOrReplacePaths.add(basePath);
if (value instanceof String || value instanceof Number || value instanceof Boolean) {
continue;
}
if (value instanceof List || value instanceof String[]) {
removeOrReplacePaths.add(basePath + "/");
continue;
}

// Handle nested objects
if (value instanceof Map) {
collectNestedClaimPaths(basePath, value, removeOrReplacePaths);
}
}
removeOrReplacePaths.add(CLAIMS_PATH_PREFIX + AccessToken.ClaimNames.AUD.getName() + "/");
return removeOrReplacePaths;
}

private String generatePathForClaim(Map.Entry<String, Object> entry) {
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;
}
Comment on lines +233 to 248
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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 AllowedOperation createAllowedOperation(Operation op, List<String> paths) {
Expand Down Expand Up @@ -298,8 +318,17 @@ private User resolveLocalUser(AuthenticatedUser authenticatedUser, String grantT
userBuilder.organization(resolveUserAuthenticatedOrganization(authenticatedUser));

if (OAuthConstants.GrantTypes.ORGANIZATION_SWITCH.equals(grantType)) {
userBuilder.accessingOrganization(buildOrganization(authenticatedUser.getAccessingOrganization(),
authenticatedUser.getTenantDomain()));
Organization accessingOrg;
if (authenticatedUser.getAccessingOrganization() != null) {
accessingOrg = buildOrganization(authenticatedUser.getAccessingOrganization(),
authenticatedUser.getTenantDomain());
// In case of org switch, if accessing org is not set, it means user is switching to root org.
} else {
accessingOrg = buildOrganization(
resolveOrganizationId(authenticatedUser.getTenantDomain()),
authenticatedUser.getTenantDomain());
}
userBuilder.accessingOrganization(accessingOrg);
}
return userBuilder.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import org.wso2.carbon.utils.DiagnosticLog;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -351,7 +352,9 @@ private OperationExecutionResult addToOtherClaims(PerformableOperation operation
}

Object claimValue = claim.getValue();
if (isValidPrimitiveValue(claimValue) || isValidListValue(claimValue)) {
if (isValidPrimitiveValue(claimValue)
|| isValidListValue(claimValue)
|| isValidMapValue(claimValue)) {
responseIDTokenDTO.getCustomOIDCClaims().put(claim.getName(), claim.getValue());
return new OperationExecutionResult(operation, OperationExecutionResult.Status.SUCCESS, "Claim added.");
} else {
Expand Down Expand Up @@ -402,6 +405,15 @@ private boolean isValidListValue(Object value) {
return list.stream().allMatch(item -> item instanceof String);
}

private boolean isValidMapValue(Object value) {

if (!(value instanceof Map<?, ?>)) {
return false;
}
Map<?, ?> map = (Map<?, ?>) value;
return true;
}

private OperationExecutionResult handleRemoveOperation(PerformableOperation operation, IDToken requestedIDToken,
IDTokenDTO responseIDTokenDTO) {

Expand Down Expand Up @@ -455,9 +467,15 @@ private OperationExecutionResult removeOtherClaims(PerformableOperation operatio
IDToken requestIDToken,
IDTokenDTO responseIDTokenDTO) {

ClaimPathInfo claimPathInfo = parseOperationPath(operation.getPath());
IDToken.Claim claim = requestIDToken.getClaim(claimPathInfo.getClaimName());
List<String> pathSegments = extractNestedClaimPath(operation.getPath());

// Nested removal
if (pathSegments.size() > 1 && !isArrayIndexPath(pathSegments)) {
return removeNestedClaim(pathSegments, requestIDToken, responseIDTokenDTO, operation);
}

ClaimPathInfo claimPathInfo = parseOperationPath(operation.getPath());
IDToken.Claim claim = requestIDToken.getClaim(claimPathInfo.getClaimName());
if (claim == null) {
return new OperationExecutionResult(operation, OperationExecutionResult.Status.FAILURE,
"Claim not found.");
Expand All @@ -471,6 +489,57 @@ private OperationExecutionResult removeOtherClaims(PerformableOperation operatio
}
}

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.");
}
Comment on lines +492 to +520
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 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);
}
Comment on lines +522 to +535
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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 List<String> extractNestedClaimPath(String operationPath) {

String relativePath = operationPath.substring(CLAIMS_PATH_PREFIX.length());
return List.of(relativePath.split("/"));
}
Comment on lines +537 to +541
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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 OperationExecutionResult removeClaimValueAtIndexFromArrayTypeClaim(PerformableOperation operation,
ClaimPathInfo claimPathInfo,
IDToken.Claim claim,
Expand Down Expand Up @@ -570,6 +639,13 @@ private OperationExecutionResult replaceOtherClaims(PerformableOperation operati
IDToken requestIDToken,
IDTokenDTO responseIDTokenDTO) {

List<String> pathSegments = extractNestedClaimPath(operation.getPath());

// Nested replace
if (pathSegments.size() > 1 && !isArrayIndexPath(pathSegments)) {
return replaceNestedClaim(pathSegments, requestIDToken, responseIDTokenDTO, operation);
}

ClaimPathInfo claimPathInfo = parseOperationPath(operation.getPath());
IDToken.Claim claim = requestIDToken.getClaim(claimPathInfo.getClaimName());

Expand All @@ -585,6 +661,63 @@ private OperationExecutionResult replaceOtherClaims(PerformableOperation operati
}
}

private OperationExecutionResult replaceNestedClaim(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 replaced = replaceInNestedMap(rootValue, nestedPath, 0, operation.getValue());
if (!replaced) {
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 replaced.");
}

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;
}
Comment on lines +695 to +719
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.


private OperationExecutionResult replaceClaimValueAtIndexFromArrayTypeClaim(PerformableOperation operation,
ClaimPathInfo claimPathInfo,
IDToken.Claim claim,
Expand Down Expand Up @@ -680,6 +813,25 @@ private boolean isValidStringOrURI(String input) {
return matcher.matches();
}

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;
}
}
Comment on lines +816 to +833
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 -5

Repository: 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 -20

Repository: 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 -20

Repository: 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.java

Repository: 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.java

Repository: 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.java

Repository: 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 -30

Repository: 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 2

Repository: 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 5

Repository: 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 -50

Repository: 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.java

Repository: 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.java

Repository: 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 -30

Repository: 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 15

Repository: 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.java

Repository: 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.java

Repository: 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 -40

Repository: 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.java

Repository: 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.java

Repository: 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 -60

Repository: 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.java

Repository: 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.java

Repository: 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.java

Repository: 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 -20

Repository: 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.java

Repository: 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.txt

Repository: 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 -80

Repository: 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.java

Repository: 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.java

Repository: 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.java

Repository: 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.java

Repository: 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 -5

Repository: 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 -20

Repository: 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 -30

Repository: 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.java

Repository: 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.


private ClaimPathInfo parseOperationPath(String operationPath) {

String[] pathSegments = operationPath.split("/");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1197,6 +1197,19 @@ private IDTokenDTO getMockIDTokenDTO() {
return idTokenDTO;
}

private void assertComplexPaths(List<String> paths) {

Assert.assertTrue(paths.contains("/idToken/claims/simple"));
Assert.assertTrue(paths.contains("/idToken/claims/nested"));
Assert.assertTrue(paths.contains("/idToken/claims/nested/intermediate"));
Assert.assertTrue(paths.contains("/idToken/claims/nested/intermediate/leaf"));

Assert.assertTrue(paths.contains("/idToken/claims/list_claim"));
Assert.assertTrue(paths.contains("/idToken/claims/list_claim/"));

Assert.assertTrue(paths.contains("/idToken/claims/aud/"));
}

@Test
public void buildActionExecutionRequestWithMultipleHeadersForTokenFlow()
throws ActionExecutionRequestBuilderException {
Expand Down Expand Up @@ -1613,4 +1626,67 @@ public void buildActionExecutionRequestWithHeadersAndParametersForAuthzFlow()
Assert.assertNotNull(request.getAdditionalParams());
Assert.assertEquals(request.getAdditionalParams().size(), 2);
}

@Test
public void testGetRemoveOrReplacePathsWithDeeplyNestedClaims() throws ActionExecutionRequestBuilderException {

Map<String, Object> customClaims = new HashMap<>();
customClaims.put("simple", "value");
Map<String, Object> level1 = new HashMap<>();
Map<String, Object> level2 = new HashMap<>();
level2.put("leaf", "value");
level1.put("intermediate", level2);
customClaims.put("nested", level1);
customClaims.put("list_claim", Arrays.asList("a", "b"));

IDTokenDTO idTokenDTO = getMockIDTokenDTO();
idTokenDTO.setCustomOIDCClaims(customClaims);

FlowContext flowContext = FlowContext.create()
.add(TOKEN_REQUEST_MESSAGE_CONTEXT, getMockTokenMessageContext())
.add(ID_TOKEN_DTO, idTokenDTO)
.add(REQUEST_TYPE, REQUEST_TYPE_TOKEN);
ActionExecutionRequest request = preIssueIDTokenRequestBuilder.buildActionExecutionRequest(
flowContext, null);

List<String> removePaths = request.getAllowedOperations().stream()
.filter(op -> op.getOp() == Operation.REMOVE)
.findFirst()
.orElseThrow(() -> new AssertionError("REMOVE operation not found"))
.getPaths();
assertComplexPaths(removePaths);

List<String> replacePaths = request.getAllowedOperations().stream()
.filter(op -> op.getOp() == Operation.REPLACE)
.findFirst()
.orElseThrow(() -> new AssertionError("REPLACE operation not found"))
.getPaths();
assertComplexPaths(replacePaths);
}

@Test
public void testGetRemoveAndReplacePathsWithArrays() throws ActionExecutionRequestBuilderException {

Map<String, Object> customClaims = new HashMap<>();
customClaims.put("array_claim", new String[]{"val1", "val2"});

IDTokenDTO idTokenDTO = getMockIDTokenDTO();
idTokenDTO.setCustomOIDCClaims(customClaims);

FlowContext flowContext = FlowContext.create()
.add(TOKEN_REQUEST_MESSAGE_CONTEXT, getMockTokenMessageContext())
.add(ID_TOKEN_DTO, idTokenDTO)
.add(REQUEST_TYPE, REQUEST_TYPE_TOKEN);

ActionExecutionRequest request = preIssueIDTokenRequestBuilder.buildActionExecutionRequest(
flowContext, null);
List<String> removePaths = request.getAllowedOperations().get(1).getPaths();
List<String> replacePaths = request.getAllowedOperations().get(2).getPaths();

Assert.assertTrue(removePaths.contains("/idToken/claims/array_claim"));
Assert.assertTrue(removePaths.contains("/idToken/claims/array_claim/"));

Assert.assertTrue(replacePaths.contains("/idToken/claims/array_claim"));
Assert.assertTrue(replacePaths.contains("/idToken/claims/array_claim/"));
}
}
Loading
Loading