Patch replace operation with MS Azure patch-filter-expression workaround deletes other items in a list

[jkolinek]

I have the Azure workaround enabled

.patchConfig(PatchConfig.builder()
        .supported(true)
        .activateMsAzureWorkaround(true)
        .build())

And there is an existing user with some multivalue complex attribute for example

"emails": [
  {
    "type": "work",
    "primary": true,
    "value": "john@work.com"
  },
  {
    "type": "home",
    "primary": false,
    "value": "john@home.com"
  }
]

When this patch request is received

{
  "schemas": [
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations": [
    {
      "op": "replace",
      "path": "emails[type eq \"work\"].value",
      "value": "updated@work.com"
    }
  ]
}

Then the emails attribute is updated to

"emails": [
  {
    "type": "work",
    "primary": false,
    "value": "updated@work.com"
  }
]

The home email was removed. It does not matter if the filter type eq \"work\" matches an existing item or not.

[Captain-P-Goldfish]

I just thought I had fixed it and then I noticed that this workaround addresses another problem which causes this problem not to be fixable.

This workaround is used to handle microsofts behaviour that if the "type"-value is missing that microsoft expects the value to be added to the email. This can only work for single-emails of course.

If the email looks like this:

"emails": [
  {
    "primary": true,
    "value": "john@work.com"
  }
]

and msAzure sends a filter like this:

{
  "schemas": [
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations": [
    {
      "op": "replace",
      "path": "emails[type eq \"work\"].value",
      "value": "updated@work.com"
    }
  ]
}

it does expect the request to succeed and that the type-attribute is also added to the email. So MsAzure expects this as a result:

"emails": [
  {
    "primary": true,
    "type": "work",
    "value": "updated@work.com"
  }
]

that is a serious problem and I have no idea how this expectation should be met based on the expected behaviour. What I just tried before I noticed that I get problems with the solution was translating the above patch-request into this:

{
  "path" : "emails[type eq \"work\"]",
  "op" : "replace",
  "value" : [ {
    "value" : "updated@work.de",
    "type" : "work"
  } ]
}

but this would cause the example from above not to be updated because the filter does not match anymore and the primary attribute would also be lost so its also not a good solution

[jkolinek]

I have observed some of the Azure requests. When a new value (such as email) is added it sends

{
  "schemas": [
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations": [
    {
      "op": "Add",
      "path": "emails[type eq \"work\"].value",
      "value": "email"
    }
  ]
}

And when the attribute is edited in Azure it sends the same but with "op": "Replace". So the fix would be to apply the workaround only for ADD operation because REPLACE works correctly without the workaround when it is found in the list. I haven't seen the case with the missing type you described.

But when the aadOptscim062020 Azure feature flag is enabled then it always sends "op": "replace" even when the item is new. I think that after this issue https://github.com/Captain-P-Goldfish/SCIM-SDK/issues/664 is fixed then we don't need to use the feature flag but MS may make this behavior default in the future.

I tried to make changes in the SDK to support the replace that should add a new item. This works but I am not sure about all the consequences :)

[Captain-P-Goldfish]

sorry, for the delay but I need some more time for this issue or it would also be very helpful if you might provide a pull-request. I have currently so many working stations with a higher priority than this. But if you are going to provide a pull-request please do the fix within the PatchWorkaround implementations

[ozivotsky]

Hi, the changes from @jkolinek works for me. It can properly handle all:

• add patch operation with path emails[type eq "home"].value
• replace patch operation with path emails[type eq "home"].value when home email doesn't exist (it is added)
• replace patch operation with path emails[type eq "home"].value when home email exists (it is replaced)

@jkolinek can you create PR, please?

[Captain-P-Goldfish]

I checked the pull-request and it got me thinking of a better solution that achieves the very same what you did. I prefer this solution because it keeps the PatchTargetHandler clean of explicit Microsoft workarounds and it even gives a new handling feature that can be toggled on and off.

