Skip to content

WPB-23177 [fix] duplicate user accounts created after expired SCIM invitation and manual re-invite - #5400

Open
battermann wants to merge 22 commits into
developfrom
WPB-23177-duplicate-user-accounts-created-after-expired-scim-invitation-and-manual-re-invite
Open

WPB-23177 [fix] duplicate user accounts created after expired SCIM invitation and manual re-invite#5400
battermann wants to merge 22 commits into
developfrom
WPB-23177-duplicate-user-accounts-created-after-expired-scim-invitation-and-manual-re-invite

Conversation

@battermann

@battermann battermann commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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

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 Jul 30, 2026
@battermann
battermann marked this pull request as ready for review August 4, 2026 16:51
@battermann
battermann requested review from a team as code owners August 4, 2026 16:51
@battermann
battermann force-pushed the WPB-23177-duplicate-user-accounts-created-after-expired-scim-invitation-and-manual-re-invite branch from 4cf17b0 to eca3093 Compare August 4, 2026 16:59
@battermann
battermann requested a lite review from Copilot August 4, 2026 16:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a SCIM/manual invitation edge case where expired SCIM invitations could leave behind pending SCIM user records (and associated handle/email claims), causing manual re-invites to create duplicates or fail unexpectedly. The change introduces a Cassandra-backed index of pending SCIM users by (team, email) and uses it to reconcile/clean up expired pending SCIM accounts during manual invitation creation.

Changes:

  • Add team_scim_pending_user_email persistence + effect API to track pending SCIM users by team/email.
  • Reconcile pending SCIM invitations when creating a manual team invitation (reject conflicts; clean up expired/stale pending SCIM users).
  • Add unit + integration coverage for pending/expired SCIM invitation scenarios and ensure cleanup also occurs on user deletion/acceptance paths.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
services/brig/src/Brig/Team/API.hs Thread InvitationStore capability into SCIM invitation creation path.
services/brig/src/Brig/Schema/V93_AddScimPendingUserEmail.hs Add schema migration for team_scim_pending_user_email.
services/brig/src/Brig/Schema/Run.hs Register migration V93 in the Brig schema runner.
services/brig/src/Brig/InternalEvent/Process.hs Add InvitationStore constraint where needed for downstream cleanup/reconciliation.
services/brig/src/Brig/Data/User.hs Remove invitationIdToUserId from Brig-local module (moved to shared Data.Id).
services/brig/src/Brig/API/User.hs Maintain pending SCIM index on SCIM invite creation and user deletion.
services/brig/src/Brig/API/Public.hs Wire InvitationStore capability through public delete flows.
services/brig/src/Brig/API/Internal.hs Broaden StoredUser import and thread InvitationStore for delete flows.
services/brig/brig.cabal Add new schema module to Brig library build.
libs/wire-subsystems/test/unit/Wire/TeamInvitationSubsystem/InterpreterSpec.hs Add properties covering SCIM pending/expired/stale scenarios for manual invites.
libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserStore.hs Add delete hook support to observe deletions during reconciliation in tests.
libs/wire-subsystems/test/unit/Wire/MockInterpreters/InvitationStore.hs Extend in-memory InvitationStore interpreter with pending SCIM index operations.
libs/wire-subsystems/test/unit/Wire/MiniBackend.hs Add pending SCIM index state to mini-backend test harness.
libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs Remove pending SCIM index entry when a team invitation is accepted.
libs/wire-subsystems/src/Wire/TeamInvitationSubsystem/Interpreter.hs Reconcile pending SCIM users before creating manual invitations; cleanup expired/stale entries.
libs/wire-subsystems/src/Wire/InvitationStore/Cassandra.hs Implement Cassandra queries for pending SCIM index CRUD.
libs/wire-subsystems/src/Wire/InvitationStore.hs Extend InvitationStore effect with pending SCIM index operations.
libs/types-common/src/Data/Id.hs Move/export invitationIdToUserId to shared ID utilities.
integration/test/Test/Spar.hs Add integration tests for pending vs expired SCIM invitation behavior around manual invites/handles.
integration/test/API/BrigInternal.hs Add helper to query internal users endpoint including pending invitations.
changelog.d/3-bug-fixes/WPB-23177 Changelog entry for handle release / cleanup behavior.
changelog.d/2-features/WPB-23177 Changelog entry for conflict behavior when matching SCIM invite is pending.
cassandra-schema.cql Add table definition for brig_test.team_scim_pending_user_email.

Comment thread libs/wire-subsystems/test/unit/Wire/TeamInvitationSubsystem/InterpreterSpec.hs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@blackheaven blackheaven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, my LLM noticed:

 The changelog (changelog.d/3-bug-fixes/WPB-23177) claims a handle is released. But:
 - newStoredUserViaScim sets handle = Nothing (Brig/Data/User.hs:144), and
 - createUserInviteViaScim (Brig/API/User.hs:646-661) performs no updateHandle/handle-claim step.

 So the brig-side pending SCIM account has no handle. deleteUserImpl only frees a handle when userHandle user is populated (Wire/UserStore/Cassandra.hs:326-327):

 ```haskell
   deleteUserImpl user = do
     for_ (userHandle user) \h -> freeHandleImpl (userId user) h
     retry x5 $ write updateUserToTombstone ...

The handle is evidently set later via a separate Spar→brig updateHandle … UpdateOriginScim call (the pending-SCIM integration test asserts the SCIM user's handle == userName is queryable). Confirm that by the time
cleanupExpiredScimUser runs, the User returned by getAccountNoFilter actually carries the handle, so freeHandleImpl releases it. Otherwise the putHandle user handle assertion in testTeamInvitationWhenScimInvitationExpired passes
trivially (nothing was ever claimed on the brig record) and the bug-fix claim is unverified. Consider adding a negative control: assert putHandle fails for that handle while the expired SCIM account still exists.

Comment on lines +78 to +79
-- Wait until the SCIM invitation has expired.
liftIO $ threadDelay 2_100_000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It sounds specific and slow, can you use eventually-like here?

Just storedUser
| storedUser.teamId /= Just tid
|| storedUser.email /= Just requestedEmail
|| storedUser.managedBy /= Just ManagedByScim -> do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
|| storedUser.managedBy /= Just ManagedByScim -> do
|| storedUser.managedBy /= Just ManagedByScim ->

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.

4 participants