yaal-coop / scim2-models

SCIM resources serialization and validation with Pydantic
https://scim2-models.readthedocs.io
Apache License 2.0
9 stars 2 forks source link

PatchOp lowercases all JSON keys on a value #70

Open verbitan opened 3 weeks ago

verbitan commented 3 weeks ago

[RFC7644] (Section 3.5.2.3) lists the following example of a valid urn:ietf:params:scim:api:messages:2.0:PatchOp object,

   The following example shows how to change a User's entire "work"
   address, using a "valuePath" filter.  Note that by setting "primary"
   to "true", the service provider will reset "primary" to "false" for
   any other existing values of "addresses".

   PATCH /Users/2819c223-7f76-453a-919d-413861904646
   Host: example.com
   Accept: application/scim+json
   Content-Type: application/scim+json
   Authorization: Bearer h480djs93hd8
   If-Match: W/"a330bc54f0671c9"

   {
     "schemas":
       ["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
     "Operations": [{
       "op":"replace",
       "path":"addresses[type eq \"work\"]",
       "value":
       {
         "type": "work",
         "streetAddress": "911 Universal City Plaza",
         "locality": "Hollywood",
         "region": "CA",
         "postalCode": "91608",
         "country": "US",
         "formatted":
   "911 Universal City Plaza\nHollywood, CA 91608 US",
         "primary": true
       }
     }]
   }

PatchOp.model_validate() validates this request as expected, but it lowercases all the keys of the JSON object, meaning they no longer match the schema. I don't believe this to be correct.

[RFC7644] (Section 3.10) does make a statement that All facets (URN, attribute, and sub-attribute name) of the fully encoded attribute name are case insensitive, and the keys of the JSON object are attributes, which is where I suspect the reasoning of lowercasing all the keys originated, but it's at odds with all other models provided by this library (e.g. User dumps userName).

PatchOp.model_validate(
    {
        "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
        "Operations": [
            {
                "op": "replace",
                "path": 'addresses[type eq "work"]',
                "value": {
                    "type": "work",
                    "streetAddress": "911 Universal City Plaza",
                    "locality": "Hollywood",
                    "region": "CA",
                    "postalCode": "91608",
                    "country": "US",
                    "formatted": "911 Universal City Plaza\nHollywood, CA 91608 US",
                    "primary": True,
                },
            }
        ],
    }
).model_dump()
{
  "schemas":[
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations":[
    {
      "op":"replace",
      "path":"addresses[type eq \"work\"]",
      "value":{
        "type":"work",
        "streetaddress":"911 Universal City Plaza",
        "locality":"Hollywood",
        "region":"CA",
        "postalcode":"91608",
        "country":"US",
        "formatted":"911 Universal City Plaza\nHollywood, CA 91608 US",
        "primary":true
      }
    }
  ]
}
azmeuk commented 3 weeks ago

Thank you for your report. Indeed, I would indeed expect the case to be kept during the export.

This might be due to BaseModel.normalize_attribute_name lowering everything, and PatchOp.value being of type Any it does not get reformatted on exports like other Resource do with that to_camel serializer..

However, I am not really sure how to fix this. There might be needed to add type parameterss to PatchOp so it can guess how to correctly case attributes (and also, reject bad attributes by the way?). Something like PatchOp[User] :thinking:

verbitan commented 3 weeks ago

Another interesting example of this is the following patch request from https://learn.microsoft.com/en-us/entra/identity/app-provisioning/application-provisioning-config-problem-scim-compatibility. In this example, the value keys are lowercased and the . is removed, so name.givenName becomes namegivenname.

PatchOp.model_validate(
    {
        "schemas": [
            "urn:ietf:params:scim:api:messages:2.0:PatchOp"
        ],
        "Operations": [
            {
                "op": "replace",
                "value": {
                    "displayName": "Bjfe",
                    "name.givenName": "Kkom",
                    "name.familyName": "Unua",
                    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber": "Aklq"
                }
            }
        ]
    }
).model_dump()
{
  "schemas":[
    "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations":[
    {
      "op":"replace",
      "value":{
        "displayname":"Bjfe",
        "namegivenname":"Kkom",
        "namefamilyname":"Unua",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:user:employeenumber":"Aklq"
      }
    }
  ]
}

I think a type parameter to PatchOp is a nice solution, as you can then validate the attributes as part of the model. Though it'd have to handle filters, which complicates things somewhat.

{
  "schemas": [
      "urn:ietf:params:scim:api:messages:2.0:PatchOp"
  ],
  "Operations": [
      {
          "op": "replace",
          "path": "emails[type eq \"work\"].value",
          "value": "TestMhvaes@test.microsoft.com"
      }
  ]
}