Enable pre profile update action to allow modifying user claims at profile update#8115
Closed
Lashen1227 wants to merge 14 commits into
Closed
Enable pre profile update action to allow modifying user claims at profile update#8115Lashen1227 wants to merge 14 commits into
Lashen1227 wants to merge 14 commits into
Conversation
…tionServiceComponentHolder for realm service
…ResponseProcessor for improved claim handling and user store management
… store management
…o prevent modification of immutable claims
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements template-based path matching for action operation permissions and a complete bidirectional claim update flow for pre-update profile actions: the request builder declares allowed operations with filtered-claims paths, and the response processor validates returned operations against claim metadata and SCIM mappings before applying changes. ChangesTemplate-Based Operation Matching and Claim Update Flow
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Lashen1227
changed the title
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileRequestBuilder.java (2)
152-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the nullable request-claims map before constructing
UpdatingUserClaims.
getPreUpdateProfileRequest()explicitly allowsuserActionRequestDTO.getClaims()to be null/empty, but this path still passes that map into helpers that callcontainsKey()unguarded. With configured action attributes and no incoming claims, this blows up with an NPE instead of building the event.Suggested fix
private UpdatingUserClaim constructMultiValuedClaim(Map<String, Object> updatingUserClaimsInRequest, String claimKey, String claimValue, String multiAttributeSeparator) throws ActionExecutionRequestBuilderException { - if (updatingUserClaimsInRequest.containsKey(claimKey)) { + if (updatingUserClaimsInRequest != null && updatingUserClaimsInRequest.containsKey(claimKey)) { Object updatingClaimValue = updatingUserClaimsInRequest.get(claimKey); if (!(updatingClaimValue instanceof String[])) { throw new ActionExecutionRequestBuilderException( "Invalid claim value format for multi-valued claim: " + claimKey + " Only String[] types are expected."); @@ private UpdatingUserClaim constructSingleValuedClaim(Map<String, Object> updatingUserClaimsInRequest, String claimKey, String claimValue) { - if (updatingUserClaimsInRequest.containsKey(claimKey)) { + if (updatingUserClaimsInRequest != null && updatingUserClaimsInRequest.containsKey(claimKey)) { Object updatingClaimValue = updatingUserClaimsInRequest.get(claimKey); return new UpdatingUserClaim(claimKey, claimValue, String.valueOf(updatingClaimValue)); }Also applies to: 224-228, 264-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileRequestBuilder.java` around lines 152 - 159, Null-check the claims map returned by UserActionRequestDTO.getClaims() in getPreUpdateProfileRequest() and any helper methods that construct UpdatingUserClaim so you never call containsKey() on a null map: either return early when claims are null/empty (as already done) or replace a null with Collections.emptyMap() before passing it to methods that inspect it; update all places where UpdatingUserClaim is built (the blocks around the helper calls referenced in the comment) to use the guarded map or empty map to avoid NPEs.
215-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
user.claimslimited topreUpdateProfileAction.getAttributes().
claimValuesis already restricted to the action’s requested attributes, but this fallback loop re-adds every incoming request claim that is missing from that map. That lets unconfigured claim URIs leak into the outbound event.Suggested fix
- setClaimsInUserBuilder(userBuilder, claimValues, userActionRequestDTO.getClaims(), multiAttributeSeparator); + setClaimsInUserBuilder(userBuilder, claimValues, userActionRequestDTO.getClaims(), + userClaimsToSetInEvent, multiAttributeSeparator); @@ private void setClaimsInUserBuilder(User.Builder userBuilder, Map<String, String> claimValues, Map<String, Object> updatingUserClaimsInRequest, + List<String> requestedClaims, String multiAttributeSeparator) throws ActionExecutionRequestBuilderException { @@ if (updatingUserClaimsInRequest != null) { for (Map.Entry<String, Object> entry : updatingUserClaimsInRequest.entrySet()) { String claimKey = entry.getKey(); - if (isRoleOrGroupClaim(claimKey) || claimValues.containsKey(claimKey) && - StringUtils.isNotBlank(claimValues.get(claimKey))) { + if (!requestedClaims.contains(claimKey) || + isRoleOrGroupClaim(claimKey) || + (claimValues.containsKey(claimKey) && + StringUtils.isNotBlank(claimValues.get(claimKey)))) { continue; }Also applies to: 274-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileRequestBuilder.java` around lines 215 - 225, The code currently builds claimValues from getClaimValues(...) but a later fallback loop re-inserts all incoming request claims into the event, allowing unconfigured claim URIs to leak; update the fallback logic that merges request claims into user claims (the loop referenced near lines 274-295) to only consider keys present in preUpdateProfileAction.getAttributes() (userClaimsToSetInEvent) — i.e., when iterating userActionRequestDTO.getClaims() only add a claim to claimValues (and to the User.Builder via userBuilder) if its claim URI is contained in userClaimsToSetInEvent, leaving other incoming claims out. Ensure this same restriction is applied wherever the fallback merge occurs so user.claims is limited to getAttributes().
🧹 Nitpick comments (2)
components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/OperationComparatorTest.java (1)
106-119: 💤 Low valueConsider adding a negative test case for template non-match.
The happy path test is good. Adding a test where the performable path doesn't match the template pattern (e.g., different prefix like
/admin/claims[uri=...]against/user/claims[uri={claim_uri}]) would strengthen coverage.Example negative test
`@Test` public void testCompareNonMatchingPathForTemplateBasedClaimFilterPath() { AllowedOperation allowedOp = new AllowedOperation(); allowedOp.setOp(Operation.ADD); allowedOp.setPaths(Arrays.asList("/user/claims[uri={claim_uri}]")); PerformableOperation performableOp = new PerformableOperation(); performableOp.setOp(Operation.ADD); performableOp.setPath("/admin/claims[uri=http://wso2.org/claims/country]"); performableOp.setValue("Sri Lanka"); assertFalse(OperationComparator.compare(allowedOp, performableOp)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/OperationComparatorTest.java` around lines 106 - 119, Add a negative unit test that mirrors testCompareMatchingPathForTemplateBasedClaimFilterPath but uses a non-matching performable path (e.g., prefix "/admin/claims[uri=...]" vs allowed "/user/claims[uri={claim_uri}]") to verify OperationComparator.compare returns false; create a method like testCompareNonMatchingPathForTemplateBasedClaimFilterPath that constructs AllowedOperation (op=ADD, paths=["/user/claims[uri={claim_uri}]"]) and PerformableOperation (op=ADD, path="/admin/claims[uri=http://wso2.org/claims/country]", value="Sri Lanka") and use assertFalse(OperationComparator.compare(allowedOp, performableOp)).components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java (1)
465-467: ⚡ Quick winLog these handled backend lookup failures before wrapping them.
These catch blocks translate claim/user-store lookup errors into
ActionExecutionResponseProcessorExceptionwithout any log at the handling boundary, which makes production triage harder if the wrapped exception is normalized further up the stack. As per coding guidelines, "Suggest log statements at error handling boundaries (catch blocks, error returns)."Also applies to: 519-521, 532-534, 694-696
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java` around lines 465 - 467, The catch blocks in PreUpdateProfileResponseProcessor that catch ClaimMetadataException (and similar catch blocks at the other locations) currently wrap and rethrow as ActionExecutionResponseProcessorException without logging; update each catch to log the error before throwing (e.g., use the class logger instance like log.error or logger.error with a clear message including claimUri or related identifier and include the caught exception), then rethrow the ActionExecutionResponseProcessorException with the original exception as the cause so the failure is both logged and propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java`:
- Around line 416-422: getFilteredModifyingClaimValues can return an empty
filteredClaims when the replacement value already exists, causing the current
branch to skip both maps and leave the old value unremoved; change
PreUpdateProfileResponseProcessor so that
simpleMultiValuedClaimsToBeRemoved.put(claimUri, Arrays.asList(oldValueName)) is
performed unconditionally (move it out of the if block) and only add
trimmedFilteredClaims into simpleMultiValuedClaimsToBeAdded when filteredClaims
is non-empty; reference the variables simpleMultiValuedClaimsToBeRemoved,
simpleMultiValuedClaimsToBeAdded and the method getFilteredModifyingClaimValues
in your change.
- Around line 540-541: The code in PreUpdateProfileResponseProcessor that builds
userValues from userClaimValues.get(claimUri) uses value.split(separator), which
treats the configured separator as a regex and can mis-split characters like '|'
or '.'. Replace that split call to use a literal separator by quoting it (e.g.,
value.split(Pattern.quote(separator))) or otherwise escaping the separator
before splitting so multivalued claim parsing, duplicate filtering and delta
computation behave consistently with the other branches.
- Around line 284-287: The response update flow currently calls
validateFlowInitiatorClaims for ADD and REPLACE but not for REMOVE, allowing
deletions of flow-initiated-blocked claims; modify the pre-update sequence in
PreUpdateProfileResponseProcessor (around where isLocalClaim,
validateImmutableClaims, validateGroupAndRoleClaims, validateSCIMLevelAttributes
are invoked) to also invoke validateFlowInitiatorClaims for REMOVE operations
(i.e., call validateFlowInitiatorClaims(claimUri, operation.getOp(),
operation.getValue()) for op == REMOVE or include REMOVE with the existing
ADD/REPLACE checks) so flow-initiator validation runs for deletions as well.
- Around line 315-321: In PreUpdateProfileResponseProcessor inside the USER
branch (check on initiatorType), skip mutating userClaimsToBeModified when
filteredValues is empty to avoid adding a trailing separator/empty value; before
computing addingClaimValue for claimUri, add a guard that if
filteredValues.isEmpty() then do not put anything into userClaimsToBeModified
(i.e., return/continue or skip this claim), otherwise build addingClaimValue
using userClaimValues.get(claimUri), separator and String.join as currently
implemented.
---
Outside diff comments:
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileRequestBuilder.java`:
- Around line 152-159: Null-check the claims map returned by
UserActionRequestDTO.getClaims() in getPreUpdateProfileRequest() and any helper
methods that construct UpdatingUserClaim so you never call containsKey() on a
null map: either return early when claims are null/empty (as already done) or
replace a null with Collections.emptyMap() before passing it to methods that
inspect it; update all places where UpdatingUserClaim is built (the blocks
around the helper calls referenced in the comment) to use the guarded map or
empty map to avoid NPEs.
- Around line 215-225: The code currently builds claimValues from
getClaimValues(...) but a later fallback loop re-inserts all incoming request
claims into the event, allowing unconfigured claim URIs to leak; update the
fallback logic that merges request claims into user claims (the loop referenced
near lines 274-295) to only consider keys present in
preUpdateProfileAction.getAttributes() (userClaimsToSetInEvent) — i.e., when
iterating userActionRequestDTO.getClaims() only add a claim to claimValues (and
to the User.Builder via userBuilder) if its claim URI is contained in
userClaimsToSetInEvent, leaving other incoming claims out. Ensure this same
restriction is applied wherever the fallback merge occurs so user.claims is
limited to getAttributes().
---
Nitpick comments:
In
`@components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/OperationComparatorTest.java`:
- Around line 106-119: Add a negative unit test that mirrors
testCompareMatchingPathForTemplateBasedClaimFilterPath but uses a non-matching
performable path (e.g., prefix "/admin/claims[uri=...]" vs allowed
"/user/claims[uri={claim_uri}]") to verify OperationComparator.compare returns
false; create a method like
testCompareNonMatchingPathForTemplateBasedClaimFilterPath that constructs
AllowedOperation (op=ADD, paths=["/user/claims[uri={claim_uri}]"]) and
PerformableOperation (op=ADD,
path="/admin/claims[uri=http://wso2.org/claims/country]", value="Sri Lanka") and
use assertFalse(OperationComparator.compare(allowedOp, performableOp)).
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java`:
- Around line 465-467: The catch blocks in PreUpdateProfileResponseProcessor
that catch ClaimMetadataException (and similar catch blocks at the other
locations) currently wrap and rethrow as
ActionExecutionResponseProcessorException without logging; update each catch to
log the error before throwing (e.g., use the class logger instance like
log.error or logger.error with a clear message including claimUri or related
identifier and include the caught exception), then rethrow the
ActionExecutionResponseProcessorException with the original exception as the
cause so the failure is both logged and propagated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 273809a0-d088-4b80-a0be-29443e769124
📒 Files selected for processing (6)
components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/internal/util/OperationComparator.javacomponents/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/OperationComparatorTest.javacomponents/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/pom.xmlcomponents/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileRequestBuilder.javacomponents/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.javacomponents/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/test/java/org/wso2/carbon/identity/user/pre/update/profile/action/execution/PreUpdateProfileResponseProcessorTest.java
Comment on lines
+315
to
+321
| if (initiatorType == PreUpdateProfileEvent.FlowInitiatorType.ADMIN || | ||
| initiatorType == PreUpdateProfileEvent.FlowInitiatorType.APPLICATION) { | ||
| simpleMultiValuedClaimsToBeAdded.put(claimUri, filteredValues); | ||
| } else if (initiatorType == PreUpdateProfileEvent.FlowInitiatorType.USER) { | ||
| String addingClaimValue = (userClaimValues.get(claimUri) == null) ? String.join(separator, filteredValues) | ||
| : userClaimValues.get(claimUri) + separator + String.join(separator, filteredValues); | ||
| userClaimsToBeModified.put(claimUri, addingClaimValue); |
Contributor
There was a problem hiding this comment.
Skip the user update when no new multivalued entries survive filtering.
If every submitted value is blank or already present, filteredValues is empty and this branch still writes existing + separator, producing a trailing separator / empty mutation for a no-op add.
Suggested fix
if (initiatorType == PreUpdateProfileEvent.FlowInitiatorType.ADMIN ||
initiatorType == PreUpdateProfileEvent.FlowInitiatorType.APPLICATION) {
simpleMultiValuedClaimsToBeAdded.put(claimUri, filteredValues);
} else if (initiatorType == PreUpdateProfileEvent.FlowInitiatorType.USER) {
+ if (filteredValues.isEmpty()) {
+ return;
+ }
String addingClaimValue = (userClaimValues.get(claimUri) == null) ? String.join(separator, filteredValues)
: userClaimValues.get(claimUri) + separator + String.join(separator, filteredValues);
userClaimsToBeModified.put(claimUri, addingClaimValue);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java`
around lines 315 - 321, In PreUpdateProfileResponseProcessor inside the USER
branch (check on initiatorType), skip mutating userClaimsToBeModified when
filteredValues is empty to avoid adding a trailing separator/empty value; before
computing addingClaimValue for claimUri, add a guard that if
filteredValues.isEmpty() then do not put anything into userClaimsToBeModified
(i.e., return/continue or skip this claim), otherwise build addingClaimValue
using userClaimValues.get(claimUri), separator and String.join as currently
implemented.
Comment on lines
+416
to
+422
| if (!filteredClaims.isEmpty()) { | ||
| simpleMultiValuedClaimsToBeRemoved.put(claimUri, Arrays.asList(oldValueName)); | ||
| List<String> trimmedFilteredClaims = filteredClaims.stream() | ||
| .map(String::trim) | ||
| .collect(Collectors.toList()); | ||
| simpleMultiValuedClaimsToBeAdded.put(claimUri, trimmedFilteredClaims); | ||
| } |
Contributor
There was a problem hiding this comment.
Specific-value replace becomes a no-op when the replacement value already exists.
getFilteredModifyingClaimValues removes duplicate target values. When oldValue is replaced with a value the user already has, filteredClaims is empty and this branch skips both maps, so the old value is left behind instead of being replaced.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 420-420: Replace this usage of 'Stream.collect(Collectors.toList())' with 'Stream.toList()' and ensure that the list is unmodified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java`
around lines 416 - 422, getFilteredModifyingClaimValues can return an empty
filteredClaims when the replacement value already exists, causing the current
branch to skip both maps and leave the old value unremoved; change
PreUpdateProfileResponseProcessor so that
simpleMultiValuedClaimsToBeRemoved.put(claimUri, Arrays.asList(oldValueName)) is
performed unconditionally (move it out of the if block) and only add
trimmedFilteredClaims into simpleMultiValuedClaimsToBeAdded when filteredClaims
is non-empty; reference the variables simpleMultiValuedClaimsToBeRemoved,
simpleMultiValuedClaimsToBeAdded and the method getFilteredModifyingClaimValues
in your change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java`:
- Around line 149-150: The code returns ProfileOperationExecutionResult using
e.getMessage() directly, which can be null and later causes Map.of("message",
...) to throw; in PreUpdateProfileResponseProcessor replace direct
e.getMessage() usage with a normalized non-null string (e.g., String msg =
e.getMessage() != null ? e.getMessage() : ""; or use
Optional.ofNullable(e.getMessage()).orElse("")); then pass msg into new
ProfileOperationExecutionResult(...). Apply the same normalization pattern where
messages are passed into Map.of or ProfileOperationExecutionResult (the other
occurrences around the blocks that create ProfileOperationExecutionResult and
Map.of at the locations you flagged).
- Around line 103-124: The loop currently ignores unknown ops and always lets
processSuccessResponse run even if some handle*Operation produced a rejected
ProfileOperationExecutionResult; update the switch default to create and add a
rejected ProfileOperationExecutionResult for unknown operation types (refer to
operation.getOp() and ProfileOperationExecutionResult), then after the loop
examine operationExecutionResultList for any rejected entries and if any exist
prevent the success path (do not call processSuccessResponse/return
SuccessStatus), instead invoke the failure path (e.g., processFailureResponse or
set FlowContext to a failed status) and ensure
logOperationExecutionResults(getSupportedActionType(),
operationExecutionResultList) still logs before returning; apply the same change
to the corresponding block at lines 135-189 where operations are processed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c49bb7a9-07c1-49b3-938a-96fedb4fd793
📒 Files selected for processing (2)
components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.javacomponents/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/model/ProfileOperationExecutionResult.java
Comment on lines
+103
to
+124
| switch (operation.getOp()) { | ||
| case ADD: | ||
| operationExecutionResultList.add(handleAddOperation(operation, responseContext, | ||
| userClaimsToBeAdded, userClaimsToBeModified, simpleMultiValuedClaimsToBeAdded, | ||
| userStoreManager)); | ||
| break; | ||
| case REPLACE: | ||
| operationExecutionResultList.add(handleReplaceOperation(operation, responseContext, | ||
| userClaimsToBeModified, simpleMultiValuedClaimsToBeRemoved, | ||
| simpleMultiValuedClaimsToBeAdded, userStoreManager)); | ||
| break; | ||
| case REMOVE: | ||
| operationExecutionResultList.add(handleRemoveOperation(operation, responseContext, | ||
| userClaimsToBeModified, userClaimsToBeRemoved, simpleMultiValuedClaimsToBeRemoved)); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| logOperationExecutionResults(getSupportedActionType(), operationExecutionResultList); |
Contributor
There was a problem hiding this comment.
Fail the response when any operation is rejected.
Unknown ops are silently ignored in the default branch, and the handle*Operation wrappers convert validation failures into ProfileOperationExecutionResult entries but still let processSuccessResponse populate FlowContext and return SuccessStatus. That turns malformed/forbidden action responses into partial claim updates instead of rejecting the response.
💡 Suggested fix
for (PerformableOperation operation : operationsToPerform) {
switch (operation.getOp()) {
case ADD:
operationExecutionResultList.add(handleAddOperation(operation, responseContext,
userClaimsToBeAdded, userClaimsToBeModified, simpleMultiValuedClaimsToBeAdded,
userStoreManager));
break;
case REPLACE:
operationExecutionResultList.add(handleReplaceOperation(operation, responseContext,
userClaimsToBeModified, simpleMultiValuedClaimsToBeRemoved,
simpleMultiValuedClaimsToBeAdded, userStoreManager));
break;
case REMOVE:
operationExecutionResultList.add(handleRemoveOperation(operation, responseContext,
userClaimsToBeModified, userClaimsToBeRemoved, simpleMultiValuedClaimsToBeRemoved));
break;
default:
- break;
+ throw new ActionExecutionResponseProcessorException(
+ "Unsupported operation type in pre-update profile response: " + operation.getOp());
}
}
}
logOperationExecutionResults(getSupportedActionType(), operationExecutionResultList);
+
+ for (ProfileOperationExecutionResult result : operationExecutionResultList) {
+ if (result.getStatus() == ProfileOperationExecutionResult.Status.FAILURE) {
+ throw new ActionExecutionResponseProcessorException(result.getMessage());
+ }
+ }Also applies to: 135-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java`
around lines 103 - 124, The loop currently ignores unknown ops and always lets
processSuccessResponse run even if some handle*Operation produced a rejected
ProfileOperationExecutionResult; update the switch default to create and add a
rejected ProfileOperationExecutionResult for unknown operation types (refer to
operation.getOp() and ProfileOperationExecutionResult), then after the loop
examine operationExecutionResultList for any rejected entries and if any exist
prevent the success path (do not call processSuccessResponse/return
SuccessStatus), instead invoke the failure path (e.g., processFailureResponse or
set FlowContext to a failed status) and ensure
logOperationExecutionResults(getSupportedActionType(),
operationExecutionResultList) still logs before returning; apply the same change
to the corresponding block at lines 135-189 where operations are processed.
Comment on lines
+149
to
+150
| return new ProfileOperationExecutionResult(operation, ProfileOperationExecutionResult.Status.FAILURE, | ||
| e.getMessage()); |
Contributor
There was a problem hiding this comment.
Normalize failure messages before adding them to Map.of(...).
e.getMessage() can be null. When that happens, Map.of("message", performedOperation.getMessage()) throws, so a handled operation failure can crash response processing as soon as diagnostic logging is enabled.
💡 Suggested fix
operationExecutionResultList.forEach(
performedOperation -> operationDetailsList.add(Map.of(
"operation", performedOperation.getOperation().getOp() + " path: "
+ performedOperation.getOperation().getPath(),
"status", performedOperation.getStatus().toString(),
- "message", performedOperation.getMessage()
+ "message", Optional.ofNullable(performedOperation.getMessage())
+ .orElse("Operation failed.")
)));As per coding guidelines, "avoid NPE risks" in added logs.
Also applies to: 168-169, 186-187, 751-757
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/user-mgt/org.wso2.carbon.identity.user.pre.update.profile.action/src/main/java/org/wso2/carbon/identity/user/pre/update/profile/action/internal/execution/PreUpdateProfileResponseProcessor.java`
around lines 149 - 150, The code returns ProfileOperationExecutionResult using
e.getMessage() directly, which can be null and later causes Map.of("message",
...) to throw; in PreUpdateProfileResponseProcessor replace direct
e.getMessage() usage with a normalized non-null string (e.g., String msg =
e.getMessage() != null ? e.getMessage() : ""; or use
Optional.ofNullable(e.getMessage()).orElse("")); then pass msg into new
ProfileOperationExecutionResult(...). Apply the same normalization pattern where
messages are passed into Map.of or ProfileOperationExecutionResult (the other
occurrences around the blocks that create ProfileOperationExecutionResult and
Map.of at the locations you flagged).
…Builder and PreUpdateProfileResponseProcessor
…fileResponseProcessor
…UpdateProfileRequestBuilderTest
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8115 +/- ##
============================================
+ Coverage 52.80% 53.10% +0.30%
- Complexity 20837 21022 +185
============================================
Files 2151 2152 +1
Lines 128129 129223 +1094
Branches 18744 18909 +165
============================================
+ Hits 67654 68629 +975
- Misses 52155 52184 +29
- Partials 8320 8410 +90
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:
|
Contributor
Author
|
Addressed on #8124 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request enhances support for modifying user claims through user profile update actions and adds comprehensive test coverage for the new functionality.
Supported claim modification operations:
ADDREPLACEREMOVESample Response Payload
{ "actionStatus": "SUCCESS", "operations": [ { "op": "add", "path": "/user/claims[uri=http://wso2.org/claims/country]", "value": "Sri Lanka" }, { "op": "replace", "path": "/user/claims[uri=http://wso2.org/claims/mobileNumbers]", "value": ["0771234567", "0717654321"] }, { "op": "remove", "path": "/user/claims[uri=http://wso2.org/claims/country]" } ] }Claim Extraction and Validation Improvements
Dependency Updates:
pom.xmlwith the required package imports for central log management utilities.Related PR
Related Issue