imulab / go-scim

Building blocks for servers implementing Simple Cloud Identity Management v2
MIT License
145 stars 56 forks source link

invalidValue: 'schemas' is required - when 'schemas' is present #57

Closed podung closed 4 years ago

podung commented 4 years ago

Hi, just trying to give this library a quick spin with Okta (syncing users and groups to mongodb). I just did the quick make docker compose and then started having Okta send requests. Creating users and groups seems to work, but a few of the patch requests are failing.

After successfully sending a patch operation to add members to the group, Okta then does a get on the group, followed by this patch:

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ],
    "Operations": [
        {
            "op": "replace",
            "value": {
                "id": "79fe65ca-65db-4840-85a9-12283543b2b8",
                "displayName": "Jeff.Scim.Test"
            }
        }
    ]
}

Its unclear to me why Okta thinks it needs to patch the group (b/c the name of the group has not changed). Regardless, I'm getting this confusing response:

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:Error"
    ],
    "status": 400,
    "scimType": "invalidValue",
    "detail": "invalidValue: 'schemas' is required"
}

But it looks like the request includes the "schemas" array and its populated with a value. Any insight into where this error is coming from and what its trying to tell me?

Thanks!

podung commented 4 years ago

This patch user request is also failing in the same way:

Request

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ],
    "Operations": [
        {
            "op": "replace",
            "value": {
                "password": "[PASSWORD HERE]"
            }
        }
    ]
}

Response

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:Error"
    ],
    "status": 400,
    "scimType": "invalidValue",
    "detail": "invalidValue: 'schemas' is required"
}
imulab commented 4 years ago

Hi @podung let me investigate this a bit further and get back to you.

imulab commented 4 years ago

Okay, I think I get the hang of it.

As of now, the replace operation when path is absent treats the value property as the whole representation of the resource. Under this behavior, any unspecified property is essentially deleted from the original resource. Since schemas is deleted, but is required, the validator essentially throws the error you saw.

I went back to review the SCIM spec definition of the replace behavior. Specifically

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.

If the target location specifies a complex attribute, a set of sub-attributes SHALL be specified in the "value" parameter, which replaces any existing values or adds where an attribute did not previously exist.  Sub-attributes that are not specified in the "value" parameter are left unchanged.

It seems the current behavior might not be the correct one.

imulab commented 4 years ago

Hi @podung, I checked in a potential fix for this issue on fix/57 branch.

There is now a test case covering the use case you described in pkg/v2/service/patch_test.go. Look for "patch to make a difference from root of the resource" sub test.

Please let me know if this fix solves the error you were seeing.

podung commented 4 years ago

Thanks @imulab ! I'll try to check this out tomorrow or Monday and report back. Really appreciate it!

imulab commented 4 years ago

Hi @podung , just following up, have you got a chance to try out the fix?

podung commented 4 years ago

Hey @imulab I finally got a chance to try this out! Yes, that corrected the issue. Really appreciate the follow up and the time you took to look at this issue!

imulab commented 4 years ago

Thanks for your feedback @podung ! The fix is now released in pkg/v2.1.2.