pingidentity / scim2

The UnboundID SCIM 2.0 SDK for Java
181 stars 74 forks source link

Multiple Add PatchOps with the same filter (i.e. address[type="work"]) generate multiple JsonAttributes instead of updating existing attribute #213

Closed egorksv closed 1 week ago

egorksv commented 10 months ago

Describe the bug When multiple Patch Add operations are issued for the same compound object in the scope of the same PatchRequest, AddOperation creates a separate record for each property.

Important node: We are building SCIM Server, so the PatchRequest is deserialised from external system (Entra ID in our case)

To Reproduce Using the code from the documentation and standard User resource:

        ScimUserResource scimUserResource = patchUserMapper.userToScimUserResource(existingUser);
        GenericScimResource gsr = scimUserResource.asGenericScimResource();
        patchRequest.apply(gsr);
        ObjectNode objectNode = gsr.getObjectNode();
        scimUserResource = JsonUtils.nodeToValue(objectNode, ScimUserResource.class);

... with this PatchOp request:

{
  "schemas" : [ "urn:ietf:params:scim:api:messages:2.0:PatchOp" ],
  "Operations" : [ {
    "op" : "add",
    "path" : "addresses[type eq \"work\"].streetAddress",
    "value" : "User Street Address"
  }, {
    "op" : "add",
    "path" : "addresses[type eq \"home\"].streetAddress",
    "value" : "User Street Address"
  }, {
    "op" : "add",
    "path" : "addresses[type eq \"work\"].locality",
    "value" : "User City"
  }, {
    "op" : "add",
    "path" : "addresses[type eq \"work\"].region",
    "value" : "User Country"
  }, {
    "op" : "add",
    "path" : "addresses[type eq \"home\"].region",
    "value" : "User Country"
  }, {
    "op" : "add",
    "path" : "addresses[type eq \"work\"].postalCode",
    "value" : "55555"
  }, {
    "op" : "add",
    "path" : "addresses[type eq \"work\"].country",
    "value" : "User Country"
  }, {
    "op" : "replace",
    "path" : "phoneNumbers[type eq \"work\"].value",
    "value" : "16176807200"
  }, {
    "op" : "replace",
    "path" : "phoneNumbers[type eq \"mobile\"].value",
    "value" : "16176809900"
  }, {
    "op" : "replace",
    "path" : "urn:ietf:params:scim:schemas:extension:acea:2.0:User:joinDate",
    "value" : "2023-06-30T20:00:00Z"
  }, {
    "op" : "add",
    "path" : "urn:ietf:params:scim:schemas:extension:acea:2.0:User:userDOB",
    "value" : "2023-06-30T20:00:00Z"
  } ]
}

The following result is achieved (skipped the irrelevant parts of User resource):

{
  "addresses": [
    {
      "formatted": null,
      "streetAddress": "User Street Address",
      "locality": null,
      "region": null,
      "postalCode": null,
      "country": null,
      "type": "work",
      "primary": null
    },
    {
      "formatted": null,
      "streetAddress": "User Street Address",
      "locality": null,
      "region": null,
      "postalCode": null,
      "country": null,
      "type": "home",
      "primary": null
    },
    {
      "formatted": null,
      "streetAddress": null,
      "locality": "User City",
      "region": null,
      "postalCode": null,
      "country": null,
      "type": "work",
      "primary": null
    },
    {
      "formatted": null,
      "streetAddress": null,
      "locality": null,
      "region": "User Country",
      "postalCode": null,
      "country": null,
      "type": "work",
      "primary": null
    },
    {
      "formatted": null,
      "streetAddress": null,
      "locality": null,
      "region": "User Country",
      "postalCode": null,
      "country": null,
      "type": "home",
      "primary": null
    },
    {
      "formatted": null,
      "streetAddress": null,
      "locality": null,
      "region": null,
      "postalCode": "55555",
      "country": null,
      "type": "work",
      "primary": null
    },
    {
      "formatted": null,
      "streetAddress": null,
      "locality": null,
      "region": null,
      "postalCode": null,
      "country": "User Country",
      "type": "work",
      "primary": null
    }
  ]
}

Expected behavior The code is expected to create only two Address records, one [type="work"] and one [type="home"]:

{
  "addresses": [
    {
      "formatted": null,
      "streetAddress": "User Street Address",
      "locality": "User City",
      "region": "User Country",
      "postalCode": "55555",
      "country": "User Country",
      "type": "work",
      "primary": null
    },
    {
      "formatted": null,
      "streetAddress": "User Street Address",
      "locality": null,
      "region": "User Country",
      "postalCode": null,
      "country": null,
      "type": "home",
      "primary": null
    }
  ]
}

Additional context Add any other context about the problem here. For example:

Proposed Solution I have copied and patched PatchRequest/PatchOperation classes into our code base to provide immediate fix. Please advise if pull request is welcome.

        private void applyAddWithValueFilter(
                @NotNull final Path path,
                @NotNull final ObjectNode existingResource,
                @NotNull final JsonNode value)
                throws BadRequestException {
            Filter valueFilter = path.getElement(0).getValueFilter();
            String filterAttributeName = valueFilter.getAttributePath().toString();
            ValueNode filterValue = valueFilter.getComparisonValue();

            // For an attribute path of the form 'emails[...].value', fetch the
            // attribute (emails) and the sub-attribute (value).
            String attributeName = path.getElement(0).getAttribute();
            String subAttributeName = path.getElement(1).getAttribute();

            JsonNode jsonAttribute = existingResource.get(attributeName);
            if (jsonAttribute == null) {
                // There are no existing values for the attribute, so we should add this
                // value ourselves.
                jsonAttribute = JsonUtils.getJsonNodeFactory().arrayNode(1);
            }
            if (!jsonAttribute.isArray()) {
                throw BadRequestException.invalidSyntax(
                        "The patch operation could not be processed because a complex"
                                + " value selection filter was provided, but '" + attributeName
                                + "' is single-valued"
                );
            }
            ArrayNode attribute = (ArrayNode) jsonAttribute;

//*** Changes begin here
            ObjectNode valueToSet = null;
            for (JsonNode existingObject : attribute) {
                if (filterValue.equals(existingObject.get(filterAttributeName))) {
                    // Actually locate the existing attribute using the filter value.
                    valueToSet = (ObjectNode) existingObject;
                    break;
                }
            }
            if (valueToSet == null) {
                // Construct the new attribute value if it does not already exist.
                valueToSet = JsonUtils.getJsonNodeFactory().objectNode();
                attribute.add(valueToSet);
                existingResource.replace(attributeName, attribute);
            }
            valueToSet.set(subAttributeName, value);
            valueToSet.set(filterAttributeName, filterValue);
        }
kqarryzada commented 3 months ago

I have some preliminary changes for this issue, but they will require more testing before we can release this fix.

kqarryzada commented 1 week ago

@egorksv, this behavior will be available in the next release of the SCIM SDK. To opt into this feature, set the following value in your code:

PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY = false;