Skip to content

[WPB-22154] fix: move user between SCIM tokens#4887

Merged
fisx merged 10 commits intodevelopfrom
WPB-22154-fix-move-user-between-scim-tokens
Dec 11, 2025
Merged

[WPB-22154] fix: move user between SCIM tokens#4887
fisx merged 10 commits intodevelopfrom
WPB-22154-fix-move-user-between-scim-tokens

Conversation

@fisx
Copy link
Contributor

@fisx fisx commented Dec 3, 2025

https://wearezeta.atlassian.net/browse/WPB-22154

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 Dec 3, 2025
@fisx fisx changed the title [Wpb 22154] fix: move user between SCIM tokens [WPB-22154] fix: move user between SCIM tokens Dec 4, 2025
@fisx fisx force-pushed the WPB-22154-fix-move-user-between-scim-tokens branch 3 times, most recently from a7ca267 to b1bf345 Compare December 9, 2025 09:24
@fisx fisx force-pushed the WPB-22154-fix-move-user-between-scim-tokens branch from b1bf345 to 9c95b9a Compare December 9, 2025 09:57
@fisx fisx marked this pull request as ready for review December 9, 2025 11:17
@fisx fisx requested review from a team as code owners December 9, 2025 11:17
Copy link
Contributor Author

@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.

two minor observations

Right veid -> lift $ do
for_ (justThere veid.validScimIdAuthInfo) (SAMLUserStore.delete uid)
ScimExternalIdStore.delete stiTeam veid.validScimIdExternal
lift $ ScimUserTimesStore.delete uid -- TODO: is this removing the record owned by caller of this end-point (ie., team owner), or correct one?!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is!

Comment on lines 1331 to 1330
mbEmail =
(emailIdentity =<< memberWithSSO.userIdentity)
<|> memberWithSSO.userEmailUnvalidated
externalId = either error id $ veidToText =<< Intra.newVeidFromBrigUser memberWithSSO (Just idpIssuer) mbEmail
externalId = either error id $ veidToText =<< Intra.newVeidFromBrigUser memberWithSSO (Just idpIssuer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here the semantics was actually correct, just really convoluted. now this what newVeidFromBrigUser always does.

@fisx fisx requested a review from eyeinsky December 10, 2025 14:33
Copy link
Contributor

@eyeinsky eyeinsky left a comment

Choose a reason for hiding this comment

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

We reviewed this together yesterday with @fisx, looks good to me!

@fisx fisx merged commit e8a28dc into develop Dec 11, 2025
10 checks passed
@fisx fisx deleted the WPB-22154-fix-move-user-between-scim-tokens branch December 11, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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