Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
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) {
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) {
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
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