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

Patch "add" operation with MS Azure patch-filter-expression workaround add new record even if matching exists #680

Closed hydraserg closed 4 months ago

hydraserg commented 4 months ago

Hi

I want to report an issue related to #668 that was present even before #667 changes. I was hoping it would be fixed in it, but issue still exists. MsAzurePatchFilterWorkaround breaks "add" patching operation for multiple subattributes in multivalued complex attribute which is used by MS Azure.

Having user resource:

{
  "schemas": [ "urn:ietf:params:scim:schemas:core:2.0:User"],
  "userName": "erika@mustermann.de"
}

With MS Azure patch request containing operations for updating multiple subattributes:

{
  "Operations": [
    {
      "op": "add",
      "path": "addresses[type eq \"work\"].formatted",
      "value": "USZLTUSJXBYV"
    },
    {
      "op": "add",
      "path": "addresses[type eq \"work\"].locality",
      "value": "OSGODHEVDCVD"
    },
    {
      "op": "add",
      "path": "addresses[type eq \"work\"].region",
      "value": "YMVMOEMXAYGD"
    },
      ],
  "schemas": ["urn:ietf:params:scim:api:messages:2.0:PatchOp"]
}

resulting user resource will look like this:

{
  "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"],
  "userName": "erika@mustermann.de",
  "addresses": [{
      "formatted": "USZLTUSJXBYV",
      "type": "work"
    }, {
      "locality": "OSGODHEVDCVD",
      "type": "work"
    }, {
      "region": "YMVMOEMXAYGD",
      "type": "work"
    }
  ]
}

but result should be with single address record:

{
  "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"],
  "userName": "erika@mustermann.de",
  "addresses": [{
      "formatted": "USZLTUSJXBYV",
      "type": "work",
      "locality": "OSGODHEVDCVD",
      "region": "YMVMOEMXAYGD"
    }
  ]
}

I like the changes that were done in #668 to change behavior in PatchTargetHandler using PatchConfig flag, as i don't see a good way to implement Azure workaround (for adding subattribute from 'eq' filter) using PatchWorkaround class. With those changes there is really no need for MsAzurePatchFilterWorkaround workaround, because default code is already working as Azure expects for both add & replace operations. So I would suggest to remove MsAzurePatchFilterWorkaround, but keep PatchConfig flag and process "add" and "replace" operations same way in PatchTargetHandler.

I'll create PR with unit-tests that cover this use case and possible solution.

Captain-P-Goldfish commented 4 months ago

Do not remove the workaround. I still have an email with a description that lets met think that there are still usecases in which the workaround might be necessary.

Captain-P-Goldfish commented 4 months ago

And please do not add more MsAzure related code into the PatchTargetHandler. I am already unhappy enough with the latest change that did require this.

hydraserg commented 4 months ago

created PR #681 with unit-test and possible fix.

@Captain-P-Goldfish yes sure, i haven't removed workaround just disabled it in PR for visibility of possible fix changes in PatchTargetHandler are pretty small

hydraserg commented 4 months ago

@Captain-P-Goldfish Thank you very much for fix and quick release !