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

cassandra-schema.cql

@@ -1005,6 +1005,30 @@ CREATE TABLE brig_test.team_invitation_info (
     AND read_repair = 'BLOCKING'
     AND speculative_retry = '99p';
 
+CREATE TABLE brig_test.team_scim_pending_user_email (
+    team uuid,
+    email text,
+    user uuid,
+    PRIMARY KEY ((team, email), user)
+) WITH CLUSTERING ORDER BY (user ASC)
+    AND additional_write_policy = '99p'
+    AND bloom_filter_fp_chance = 0.01
+    AND caching = {'keys': 'ALL', 'rows_per_partition': 'NONE'}
+    AND cdc = false
+    AND comment = ''
+    AND compaction = {'class': 'org.apache.cassandra.db.compaction.SizeTieredCompactionStrategy', 'max_threshold': '32', 'min_threshold': '4'}
+    AND compression = {'chunk_length_in_kb': '16', 'class': 'org.apache.cassandra.io.compress.LZ4Compressor'}
+    AND memtable = 'default'
+    AND crc_check_chance = 1.0
+    AND default_time_to_live = 0
+    AND extensions = {}
+    AND gc_grace_seconds = 864000
+    AND max_index_interval = 2048
+    AND memtable_flush_period_in_ms = 0
+    AND min_index_interval = 128
+    AND read_repair = 'BLOCKING'
+    AND speculative_retry = '99p';
+
 CREATE TABLE brig_test.unique_claims (
     value text PRIMARY KEY,
     claims set<uuid>

changelog.d/2-features/WPB-23177

@@ -0,0 +1 @@
+Manual team invitations now conflict when a matching pending SCIM invitation already exists for the same team and email address.

changelog.d/3-bug-fixes/WPB-23177

@@ -0,0 +1 @@
+Release a handle claimed after a SCIM invitation expired, before cleanup, preventing a subsequent team invitation from using that handle.

integration/test/API/BrigInternal.hs

@@ -76,6 +76,16 @@ getUsersId domain ids = do
   req <- baseRequest domain Brig Unversioned "/i/users"
   submit "GET" $ req & addQueryParams [("ids", intercalate "," ids)]
 
+getUsersIdIncludingPending :: (HasCallStack, MakesValue domain) => domain -> [String] -> App Response
+getUsersIdIncludingPending domain ids = do
+  req <- baseRequest domain Brig Unversioned "/i/users"
+  submit "GET" $
+    req
+      & addQueryParams
+        [ ("ids", intercalate "," ids),
+          ("includePendingInvitations", "true")
+        ]
+
 getUsersByEmail :: (HasCallStack, MakesValue domain) => domain -> [String] -> App Response
 getUsersByEmail domain emails = do
   req <- baseRequest domain Brig Unversioned "/i/users"

integration/test/Test/Spar.hs

@@ -43,6 +43,7 @@ import qualified SAML2.WebSSO.Test.MockResponse as SAML
 import qualified SAML2.WebSSO.Test.Util as SAML
 import qualified SAML2.WebSSO.XML as SAMLXML
 import SetupHelpers
+import Testlib.Assertions
 import Testlib.JSON
 import Testlib.PTest
 import Testlib.Prelude
@@ -52,6 +53,110 @@ import qualified Time.System as Hourglass
 ----------------------------------------------------------------------
 -- scim stuff
 
+testTeamInvitationWhenScimInvitationExpired :: (HasCallStack) => App ()
+testTeamInvitationWhenScimInvitationExpired = do
+  let settings =
+        def
+          { brigCfg =
+              -- timeout for both SCIM and team invitations
+              setField "optSettings.setTeamInvitationTimeout" (2 :: Int)
+                -- Controls when asynchronous cleanup removes expired SCIM pending accounts.
+                . setField "optSettings.setExpiredUserCleanupTimeout" (3600 :: Int)
+          }
+  withModifiedBackend settings $ \domain -> do
+    (owner, _tid, _) <- createTeam domain 1
+    token <- createScimToken owner def >>= getJSON 200 >>= (%. "token") >>= asString
+
+    -- Create a SCIM user and let its invitation expire. Cleanup is deliberately
+    -- delayed so the expired pending account still exists at this point.
+    email <- randomEmail
+    externalId <- randomExternalId
+    scimUser <- randomScimUserWithEmail externalId email
+    scid <- createScimUser domain token scimUser >>= getJSON 201 >>= (%. "id") >>= asString
+    handle <- scimUser %. "userName" >>= asString
+
+    -- assert that the SCIM handle is claimed
+    putHandle owner handle >>= assertStatus 409
+
+    -- Wait until the SCIM invitation has expired.
+    eventually $ getInvitationByEmail domain email >>= assertStatus 404
+
+    -- Create and accept a manual team invitation for the same email. This is
+    -- expected to succeed after the expired SCIM account has been cleaned up.
+    invitation <- postInvitation owner (def {email = Just email}) >>= getJSON 201
+    code <- getInvitationCode owner invitation >>= getJSON 200 >>= (%. "code") >>= asString
+    registerUserWith domain email code "Alice" >>= assertStatus 201
+    user <- getUsersByEmail domain [email] >>= getJSON 200 >>= asList >>= assertOne
+    manualUserId <- user %. "id" >>= asString
+
+    -- The handle previously held by the SCIM account is available again.
+    putHandle user handle >>= assertSuccess
+
+    manualUser <- getUsersId domain [manualUserId] >>= getJSON 200 >>= asList >>= assertOne
+    manualUser %. "id" `shouldMatch` manualUserId
+    manualUser %. "email" `shouldMatch` email
+    manualUser %. "handle" `shouldMatch` handle
+    manualUser %. "managed_by" `shouldMatch` "wire"
+    manualUser %. "status" `shouldMatch` "active"
+
+    -- The regular internal users API filters deleted records, so it cannot
+    -- distinguish a deleted SCIM account from an account that is not found.
+    shouldBeEmpty $ getUsersId domain [scid] >>= getJSON 200 >>= asList
+
+testTeamInvitationWhenScimInvitationPending :: (HasCallStack) => App ()
+testTeamInvitationWhenScimInvitationPending = do
+  (owner, _tid, _) <- createTeam OwnDomain 1
+  (otherOwner, _otherTid, _) <- createTeam OwnDomain 1
+  token <- createScimToken owner def >>= getJSON 200 >>= (%. "token") >>= asString
+
+  -- Create a SCIM user; this sends a SCIM invitation that remains pending.
+  email <- randomEmail
+  externalId <- randomExternalId
+  scimUser <- randomScimUserWithEmail externalId email
+  scid <- createScimUser OwnDomain token scimUser >>= getJSON 201 >>= (%. "id") >>= asString
+  handle <- scimUser %. "userName" >>= asString
+
+  -- The SCIM invitation is still pending. A second team invitation for the
+  -- same email and team must be rejected with a conflict.
+  postInvitation owner (def {email = Just email}) >>= assertStatus 409
+
+  -- The email must still be invit-able by a different team; otherwise a
+  -- pending SCIM invitation could be used for an email-registration DoS.
+  postInvitation otherOwner (def {email = Just email}) >>= assertStatus 201
+
+  users <- getUsersIdIncludingPending OwnDomain [scid] >>= getJSON 200 >>= asList
+  user <- assertOne users
+  user %. "email" `shouldMatch` email
+  user %. "handle" `shouldMatch` handle
+  user %. "managed_by" `shouldMatch` "scim"
+  user %. "status" `shouldMatch` "pending-invitation"
+
+testTeamInvitationWhenScimAccountExists :: (HasCallStack) => App ()
+testTeamInvitationWhenScimAccountExists = do
+  (owner, tid, _) <- createTeam OwnDomain 1
+  token <- createScimToken owner def >>= getJSON 200 >>= (%. "token") >>= asString
+
+  -- Create a SCIM user and accept the resulting SCIM invitation below.
+  email <- randomEmail
+  externalId <- randomExternalId
+  scimUser <- randomScimUserWithEmail externalId email
+  scid <- createScimUser OwnDomain token scimUser >>= getJSON 201 >>= (%. "id") >>= asString
+  handle <- scimUser %. "userName" >>= asString
+
+  -- Accept the SCIM invitation so the SCIM-managed account is active.
+  registerInvitedUser OwnDomain tid email
+
+  -- An active SCIM account already owns the email. A second team invitation
+  -- for the same email must therefore be rejected with a conflict.
+  postInvitation owner (def {email = Just email}) >>= assertStatus 409
+
+  users <- getUsersId OwnDomain [scid] >>= getJSON 200 >>= asList
+  user <- assertOne users
+  user %. "email" `shouldMatch` email
+  user %. "handle" `shouldMatch` handle
+  user %. "managed_by" `shouldMatch` "scim"
+  user %. "status" `shouldMatch` "active"
+
 testSparUserCreationInvitationTimeout :: (HasCallStack) => App ()
 testSparUserCreationInvitationTimeout = do
   (owner, tid, _) <- createTeam OwnDomain 1

libs/types-common/src/Data/Id.hs

@@ -41,6 +41,7 @@ module Data.Id
     parseIdFromText,
     idToText,
     idToString,
+    invitationIdToUserId,
     idObjectSchema,
     IdObject (..),
 
@@ -102,6 +103,10 @@ import System.Logger (ToBytes)
 import Test.QuickCheck
 import Test.QuickCheck.Instances ()
 
+-- | Pending invitation users reuse the invitation UUID as the user UUID.
+invitationIdToUserId :: InvitationId -> UserId
+invitationIdToUserId = Id . toUUID
+
 data IdTag
   = Asset
   | Conversation

libs/wire-subsystems/src/Wire/InvitationStore.hs

@@ -96,6 +96,9 @@ data InvitationStore :: Effect where
   LookupInvitation :: TeamId -> InvitationId -> InvitationStore m (Maybe StoredInvitation)
   LookupInvitationByCode :: InvitationCode -> InvitationStore m (Maybe StoredInvitation)
   LookupInvitationsByEmail :: EmailAddress -> InvitationStore m [StoredInvitation]
+  InsertPendingScimUser :: TeamId -> EmailAddress -> UserId -> InvitationStore m ()
+  LookupPendingScimUsers :: TeamId -> EmailAddress -> InvitationStore m [UserId]
+  DeletePendingScimUser :: TeamId -> EmailAddress -> UserId -> InvitationStore m ()
   -- | Range is page size, it defaults to 100
   LookupInvitationsPaginated :: Maybe (Range 1 500 Int32) -> TeamId -> Maybe InvitationId -> InvitationStore m (PaginatedResult [StoredInvitation])
   CountInvitations :: TeamId -> InvitationStore m Int64

libs/wire-subsystems/src/Wire/InvitationStore/Cassandra.hs

@@ -44,6 +44,9 @@ interpretInvitationStoreToCassandra casClient =
       InsertInvitation newInv timeout -> embed $ insertInvitationImpl newInv timeout
       LookupInvitation tid iid -> embed $ lookupInvitationImpl tid iid
       LookupInvitationsByEmail email -> embed $ lookupInvitationsByEmailImpl email
+      InsertPendingScimUser tid email uid -> embed $ insertPendingScimUserImpl tid email uid
+      LookupPendingScimUsers tid email -> embed $ lookupPendingScimUsersImpl tid email
+      DeletePendingScimUser tid email uid -> embed $ deletePendingScimUserImpl tid email uid
       LookupInvitationByCode code -> embed $ lookupInvitationByCodeImpl code
       LookupInvitationsPaginated mSize tid miid -> embed $ lookupInvitationsPaginatedImpl mSize tid miid
       CountInvitations tid -> embed $ countInvitationsImpl tid
@@ -152,6 +155,36 @@ lookupInvitationsByEmailImpl email = do
       SELECT team, role, id, created_at, created_by, email, name, code FROM team_invitation WHERE team = ? AND id = ?
       |]
 
+insertPendingScimUserImpl :: TeamId -> EmailAddress -> UserId -> Client ()
+insertPendingScimUserImpl team email uid =
+  retry x5 $ write cql (params LocalQuorum (team, email, uid))
+  where
+    cql :: PrepQuery W (TeamId, EmailAddress, UserId) ()
+    cql =
+      [sql|
+        INSERT INTO team_scim_pending_user_email (team, email, user) VALUES (?, ?, ?)
+      |]
+
+lookupPendingScimUsersImpl :: TeamId -> EmailAddress -> Client [UserId]
+lookupPendingScimUsersImpl team email =
+  map runIdentity <$> retry x1 (query cql (params LocalQuorum (team, email)))
+  where
+    cql :: PrepQuery R (TeamId, EmailAddress) (Identity UserId)
+    cql =
+      [sql|
+        SELECT user FROM team_scim_pending_user_email WHERE team = ? AND email = ?
+      |]
+
+deletePendingScimUserImpl :: TeamId -> EmailAddress -> UserId -> Client ()
+deletePendingScimUserImpl team email uid =
+  retry x5 $ write cql (params LocalQuorum (team, email, uid))
+  where
+    cql :: PrepQuery W (TeamId, EmailAddress, UserId) ()
+    cql =
+      [sql|
+        DELETE FROM team_scim_pending_user_email WHERE team = ? AND email = ? AND user = ?
+      |]
+
 lookupInvitationImpl :: TeamId -> InvitationId -> Client (Maybe StoredInvitation)
 lookupInvitationImpl tid iid =
   fmap asRecord

libs/wire-subsystems/src/Wire/TeamInvitationSubsystem/Interpreter.hs

@@ -54,11 +54,14 @@ import Wire.Sem.Now (Now)
 import Wire.Sem.Now qualified as Now
 import Wire.Sem.Random (Random)
 import Wire.Sem.Random qualified as Random
+import Wire.StoredUser (StoredUser (email, managedBy, status, teamId))
 import Wire.TeamInvitationSubsystem
 import Wire.TeamInvitationSubsystem.Error
 import Wire.TeamSubsystem
 import Wire.UserKeyStore
-import Wire.UserSubsystem (UserSubsystem, getLocalUserAccountByUserKey, getSelfProfile, isBlocked)
+import Wire.UserStore (UserStore)
+import Wire.UserStore qualified as UserStore
+import Wire.UserSubsystem (UserSubsystem, getAccountNoFilter, getLocalUserAccountByUserKey, getSelfProfile, isBlocked)
 
 data TeamInvitationSubsystemConfig = TeamInvitationSubsystemConfig
   { maxTeamSize :: Word32,
@@ -75,6 +78,7 @@ runTeamInvitationSubsystem ::
     Member UserSubsystem r,
     Member Random r,
     Member InvitationStore r,
+    Member UserStore r,
     Member Now r,
     Member EmailSubsystem r,
     Member EnterpriseLoginSubsystem r,
@@ -88,6 +92,12 @@ runTeamInvitationSubsystem cfg = interpret $ \case
   InternalCreateInvitation tid mExpectedInvId role mbInviterUid inviterEmail invRequest ->
     runInputConst cfg $ createInvitation' tid mExpectedInvId role mbInviterUid inviterEmail invRequest
 
+data ScimInvitationState
+  = ScimInvitationConflict
+  | ScimInvitationExpired UserId
+  | ScimInvitationStale UserId
+  deriving (Eq, Show)
+
 inviteUserImpl ::
   ( Member (Error TeamInvitationSubsystemError) r,
     Member GalleyAPIAccess r,
@@ -100,7 +110,8 @@ inviteUserImpl ::
     Member EmailSubsystem r,
     Member EnterpriseLoginSubsystem r,
     Member TeamSubsystem r,
-    Member UserKeyStore r
+    Member UserKeyStore r,
+    Member UserStore r
   ) =>
   Local UserId ->
   TeamId ->
@@ -111,6 +122,7 @@ inviteUserImpl luid tid request = do
 
   let inviteePerms = Teams.rolePermissions inviteeRole
   ensurePermissionToAddUser (tUnqualified luid) tid inviteePerms
+  reconcileScimInvitation request.inviteeEmail
 
   inviterEmail <-
     note TeamInvitationNoEmail =<< runMaybeT do
@@ -131,6 +143,63 @@ inviteUserImpl luid tid request = do
     loc inv =
       InvitationLocation $ "/teams/" <> toByteString' tid <> "/invitations/" <> toByteString' inv.invitationId
 
+    reconcileScimInvitation email = do
+      pendingScimUsers <- Store.lookupPendingScimUsers tid email
+      invitations <-
+        if null pendingScimUsers
+          then pure []
+          else Store.lookupInvitationsByEmail email
+      invitationStates <- traverse (classifyScimUser email invitations) pendingScimUsers
+
+      for_ invitationStates $ \case
+        ScimInvitationExpired uid -> cleanupExpiredScimUser email uid
+        ScimInvitationStale uid -> Store.deletePendingScimUser tid email uid
+        ScimInvitationConflict -> pure ()
+
+      when (ScimInvitationConflict `elem` invitationStates) $
+        throw TeamInvitationEmailTaken
+
+    classifyScimUser requestedEmail invitations uid = do
+      mStoredUser <- UserStore.getUser uid
+      pure $ case mStoredUser of
+        Nothing -> ScimInvitationStale uid
+        Just storedUser
+          | storedUser.teamId /= Just tid
+              || storedUser.email /= Just requestedEmail
+              || storedUser.managedBy /= Just ManagedByScim ->
+              ScimInvitationStale uid
+          | otherwise ->
+              case storedUser.status of
+                Just PendingInvitation ->
+                  if (not (any (invitationIsLive uid) invitations))
+                    then
+                      -- Only a matching pending SCIM account can be cleaned
+                      -- up when its invitation has expired.
+                      ScimInvitationExpired uid
+                    else
+                      -- The SCIM invitation is still usable, so the existing
+                      -- pending account must not be deleted or replaced.
+                      ScimInvitationConflict
+                _ ->
+                  -- An active SCIM account must continue to block a manual
+                  -- invitation, even if the index was not removed on activation.
+                  ScimInvitationConflict
+
+    invitationIsLive uid inv =
+      inv.teamId == tid
+        && invitationIdToUserId inv.invitationId == uid
+
+    cleanupExpiredScimUser requestedEmail uid = do
+      -- Delete the account synchronously so UserStore releases its handle before
+      -- the manual invitation is created. Keep the index entry if deletion fails.
+      mUser <- getAccountNoFilter (qualifyAs luid uid)
+      case mUser of
+        Nothing -> pure ()
+        Just user -> do
+          UserStore.deleteUser user
+          deleteKeyForUser uid (mkEmailKey requestedEmail)
+      Store.deletePendingScimUser tid requestedEmail uid
+
 createInvitation' ::
   ( Member GalleyAPIAccess r,
     Member UserSubsystem r,

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

@@ -1206,6 +1206,8 @@ acceptTeamInvitationImpl luid pw code = do
   unless added $ throw UserSubsystemTooManyTeamMembers
   updateUserTeam uid tid
   deleteInvitation inv.teamId inv.invitationId
+  for_ (userEmail . selfUser =<< mSelfProfile) $ \email ->
+    deletePendingScimUser tid email uid
   syncUserIndex uid
   generateUserEvent uid Nothing (teamUpdated uid tid)