Skip to content

Enhance SCIM user claim update with pre-update profile action execution #793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Lashen1227 wants to merge 6 commits into from
+250 −10
Closed

Enhance SCIM user claim update with pre-update profile action execution #793

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3734e6e
feat: enhance pre-update profile action to return execution status an…
Lashen1227 May 18, 2026
6a8f7c3
test: update SCIMUserManagerTest to mock execution status of PreUpdat…
Lashen1227 May 22, 2026
291c41d
Merge branch 'wso2-extensions:master' into feat/pre-profile-update-cl…
Lashen1227 May 22, 2026
afb1e73
refactor: pre-update profile action to return execution status and up…
Lashen1227 May 26, 2026
c7a97f6
test: add unit tests for executeAndGetStatus in PreUpdateProfileActio…
Lashen1227 May 27, 2026
6589058
test: add unit tests for updating user claims with action response
Lashen1227 May 27, 2026
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
65 changes: 59 additions & 6 deletions ...cim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.json.JSONObject;
import org.wso2.carbon.CarbonConstants;
import org.wso2.carbon.context.PrivilegedCarbonContext;
import org.wso2.carbon.identity.action.execution.api.model.ActionExecutionStatus;
import org.wso2.carbon.identity.application.authentication.framework.util.FrameworkUtils;
import org.wso2.carbon.identity.application.common.IdentityApplicationManagementException;
import org.wso2.carbon.identity.application.common.model.ServiceProvider;
Expand Down Expand Up @@ -5931,13 +5932,44 @@ private void updateUserClaims(User user, Map<String, String> oldClaimList,

if (isExecutableUserProfileUpdate) {

Map<String, String> multiValuedClaimsToModify =
SCIMCommonUtils.getSimpleMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
simpleMultiValuedClaimsToBeRemoved);
userClaimsToBeModifiedIncludingMultiValueClaims.putAll(multiValuedClaimsToModify);
populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims);

preUpdateProfileActionExecutor.execute(user, userClaimsToBeModifiedIncludingMultiValueClaims,
userClaimsToBeDeleted);
ActionExecutionStatus<?> actionExecutionStatus = preUpdateProfileActionExecutor.execute(user,
userClaimsToBeModifiedIncludingMultiValueClaims, userClaimsToBeDeleted);
if (actionExecutionStatus != null) {
Comment thread
Lashen1227 marked this conversation as resolved.
Outdated

Map<String, String> userClaimsToBeAddedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeAdded");
Map<String, String> userClaimsToBeModifiedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeModified");
Map<String, String> userClaimsToBeDeletedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeRemoved");
Map<String, List<String>> simpleMultiValuedClaimsToBeAddedFromAction = (HashMap<String, List<String>>)
actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeAdded");
Map<String, List<String>> simpleMultiValuedClaimsToBeRemovedFromAction = (HashMap<String, List<String>>)
actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeRemoved");

if (userClaimsToBeAddedFromAction != null) {
userClaimsToBeAdded.putAll(userClaimsToBeAddedFromAction);
Comment thread
Lashen1227 marked this conversation as resolved.
userClaimsToBeModified.putAll(userClaimsToBeAddedFromAction);
}
if (userClaimsToBeModifiedFromAction != null) {
userClaimsToBeModified.putAll(userClaimsToBeModifiedFromAction);
}
if (userClaimsToBeDeletedFromAction != null) {
userClaimsToBeDeleted.putAll(userClaimsToBeDeletedFromAction);
userClaimsToBeModified.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet());
}
if (simpleMultiValuedClaimsToBeAddedFromAction != null) {
simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction);
}
if (simpleMultiValuedClaimsToBeRemovedFromAction != null) {
simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction);
Comment on lines +5964 to +5968

@coderabbitai coderabbitai Bot May 26, 2026

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 | ⚡ Quick win

Merge same-claim multi-valued deltas instead of overwriting them.

These putAll(...) calls replace the entire value list when both the original request and the action modify the same multi-valued claim. In that case one side's add/remove set is silently lost.

