Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WPB-6577 Fix user creation conflict in SCIM #3914

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Feb 29, 2024

For a description of the manual test approach, see comments of https://wearezeta.atlassian.net/browse/WPB-6577

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Feb 29, 2024
@battermann battermann changed the title brig error handler WPB-6577 Fix user creation conflict in SCIM Mar 1, 2024
@battermann battermann force-pushed the WPB-6577-on-prem-duplicate-user-creation-and-conflict-error-in-scim-provisioning-process branch from 1aafc87 to 48619a8 Compare March 6, 2024 11:50
@battermann battermann marked this pull request as ready for review March 6, 2024 13:01
@battermann battermann force-pushed the WPB-6577-on-prem-duplicate-user-creation-and-conflict-error-in-scim-provisioning-process branch from c4a5470 to d6fe6fd Compare March 6, 2024 13:03
@battermann battermann requested a review from fisx March 6, 2024 13:03
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! i have one actual change request, otherwise looks good! let me know if you want to discuss this.

@@ -45,15 +46,17 @@ propsForInterpreter interpreter extract lower = do
prop "insert/lookup" $ prop_insertLookup (Just $ show . void . extract) lower
prop "insert/insert" $ prop_insertInsert (Just $ show . void . extract) lower

-- FUTUREWORK: Add prop tests for missing operations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you given this a try? if not i can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can go ahead, but maybe better in a separate PR.

-- something went wrong while storing the user in brig
-- we can try clean up now, but if brig is down, we can't do much
-- maybe retrying the user creation in brig is also an option?
-- after clean up we rethrow the error so the handler returns the correct failure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about not rethrowing, but looking up the status again once or twice? that'd make for swifter cleanup, especially when the batch job only runs once a day.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only rethrows if deleteScimUser fails. AFAICT the deletion is synchronous.

@battermann battermann merged commit 3577ea3 into develop Mar 8, 2024
8 checks passed
@battermann battermann deleted the WPB-6577-on-prem-duplicate-user-creation-and-conflict-error-in-scim-provisioning-process branch March 8, 2024 07:42
@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: unplanned Any work item that isn’t part of the product or technical roadmap. ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants