From f52a07d0ffb289d760fe21b8bdb5984259f30b33 Mon Sep 17 00:00:00 2001 From: Khalid Qarryzada Date: Fri, 6 Sep 2024 10:55:05 -0500 Subject: [PATCH] Support ADD patch ops targeting existing values In the 3.0.0 release, we implemented support for ADD patch operations with a value filter. This was configured to always append a new value to the array, since it is technically an "add". However, this does not play well with SCIM provisioners that send multiple individual updates that are meant to target the same value within a multi-valued attribute, e.g., a work email. The intention is to add a new value to the field that matches this path filter (e.g., "type eq \"work\""), not that the "type" field should also be added. The exception to this is if the provided type does not already exist on the resource; in this case, it makes sense to add that data. The new behavior is available via an opt-in setting in a static boolean variable, PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY. To opt into this setting, set the value of this variable to false. As a result of this change, the version of the SCIM SDK has been updated to 3.2.0-SNAPSHOT. Reviewer: vyhhuang Reviewer: dougbulkley JiraIssue: DS-49194 --- CHANGELOG.md | 23 ++- pom.xml | 2 +- scim2-assembly/pom.xml | 2 +- scim2-sdk-client/pom.xml | 2 +- scim2-sdk-common/pom.xml | 2 +- .../scim2/common/messages/PatchOperation.java | 133 +++++++++++++++-- .../AddOperationValueFilterTestCase.java | 134 ++++++++++++++---- scim2-sdk-server/pom.xml | 2 +- scim2-ubid-extensions/pom.xml | 2 +- 9 files changed, 263 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 357c0365..73ad5da9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,33 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](https://semver.org/). -## v3.1.1 - TBD +## v3.2.0 - TBD Fixed an issue where `AndFilter.equals()` and `OrFilter.equals()` could incorrectly evaluate to true. Updated Jackson dependencies to 2.17.2. +Added a property that allows ADD patch operations with value filters to target an existing value. +For example, for the following patch request: +```json +{ + "schemas": [ "urn:ietf:params:scim:api:messages:2.0:PatchOp" ], + "Operations": [ + { + "op": "add", + "path": "emails[type eq \"work\"].display", + "value": "newEmail@example.com" + } + ] +} +``` +When the new `PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY` property is set to `false`, this +operation will search the resource for an existing "work" email and add the value to that email. +This allows for better integration with SCIM provisioners that send individual requests such as +`emails[type eq "work"].display` followed by `emails[type eq "work"].value`, which are intended to +target the same email. The default value of `PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY` is +`true`, which will always create a new value on the multi-valued attribute. + ## v3.1.0 - 2024-Jun-25 Updated all classes within the UnboundID SCIM 2 SDK to utilize `@Nullable` and `@NotNull` annotations for all non-primitive input parameters, member variables, and return values. These diff --git a/pom.xml b/pom.xml index a7d8b812..ef2f4ae6 100644 --- a/pom.xml +++ b/pom.xml @@ -18,7 +18,7 @@ 4.0.0 com.unboundid.product.scim2 scim2-parent - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT pom UnboundID SCIM2 SDK Parent diff --git a/scim2-assembly/pom.xml b/scim2-assembly/pom.xml index e58e7aae..f3388754 100644 --- a/scim2-assembly/pom.xml +++ b/scim2-assembly/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-assembly pom diff --git a/scim2-sdk-client/pom.xml b/scim2-sdk-client/pom.xml index dcf5cdcf..7d32fdac 100644 --- a/scim2-sdk-client/pom.xml +++ b/scim2-sdk-client/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-sdk-client jar diff --git a/scim2-sdk-common/pom.xml b/scim2-sdk-common/pom.xml index 84fb74ff..485880f0 100644 --- a/scim2-sdk-common/pom.xml +++ b/scim2-sdk-common/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-sdk-common jar 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 32d679f5..770f0c67 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 @@ -42,6 +42,7 @@ import com.unboundid.scim2.common.filters.EqualFilter; import com.unboundid.scim2.common.filters.Filter; import com.unboundid.scim2.common.filters.FilterType; +import com.unboundid.scim2.common.utils.FilterEvaluator; import com.unboundid.scim2.common.utils.JsonUtils; import com.unboundid.scim2.common.utils.SchemaUtils; @@ -126,6 +127,39 @@ public abstract class PatchOperation @Nullable private final Path path; + /** + * This represents a property that, when enabled, will always append new + * values when processing ADD operations with a value filter. This is used for + * multi-valued attributes such as {@code emails}. For example, for the + * following patch request: + * 
+   *   {
+   *     "schemas": [ "urn:ietf:params:scim:api:messages:2.0:PatchOp" ],
+   *     "Operations": [
+   *       {
+   *         "op": "add",
+   *         "path": "emails[type eq \"work\"].display",
+   *         "value": "newEmail@example.com"
+   *       }
+   *     ]
+   *   }
+   *
+ * + * When this property is enabled and the above patch request is applied, the + * following JSON will be appended to the {@code emails} of the user resource: + * 
+   *   {
+   *     "type": "work",
+   *     "display": "newEmail@example.com"
+   *   }
+   *
+ * + * If this property is disabled, then the {@code display} field will be added + * to the resource's existing work email, if it exists. If the work email does + * not exist, then a new value will be appended. + */ + public static boolean APPEND_NEW_PATCH_VALUES_PROPERTY = true; + static final class AddOperation extends PatchOperation { @@ -215,7 +249,6 @@ public void apply(@NotNull final ObjectNode node) throws ScimException Path path = (getPath() == null) ? Path.root() : getPath(); if (hasValueFilter(path)) { - validateAddOpWithFilter(path, value); applyAddWithValueFilter(path, node, value); } else @@ -377,8 +410,10 @@ private void applyAddWithValueFilter( @NotNull final Path path, @NotNull final ObjectNode existingResource, @NotNull final JsonNode value) - throws BadRequestException + throws ScimException { + validateAddOpWithFilter(path, value); + Filter valueFilter = path.getElement(0).getValueFilter(); String filterAttributeName = valueFilter.getAttributePath().toString(); ValueNode filterValue = valueFilter.getComparisonValue(); @@ -405,13 +440,95 @@ private void applyAddWithValueFilter( } ArrayNode attribute = (ArrayNode) jsonAttribute; - // Construct the new attribute value that should be added to the resource. - ObjectNode newValue = JsonUtils.getJsonNodeFactory().objectNode(); - newValue.set(subAttributeName, value); - newValue.set(filterAttributeName, filterValue); + // When operations with a value filter add data, we can either append + // the data to a new value in the multi-valued attribute, or we can update + // an existing value. + // + // If the relevant property is enabled, the provided data should be added + // as a new value regardless of the existing resource's state (so we + // pretend that there are no matched values). Otherwise, any new data + // should update the existing value, if it is present. + ObjectNode matchedValue = null; + if (!APPEND_NEW_PATCH_VALUES_PROPERTY) + { + matchedValue = fetchExistingValue(attribute, valueFilter, attributeName); + } + + // If there are no existing values that match the filter, then we should + // add the two attribute values to the array. + if (matchedValue == null) + { + ObjectNode newValue = JsonUtils.getJsonNodeFactory().objectNode(); + newValue.set(subAttributeName, value); + newValue.set(filterAttributeName, filterValue); + + attribute.add(newValue); + existingResource.replace(attributeName, attribute); + return; + } + + // Ensure that the add does not conflict with an existing value. + if (FilterEvaluator.evaluate(Filter.pr(subAttributeName), matchedValue)) + { + throw BadRequestException.invalidValue(String.format( + "The add operation attempted to add a new '%s' field, but the" + + " specified path already has a '%s' defined.", + subAttributeName, + subAttributeName + )); + } + + matchedValue.set(subAttributeName, value); + } + + /** + * Checks a multi-valued attribute for an existing value and returns a value + * based on the following conditions: + * + * + * @param attribute The multi-valued attribute. + * @param valueFilter The value selection filter provided with the patch + * add operation. + * @param attributeName The name of {@code attribute}. + * + * @return An ObjectNode representing the single value that matched the + * criteria of the {@code valueFilter}, or {@code null} if no + * attributes matched the filter. + * + * @throws ScimException If there was an error processing the filter, or + * if multiple values were matched. + */ + @Nullable + private static ObjectNode fetchExistingValue( + @NotNull final ArrayNode attribute, + @NotNull final Filter valueFilter, + @NotNull final String attributeName) + throws ScimException + { + ObjectNode matchedValue = null; + + for (var arrayVal : attribute) + { + if (FilterEvaluator.evaluate(valueFilter, arrayVal)) + { + if (matchedValue != null) + { + throw BadRequestException.noTarget( + "The operation could not be applied on the resource because the" + + " value filter matched more than one element in the '" + + attributeName + "' array of the resource."); + } + matchedValue = (ObjectNode) arrayVal; + } + } - attribute.add(newValue); - existingResource.replace(attributeName, attribute); + return matchedValue; } /** diff --git a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java index 0684c288..64772cba 100644 --- a/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java +++ b/scim2-sdk-common/src/test/java/com/unboundid/scim2/common/AddOperationValueFilterTestCase.java @@ -30,6 +30,7 @@ import com.unboundid.scim2.common.types.PhoneNumber; import com.unboundid.scim2.common.types.UserResource; import com.unboundid.scim2.common.utils.JsonUtils; +import org.testng.annotations.AfterMethod; import org.testng.annotations.Test; import java.util.List; @@ -51,6 +52,15 @@ */ public class AddOperationValueFilterTestCase { + /** + * Reset the configurable "append" property to the default value. + */ + @AfterMethod + public void tearDown() + { + PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY = true; + } + /** * Ensure that patch ADD operations with a value selection filter are not * permitted for filter types other than equality filters. @@ -97,6 +107,9 @@ public void testBasic() throws Exception PatchRequest request; UserResource resource = new UserResource(); + // Unset the property to use the new behavior. + PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY = false; + // Add a work email to a list of existing emails. resource.setEmails( new Email().setValue("existing@example.com").setType("home"), @@ -122,32 +135,20 @@ public void testBasic() throws Exception .containsOnly( new Address().setStreetAddress("The Batcave").setType("secret")); - // Add a 'mobile' phone number to a user when an existing 'mobile' phone - // number already exists. This should not be rejected. - resource = new UserResource(); - resource.setPhoneNumbers( - new PhoneNumber().setValue("+1 314-159-2653").setType("mobile") + // An operation should be able to append data to another field. This + // resource begins with the street address populated, and the patch request + // should be able to update the "formatted" field when using a filter. + resource = new UserResource().setAddresses( + new Address().setType("home").setStreetAddress("8 Mile Rd.") ); - path = Path.fromString("phoneNumbers[type eq \"mobile\"].value"); - request = createAddRequest(path, "+1 271-828-1828"); + path = Path.fromString("addresses[type eq \"home\"].country"); + request = createAddRequest(path, "US"); resource = applyPatchRequest(request, resource); - assertThat(resource.getPhoneNumbers()) - .hasSize(2) - .containsExactly( - new PhoneNumber().setValue("+1 314-159-2653").setType("mobile"), - new PhoneNumber().setValue("+1 271-828-1828").setType("mobile")); - - // Add two photos with the same 'type' value within a single patch request. - resource = new UserResource(); - path = Path.fromString("photos[type eq \"thumbnail\"].value"); - request = new PatchRequest( - PatchOperation.add(path, TextNode.valueOf("https://example.com/1.png")), - PatchOperation.add(path, TextNode.valueOf("https://example.com/2.png")) - ); - resource = applyPatchRequest(request, resource); - assertThat(resource.getPhotos()) - .filteredOn(photo -> photo.getType().equals("thumbnail")) - .hasSize(2); + assertThat(resource.getAddresses()).hasSize(1); + var address = resource.getAddresses().get(0); + assertThat(address.getCountry()).isEqualTo("US"); + assertThat(address.getType()).isEqualTo("home"); + assertThat(address.getStreetAddress()).isEqualTo("8 Mile Rd."); // Only a single value selection filter should be permitted. Path multipleFilter = Path.fromString( @@ -177,6 +178,47 @@ public void testBasic() throws Exception .isInstanceOf(BadRequestException.class) .hasMessageContaining("needs to be 'attribute[filter].subAttribute'"); + // Attempt adding a 'streetAddress' field to a home address in the case + // where the streetAddress is already populated. This should be rejected, + // since the add operation should not act as a replace operation. + UserResource userWithStreet = new UserResource().setAddresses( + new Address().setType("home").setStreetAddress("8 Mile Rd.") + ); + path = Path.fromString("addresses[type eq \"home\"].streetAddress"); + PatchRequest existingSubAttrRequest = createAddRequest(path, "7 Mile Rd."); + assertThatThrownBy(() -> applyPatchRequest(existingSubAttrRequest, userWithStreet)) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("The add operation attempted to add a new 'streetAddress' field") + .hasMessageContaining("already has a 'streetAddress' defined"); + + // Adding two photos with the same 'type' value within a single patch + // request should also be forbidden. + path = Path.fromString("photos[type eq \"thumbnail\"].value"); + PatchRequest requestWithConflict = new PatchRequest( + PatchOperation.add(path, TextNode.valueOf("https://example.com/1.png")), + PatchOperation.add(path, TextNode.valueOf("https://example.com/2.png")) + ); + assertThatThrownBy(() -> applyPatchRequest(requestWithConflict, new UserResource())) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("The add operation attempted to add a new 'value' field") + .hasMessageContaining("already has a 'value' defined"); + + // Attempt to apply an operation when the existing resource already has + // multiple matching elements. + UserResource existingUser = new UserResource().setAddresses( + new Address().setStreetAddress("street1").setType("home"), + new Address().setStreetAddress("street2").setType("home") + ); + path = Path.fromString("addresses[type eq \"home\"].streetAddress"); + PatchRequest requestOnInvalidResource = new PatchRequest( + PatchOperation.add(path, TextNode.valueOf("aThirdStreet")) + ); + assertThatThrownBy(() -> applyPatchRequest(requestOnInvalidResource, existingUser)) + .isInstanceOf(BadRequestException.class) + .hasMessageContaining("The operation could not be applied on the resource because") + .hasMessageContaining("the value filter matched more than one element in") + .hasMessageContaining("the 'addresses' array"); + // Assemble an invalid patch request by placing the value in an array, as // opposed to providing it as a single string value. path = Path.fromString("emails[type eq \"home\"].value"); @@ -264,6 +306,50 @@ public void testDeserializedObject() throws Exception assertThat(resourceString).isEqualTo(expected); } + /** + * Tests the behavior of the + * {@link PatchOperation#APPEND_NEW_PATCH_VALUES_PROPERTY} when it is + * configured to always append data targeted with a value filter. + */ + @Test + public void testUseAppendMode() throws Exception + { + UserResource resource; + Path path; + PatchRequest request; + + // Set the property so that the patch operation logic will always append new + // values for add operations with a filtered path. + PatchOperation.APPEND_NEW_PATCH_VALUES_PROPERTY = true; + + // Add a 'mobile' phone number to a user when an existing 'mobile' phone + // number already exists. This should not be rejected. + resource = new UserResource(); + resource.setPhoneNumbers( + new PhoneNumber().setValue("+1 314-159-2653").setType("mobile") + ); + path = Path.fromString("phoneNumbers[type eq \"mobile\"].value"); + request = createAddRequest(path, "+1 271-828-1828"); + resource = applyPatchRequest(request, resource); + assertThat(resource.getPhoneNumbers()) + .hasSize(2) + .containsExactly( + new PhoneNumber().setValue("+1 314-159-2653").setType("mobile"), + new PhoneNumber().setValue("+1 271-828-1828").setType("mobile")); + + // Add two photos with the same 'type' value within a single patch request. + resource = new UserResource(); + path = Path.fromString("photos[type eq \"thumbnail\"].value"); + request = new PatchRequest( + PatchOperation.add(path, TextNode.valueOf("https://example.com/1.png")), + PatchOperation.add(path, TextNode.valueOf("https://example.com/2.png")) + ); + resource = applyPatchRequest(request, resource); + assertThat(resource.getPhotos()) + .filteredOn(photo -> "thumbnail".equals(photo.getType())) + .hasSize(2); + } + /** * This helper method is shorthand for a new patch request that contains a * single add operation with a string value. diff --git a/scim2-sdk-server/pom.xml b/scim2-sdk-server/pom.xml index 8410d172..8cad8347 100644 --- a/scim2-sdk-server/pom.xml +++ b/scim2-sdk-server/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-sdk-server jar diff --git a/scim2-ubid-extensions/pom.xml b/scim2-ubid-extensions/pom.xml index aca88911..821a21a1 100644 --- a/scim2-ubid-extensions/pom.xml +++ b/scim2-ubid-extensions/pom.xml @@ -19,7 +19,7 @@ scim2-parent com.unboundid.product.scim2 - 3.1.1-SNAPSHOT + 3.2.0-SNAPSHOT scim2-ubid-extensions jar