Suggested fix 
                 if (simpleMultiValuedClaimsToBeAddedFromAction != null) {
-                    simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction);
+                    simpleMultiValuedClaimsToBeAddedFromAction.forEach((claim, values) ->
+                            simpleMultiValuedClaimsToBeAdded
+                                    .computeIfAbsent(claim, key -> new ArrayList<>())
+                                    .addAll(values));
                 }
                 if (simpleMultiValuedClaimsToBeRemovedFromAction != null) {
-                    simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction);
+                    simpleMultiValuedClaimsToBeRemovedFromAction.forEach((claim, values) ->
+                            simpleMultiValuedClaimsToBeRemoved
+                                    .computeIfAbsent(claim, key -> new ArrayList<>())
+                                    .addAll(values));
                 }
🤖 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/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java`
around lines 5964 - 5968, The current putAll calls in SCIMUserManager (merging
simpleMultiValuedClaimsToBeAddedFromAction into simpleMultiValuedClaimsToBeAdded
and simpleMultiValuedClaimsToBeRemovedFromAction into
simpleMultiValuedClaimsToBeRemoved) overwrite existing entries when the same
multi-valued claim is modified by both the request and the action; change these
to merge per-claim lists/sets instead: for each entry in
simpleMultiValuedClaimsToBeAddedFromAction (and similarly for
simpleMultiValuedClaimsToBeRemovedFromAction) fetch the existing collection from
simpleMultiValuedClaimsToBeAdded (or simpleMultiValuedClaimsToBeRemoved),
create/ensure a mutable collection if absent, addAll new values (avoiding
duplicate entries if desired), and put the merged collection back so both
sources’ deltas are preserved.

}
populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims);
Comment on lines +5942 to +5971

@coderabbitai coderabbitai Bot May 26, 2026

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 | ⚡ Quick win

Rebuild the post-action modified-claims snapshot before publishing events.

userClaimsToBeModifiedIncludingMultiValueClaims is copied before the action response is merged, but only multi-valued claims are re-populated afterward. That means single-valued claims added/modified by the action never reach the map later used by containsNonAccountStateOrNonVerificationClaim(...) and publishUserProfileUpdateEvent(...), while action-deleted single-valued claims can remain there.

Suggested fix 
                 if (simpleMultiValuedClaimsToBeRemovedFromAction != null) {
                     simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction);
                 }
+                userClaimsToBeModifiedIncludingMultiValueClaims = new HashMap<>(userClaimsToBeModified);
                 populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
                         simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims);
             }
📝 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
Map<String, String> userClaimsToBeAddedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeAdded");
Map<String, String> userClaimsToBeModifiedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeModified");
Map<String, String> userClaimsToBeDeletedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeRemoved");
Map<String, List<String>> simpleMultiValuedClaimsToBeAddedFromAction = (HashMap<String, List<String>>)
actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeAdded");
Map<String, List<String>> simpleMultiValuedClaimsToBeRemovedFromAction = (HashMap<String, List<String>>)
actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeRemoved");
if (userClaimsToBeAddedFromAction != null) {
userClaimsToBeAdded.putAll(userClaimsToBeAddedFromAction);
userClaimsToBeModified.putAll(userClaimsToBeAddedFromAction);
}
if (userClaimsToBeModifiedFromAction != null) {
userClaimsToBeModified.putAll(userClaimsToBeModifiedFromAction);
}
if (userClaimsToBeDeletedFromAction != null) {
userClaimsToBeDeleted.putAll(userClaimsToBeDeletedFromAction);
userClaimsToBeModified.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet());
}
if (simpleMultiValuedClaimsToBeAddedFromAction != null) {
simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction);
}
if (simpleMultiValuedClaimsToBeRemovedFromAction != null) {
simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction);
}
populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims);
Map<String, String> userClaimsToBeAddedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeAdded");
Map<String, String> userClaimsToBeModifiedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeModified");
Map<String, String> userClaimsToBeDeletedFromAction = (HashMap<String, String>)
actionExecutionStatus.getResponseContext().get("userClaimsToBeRemoved");
Map<String, List<String>> simpleMultiValuedClaimsToBeAddedFromAction = (HashMap<String, List<String>>)
actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeAdded");
Map<String, List<String>> simpleMultiValuedClaimsToBeRemovedFromAction = (HashMap<String, List<String>>)
actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeRemoved");
if (userClaimsToBeAddedFromAction != null) {
userClaimsToBeAdded.putAll(userClaimsToBeAddedFromAction);
userClaimsToBeModified.putAll(userClaimsToBeAddedFromAction);
}
if (userClaimsToBeModifiedFromAction != null) {
userClaimsToBeModified.putAll(userClaimsToBeModifiedFromAction);
}
if (userClaimsToBeDeletedFromAction != null) {
userClaimsToBeDeleted.putAll(userClaimsToBeDeletedFromAction);
userClaimsToBeModified.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet());
}
if (simpleMultiValuedClaimsToBeAddedFromAction != null) {
simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction);
}
if (simpleMultiValuedClaimsToBeRemovedFromAction != null) {
simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction);
}
userClaimsToBeModifiedIncludingMultiValueClaims = new HashMap<>(userClaimsToBeModified);
populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims);
🤖 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/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java`
around lines 5942 - 5971, The post-action snapshot
userClaimsToBeModifiedIncludingMultiValueClaims is built before merging action
responses, so single-valued claims added/modified by the action never make it
into the final snapshot used by
containsNonAccountStateOrNonVerificationClaim(...) and
publishUserProfileUpdateEvent(...). After you merge
userClaimsToBeAddedFromAction, userClaimsToBeModifiedFromAction and apply
deletions (userClaimsToBeDeletedFromAction) and after merging multi-valued
additions/removals, rebuild userClaimsToBeModifiedIncludingMultiValueClaims from
the up-to-date userClaimsToBeModified (clear it and putAll
userClaimsToBeModified, remove deleted keys) and then call
populateMultiValuedClaimsToModify(oldClaimList,
simpleMultiValuedClaimsToBeAdded, simpleMultiValuedClaimsToBeRemoved,
userClaimsToBeModifiedIncludingMultiValueClaims) so the final snapshot contains
both single- and multi-valued changes before any calls to
containsNonAccountStateOrNonVerificationClaim(...) or
publishUserProfileUpdateEvent(...).

}
}

