Azure AD patch add compatibility

[davidmc24]

Thank you for the library. As I dig deeper, I can see how much effort has gone into this project.

What I'm seeing looks similar to #267, but doesn't appear to be working.

I've got an application using scim-for-keycloak kc-18-b2.

When I run the application and use the Microsoft Azure AD SCIM Validator to attempt to verify compatibility, most user interactions are passing. However, "Patch User - Add Attributes" is not.

I'm running it with the following attributes configured (slimmed down to avoid noise about other attributes that are less important):

• userName
• active
• emails[type eq "work"].value
• name.givenName
• name.familyName

It says that it is failing because:

• The value of name.givenName is Missing from the fetched Resource
• The value of name.familyName is Missing from the fetched Resource
• The value of emails[type eq "work"].value is Missing from the fetched Resource

Here's the information it gives on the requests sent and the responses received. The only thing I've censored is the host/port (replaced with MYTESTHOST).

Initial Resource Creation Request
POST http://MYTESTHOST:5086/identity/scim/v2/Users 1.1
Host: MYTESTHOST
Content-Type: application/scim+json; charset=utf-8
{
  "schemas": [
    "urn:ietf:params:scim:schemas:core:2.0:User"
  ],
  "userName": "[email protected]"
}
HTTP Request
PATCH http://MYTESTHOST:5086/identity/scim/v2/Users/8c57566f-2eb3-4b19-bef4-6177133b1baf 1.1
Host: MYTESTHOST
Content-Type: application/scim+json; charset=utf-8
{
  "Operations": [
    {
      "op": "add",
      "path": "emails[type eq \"work\"].value",
      "value": "[email protected]"
    },
    {
      "op": "add",
      "value": {
        "active": true,
        "name.givenName": "Terrence",
        "name.familyName": "William"
      }
    }
  ],
  "schemas": [
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ]
}
Response Received
Response Status: 200 OK

Response Headers
Referrer-Policy: no-referrer
Strict-Transport-Security: max-age=31536000; includeSubDomains
Location: http://localhost:5086/identity/scim/v2/Users/8c57566f-2eb3-4b19-bef4-6177133b1baf
Cache-Control: no-store, must-revalidate, no-cache
Pragma: no-cache
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
Content-Security-Policy: report-uri /enterprise-app/csp-report; default-src 'self'; img-src 'self' https://secure.gravatar.com data: https://secure.gravatar.com; style-src 'self' 'unsafe-inline' https://secure.gravatar.com; font-src 'self' https://secure.gravatar.com; base-uri 'self'; object-src 'none'; script-src 'self' https://secure.gravatar.com 'nonce-ufrrmzle5mbjcxdspnro7xigia';
X-XSS-Protection: 1; mode=block
Content-Type: application/scim+json
Content-Length: 359
Expires: 0
Response Body
{
  "active": true,
  "id": "8c57566f-2eb3-4b19-bef4-6177133b1baf",
  "meta": {
    "resourceType": "User",
    "created": "2023-02-02T21:23:51.351Z",
    "lastModified": "2023-02-02T21:23:51.588Z",
    "location": "http://localhost:5086/identity/scim/v2/Users/8c57566f-2eb3-4b19-bef4-6177133b1baf"
  },
  "schemas": [
    "urn:ietf:params:scim:schemas:core:2.0:User"
  ],
  "userName": "[email protected]"
}

In my other automated tests, I can verify that patch add operations are working in general; it's just the particular (probably non-standards-compliant) form that the SCIM Validator is using that isn't working.

[Captain-P-Goldfish]

Hi,

