From 5da851c3f109c250a7a5e39ce5b4f52466c9178c Mon Sep 17 00:00:00 2001 From: Kyle Thorpe Date: Tue, 12 Dec 2023 14:36:10 +0000 Subject: [PATCH 1/3] Handle Azure style Group member remove operations --- .../core/repository/PatchHandlerTest.java | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index 9dcafeea7..a145b8cc8 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -31,7 +31,9 @@ import org.apache.directory.scim.spec.resources.Email; import org.apache.directory.scim.spec.resources.Name; import org.apache.directory.scim.spec.resources.PhoneNumber; +import org.apache.directory.scim.spec.resources.ScimGroup; import org.apache.directory.scim.spec.resources.ScimUser; +import org.apache.directory.scim.spec.schema.ResourceReference; import org.junit.jupiter.api.Test; import static org.apache.directory.scim.spec.patch.PatchOperation.Type.*; @@ -465,6 +467,39 @@ public void deleteItemWithComplexFilter() throws FilterParseException { )); } + @Test + public void addItemToCollection() throws FilterParseException { + PatchOperation op = new PatchOperation(); + op.setOperation(ADD); + op.setPath(PatchOperationPath.fromString("members")); + op.setValue(List.of( + Map.of( + "value", "9876", + "display", "testUser2", + "type", "User") + )); + + ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op)); + assertThat(updatedGroup.getMembers().size()).isEqualTo(3); + } + + /** + * This test covers Azure-style remove member operations, where a value is + * specified in the operation body, rather than in the path. + * For example: { "op": "remove", "path": "members", "value": [ { "value": "1234" } ] } + */ + @Test + public void removeItemFromCollection() throws FilterParseException { + PatchOperation op = new PatchOperation(); + op.setOperation(REMOVE); + op.setPath(PatchOperationPath.fromString("members")); + op.setValue(List.of(Map.of("value", "1234"))); + + ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op)); + assertThat(updatedGroup.getMembers()).isNotNull(); + assertThat(updatedGroup.getMembers().size()).isEqualTo(1); + } + @Test public void addAttribute() throws FilterParseException { PatchOperation op = new PatchOperation(); @@ -629,4 +664,24 @@ private static ScimUser user() { throw new IllegalStateException("Invalid phone number", e); } } + + private static ScimGroup group() { + ResourceReference member1 = userRef("1234"); + ResourceReference member2 = userRef("5678"); + + return new ScimGroup() + .setDisplayName("Test Group") + .setExternalId("test-group-external-id") + .setMembers(List.of(member1, member2)); + } + + private static ResourceReference userRef(String id) { + ResourceReference member = new ResourceReference(); + member.setType(ResourceReference.ReferenceType.USER); + member.setValue(id); + member.setRef("https://example.com/Users/" + id); + member.setDisplay("Test User" + id); + + return member; + } } From cd7bd9050327e657601cab5099d255e9db17e838 Mon Sep 17 00:00:00 2001 From: Brian Demers Date: Mon, 18 Dec 2023 20:05:11 -0500 Subject: [PATCH 2/3] Fix: merge conflicts --- .../scim/core/repository/PatchHandlerTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index a145b8cc8..7c559cbe7 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -29,11 +29,11 @@ import org.apache.directory.scim.spec.phonenumber.PhoneNumberParseException; import org.apache.directory.scim.spec.resources.Address; import org.apache.directory.scim.spec.resources.Email; +import org.apache.directory.scim.spec.resources.GroupMembership; import org.apache.directory.scim.spec.resources.Name; import org.apache.directory.scim.spec.resources.PhoneNumber; import org.apache.directory.scim.spec.resources.ScimGroup; import org.apache.directory.scim.spec.resources.ScimUser; -import org.apache.directory.scim.spec.schema.ResourceReference; import org.junit.jupiter.api.Test; import static org.apache.directory.scim.spec.patch.PatchOperation.Type.*; @@ -51,6 +51,7 @@ public class PatchHandlerTest { public PatchHandlerTest() { SchemaRegistry schemaRegistry = new SchemaRegistry(); schemaRegistry.addSchema(ScimUser.class, List.of(EnterpriseExtension.class)); + schemaRegistry.addSchema(ScimGroup.class, null); this.patchHandler = new DefaultPatchHandler(schemaRegistry); } @@ -666,8 +667,8 @@ private static ScimUser user() { } private static ScimGroup group() { - ResourceReference member1 = userRef("1234"); - ResourceReference member2 = userRef("5678"); + GroupMembership member1 = userRef("1234"); + GroupMembership member2 = userRef("5678"); return new ScimGroup() .setDisplayName("Test Group") @@ -675,9 +676,9 @@ private static ScimGroup group() { .setMembers(List.of(member1, member2)); } - private static ResourceReference userRef(String id) { - ResourceReference member = new ResourceReference(); - member.setType(ResourceReference.ReferenceType.USER); + private static GroupMembership userRef(String id) { + GroupMembership member = new GroupMembership(); + member.setType(GroupMembership.Type.USER); member.setValue(id); member.setRef("https://example.com/Users/" + id); member.setDisplay("Test User" + id); From eabee15dd834cc08aa5dad30a8144a70f3c78414 Mon Sep 17 00:00:00 2001 From: Brian Demers Date: Sat, 16 Dec 2023 01:22:27 -0500 Subject: [PATCH 3/3] Adds support for Azure non-standard patch REMOVE requests Detects Azure Quirk mode. Azure uses an out of spec patch operation that does not use an express, but instead uses a remove with a value. Detect this and convert it to an expression { "op":"remove", "path":"members", "value":[{ "value":"" }] } --- .../core/repository/DefaultPatchHandler.java | 64 +++++++++++++++++++ .../core/repository/PatchHandlerTest.java | 10 ++- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java b/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java index f03d24a9d..661a0cc20 100644 --- a/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java +++ b/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java @@ -29,6 +29,7 @@ import org.apache.directory.scim.spec.exception.MutabilityException; import org.apache.directory.scim.spec.exception.UnsupportedFilterException; import org.apache.directory.scim.spec.filter.AttributeComparisonExpression; +import org.apache.directory.scim.spec.filter.CompareOperator; import org.apache.directory.scim.spec.filter.FilterExpressions; import org.apache.directory.scim.spec.filter.FilterParseException; import org.apache.directory.scim.spec.filter.ValuePathExpression; @@ -61,6 +62,8 @@ public class DefaultPatchHandler implements PatchHandler { public static final String PRIMARY = "primary"; + private static final String VALUE_ATTRIBUTE_NAME = "value"; + private static final TypeReference> MAP_TYPE = new TypeReference<>() {}; private final Map patchOperationHandlers = Map.of( @@ -378,6 +381,24 @@ public void applyMultiValue(T source, Map void applyValue(final T source, Map sourceAsMap, Schema schema, Attribute attribute, ValuePathExpression valuePathExpression, Object value) { + // detect Azure off-spec request + if (isAzureRemoveQuirk(attribute, valuePathExpression, value)) { + Collection valuesToRemove = (Collection) value; + AttributeReference valueAttributeRef = new AttributeReference(attribute.getUrn(), attribute.getName(), VALUE_ATTRIBUTE_NAME); + + // map the Azure formatted examples in to a _normal_ scim filter expression + azureQuirkValuesToRemove(valuesToRemove, attribute).forEach(itemToRemove -> { + ValuePathExpression adjustedValuePathExpression = new ValuePathExpression(valuePathExpression.getAttributePath(), new AttributeComparisonExpression(valueAttributeRef, CompareOperator.EQ, itemToRemove)); + applyMultiValue(source, sourceAsMap, schema, attribute, adjustedValuePathExpression, itemToRemove); + }); + + } else { + // call super (default method of interface) + PatchOperationHandler.super.applyValue(source, sourceAsMap, schema, attribute, valuePathExpression, value); + } + } + @Override public void applySingleValue(Map sourceAsMap, Attribute attribute, AttributeReference attributeReference, Object value) { if (attributeReference.hasSubAttribute()) { @@ -426,5 +447,48 @@ public void applyMultiValue(T source, Map + * { + * "op":"remove", + * "path":"members", + * "value":[{ + * "value":"" + * }] + * } + * + * @param valuePathExpression The valuePathExpression to check if it has a null attribute + * @return true, if Azure patch REMOVE detected. + */ + private static boolean isAzureRemoveQuirk(Attribute attribute, ValuePathExpression valuePathExpression, Object value) { + return attribute.isMultiValued() + && attribute.getAttribute(VALUE_ATTRIBUTE_NAME) != null + && valuePathExpression.getAttributeExpression() == null + && value instanceof Collection; + } + + private static List azureQuirkValuesToRemove(Collection listOfMaps, Attribute attribute) { + return listOfMaps.stream() + .map(item -> { + if (!(item instanceof Map)) { + throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but 'value' is not a list of maps"); + } + return (Map) item; + }) + .map(item -> { + Attribute valueAttribute = attribute.getAttribute(VALUE_ATTRIBUTE_NAME); + Object itemValue = item.get(valueAttribute.getName()); + if (!(itemValue instanceof String)) { + throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but item 'value' is not a string"); + } + + return (String) itemValue; + }) + .collect(toList()); + } } } diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index 7c559cbe7..2df84326f 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -38,6 +38,7 @@ import static org.apache.directory.scim.spec.patch.PatchOperation.Type.*; import static java.util.Map.entry; +import static org.apache.directory.scim.test.assertj.ScimpleAssertions.scimAssertThat; import static org.assertj.core.api.Assertions.assertThat; import java.util.List; @@ -480,8 +481,15 @@ public void addItemToCollection() throws FilterParseException { "type", "User") )); + GroupMembership member1 = userRef("1234"); + GroupMembership member2 = userRef("5678"); + GroupMembership member3 = new GroupMembership() + .setValue("9876") + .setDisplay("testUser2") + .setType(GroupMembership.Type.USER); + ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op)); - assertThat(updatedGroup.getMembers().size()).isEqualTo(3); + scimAssertThat(updatedGroup).containsOnlyMembers(member1, member2, member3); } /**