WPB-21744: invalidate pending email update when a user is put under SCIM control

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.

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

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"

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

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

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 ()

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

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)

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

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