if (log.isDebugEnabled()) {
Expand Down Expand Up @@ -5983,6 +6015,27 @@ private void updateUserClaims(User user, Map<String, String> oldClaimList,
}
}

/**
* Populate both modifiable single and multi-valued claims.
*
* @param oldClaimList User claim list for the user's existing state.
* @param simpleMultiValuedClaimsToBeAdded Map of simple multi-valued claims to be added.
* @param simpleMultiValuedClaimsToBeRemoved Map of simple multi-valued claims to be removed.
* @param userClaimsToBeModifiedIncludingMultiValueClaims Map of both single and multi valued claims to be modified.
* @return Map with the same keys and values as lists of strings, splitting multi-valued claims if necessary.
*/
private void populateMultiValuedClaimsToModify(Map<String, String> oldClaimList, Map<String, List<String>>
simpleMultiValuedClaimsToBeAdded, Map<String,
List<String>> simpleMultiValuedClaimsToBeRemoved,
Map<String, String> userClaimsToBeModifiedIncludingMultiValueClaims)
{

Map<String, String> multiValuedClaimsToModify =
SCIMCommonUtils.getSimpleMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
simpleMultiValuedClaimsToBeRemoved);
userClaimsToBeModifiedIncludingMultiValueClaims.putAll(multiValuedClaimsToModify);
}

