diff --git a/changelog.d/3-bug-fixes/WPB-18929 b/changelog.d/3-bug-fixes/WPB-18929 new file mode 100644 index 00000000000..29da72e51fd --- /dev/null +++ b/changelog.d/3-bug-fixes/WPB-18929 @@ -0,0 +1,3 @@ +Revoking a pending SCIM invitation now removes the associated Brig account and +Spar SCIM metadata synchronously, allowing the same SCIM user to be invited +again. diff --git a/integration/test/API/Brig.hs b/integration/test/API/Brig.hs index 480bb781a15..3b115345841 100644 --- a/integration/test/API/Brig.hs +++ b/integration/test/API/Brig.hs @@ -990,6 +990,11 @@ getInvitationByCode user code = do req <- baseRequest user Brig Versioned $ joinHttpPath ["teams", "invitations", "info"] submit "GET" (req & addQueryParams [("code", code)]) +deleteTeamInvitation :: (HasCallStack, MakesValue user) => user -> String -> String -> App Response +deleteTeamInvitation user tid iid = do + req <- baseRequest user Brig Versioned (joinHttpPath ["teams", tid, "invitations", iid]) + submit "DELETE" req + passwordReset :: (HasCallStack, MakesValue domain) => domain -> String -> App Response passwordReset domain email = do req <- baseRequest domain Brig Versioned "password-reset" diff --git a/integration/test/Test/Spar.hs b/integration/test/Test/Spar.hs index dbdaf7eb0a0..d2a2f4770c5 100644 --- a/integration/test/Test/Spar.hs +++ b/integration/test/Test/Spar.hs @@ -131,6 +131,36 @@ testTeamInvitationWhenScimInvitationPending = do user %. "managed_by" `shouldMatch` "scim" user %. "status" `shouldMatch` "pending-invitation" +testScimReinviteAfterRevoke :: (HasCallStack) => App () +testScimReinviteAfterRevoke = do + let settings = + def + { brigCfg = + -- 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 + + 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 + + -- cancel the invitation + void $ Brig.listInvitations owner tid >>= getJSON 200 >>= (%. "invitations") >>= asList >>= assertOne + Brig.deleteTeamInvitation owner tid scid >>= assertSuccess + void $ Brig.listInvitations owner tid >>= getJSON 200 >>= (%. "invitations") >>= shouldBeEmpty + + -- retry the invite should work + createScimUser domain token scimUser `bindResponse` \resp -> do + resp.status `shouldMatchInt` 201 + testTeamInvitationWhenScimAccountExists :: (HasCallStack) => App () testTeamInvitationWhenScimAccountExists = do (owner, tid, _) <- createTeam OwnDomain 1 diff --git a/libs/wire-api/src/Wire/API/Routes/Internal/Spar.hs b/libs/wire-api/src/Wire/API/Routes/Internal/Spar.hs index e2a23c2d1c1..31233e07bda 100644 --- a/libs/wire-api/src/Wire/API/Routes/Internal/Spar.hs +++ b/libs/wire-api/src/Wire/API/Routes/Internal/Spar.hs @@ -32,6 +32,7 @@ type InternalAPI = "i" :> ( Named "i_status" ("status" :> Get '[JSON] NoContent) :<|> Named "i_delete_team" ("teams" :> Capture "team" TeamId :> DeleteNoContent) + :<|> Named "i_delete_scim_user" ("scim" :> "users" :> Capture "team" TeamId :> Capture "user" UserId :> DeleteNoContent) :<|> Named "i_put_sso_settings" ("sso" :> "settings" :> ReqBody '[JSON] SsoSettings :> Put '[JSON] NoContent) :<|> Named "i_post_scim_user_info" ("scim" :> "userinfo" :> Capture "user" UserId :> Post '[JSON] ScimUserInfo) :<|> Named "i_get_identity_providers" ("identity-providers" :> Capture "team" TeamId :> Get '[JSON] IdPList) diff --git a/libs/wire-subsystems/src/Wire/SparAPIAccess.hs b/libs/wire-subsystems/src/Wire/SparAPIAccess.hs index b2df76bd01a..0a6005162d7 100644 --- a/libs/wire-subsystems/src/Wire/SparAPIAccess.hs +++ b/libs/wire-subsystems/src/Wire/SparAPIAccess.hs @@ -27,6 +27,7 @@ import Wire.API.User.IdentityProvider data SparAPIAccess m a where GetIdentityProviders :: TeamId -> SparAPIAccess m IdPList DeleteTeam :: TeamId -> SparAPIAccess m () + DeleteScimUser :: TeamId -> UserId -> SparAPIAccess m () LookupScimUserInfo :: UserId -> SparAPIAccess m ScimUserInfo makeSem ''SparAPIAccess diff --git a/libs/wire-subsystems/src/Wire/SparAPIAccess/Rpc.hs b/libs/wire-subsystems/src/Wire/SparAPIAccess/Rpc.hs index fc08de1b81d..a76692c5ffd 100644 --- a/libs/wire-subsystems/src/Wire/SparAPIAccess/Rpc.hs +++ b/libs/wire-subsystems/src/Wire/SparAPIAccess/Rpc.hs @@ -49,6 +49,7 @@ interpretSparAPIAccessToRpc sparEndpoint = runInputConst sparEndpoint . \case GetIdentityProviders tid -> getIdentityProvidersImpl tid DeleteTeam tid -> deleteTeamImpl tid + DeleteScimUser tid uid -> deleteScimUserImpl tid uid LookupScimUserInfo uid -> lookupScimUserInfoImpl uid sparRequest :: @@ -93,6 +94,15 @@ deleteTeamImpl tid = do . paths ["i", "teams", toByteString' tid] . expect2xx +deleteScimUserImpl :: (Member (Input Endpoint) r, Member Rpc r) => TeamId -> UserId -> Sem r () +deleteScimUserImpl tid uid = do + void $ sparRequest delReq + where + delReq = + method DELETE + . paths ["i", "scim", "users", toByteString' tid, toByteString' uid] + . expect2xx + -- | Get the SCIM user info for a user. lookupScimUserInfoImpl :: ( Member (Error ParseException) r, diff --git a/libs/wire-subsystems/test/unit/Wire/MockInterpreters/SparAPIAccess.hs b/libs/wire-subsystems/test/unit/Wire/MockInterpreters/SparAPIAccess.hs index 4122706e412..48f314d0528 100644 --- a/libs/wire-subsystems/test/unit/Wire/MockInterpreters/SparAPIAccess.hs +++ b/libs/wire-subsystems/test/unit/Wire/MockInterpreters/SparAPIAccess.hs @@ -31,6 +31,7 @@ miniSparAPIAccess = interpret $ \case GetIdentityProviders tid -> Map.findWithDefault (IdPList []) tid <$> input DeleteTeam {} -> error "DeleteTeam not implemented in miniSparAPIAccess" + DeleteScimUser {} -> error "DeleteScimUser not implemented in miniSparAPIAccess" LookupScimUserInfo {} -> error "LookupScimUserInfo not implemented in miniSparAPIAccess" emptySparAPIAccess :: InterpreterFor SparAPIAccess r diff --git a/services/brig/src/Brig/Team/API.hs b/services/brig/src/Brig/Team/API.hs index 48430fbebab..ce88b50a63f 100644 --- a/services/brig/src/Brig/Team/API.hs +++ b/services/brig/src/Brig/Team/API.hs @@ -69,6 +69,7 @@ import Wire.API.Team.Size import Wire.API.User hiding (fromEmail) import Wire.AuthenticationSubsystem import Wire.BlockListStore +import Wire.ClientStore (ClientStore) import Wire.EmailSubsystem.Interpreter (renderInvitationUrl) import Wire.Error import Wire.Events (Events) @@ -77,18 +78,25 @@ import Wire.GalleyAPIAccess qualified as GalleyAPIAccess import Wire.IndexedUserStore (IndexedUserStore, getTeamSize) import Wire.InvitationStore (InvitationStore (..), PaginatedResult (..), StoredInvitation (..)) import Wire.InvitationStore qualified as Store +import Wire.NotificationSubsystem (NotificationSubsystem) +import Wire.PropertySubsystem (PropertySubsystem) import Wire.Sem.Concurrency +import Wire.SparAPIAccess (SparAPIAccess) +import Wire.SparAPIAccess qualified as SparAPIAccess import Wire.TeamInvitationSubsystem import Wire.TeamInvitationSubsystem.Interpreter (toInvitation) import Wire.TeamSubsystem (TeamSubsystem) import Wire.TeamSubsystem qualified as TeamSubsystem +import Wire.UserGroupSubsystem (UserGroupSubsystem) import Wire.UserKeyStore import Wire.UserPendingActivationStore (UserPendingActivationStore) +import Wire.UserPendingActivationStore qualified as UserPendingActivationStore import Wire.UserStore import Wire.UserSubsystem import Wire.UserSubsystem.Error servantAPI :: + forall p r. ( Member GalleyAPIAccess r, Member TeamInvitationSubsystem r, Member UserSubsystem r, @@ -98,7 +106,18 @@ servantAPI :: Member (Input (Local ())) r, Member (Error UserSubsystemError) r, Member IndexedUserStore r, - Member TeamSubsystem r + Member TeamSubsystem r, + Member SparAPIAccess r, + Member (Embed App.HttpClientIO) r, + Member NotificationSubsystem r, + Member ClientStore r, + Member PropertySubsystem r, + Member UserGroupSubsystem r, + Member Events r, + Member AuthenticationSubsystem r, + Member UserStore r, + Member UserKeyStore r, + Member (UserPendingActivationStore p) r ) => ServerT TeamsAPI (Handler r) servantAPI = @@ -202,9 +221,24 @@ logInvitationRequest context action = pure (Right result) deleteInvitation :: + forall p r. ( Member InvitationStore r, Member (Error UserSubsystemError) r, - Member TeamSubsystem r + Member TeamSubsystem r, + Member SparAPIAccess r, + Member TinyLog r, + Member (Embed App.HttpClientIO) r, + Member NotificationSubsystem r, + Member ClientStore r, + Member PropertySubsystem r, + Member UserGroupSubsystem r, + Member Events r, + Member AuthenticationSubsystem r, + Member UserSubsystem r, + Member UserStore r, + Member UserKeyStore r, + Member (UserPendingActivationStore p) r, + Member (Input (Local ())) r ) => UserId -> TeamId -> @@ -212,6 +246,30 @@ deleteInvitation :: Sem r () deleteInvitation uid tid iid = do ensurePermissions uid tid [AddTeamMember] + mInvitation <- Store.lookupInvitation tid iid + let scimUid = invitationIdToUserId iid + mUser <- getAccountNoFilter =<< qualifyLocal' scimUid + for_ mUser $ \user -> + for_ (userEmail user) $ \email -> do + pendingScimUsers <- Store.lookupPendingScimUsers tid email + let invitationMatches = maybe True (\inv -> inv.email == email) mInvitation + when + ( userId user == scimUid + && user.userTeam == Just tid + && user.userManagedBy == ManagedByScim + && user.userStatus == PendingInvitation + && invitationMatches + && scimUid `elem` pendingScimUsers + ) + $ do + -- Remove Spar's external-id mapping before deleting the Brig account. + -- Otherwise a SCIM retry still sees the old external ID as owned. + SparAPIAccess.deleteScimUser tid scimUid + UserPendingActivationStore.remove scimUid + -- Use the same complete deletion logic as the asynchronous user + -- deletion worker, but run it synchronously before the invitation is + -- removed so a replacement SCIM invitation can be created safely. + API.deleteAccount user Store.deleteInvitation tid iid listInvitations :: diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index f18e882b496..6886106b97f 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -263,6 +263,7 @@ apiINTERNAL :: Member IdPConfigStore r, Member (Error SparError) r, Member SAMLUserStore r, + Member ScimExternalIdStore r, Member ScimUserMetaStore r, Member (Logger (Msg -> Msg)) r, Member Random r, @@ -273,6 +274,7 @@ apiINTERNAL :: apiINTERNAL = Named @"i_status" internalStatus :<|> Named @"i_delete_team" internalDeleteTeam + :<|> Named @"i_delete_scim_user" internalDeleteScimUser :<|> Named @"i_put_sso_settings" internalPutSsoSettings :<|> Named @"i_post_scim_user_info" internalGetScimUserInfo :<|> Named @"i_get_identity_providers" idpGetAllByTeamId @@ -1132,6 +1134,30 @@ internalDeleteTeam teamId = do deleteTeam teamId pure NoContent +internalDeleteScimUser :: + ( Member BrigAPIAccess r, + Member ScimExternalIdStore r, + Member ScimUserMetaStore r, + Member SAMLUserStore r, + Member (Logger (Msg -> Msg)) r + ) => + TeamId -> + UserId -> + Sem r NoContent +internalDeleteScimUser teamId uid = do + Logger.info $ + Log.msg ("Attempting to delete SCIM user data" :: String) + . Log.field "team" (idToText teamId) + . Log.field "user" (idToText uid) + BrigAPIAccess.getAccount WithPendingInvitations uid >>= \case + Just user + | userTeam user == Just teamId + && userManagedBy user == ManagedByScim + && userStatus user == PendingInvitation -> + deleteScimUserData teamId user + _ -> pure () + pure NoContent + internalPutSsoSettings :: ( Member DefaultSsoCode r, Member (Error SparError) r, diff --git a/services/spar/src/Spar/Scim/User.hs b/services/spar/src/Spar/Scim/User.hs index 95e040bc661..c9c6e3a5150 100644 --- a/services/spar/src/Spar/Scim/User.hs +++ b/services/spar/src/Spar/Scim/User.hs @@ -41,6 +41,7 @@ module Spar.Scim.User mkValidScimId, scimFindUserByExternalId, deleteScimUser, + deleteScimUserData, ) where @@ -899,6 +900,20 @@ deleteScimUser tokeninfo@ScimTokenInfo {stiTeam, stiIdP} uid = ScimExternalIdStore.delete stiTeam veid.validScimIdExternal lift $ ScimUserMetaStore.delete uid +deleteScimUserData :: + ( Member ScimExternalIdStore r, + Member ScimUserMetaStore r, + Member SAMLUserStore r + ) => + TeamId -> + User -> + Sem r () +deleteScimUserData teamId account = do + for_ (Intra.oldVeidFromBrigUser account) $ \veid -> do + for_ (justThere veid.validScimIdAuthInfo) (SAMLUserStore.delete (userId account)) + ScimExternalIdStore.delete teamId veid.validScimIdExternal + ScimUserMetaStore.delete (userId account) + ---------------------------------------------------------------------------- -- Utilities