search

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
121 stars 38 forks source link

Issue with concurrent Patch requests #646

Open ekos2001 opened 4 months ago

ekos2001 commented 4 months ago

Hello, We have some problems with the Patch implementation provided by the library when adding members to a group. We have an Azure with 1000 or more members and during initial sync for every member, Azure sends separate Patch request. And these requests are concurrent, in my tests, I'm getting up to 5 requests at a time.

Our logic of the Update method in the GroupHandler is similar to what you have here

In some cases, it may happen that in the GroupHandler#Update method, the data in the database is changed by another thread (or another server instance) and after comparison, we delete the member that was just added in the concurrent thread. See the attached diagram, it may help you better understand the problem.

Do you have any recommendations on how to resolve this?

We were thinking that one of the options could be to synchronize all Patch calls for the same group, but in case of multiple servers it can be not that easy to implement. Another option would be to pass the original group to the GroupHandler#Update (or another new handler method) together with the patched object, so we can compare it to know the diff without reading the data from the DB a second time in the GroupHandler#Update.

Thank you

image image

Captain-P-Goldfish commented 4 months ago

I had the same problem and solved it by implementing the patch-operations on atomic level. Patch was completely reimplemented with version 1.22.0 in order to bypass this problem: https://github.com/Captain-P-Goldfish/SCIM-SDK/wiki/Patching-resources#catch-patch-operations-and-handle-them-manually-since-1220.

ekos2001 commented 4 months ago

Nice! That is so good you added such a possibility. Do you have any examples of how you would override the DefaultPatchOperationHandler and implement handleOperation?

Thank you

Captain-P-Goldfish commented 4 months ago

Only my proprietary solution from https://scim-for-keycloak.de. So nothing for public view at the moment I am afraid...

EDIT: But I want to say with this that I already proved it to work :-)

ekos2001 commented 4 months ago

Got it :)

ekos2001 commented 3 months ago

Can I get some ideas on how to handle the REMOVE operation? It looks like we need to parse the attributePath?

image

Captain-P-Goldfish commented 3 months ago

sorry for the late reply. this is a tricky thing. I actually implemented a FilterResolver for this using the RequestUtils.parsePatchPath(...) method. There is a similar implementation that can act as base for your filter implementation in FilterResourceResolver.filterResources(...) I have implemented this in the SCIM for Keycloak project but I noticed that my implementation might be extended to a generic implementation that could become OpenSource and should be usable in most cases. But at the moment I do not have the time to generify the implementation.

ekos2001 commented 3 months ago

Would it be possible for you to work on this sometime in the next week or two? Thank you

Captain-P-Goldfish commented 3 months ago

sorry, but my plate is completely full and this is nothing that is appropriately done in a few days. I will not be able to provide an implementation of this size in the next time.