Skip to content
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

Fix user patch remove operation for migrating users #423

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Fix user patch remove operation for migrating users #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd2aca7
fix user patch remove op for migrating users
amanda-ariyaratne Feb 4, 2025
1598c3f
improve bad request condition and add descriptive comments
amanda-ariyaratne Feb 5, 2025
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
47 changes: 39 additions & 8 deletions ...aron-core/src/main/java/org/wso2/charon3/core/protocol/endpoints/UserResourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@

import static org.wso2.charon3.core.schema.SCIMConstants.OperationalConstants.COLON;
import static org.wso2.charon3.core.schema.SCIMConstants.OperationalConstants.DOT_SEPARATOR;
import static org.wso2.charon3.core.schema.SCIMConstants.OperationalConstants.OPEN_SQUARE_BRACKET;

/**
* REST API exposed by Charon-Core to perform operations on UserResource.
Expand Down Expand Up @@ -670,6 +671,7 @@ public SCIMResponse updateWithPATCH(String existingId, String scimObjectString,
User newUser = null;

Map<String, String> syncedAttributes = userManager.getSyncedUserAttributes();
List<String> deletedSyncedAttributes = new ArrayList<>();
for (PatchOperation operation : opList) {

if (operation.getOperation().equals(SCIMConstants.OperationalConstants.ADD)) {
Expand All @@ -685,14 +687,31 @@ public SCIMResponse updateWithPATCH(String existingId, String scimObjectString,

}
} else if (operation.getOperation().equals(SCIMConstants.OperationalConstants.REMOVE)) {
if (newUser == null) {
newUser = (User) PatchOperationUtil.doPatchRemove(operation, oldUser, copyOfOldUser, schema);
copyOfOldUser = (User) CopyUtil.deepCopy(newUser);

} else {
newUser = (User) PatchOperationUtil.doPatchRemove(operation, newUser, copyOfOldUser, schema);
copyOfOldUser = (User) CopyUtil.deepCopy(newUser);
try {
if (newUser == null) {
newUser = (User) PatchOperationUtil.doPatchRemove(operation, oldUser, copyOfOldUser,
schema);
copyOfOldUser = (User) CopyUtil.deepCopy(newUser);

} else {
newUser = (User) PatchOperationUtil.doPatchRemove(operation, newUser, copyOfOldUser,
schema);
copyOfOldUser = (User) CopyUtil.deepCopy(newUser);
}
} catch (BadRequestException e) {
amanda-ariyaratne marked this conversation as resolved.
Show resolved Hide resolved
/*
* This condition is for the migrated users who have enterprise user attributes and system
* schema attributes both mapped to a single local claim. In such cases, if both the scim
* attributes are specified to be removed in the patch request, for the second attribute, there
* will be an error thrown because it is already removed when processing the first attribute.
*/
if (!ResponseCodeConstants.INVALID_PATH.equals(e.getScimType()) ||
determineScimAttributes(operation).stream()
.noneMatch(deletedSyncedAttributes::contains)) {
throw e;
}
}

} else if (operation.getOperation().equals(SCIMConstants.OperationalConstants.REPLACE)) {
if (newUser == null) {
newUser = (User) PatchOperationUtil.doPatchReplace
Expand All @@ -708,6 +727,12 @@ public SCIMResponse updateWithPATCH(String existingId, String scimObjectString,
throw new BadRequestException("Unknown operation.", ResponseCodeConstants.INVALID_SYNTAX);
}

/*
* The following logic is for the migrated users who have enterprise user attributes and system schema
* attributes both mapped to a single local claim. In such cases, if any operation is only sent for
* one scim attributes, there will be conflicts for the final value because the other scim attribute's
* value is not changed. Therefore, we are removing the other scim attribute from the user object.
*/
if (syncedAttributes == null) {
continue;
}
Expand All @@ -729,9 +754,11 @@ public SCIMResponse updateWithPATCH(String existingId, String scimObjectString,
switch (subAttributes.length) {
case 1:
newUser.deleteSubAttribute(baseAttributeName, subAttributes[0]);
deletedSyncedAttributes.add(syncedAttribute);
break;
case 2:
newUser.deleteSubSubAttribute(baseAttributeName, subAttributes[0], subAttributes[1]);
deletedSyncedAttributes.add(syncedAttribute);
break;
default:
break;
Expand Down Expand Up @@ -836,8 +863,12 @@ private static List<String> determineScimAttributes(PatchOperation operation) {
}
List<String> attributes = new ArrayList<>();
String path = operation.getPath();
Object values = operation.getValues();
if (path != null) {
int bracketIndex = path.indexOf(OPEN_SQUARE_BRACKET);
path = (bracketIndex != -1) ? path.substring(0, bracketIndex) : path;
}

Object values = operation.getValues();
if (values instanceof JSONObject) {
extractScimAttributes((JSONObject) values, path, attributes);
} else if (values instanceof JSONArray) {
Expand Down
1 change: 1 addition & 0 deletions modules/charon-core/src/main/java/org/wso2/charon3/core/schema/SCIMConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,7 @@ public static class OperationalConstants {
public static final String COLON = ":";
public static final String URL_SEPARATOR = "/";
public static final String DOT_SEPARATOR = ".";
public static final String OPEN_SQUARE_BRACKET = "[";

}
}
Loading