diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a92acde..e4929ac7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,10 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](https://semver.org/). ## v3.0.1 - TBD -Placeholder for the next release. +Resolved an issue with replace operations that set the `value` field to an empty array. When these +operations are applied, the SCIM SDK now clears all matching values of the targeted multi-valued +attribute. If the `path` of the replace operation does not have a filter, then the multi-valued +attribute will be deleted from the resource. ## v3.0.0 - 2023-Oct-03 Removed support for Java 8. The UnboundID SCIM 2 SDK now requires Java 11 or a later release. diff --git a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java index 1ce06195..d708c8db 100644 --- a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java +++ b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/messages/PatchOperation.java @@ -594,8 +594,8 @@ public List getValues(final Class cls) @Override public void apply(final ObjectNode node) throws ScimException { - JsonUtils.replaceValue(getPath() == null ? Path.root() : - getPath(), node, value); + Path path = (getPath() == null) ? Path.root() : getPath(); + JsonUtils.replaceValue(path, node, value); addMissingSchemaUrns(node); } diff --git a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java index 0d0d280c..da985248 100644 --- a/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java +++ b/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java @@ -28,6 +28,8 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import com.fasterxml.jackson.databind.type.CollectionType; import com.unboundid.scim2.common.Path; +import com.unboundid.scim2.common.annotations.NotNull; +import com.unboundid.scim2.common.annotations.Nullable; import com.unboundid.scim2.common.exceptions.BadRequestException; import com.unboundid.scim2.common.exceptions.ScimException; import com.unboundid.scim2.common.filters.Filter; @@ -35,6 +37,7 @@ import com.unboundid.scim2.common.types.AttributeDefinition; import java.io.IOException; +import java.util.ArrayList; import java.util.Date; import java.util.Iterator; import java.util.LinkedList; @@ -254,55 +257,131 @@ else if(node.isArray()) /** * {@inheritDoc} */ - protected void visitLeafNode(final ObjectNode parent, - final String field, - final Filter valueFilter) + protected void visitLeafNode(@NotNull final ObjectNode parent, + @Nullable final String field, + @Nullable final Filter valueFilter) throws ScimException { - if(field != null) + boolean matchesFound = false; + + if (field == null) + { + updateNode(parent, null, value); + return; + } + + // Operations without a value filter can be updated normally without any + // special considerations. Note that add operations with value filters are + // already handled in PatchOperation#applyAddWithValueFilter. + if (appendValues || valueFilter == null) + { + updateNode(parent, field, value); + return; + } + + // in replace mode, a value filter requires that the target node + // be an array and that we can find matching value(s) + JsonNode node = parent.path(field); + if (node.isArray()) { - JsonNode node = parent.path(field); - if (!appendValues && valueFilter != null) + var array = (ArrayNode) node; + matchesFound = processArrayReplace(parent, field, array, valueFilter); + } + // exception: this allows filters on singular values if + // and only if the filter uses the "value" attribute to + // reference the value of the value node. + else if (FilterEvaluator.evaluate(valueFilter, node)) + { + matchesFound = true; + updateNode(parent, field, value); + } + + if (!matchesFound) + { + throw BadRequestException.noTarget("Attribute " + + field + " does not have a value matching " + + "the filter " + valueFilter); + } + } + + /** + * This method processes a replace operation for a multi-valued attribute + * (e.g., {@code emails, addresses}) with a value selection filter. If the + * {@code value} of the patch operation is an empty array, then attribute + * values that match the filter will be removed from the target resource. + * + * @param parent The resource that contains the multi-valued + * attribute. + * @param field The name of the multi-valued attribute (e.g., + * {@code emails}). + * @param array The current value of the multi-valued attribute. + * @param valueFilter The value filter indicating the subset of attribute + * values that should be modified. + * + * @return {@code true} if at least one value in the {@code array} was + * updated with the new value. + * + * @throws ScimException If an error occurred while processing the filter. + */ + private boolean processArrayReplace(@NotNull final ObjectNode parent, + @NotNull final String field, + @NotNull final ArrayNode array, + @NotNull final Filter valueFilter) + throws ScimException + { + boolean nodeUpdated = false; + + // If the 'value' is an empty array, then the client is actually + // requesting that we delete values that match the filter. + boolean isDelete = isNullNodeOrEmptyArray(value); + List indexesToDelete = new ArrayList<>(); + + for (int i = 0; i < array.size(); i++) + { + JsonNode attributeValue = array.get(i); + + // We should only perform processing on this inner node if it matches + // the filter. + if (!FilterEvaluator.evaluate(valueFilter, attributeValue)) { - // in replace mode, a value filter requires that the target node - // be an array and that we can find matching value(s) - boolean matchesFound = false; - if (node.isArray()) - { - for(int i = 0; i < node.size(); i++) - { - if(FilterEvaluator.evaluate(valueFilter, node.get(i))) - { - matchesFound = true; - if(node.get(i).isObject() && value.isObject()) - { - updateNode((ObjectNode) node.get(i), null, value); - } - else - { - ((ArrayNode) node).set(i, value); - } - } - } - } - // exception: this allows filters on singular values if - // and only if the filter uses the "value" attribute to - // reference the value of the value node. - else if (FilterEvaluator.evaluate(valueFilter, node)) - { - matchesFound = true; - updateNode(parent, field, value); - } - if(!matchesFound) - { - throw BadRequestException.noTarget("Attribute " + - field + " does not have a value matching " + - "the filter " + valueFilter); - } - return; + continue; + } + nodeUpdated = true; + + + if (attributeValue.isObject() && value.isObject()) + { + updateNode((ObjectNode) attributeValue, null, value); + } + else if (isDelete && attributeValue.isObject()) + { + // Mark the current index as a child that must be deleted. We cannot + // delete this yet because it would modify the 'i' index, which we are + // currently iterating over. + indexesToDelete.add(i); + } + else + { + array.set(i, value); } } - updateNode(parent, field, value); + + for (int j = indexesToDelete.size() - 1; j >= 0; j--) + { + var nodeIndex = indexesToDelete.get(j); + array.remove(nodeIndex); + } + + // If we have pruned all remaining values on the field, then we should + // completely delete the attribute. For example, if a resource only has a + // single email that is deleted, the 'emails' field in the JsonNode should + // explicitly be set to null. + if (array.isEmpty()) + { + updateNode(parent, field, NullNode.getInstance()); + } + + return nodeUpdated; } /** @@ -313,17 +392,27 @@ else if (FilterEvaluator.evaluate(valueFilter, node)) * @param key The key of the field to update. * @param value The update value. */ - protected void updateNode(final ObjectNode parent, final String key, - final JsonNode value) + protected void updateNode(@NotNull final ObjectNode parent, + @Nullable final String key, + @Nullable final JsonNode value) { - if(value.isNull() || value.isArray() && value.size() == 0) + // RFC 7643 section 2.5 states: + // "Unassigned attributes, the null value, or empty array (in the case of + // a multi-valued attribute) SHALL be considered to be equivalent in + // 'state'. Assigning an attribute with the value 'null' or an empty + // array... has the effect of making the attribute 'unassigned'." + if (isNullNodeOrEmptyArray(value)) { - // draft-ietf-scim-core-schema section 2.4 states "Unassigned - // attributes, the null value, or empty array (in the case of - // a multi-valued attribute) SHALL be considered to be - // equivalent in 'state'". + // Replace operations should remove the field. If this is an add + // operation, there is no update to make. + if (!appendValues && key != null) + { + parent.remove(key); + } + return; } + // When key is null, the node to update is the parent itself. JsonNode node = key == null ? parent : parent.path(key); if(node.isObject()) @@ -1056,4 +1145,17 @@ public static void setCustomMapperFactory(final MapperFactory customMapperFactor SDK_OBJECT_MAPPER = customMapperFactory.createObjectMapper(); } + + /** + * Validates whether the provided node is null or an empty array. + */ + private static boolean isNullNodeOrEmptyArray(@Nullable final JsonNode node) + { + if (node == null || node.isNull()) + { + return true; + } + + return (node.isArray() && node.isEmpty()); + } } diff --git a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/PatchOpTestCase.java b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/PatchOpTestCase.java index 9f68e9cf..a6e8db56 100644 --- a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/PatchOpTestCase.java +++ b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/PatchOpTestCase.java @@ -18,9 +18,11 @@ package com.unboundid.scim2.common; import com.fasterxml.jackson.core.Base64Variants; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.NullNode; import com.fasterxml.jackson.databind.node.ObjectNode; import com.google.common.collect.Lists; import com.unboundid.scim2.common.exceptions.BadRequestException; @@ -28,6 +30,10 @@ import com.unboundid.scim2.common.messages.PatchOpType; import com.unboundid.scim2.common.messages.PatchOperation; import com.unboundid.scim2.common.messages.PatchRequest; +import com.unboundid.scim2.common.types.Address; +import com.unboundid.scim2.common.types.Email; +import com.unboundid.scim2.common.types.Photo; +import com.unboundid.scim2.common.types.UserResource; import com.unboundid.scim2.common.utils.JsonUtils; import org.testng.Assert; import org.testng.annotations.Test; @@ -37,6 +43,8 @@ import java.util.Arrays; import java.util.Date; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertThrows; @@ -358,10 +366,9 @@ public void getTestPatch() throws IOException, ScimException * Test bad patch requests. * * @throws IOException If an error occurs. - * @throws ScimException If an error occurs. */ @Test - public void getTestBadPatch() throws IOException, ScimException + public void getTestBadPatch() throws IOException { try { @@ -774,7 +781,6 @@ public void testEmptyValues() throws Exception assertThrows(IllegalArgumentException.class, () -> PatchOperation.create(PatchOpType.REPLACE, "attr", null)); - assertThrows(IllegalArgumentException.class, () -> PatchOperation.add("attr", EMPTY_OBJECT)); assertThrows(IllegalArgumentException.class, @@ -784,6 +790,15 @@ public void testEmptyValues() throws Exception assertThrows(IllegalArgumentException.class, () -> PatchOperation.create(PatchOpType.REPLACE, "attr", EMPTY_OBJECT)); + JsonNode nullNode = NullNode.getInstance(); + assertThrows(IllegalArgumentException.class, + () -> PatchOperation.add("attr", nullNode)); + assertThrows(IllegalArgumentException.class, + () -> PatchOperation.replace("attr", nullNode)); + assertThrows(IllegalArgumentException.class, + () -> PatchOperation.create(PatchOpType.ADD, "attr", nullNode)); + assertThrows(IllegalArgumentException.class, + () -> PatchOperation.create(PatchOpType.REPLACE, "attr", nullNode)); // Empty array values should be accepted. PatchOperation.add("myArray", EMPTY_ARRAY); @@ -792,6 +807,149 @@ public void testEmptyValues() throws Exception PatchOperation.create(PatchOpType.REPLACE, "myArray", EMPTY_ARRAY); } + /** + * This test validates the behavior of patch operations when the value is an + * empty array. When a patch operations sets the {@code value} field to an + * empty array, this signifies that the relevant multi-valued attribute must + * be cleared of all existing values. + */ + @Test + public void testApplyEmptyArray() throws Exception + { + PatchRequest request; + UserResource user = new UserResource(); + user.setUserName("muhammad.ali"); + user.setEmails(new Email().setValue("muhammad.ali@example.com")); + + // "Add" an empty array. The resource should be unaffected. + request = new PatchRequest( + PatchOperation.add("emails", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getEmails()).hasSize(1); + + // Replace the attribute with an empty array. This should delete all email + // values on the resource. + request = new PatchRequest( + PatchOperation.replace("emails", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getEmails()).isNull(); + + // Apply an "empty" value to a resource that already does not have a value + // for the requested attribute. + assertThat(user.getAddresses()).isNull(); + request = new PatchRequest( + PatchOperation.add("addresses", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getAddresses()).isNull(); + + // Delete all attribute values when none previously exist. + request = new PatchRequest( + PatchOperation.replace("addresses", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getAddresses()).isNull(); + + // If multiple values exist on a resource, the behavior of an 'add' should + // be unchanged. All values should still be cleared when replaced with an + // empty array. + user.setPhotos( + new Photo().setValue(URI.create("https://example.com/1.png")).setType("profile"), + new Photo().setValue(URI.create("https://example.com/2.png")).setType("wallpaper"), + new Photo().setValue(URI.create("https://example.com/3.png")).setType("mystery") + ); + request = new PatchRequest( + PatchOperation.add("photos", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getPhotos()).hasSize(3); + request = new PatchRequest( + PatchOperation.replace("photos", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getPhotos()).isNull(); + } + + + /** + * Similar to {@link #testApplyEmptyArray}, but involves replace operations + * containing paths with a value selection filter. Validation for add + * operations can be found in {@link AddOperationValueFilterTestCase}. + */ + @Test + public void testReplaceEmptyArrayAndValueFilter() throws Exception + { + PatchRequest request; + UserResource user = new UserResource().setUserName("MuhammadAli").setEmails( + new Email().setValue("fewCupsOfLove@example.com").setType("work"), + new Email().setValue("oneTbspPatience@example.com").setType("work"), + new Email().setValue("oneTspOfGenerosity@example.com").setType("home"), + new Email().setValue("onePintOfKindness@example.com").setType("other") + ); + + // Delete the home email by replacing its value with an empty array. + request = new PatchRequest( + PatchOperation.replace("emails[type eq \"home\"]", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getEmails()).hasSize(3); + assertThat(user.getEmails()).noneMatch( + email -> "home".equalsIgnoreCase(email.getType()) + ); + + // Delete both work emails. + request = new PatchRequest( + PatchOperation.replace("emails[type eq \"work\"]", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getEmails()).hasSize(1); + assertThat(user.getEmails()).first().matches( + email -> "other".equalsIgnoreCase(email.getType()) + ); + + // Delete the last value. The emails attribute should be null. + request = new PatchRequest( + PatchOperation.replace("emails[type eq \"other\"]", EMPTY_ARRAY) + ); + user = applyPatchRequest(user, request); + assertThat(user.getEmails()).isNull(); + + // Send a delete request that does not match any values on the resource. + // This should result in an exception. + UserResource newUser = new UserResource().setAddresses( + new Address().setStreetAddress("1234 Tarrey Town Blvd.").setType("home"), + new Address().setStreetAddress("0001 Hyrule Court").setType("castle") + ); + PatchRequest unmatchedRequest = new PatchRequest( + PatchOperation.replace("addresses[type eq \"work\"]", EMPTY_ARRAY) + ); + assertThatThrownBy(() -> applyPatchRequest(newUser, unmatchedRequest)) + .isInstanceOf(BadRequestException.class) + .satisfies(ex -> { + var e = (BadRequestException) ex; + assertThat(e.getMessage()).contains("does not have a value matching the filter"); + }); + + // newUser should still have two addresses since nothing was removed. + assertThat(newUser.getAddresses()).hasSize(2); + + // A BadRequestException should be thrown if a replace operation targets an + // attribute that did not contain any initial values. + UserResource emptyUser = new UserResource().setUserName("emptyUser"); + PatchRequest emailRequest = new PatchRequest( + PatchOperation.replace("emails[type eq \"home\"]", EMPTY_ARRAY) + ); + assertThatThrownBy(() -> applyPatchRequest(emptyUser, emailRequest)) + .isInstanceOf(BadRequestException.class) + .satisfies(ex -> { + var e = (BadRequestException) ex; + assertThat(e.getMessage()).contains("does not have a value matching the filter"); + }); + assertThat(emptyUser.getEmails()).isNull(); + } + /** * Tests methods that accept varargs on the PatchOperation class, such as @@ -856,4 +1014,17 @@ public void testVarArgs() throws Exception URI.create("https://example.com/cool")); assertEquals(operation, operation2); } + + /** + * This method applies a patch request to a UserResource object and returns + * a new UserResource reflecting the modifications. + */ + private static UserResource applyPatchRequest(UserResource userResource, + PatchRequest request) + throws JsonProcessingException, ScimException + { + GenericScimResource user = userResource.asGenericScimResource(); + request.apply(user); + return JsonUtils.nodeToValue(user.getObjectNode(), UserResource.class); + } }