Captain-P-Goldfish / SCIM-SDK

a scim implementation as described in RFC7643 and RFC7644
https://github.com/Captain-P-Goldfish/SCIM/wiki
BSD 3-Clause "New" or "Revised" License
122 stars 38 forks source link

Patching Extension Attributes without an Operation Path property #193

Closed Kilemonn closed 3 years ago

Kilemonn commented 3 years ago

Hi Pascal,

I have identified an issue in version 1.11.1 where any extension property (for example anything in the enterprise user schema, or a custom schema) that is updated via PATCH and is supplied in the below format will not map correctly into the User object after the incoming patch object is merged with the existing object is completed.

For example, if I perform the following PATCH request on a User to update their employeeNumber attribute in the enterprise extension schema:

Incoming PATCH Request

{
    "Operations": [
        {
            "op": "replace",
            "value": {
                ...
                "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber": "1111"
            }
        }
    ],
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ]
}

From what I can see, in this specific scenario, the way which the "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber" is resolved in PatchResourceHandler.addResourceValues() seems to be incorrectly mapped as a "root" level property, instead of an "extension" property. After the incoming patch object is merged with the existing object. The User object in ResourceHandler<User>.updateResource(User updatedUser, Authorization authorization) looks like the following:

Current Response

{
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
        "employeeNumber": "2222" // Old employee ID
    },
    "employeeNumber": "1111", // New Employee ID
    "id": "1",
    "userName": "user name",
    ...
    "meta": {
        ...
    }
}

Expected Response

I would expect the User object would look like this instead:

{
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
        "employeeNumber": "1111" // Updated extension property
    },
    "id": "1",
    "userName": "user name",
    ...
    "meta": {
        ...
    }
}

Findings

I believe a fix could be put in around the method PatchResourceHandler.addResourceValues() specifically around line 77 to better determine whether the incoming key is an extension property. E.g. The key would be "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber", but the schema would be "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User" which does not match.

Furthermore, there maybe some changes required to update the key value before it is passed into the recursive call at line 87.

In my investigation this issue is not just limited to the User resource but is also effecting Group too. But I think a fix in this handler would resolve it for all.

Thank you very much for your time, taking a look at this issue. Please let me know if you need any further information or clarification.

Kyle

Captain-P-Goldfish commented 3 years ago

The beavhiour your are experiencing is correct. The patch request contains a problem. There are two types of patch requests:

  1. patch requests that contain the resource itself
  2. patch requests that contain single attributes with a path definition

your case is the first one and since the attribute is unknown to the users-schema definition it is simply ignored. The request should look like one of these

{
    "Operations": [
        {
            "op": "replace",
            "value": {
                ...
                "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
                    "employeeNumber": "1111"
                 }
            }
        }
    ],
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ]
}

or

```json
{
    "Operations": [
        {
            "op": "replace",
            "path": "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber",
            "value": "1111"
        }
    ],
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ]
}
Captain-P-Goldfish commented 3 years ago

is this still an issue?

Kilemonn commented 3 years ago

Hi Pascal,

Sorry for my late response.

Thank you so much for your clarification, I will review the mapping for our custom attributes from the SCIM client and take it from there.

Thanks again! I will close this ticket 😀

Kilemonn commented 3 years ago

Hi Pascal,

I hope you are doing well.

I was reviewing the mapping that I have configured and also reached out to Microsoft to confirm the PATCH payload that I was seeing, since the SCIM request above is coming from Azure AD. One of their product engineers has confirmed that the below payload format is as expected from their point of view and should be supported:

{
    "Operations": [
        {
            "op": "replace",
            "value": {
                ...
                "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber": "1111"
            }
        }
    ],
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ]
}

Have you seen this type of payload in any Azure AD testing you may have done? Is there a possibility you would support this format for extension attributes given Microsoft's response above?

Captain-P-Goldfish commented 3 years ago

Based on the scim-specification this is definetely a violation

Replace Operation

   The "replace" operation replaces the value at the target location
   specified by the "path".  The operation performs the following
   functions, depending on the target location specified by "path":

   o  If the "path" parameter is omitted, the target is assumed to be
      the resource itself.  In this case, the "value" attribute SHALL
      contain a list of one or more attributes that are to be replaced.
...

in all other cases the path attribute is required.

The Azure product has also some other violations implemented... I already added a workaround for patch-remove operations due to this. I will take a look into this issue if I find some time and see if its possible to add another workaround. But this is definetely an error on Azure side.

Captain-P-Goldfish commented 3 years ago

I added an explanation to the wiki of what exactly is supported here: https://github.com/Captain-P-Goldfish/SCIM-SDK/wiki/Support-for-MS-Azure-requests

Kilemonn commented 11 months ago

Hi @Captain-P-Goldfish

Again, an incredible job you have been doing with this project and I really appreciate all that you are doing to maintain and implement a project to such a high standard its awesome!

I'm just following up on this ticket to confirm the changes that it seems you have made since we last spoke and there have been some other tickets raised and resolved that discuss other aspects of this. We currently have a work around implemented but can see you have done some improvements that we would prefer to pick up instead of our work around.

You've provided documentation around https://github.com/Captain-P-Goldfish/SCIM-SDK/wiki/Support-for-MS-Azure-requests#rebuilding-of-addreplace-requests Can you confirm if this feature change requires an flag to enable? And which version is available from?

Thank you again for all your work!

Captain-P-Goldfish commented 11 months ago

Hi,

sorry for the late reply. The fix does work and is available since version 1.12.1

The workaround must not be activated explicitly since the check - if it must be executed or not - is just a simple additional comparison-check.

I would recommend to update to the newest version 1.21.1 though. It contains a lot of bug-fixes.

Kilemonn commented 10 months ago

Hi,

Thank you for confirming, sounds great! I will definitely upgrade to the latest version too.

Thank you again!