SCIM: Add support for soft delete#50533
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 64b9248...fa344a0.
|
|
Sorry for the big change, there are really 2 main commits: This commit contains all moving of files and separating user specific logic from the dealing with SCIM attributes logic. I wanted to do this because if/when we add group support we would need to do it and because manipulating the attributes code is already confusing enough that mixing in entity specific things is difficult. This commit contains the logic changes for the soft delete. |
vdavid
left a comment
There was a problem hiding this comment.
I've gone through it!
Wow, the first commit was a thick one!
I'm not saying that I went through that commit line by line, but I'm trusting you and our test coverage that it's fine.
I think your cleanup will make us very grateful to ourselves one day when we add group sync support! I didn't even realize that user-related logic was so coupled with general SCIM stuff, and the new architecture is great!
I agree that applySCIMUpdates is not ideal, but I have no better take on it now.
Overall, awesome work! Please ping me when you looked into my suggestions, and I'll do another quick review round!
Yeah the first one is a bit much 😞 but >80% of it is just moving code not changing it at least that is what I tell myself to feel better about it. |
When modifying users using PUT/PATCH via SCIM toggling the `active` property should have the effect of soft deleting and restoring the user. This code first updates the SCIM logic to remove any user specific entity logic from the generic logic that would apply to any scim entity such as how to manipulate attributes. The effect of this is that each possible type of change is separated, and runs a check on the attributes before/after the changes to determine if it has occurred and an action needs to be taken. After this change I replaced the existing hard delete code for `active` changes with a soft delete/reactivate.
When modifying users using PUT/PATCH via SCIM toggling the `active` property should have the effect of soft deleting and restoring the user. This code first updates the SCIM logic to remove any user specific entity logic from the generic logic that would apply to any scim entity such as how to manipulate attributes. The effect of this is that each possible type of change is separated, and runs a check on the attributes before/after the changes to determine if it has occurred and an action needs to be taken. After this change I replaced the existing hard delete code for `active` changes with a soft delete/reactivate.
When modifying users using PUT/PATCH via SCIM toggling the
activeproperty should have the effect of soft deleting and restoring the user.This code first updates the SCIM logic to remove any user specific entity logic from the generic logic that would apply to any scim entity such as how to manipulate attributes. The effect of this is that each possible type of change is separated, and runs a check on the attributes before/after the changes to determine if it has occurred and an action needs to be taken. After this change I replaced the existing hard delete code for
activechanges with a soft delete/reactivate.closes https://github.com/sourcegraph/sourcegraph/issues/49961
Test plan
Manual Test
SCIM POST (create user)
Set password
Sign in
SCIM GET
Verify user is listed and active
SCIM PATCH - active -> false
Verify user can not login
SCIM GET
Verify user listed with active = false
SCIM PATCH - active -> true
Verify User can login again (only using username)