1. the Azure AD workaround was added for replace-operations only. There is no good reason for that and it could also be activated for add-operations.
2. The path-based expression emails[type eq \"work\"].value is a problem though. The specification does not define clearly how PATCH should be handled in specific situations and this is one of those. How the SCIM-SDK behaves in this case was described here: https://github.com/Captain-P-Goldfish/SCIM-SDK/wiki/Patching-resources#add-replace-5.
   To me it seems that Azure expects the email to be created even if the filter-expression has no valid target. Basically the whole request should actually fail with a 400 status code de.captaingoldfish.scim.sdk.common.exceptions.BadRequestException: No target found for path-filter 'emails[type eq \"work\"].value' since there is no emails-attribute to begin with within the initial resource.
   What kind of behaviour is MS Azure expecting here? Should a email be created with type "work"? That does not seem right to me.

[davidmc24]

Thanks for the prompt reply!

I think your reasoning in point 1 makes sense. Adding support for add-operations seems like it would fix that aspect of the issue, is consistent with the direction of the previous workaround, and doesn't appear to have any downside.

As for point 2, I suspect that you're right in thinking that it expects that an email of type work is created. I can't think of any other explanation that would make sense, given that:

• They have a similar "Patch User - Replace Attributes" scenario that creates all the supported attributes in the initial request before replacing them in the PATCH, meaning that not creating the attributes in the initial request is likely intentional.
• The error messages indicate that they were checking for a value emails[type eq "work"].value to exist in the final response.

I think the intention is that

    {
      "op": "add",
      "path": "emails[type eq \"work\"].value",
      "value": "[email protected]"
    }

should be interpreted as

    {
      "op": "add",
      "path": "emails",
      "value": {
        "type": "work",
        "value": "[email protected]"
      }
    }

Re-reading https://www.rfc-editor.org/rfc/rfc7644.html#section-3.5.2.1, I think that I agree that this particular case is unspecified. It doesn't seem to talk about non-matching paths at all in the context of patching. The closest that appears to be relevant is:

• "If the target location does not exist, the attribute and value are added."
• "If the target location specifies an attribute that does not exist (has no value), the attribute is added with the new value."

In this case, the target location is a value path, where both the attribute and the value do not exist. The capability to add attributes based on arbitrary valFilters seems like a lot of complexity to expect without calling out explicitly in the spec, though, if that was the intent. As for what you support, I could see it being reasonable to not support this particular style of call, but expect that it will continue causing pain for people trying to use Azure AD if that's the decision. If we were interested in pursuing an Azure AD workaround for this, it might be sufficient to support only valFilters with a single "eq" clause, decreasing the complexity of potential operation re-writes needed.

For example, here are the attributes of this type that the SCIM Validator includes in their default schema:

• emails[type eq "work"].value
• emails[type eq "work"].primary
• addresses[type eq "work"].formatted
• addresses[type eq "work"].streetAddress
• addresses[type eq "work"].locality
• addresses[type eq "work"].region
• addresses[type eq "work"].postalCode
• addresses[type eq "work"].primary
• addresses[type eq "work"].country
• phoneNumbers[type eq "work"].value
• phoneNumbers[type eq "work"].primary
• phoneNumbers[type eq "mobile"].value
• phoneNumbers[type eq "fax"].value
• roles[primary eq "True"].display
• roles[primary eq "True"].value
• roles[primary eq "True"].type

[Captain-P-Goldfish]

I do not consider what I said a fix decision. I just need valid arguments and you presented a very good reason :-)
So now I simply need to think of a solution to fix this problem.
And I wonder if I should change the current patch behaviour or if I should add an additional configuration option to acivate the behaviour needed for MS Azure?

[davidmc24]

Of course. Thank you for the clarification.

[Captain-P-Goldfish]

Okay I think I was able to fix this problem.
I decided to add a configuration option to activate the behaviour for patch-filter expressions because I consider the behaviour simply wrong due to the following reasons:

1. The patch-filter nothing more than a pointer that is used to target specific objects within a multivalued-complex type
2. The expected behaviour from ms-azure cannot be used on other comparators than "eq". If the comparator "co" or any other comparator is used there is no valid result that could be created

Are you able to verify that the solution is working for you?

EDIT:
sorry forgot to tell you how to enable the workaround:

serviceProvider.getPatchConfig().setMsAzureFilterWorkaroundActive(true);

[davidmc24]

I've updated my test environment to use the new logic, and it appears to be working nicely.

I didn't test the patch config setting you put in place, as I'm using it through scim-for-keycloak which doesn't appear to expose the config yet (or at least I don't know how yet). Here's the process I used:

1. Pull the latest changes from master into my SCIM-SDK checkout
2. Locally update PatchConfig to always return true from isMsAzureFilterWorkaroundActive and run mvn install
3. Locally update my scim-for-keycloak checkout to set version.scim.sdk to 1.16.0-SNAPSHOT and run mvn install
4. Update my application to use the locally built scim-for-keycloak-kc-19-b1-SNAPSHOT.jar
5. Re-run the testing procedure

I think this adjustment is enough to fully satisfy my needs, once it's available from a released scim-for-keycloak with a way to enable it.

Thanks for the rapid turnaround.

[Captain-P-Goldfish]

great. That it works.
I've got a new complete new version of scim-for-keycloak already in development. But this will consume some more time. All options will be configurable then.