elimity-com / scim

Golang Implementation of the SCIM v2 Specification
MIT License
183 stars 55 forks source link

Cannot PATCH complex attribute #113

Closed mokiat closed 3 years ago

mokiat commented 3 years ago

If there is a Complex attribute that has multiple sub-attributes that are required, it is not possible to modify just one of those fields with a PATCH operation.

For example, let's consider a User schema where givenName and familyName are required. Then the following queries fail:

{
  "op": "replace",
  "path": "name.givenName",
  "value": "John"
}
{
  "op": "replace",
  "path": "name",
  "value": {
    "givenName": "John"
  }
}

This is due to the following piece of code: https://github.com/elimity-com/scim/blob/7f9449eb6fe0b544af7a2e537c9bd30f9182c3ca/schema/core.go#L260-L279

The found variable is just used to check duplicates but does not handle when a field is missing. This might be correct for a PUT request, where you would expect both sub-attributes to be present, but for PATCH the specification states that it is ok to just modify one of the fields and the other should be left as is.

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. https://tools.ietf.org/html/rfc7644#section-3.5.2.3

Changing the code to the following solves the problem and both operations pass the validation:

if found {
    attr, scimErr := sub.validate(hit)
    if scimErr != nil {
        return nil, scimErr
    }
    attributes[sub.name] = attr
}

However this probably breaks the validation for PUT, so likely a better approach is needed.

jdeflander commented 3 years ago

Thank you for another comprehensive issue description! Both patch requests you listed should be valid, indeed. Maybe we can look into this sometime next week @di-wu?

mokiat commented 3 years ago

Thank you for fixing this. I will give it a try these days.