Handle Azure style Group member remove operations

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<String, Object>> MAP_TYPE = new TypeReference<>() {};
 
   private final Map<PatchOperation.Type, PatchOperationHandler> patchOperationHandlers = Map.of(
@@ -378,6 +381,24 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
 
   private static class RemoveOperationHandler implements PatchOperationHandler {
 
+    public <T extends ScimResource> void applyValue(final T source, Map<String, Object> 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<String, Object> sourceAsMap, Attribute attribute, AttributeReference attributeReference, Object value) {
       if (attributeReference.hasSubAttribute()) {
@@ -426,5 +447,48 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
         }
       }
     }
+
+    /**
+     *  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
+     *  <pre><code>
+     *   {
+     *     "op":"remove",
+     *     "path":"members",
+     *     "value":[{
+     *         "value":"<id>"
+     *     }]
+     *   }
+     *   </code></pre>
+     * @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<String> 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());
+    }
   }
 }

scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java

@@ -29,13 +29,16 @@
 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.junit.jupiter.api.Test;
 
 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;
@@ -49,6 +52,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);
   }
 
@@ -465,6 +469,46 @@ 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")
+    ));
+
+    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));
+    scimAssertThat(updatedGroup).containsOnlyMembers(member1, member2, member3);
+  }
+
+  /**
+   * 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 +673,24 @@ private static ScimUser user() {
       throw new IllegalStateException("Invalid phone number", e);
     }
   }
+
+  private static ScimGroup group() {
+    GroupMembership member1 = userRef("1234");
+    GroupMembership member2 = userRef("5678");
+
+    return new ScimGroup()
+      .setDisplayName("Test Group")
+      .setExternalId("test-group-external-id")
+      .setMembers(List.of(member1, member2));
+  }
+
+  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);
+
+    return member;
+  }
 }