diff --git a/changelog.d/3-bug-fixes/WPB-21744 b/changelog.d/3-bug-fixes/WPB-21744 new file mode 100644 index 00000000000..3825ca54830 --- /dev/null +++ b/changelog.d/3-bug-fixes/WPB-21744 @@ -0,0 +1 @@ +When a user is put under SCIM control, any pending email-address update is now invalidated (the unvalidated email and its activation token are removed). Previously, team settings kept offering a "resend verification" action that could not succeed (failing with `403 managed-by-scim`), and a stale activation link could still change a SCIM-managed user's email outside of SCIM. diff --git a/integration/test/Test/Spar.hs b/integration/test/Test/Spar.hs index 26cd2f43207..fff111393d5 100644 --- a/integration/test/Test/Spar.hs +++ b/integration/test/Test/Spar.hs @@ -1564,3 +1564,47 @@ testScimUserChangeNameOnRegisteringIgnoredV16 = do registerUserWithVersioned (ExplicitVersion 16) OwnDomain email code newProfilename `bindResponse` \resp -> do resp.status `shouldMatchInt` 201 resp.json %. "name" `shouldMatch` scimUserDisplayName + +-- | A pending email update (emailUnvalidated + activation code) must be +-- invalidated when the user transitions to SCIM control. Getting a Wire-managed +-- user via the SCIM API triggers 'getUserById' -> 'synthesizeStoredUser' -> +-- 'ManagedByScim' -> 'deletePendingEmailUpdate' in spar, which calls the brig +-- internal endpoint that deletes both the activation code and the pending +-- email entry. See WPB-21744 / PR #5333. +testSparScimInvalidatesPendingEmail :: (HasCallStack) => App () +testSparScimInvalidatesPendingEmail = do + -- 1. Create a team with an owner and one Wire-managed member. + (owner, _tid, [mem]) <- createTeam OwnDomain 2 + memberId <- mem %. "id" >>= asString + memberEmail <- mem %. "email" >>= asString + + -- 2. Login as the member and initiate an email update. This creates a + -- pending emailUnvalidated entry plus an activation code for the new email. + (cookie, token) <- + login mem memberEmail defPassword `bindResponse` \resp -> do + resp.status `shouldMatchInt` 200 + tok <- resp.json %. "access_token" & asString + let c = fromJust $ getCookie "zuid" resp + pure ("zuid=" <> c, tok) + newEmail <- randomEmail + updateEmail mem newEmail cookie token >>= assertSuccess + + -- 3. Verify the activation code exists for the pending (unvalidated) email. + getActivationCode OwnDomain newEmail >>= assertStatus 200 + + -- 4. Transition the member to SCIM control. Reading the user through the + -- SCIM API triggers getUserById -> synthesizeStoredUser -> (since the user + -- is email-only and not yet SCIM-managed) setManagedBy ManagedByScim and + -- deletePendingEmailUpdate. + tok <- createScimTokenV6 owner def >>= getJSON 200 >>= (%. "token") >>= asString + getScimUser OwnDomain tok memberId >>= assertStatus 200 + + -- 5. The activation code for the pending email has been invalidated. + getActivationCode OwnDomain newEmail >>= assertStatus 404 + + -- 6. The member's email is unchanged: the SCIM transition must not promote + -- the unvalidated email, only drop the pending update. + bindResponse (getUsersId OwnDomain [memberId]) $ \resp -> do + resp.status `shouldMatchInt` 200 + u <- resp.json & asList >>= assertOne + u %. "email" `shouldMatch` memberEmail diff --git a/libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs b/libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs index 4a0e907c808..0bf24d834c0 100644 --- a/libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs +++ b/libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs @@ -535,6 +535,19 @@ type AccountAPI = :> ReqBody '[Servant.JSON] ManagedByUpdate :> Put '[Servant.JSON] NoContent ) + :<|> Named + "iDeletePendingEmailUpdate" + ( Summary + "Invalidate a pending email-address update for a user. \ + \Used by spar when a user is put under SCIM control, so that team \ + \settings no longer offers a 'resend verification' action that cannot \ + \succeed, and stale activation links cannot change a SCIM-managed \ + \user's email." + :> "users" + :> Capture "uid" UserId + :> "pending-email-update" + :> Delete '[Servant.JSON] NoContent + ) :<|> Named "iPutRichInfo" ( "users" diff --git a/libs/wire-subsystems/src/Wire/ActivationCodeStore.hs b/libs/wire-subsystems/src/Wire/ActivationCodeStore.hs index 06b331cebb1..0b6f656e363 100644 --- a/libs/wire-subsystems/src/Wire/ActivationCodeStore.hs +++ b/libs/wire-subsystems/src/Wire/ActivationCodeStore.hs @@ -54,5 +54,11 @@ data ActivationCodeStore :: Effect where -- | The user with whom to associate the activation code. Maybe UserId -> ActivationCodeStore m Activation + -- | Delete a pending activation code for a given 'EmailKey', if any. + -- This is used to invalidate a pending email-address update (e.g. when a + -- user is put under SCIM control). + DeleteActivationCode :: + EmailKey -> + ActivationCodeStore m () makeSem ''ActivationCodeStore diff --git a/libs/wire-subsystems/src/Wire/ActivationCodeStore/Cassandra.hs b/libs/wire-subsystems/src/Wire/ActivationCodeStore/Cassandra.hs index 56ff6f247a5..24d2f3c2737 100644 --- a/libs/wire-subsystems/src/Wire/ActivationCodeStore/Cassandra.hs +++ b/libs/wire-subsystems/src/Wire/ActivationCodeStore/Cassandra.hs @@ -42,6 +42,7 @@ interpretActivationCodeStoreToCassandra casClient = liftIO (mkActivationKey ek) >>= retry x1 . query1 cql . params LocalQuorum . Identity NewActivationCode ek timeout uid -> newActivationCodeImpl ek timeout uid + DeleteActivationCode ek -> deleteActivationCodeImpl ek where cql :: PrepQuery R (Identity ActivationKey) (Maybe UserId, ActivationCode) cql = @@ -72,6 +73,15 @@ newActivationCodeImpl uk timeout u = do ActivationCode . Ascii.unsafeFromText . pack . printf "%06d" <$> randIntegerZeroToNMinusOne 1000000 +-- | Delete a pending activation code for a given 'EmailKey', if any. +deleteActivationCodeImpl :: + (MonadClient m) => + EmailKey -> + m () +deleteActivationCodeImpl uk = do + key <- liftIO $ mkActivationKey uk + retry x5 . write keyDelete $ params LocalQuorum (Identity key) + -------------------------------------------------------------------------------- -- Utilities @@ -91,6 +101,9 @@ keyInsert = \(key, key_type, key_text, code, user, retries) VALUES \ \(? , ? , ? , ? , ? , ? ) USING TTL ?" +keyDelete :: PrepQuery W (Identity ActivationKey) () +keyDelete = "DELETE FROM activation_keys WHERE key = ?" + -- | Max. number of activation attempts per 'ActivationKey'. maxAttempts :: Int32 maxAttempts = 3 diff --git a/libs/wire-subsystems/src/Wire/BrigAPIAccess.hs b/libs/wire-subsystems/src/Wire/BrigAPIAccess.hs index ceec1624944..972df7782cf 100644 --- a/libs/wire-subsystems/src/Wire/BrigAPIAccess.hs +++ b/libs/wire-subsystems/src/Wire/BrigAPIAccess.hs @@ -151,6 +151,7 @@ data BrigAPIAccess m a where SetName :: UserId -> Name -> BrigAPIAccess m () SetHandle :: UserId -> Handle -> BrigAPIAccess m () SetManagedBy :: UserId -> ManagedBy -> BrigAPIAccess m () + DeletePendingEmailUpdate :: UserId -> BrigAPIAccess m () SetSSOId :: UserId -> UserSSOId -> BrigAPIAccess m () SetRichInfo :: UserId -> RichInfo -> BrigAPIAccess m () SetLocale :: UserId -> Maybe Locale -> BrigAPIAccess m () diff --git a/libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs b/libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs index f4bd3d9ea47..0c754c43f5a 100644 --- a/libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs +++ b/libs/wire-subsystems/src/Wire/BrigAPIAccess/Rpc.hs @@ -167,6 +167,8 @@ interpretBrigAccess brigEndpoint = setHandle uid handle SetManagedBy uid managedBy -> setManagedBy uid managedBy + DeletePendingEmailUpdate uid -> + deletePendingEmailUpdate uid SetSSOId uid ssoId -> setSSOId uid ssoId SetRichInfo uid richInfo -> @@ -996,6 +998,18 @@ setManagedBy buid managedBy = do unless (statusCode resp == 200) $ rethrow "brig" resp +deletePendingEmailUpdate :: + (Member Rpc r, Member (Input Endpoint) r, Member (Error RpcException) r) => + UserId -> + Sem r () +deletePendingEmailUpdate buid = do + resp <- + brigRequest $ + method DELETE + . paths ["i", "users", toByteString' buid, "pending-email-update"] + unless (statusCode resp == 200) $ + rethrow "brig" resp + setSSOId :: (Member Rpc r, Member (Input Endpoint) r, Member (Error RpcException) r) => UserId -> diff --git a/libs/wire-subsystems/test/unit/Wire/MockInterpreters/ActivationCodeStore.hs b/libs/wire-subsystems/test/unit/Wire/MockInterpreters/ActivationCodeStore.hs index dfb21478e8c..7250d3047fa 100644 --- a/libs/wire-subsystems/test/unit/Wire/MockInterpreters/ActivationCodeStore.hs +++ b/libs/wire-subsystems/test/unit/Wire/MockInterpreters/ActivationCodeStore.hs @@ -42,14 +42,16 @@ emailKeyToCode = inMemoryActivationCodeStoreInterpreter :: (Member (State (Map EmailKey (Maybe UserId, ActivationCode))) r) => InterpreterFor ActivationCodeStore r -inMemoryActivationCodeStoreInterpreter = interpret \case - LookupActivationCode ek -> gets (!? ek) - NewActivationCode ek _ uid -> do - let key = - ActivationKey - . Ascii.encodeBase64Url - . T.encodeUtf8 - . emailKeyUniq - $ ek - c = emailKeyToCode ek - modify (insert ek (uid, c)) $> Activation key c +inMemoryActivationCodeStoreInterpreter = + interpret \case + LookupActivationCode ek -> gets (!? ek) + NewActivationCode ek _ uid -> do + let key = + ActivationKey + . Ascii.encodeBase64Url + . T.encodeUtf8 + . emailKeyUniq + $ ek + c = emailKeyToCode ek + modify (insert ek (uid, c)) $> Activation key c + DeleteActivationCode ek -> modify (delete ek) diff --git a/services/brig/src/Brig/API/Internal.hs b/services/brig/src/Brig/API/Internal.hs index 6a6f17dbb80..f175554db71 100644 --- a/services/brig/src/Brig/API/Internal.hs +++ b/services/brig/src/Brig/API/Internal.hs @@ -90,6 +90,7 @@ import Wire.API.UserGroup (UserGroup) import Wire.API.UserGroup.Pagination import Wire.API.UserMap import Wire.ActivationCodeStore (ActivationCodeStore) +import Wire.ActivationCodeStore qualified as ActivationCode import Wire.AppStore (AppStore) import Wire.AppStore qualified as AppStore import Wire.AppSubsystem (AppSubsystem) @@ -128,6 +129,7 @@ import Wire.Sem.Concurrency import Wire.Sem.Now (Now) import Wire.Sem.Random (Random) import Wire.SparAPIAccess (SparAPIAccess) +import Wire.StoredUser (StoredUser (emailUnvalidated)) import Wire.TeamInvitationSubsystem import Wire.TeamSubsystem (TeamSubsystem) import Wire.UserGroupSubsystem @@ -282,6 +284,7 @@ accountAPI = :<|> Named @"iPutUserSsoId" updateSSOIdH :<|> Named @"iDeleteUserSsoId" deleteSSOIdH :<|> Named @"iPutManagedBy" updateManagedByH + :<|> Named @"iDeletePendingEmailUpdate" deletePendingEmailUpdateH :<|> Named @"iPutRichInfo" updateRichInfoH :<|> Named @"iPutHandle" updateHandleH :<|> Named @"iPutUserName" updateUserNameH @@ -898,6 +901,20 @@ updateManagedByH :: (Member UserStore r) => UserId -> ManagedByUpdate -> (Handle updateManagedByH uid (ManagedByUpdate managedBy) = do NoContent <$ lift (liftSem $ UserStore.updateManagedBy uid managedBy) +deletePendingEmailUpdateH :: + ( Member UserStore r, + Member ActivationCodeStore r + ) => + UserId -> + (Handler r) NoContent +deletePendingEmailUpdateH uid = do + mUser <- lift . liftSem $ UserStore.getUser uid + for_ (emailUnvalidated =<< mUser) $ \email -> + lift . liftSem $ do + ActivationCode.deleteActivationCode (mkEmailKey email) + UserStore.deleteEmailUnvalidated uid + pure NoContent + updateRichInfoH :: (Member UserStore r) => UserId -> RichInfoUpdate -> (Handler r) NoContent updateRichInfoH uid rup = NoContent <$ do diff --git a/services/spar/src/Spar/Scim/User.hs b/services/spar/src/Spar/Scim/User.hs index 118e00d2ed2..ad530d8412c 100644 --- a/services/spar/src/Spar/Scim/User.hs +++ b/services/spar/src/Spar/Scim/User.hs @@ -1000,8 +1000,12 @@ synthesizeStoredUser acc veid = writeState oldAccessTimes oldManagedBy oldRichInfo storedUser = do when (isNothing oldAccessTimes) $ ScimUserTimesStore.write storedUser - when (oldManagedBy /= ManagedByScim) $ + when (oldManagedBy /= ManagedByScim) $ do BrigAPIAccess.setManagedBy uid ManagedByScim + -- Invalidate any pending email-address update: a SCIM-managed user's + -- email can only be changed through SCIM, so the pending update token + -- and the unvalidated email must be removed. + BrigAPIAccess.deletePendingEmailUpdate uid let newRichInfo = view ST.sueRichInfo . Scim.extra . Scim.value . Scim.thing $ storedUser when (oldRichInfo /= newRichInfo) $ BrigAPIAccess.setRichInfo uid newRichInfo @@ -1130,6 +1134,8 @@ getUserById midp stiTeam uid = do -- set managed_by when (userManagedBy brigUser /= ManagedByScim) do lift $ BrigAPIAccess.setManagedBy uid ManagedByScim + -- Invalidate any pending email-address update (see comment above). + lift $ BrigAPIAccess.deletePendingEmailUpdate uid -- remove dangling entry from spar.user_v2 table (cassandra) case mbOldVeid of Just oldVeid | ST.veidUref newVeid /= ST.veidUref oldVeid -> do