The PATCH endpoint’s operation string validation is case sensitive

[mdilts]

A PATCH call using:

[{"op": “Replace", "value": { "active": false }}]

… is rejected as an invalid PATCH call. Changing ‘Replace’ to ‘replace’ allows the call to succeed.

To repro:

Curl -X PATCH 'http://dev:api_key@localhost:3000/scim/v2/Users/819216' -d '{"schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "Operations": [{"op": “Replace", "value": { "active": false }}]}' -H 'Content-Type: application/scim+json’

[rreinhardt9]

Oof, great point! This would have been introduce in the stricter checking I implemented for the patch endpoint in #29

I'm not as familiar with the scim protocol, do we know if it's in the protocol to accept both? I can look that up as well if we don't know... @wernull is that something you are familiar with?

[rreinhardt9]

Hmm, from what I can tell in the standards only the lower case values should be accepted: https://tools.ietf.org/html/rfc7644#section-3.5.2

The body of an HTTP PATCH request MUST contain the attribute "Operations", whose value is an array of one or more PATCH operations. Each PATCH operation object MUST have exactly one "op" member, whose value indicates the operation to perform and MAY be one of "add", "remove", or "replace".

So maybe this is the expected behavior? If they are passing a string with an uppercase first letter, then we will reject that request as not supported?

Iike I mentioned, I'm very new to understanding the scim spec; so I could be misreading the intention there.

[mdilts]

The downside is if ops are sent in such as 'Replace', the error is pretty generic. I suggest leaving it as-is for the time being, unless it is very simple/low risk to require lower-case ops, and providing a useful/direct error message if that particular validation fails.

[rreinhardt9]

That's true, they current just receive the "unsupported patch request" response. It looks like PATCH endpoint is kind of special in the gem currently because it's not really built out to specification yet and only accepts the very specific provisioning/ de-provisioning of a user request.

I sense that like you said returning that exception isn't very helpful or semantic.

Looking at the specifications, I think in this case we would actually want to return a 400 http response code, a scimType of invalidValue, and maybe an optional human readable message along with that.

This would be different behavior than was allowed by the library before though; before the change in #29 it was technically possible to pass any casing (or for that matter, any name at all for the Operation since it wasn't really checked). Since it's a "breaking change" it might be an argument for accepting either like you said and adding better validation later... but another part of me feels bad writing in something that will explicitly allow a none-spec value for that won't be supported long term. 🤔 I'm leaning towards accepting either right now but I could be swayed either way at the moment.

[rreinhardt9]

If we were to go the way of accepting either, we could change this location

https://github.com/lessonly/scim_rails/blob/0b5c207fca9aca403d1b3a9be2fd4b1645d41aa1/app/controllers/scim_rails/scim_users_controller.rb#L142-L146

To be like

 def valid_patch_operation?(operation) 
   operation["op"].casecmp?("replace") && 
     operation["value"] && 
     operation["value"]["active"] 
 end