1. Add a new configuration attribute to PatchConfig -> addNodeOnMissingTarget
2. remove the current change from PatchTargetHandler
3. Add the new change to PatchTargetHandler in line 780

     if (patchConfig.isAddNodeOnMissingTarget())
     {
       ObjectNode newNode = new ScimObjectNode(schemaAttribute);
       multiValued.add(newNode);
       matchingComplexNodes.add(new IndexNode(multiValued.size() - 1, newNode));
     }
    else if (patchConfig.isDoNotFailOnNoTarget())
     {
       return false;
     }
     else
     {
       throw new BadRequestException(String.format("Cannot '%s' value on path '%s' for no matching object was found",
                                                   patchOp,
                                                   path),
                                     null, ScimType.RFC7644.NO_TARGET);
     }

4. Return the change in class MsAzurePatchFilterWorkaround and exchange the method logic of fixPatchRequestOperation for this:

   @Override
   public PatchRequestOperation fixPatchRequestOperaton(ResourceType resourceType, PatchRequestOperation operation)
   {
   PatchOp patchOp = operation.getOp();
   final String path = operation.getPath().get();
   List<String> values = operation.getValues();
   
   AttributePathRoot attributePathRoot;
   SchemaAttribute schemaAttribute;
   
   if (resourceType.getAllSchemaExtensions().stream().anyMatch(schema -> schema.getNonNullId().equals(path)))
   {
     return operation;
   }
   else
   {
     attributePathRoot = RequestUtils.parsePatchPath(resourceType, path);
     schemaAttribute = attributePathRoot.getSchemaAttribute();
   }
   
   FilterNode childNode = attributePathRoot.getChild();
   if (!(childNode instanceof AttributeExpressionLeaf))
   {
     return operation;
   }
   
   String newPath;
   if (patchOp == PatchOp.ADD)
   {
     newPath = schemaAttribute.getFullResourceName();
   }
   else
   {
     newPath = path.replaceFirst(String.format("(.*?)\\.%s$", childNode.getSubAttributeName()), "$1");
   }
   // handle replace operations of this type for MsAzure always as add
   patchOp = PatchOp.ADD;
   
   ScimObjectNode value = new ScimObjectNode();
   value.set(attributePathRoot.getSubAttributeName(), new ScimTextNode(null, values.get(0)));
   
   AttributeExpressionLeaf attributeExpressionLeaf = (AttributeExpressionLeaf)childNode;
   value.set(attributeExpressionLeaf.getSchemaAttribute().getName(),
             new ScimTextNode(null, attributeExpressionLeaf.getValue()));
   
   return PatchRequestOperation.builder().op(patchOp).path(newPath).valueNode(value).build();
   }

Eventhough it is not supported for subattribute-filter-paths like emails[value eq "no-match"].value. I will need to add this later then.

[Captain-P-Goldfish]

Okay, I just noticed that I missed something with my solution. It turns on a new behaviour for all other requests which might be an unwanted behaviour for other than this specific Microsoft request... I am just desperately trying to keep the Mircrosoft workarounds out of the rest of the code... I am going to think about it until tomorrow.

[Captain-P-Goldfish]

I wasn't able to think of another solution that achieves the same and keeps the PatchTargetHandler clean of workarounds. Therefore I have merged the pull-request

[ondrejzivotsky]

I agree, I don't like it either, but I couldn't find a better solution. It would be better if Microsoft fixed the bugs, but we probably never see that :).

[ondrejzivotsky]

Are you planning to release a new version of the library?

[Captain-P-Goldfish]

yes I will release it next week. Cannot give you the exact date, but if nothing crashes my plan the latest date for the next release should be 29.06

[ondrejzivotsky]

yes I will release it next week. Cannot give you the exact date, but if nothing crashes my plan the latest date for the next release should be 29.06

🤞 😉

[Captain-P-Goldfish]

did not forget but I am waiting for #681