/**
* Convert the claim values to list of strings.
*
Expand Down
12 changes: 6 additions & 6 deletions ...org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,13 @@ public class PreUpdateProfileActionExecutor {
* @param userClaimsToBeDeleted Collection of existing claims that deletes from the profile
* @throws UserStoreException If an error occurs while executing the action
*/
public void execute(User user, Map<String, String> userClaimsToBeModified,
Map<String, String> userClaimsToBeDeleted) throws UserStoreException {
public ActionExecutionStatus<?> execute(User user, Map<String, String> userClaimsToBeModified,
Comment thread
Lashen1227 marked this conversation as resolved.
Outdated
Map<String, String> userClaimsToBeDeleted) throws UserStoreException {

ActionExecutorService actionExecutorService = SCIMCommonComponentHolder.getActionExecutorService();

if (!actionExecutorService.isExecutionEnabled(ActionType.PRE_UPDATE_PROFILE)) {
return;
return null;
}
Comment thread
Lashen1227 marked this conversation as resolved.

LOG.debug("Executing pre update profile action for user: " + user.getId());
Expand All @@ -96,7 +96,7 @@ public void execute(User user, Map<String, String> userClaimsToBeModified,
actionExecutorService.execute(ActionType.PRE_UPDATE_PROFILE, flowContext,
IdentityContext.getThreadLocalIdentityContext().getTenantDomain());

handleActionExecutionStatus(actionExecutionStatus);
return handleActionExecutionStatus(actionExecutionStatus);
} catch (ActionExecutionException e) {
throw new UserActionExecutionServerException(UserActionError.PRE_UPDATE_PROFILE_ACTION_SERVER_ERROR,
"Error while executing pre update profile action.", e);
Expand All @@ -119,12 +119,12 @@ private UserActionRequestDTO buildUserActionRequestDTO(User user, Map<String, St
return userActionRequestDTOBuilder.build();
}

private void handleActionExecutionStatus(ActionExecutionStatus<?> actionExecutionStatus)
private ActionExecutionStatus<?> handleActionExecutionStatus(ActionExecutionStatus<?> actionExecutionStatus)
throws UserStoreException {

switch (actionExecutionStatus.getStatus()) {
case SUCCESS:
return;
return actionExecutionStatus;
Comment thread
Lashen1227 marked this conversation as resolved.
case FAILED:
Failure failure = (Failure) actionExecutionStatus.getResponse();
throw new UserActionExecutionClientException(UserActionError.PRE_UPDATE_PROFILE_ACTION_EXECUTION_FAILED,
Expand Down
6 changes: 3 additions & 3 deletions ....common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2620,7 +2620,7 @@ public void testUpdateUserClaimsWithNoChanges() throws Exception {
PreUpdateProfileActionExecutor executorMock = mock(PreUpdateProfileActionExecutor.class);
preUpdateField.set(scimUserManager, executorMock);

doNothing().when(executorMock).execute(any(User.class), anyMap(), anyMap());
when(executorMock.execute(any(User.class), anyMap(), anyMap())).thenReturn(null);

Field carbonUMField = SCIMUserManager.class.getDeclaredField("carbonUM");
carbonUMField.setAccessible(true);
Expand Down Expand Up @@ -2668,7 +2668,7 @@ public void testUpdateUserClaims_AddDeleteModifyClaims() throws Exception {
PreUpdateProfileActionExecutor executorMock = mock(PreUpdateProfileActionExecutor.class);
preUpdateField.set(scimUserManager, executorMock);

doNothing().when(executorMock).execute(any(User.class), anyMap(), anyMap());
when(executorMock.execute(any(User.class), anyMap(), anyMap())).thenReturn(null);

Field carbonUMField = SCIMUserManager.class.getDeclaredField("carbonUM");
carbonUMField.setAccessible(true);
Expand Down Expand Up @@ -2724,7 +2724,7 @@ public void testUpdateUserClaims_MultiValuedClaims() throws Exception {
PreUpdateProfileActionExecutor executorMock = mock(PreUpdateProfileActionExecutor.class);
preUpdateField.set(scimUserManager, executorMock);

doNothing().when(executorMock).execute(any(User.class), anyMap(), anyMap());
when(executorMock.execute(any(User.class), anyMap(), anyMap())).thenReturn(null);

Field carbonUMField = SCIMUserManager.class.getDeclaredField("carbonUM");
carbonUMField.setAccessible(true);
Expand Down
Loading