[WPB-11122] Disallow searching user by old email

cassandra-schema.cql

@@ -666,7 +666,8 @@ CREATE TABLE brig_test.user (
     status int,
     supported_protocols int,
     team uuid,
-    text_status text
+    text_status text,
+    write_time_bumper int
 ) WITH bloom_filter_fp_chance = 0.1
     AND caching = {'keys': 'ALL', 'rows_per_partition': 'NONE'}
     AND comment = ''

changelog.d/3-bug-fixes/WPB-11122-disallow-searching-user-by-old-email

@@ -0,0 +1 @@
+Users with SAML-SSO are allowed to delete their email address on the rest api. If they do that, the search indices are not updated correctly, and finding the user by the removed email address is still possible.

integration/test/API/Brig.hs

@@ -249,6 +249,13 @@ searchContacts user searchTerm domain = do
   d <- objDomain domain
   submit "GET" (req & addQueryParams [("q", q), ("domain", d)])
 
+-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_teams__tid__search
+searchTeam :: (HasCallStack, MakesValue user) => user -> String -> App Response
+searchTeam user q = do
+  tid <- user %. "team" & asString
+  req <- baseRequest user Brig Versioned $ joinHttpPath ["teams", tid, "search"]
+  submit "GET" (req & addQueryParams [("q", q)])
+
 getAPIVersion :: (HasCallStack, MakesValue domain) => domain -> App Response
 getAPIVersion domain = do
   req <- baseRequest domain Brig Unversioned $ "/api-version"
@@ -402,6 +409,12 @@ putSelfEmail caller emailAddress = do
   req <- baseRequest caller Brig Versioned $ joinHttpPath ["users", callerid, "email"]
   submit "PUT" $ req & addJSONObject ["email" .= emailAddress]
 
+-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/delete_self_email
+deleteSelfEmail :: (HasCallStack, MakesValue caller) => caller -> App Response
+deleteSelfEmail caller = do
+  req <- baseRequest caller Brig Versioned $ joinHttpPath ["self", "email"]
+  submit "DELETE" req
+
 -- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_self_handle
 -- FUTUREWORK: rename to putSelfHandle for consistency
 putHandle :: (HasCallStack, MakesValue user) => user -> String -> App Response

integration/test/API/Spar.hs

@@ -12,7 +12,7 @@ getScimTokens caller = do
   req <- baseRequest caller Spar Versioned "/scim/auth-tokens"
   submit "GET" req
 
--- https://staging-nginz-https.zinfra.io/v5/api/swagger-ui/#/default/post_scim_auth_tokens
+-- | https://staging-nginz-https.zinfra.io/v5/api/swagger-ui/#/default/post_scim_auth_tokens
 createScimToken :: (HasCallStack, MakesValue caller) => caller -> App Response
 createScimToken caller = do
   req <- baseRequest caller Spar Versioned "/scim/auth-tokens"

integration/test/Test/Brig.hs

@@ -1,14 +1,20 @@
+{-# LANGUAGE DuplicateRecordFields #-}
+{-# OPTIONS_GHC -Wno-incomplete-patterns #-}
+
 module Test.Brig where
 
-import API.Brig
+import API.Brig as BrigP
 import qualified API.BrigInternal as BrigI
 import API.Common
+import API.GalleyInternal (setTeamFeatureStatus)
+import API.Spar
 import Data.Aeson.Types hiding ((.=))
 import Data.List.Split
 import Data.String.Conversions
 import qualified Data.UUID as UUID
 import qualified Data.UUID.V4 as UUID
 import GHC.Stack
+import SAML2.WebSSO.Test.Util (SampleIdP (..), makeSampleIdPMetadata)
 import SetupHelpers
 import System.IO.Extra
 import Testlib.Assertions
@@ -229,3 +235,37 @@ testSFTFederation = do
           maybe (assertFailure "is_federating missing") asBool
             =<< lookupField resp.json "is_federating"
         when isFederating $ assertFailure "is_federating should be false"
+
+testDeleteEmail :: (HasCallStack) => App ()
+testDeleteEmail = do
+  (owner, tid, [usr]) <- createTeam OwnDomain 2
+  putSelf usr (PutSelf Nothing Nothing (Just "Alice") Nothing) >>= assertSuccess
+  email <- getSelf usr >>= getJSON 200 >>= (%. "email") >>= asString
+
+  let associateUsrWithSSO :: (HasCallStack) => App ()
+      associateUsrWithSSO = do
+        void $ setTeamFeatureStatus owner tid "sso" "enabled"
+        registerTestIdPWithMeta owner >>= assertSuccess
+        tok <- createScimToken owner >>= getJSON 200 >>= (%. "token") >>= asString
+        void $ findUsersByExternalId owner tok email
+
+      searchShouldBe :: (HasCallStack) => String -> App ()
+      searchShouldBe expected = do
+        BrigI.refreshIndex OwnDomain
+        bindResponse (BrigP.searchTeam owner email) $ \resp -> do
+          resp.status `shouldMatchInt` 200
+          numDocs <- length <$> (resp.json %. "documents" >>= asList)
+          case expected of
+            "empty" -> numDocs `shouldMatchInt` 0
+            "non-empty" -> numDocs `shouldMatchInt` 1
+
+  deleteSelfEmail usr >>= assertStatus 403
+  searchShouldBe "non-empty"
+  associateUsrWithSSO
+  deleteSelfEmail usr >>= assertSuccess
+  searchShouldBe "empty"
+
+registerTestIdPWithMeta :: (HasCallStack, MakesValue owner) => owner -> App Response
+registerTestIdPWithMeta owner = do
+  SampleIdP idpmeta _ _ _ <- makeSampleIdPMetadata
+  createIdp owner idpmeta

libs/cassandra-util/src/Cassandra/Util.hs

@@ -109,7 +109,7 @@ initCassandra settings Nothing logger = do
 -- | Read cassandra's writetimes https://docs.datastax.com/en/dse/5.1/cql/cql/cql_using/useWritetime.html
 -- as UTCTime values without any loss of precision
 newtype Writetime a = Writetime {writetimeToUTC :: UTCTime}
-  deriving (Functor)
+  deriving (Eq, Show, Functor)
 
 instance Cql (Writetime a) where
   ctype = Tagged BigIntColumn

libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs

@@ -186,10 +186,9 @@ type AccountAPI =
     :<|> Named
            "putSelfEmail"
            ( Summary
-               "internal email activation (used in tests and in spar for validating emails obtained as \
-               \SAML user identifiers).  if the validate query parameter is false or missing, only set \
-               \the activation timeout, but do not send an email, and do not do anything about \
-               \activating the email."
+               "Internal email update and activation. Used in tests and in spar for validating emails \
+               \obtained via scim or saml implicit user creation. If the `validate` query parameter is \
+               \false or missing, only update the email and do not activate."
                :> ZUser
                :> "self"
                :> "email"

libs/wire-subsystems/src/Wire/UserSearch/Types.hs

@@ -205,3 +205,18 @@ data BrowseTeamFilters = BrowseTeamFilters
 
 userIdToDocId :: UserId -> DocId
 userIdToDocId uid = DocId (idToText uid)
+
+-- | We use cassandra writetimes to construct the ES index version. Since nulling fields in
+-- cassandra also nulls the writetime, re-indexing does not happen when nulling a field, and
+-- the old search key can still effectively be used.
+--
+-- `write_time_bumper type int` is an extra field that we can update whenever we null a field
+-- and want to update the write time of the table.  `WriteTimeBumper` writes to 'int' fields,
+-- but only cares about the field's writetime.
+data WriteTimeBumper = WriteTimeBumper
+  deriving (Eq, Show)
+
+instance C.Cql WriteTimeBumper where
+  ctype = C.Tagged C.IntColumn
+  toCql WriteTimeBumper = C.CqlInt 0
+  fromCql _ = pure WriteTimeBumper

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

@@ -55,7 +55,7 @@ getIndexUserBaseQuery :: LText
 getIndexUserBaseQuery =
   [sql|
     SELECT
-    id, 
+    id,
     team, writetime(team),
     name, writetime(name),
     status, writetime(status),
@@ -66,7 +66,8 @@ getIndexUserBaseQuery =
     service, writetime(service),
     managed_by, writetime(managed_by),
     sso_id, writetime(sso_id),
-    email_unvalidated, writetime(email_unvalidated)
+    email_unvalidated, writetime(email_unvalidated),
+    writetime(write_time_bumper)
     FROM user
   |]
 

libs/wire-subsystems/src/Wire/UserStore/IndexUser.hs

@@ -22,6 +22,7 @@ import Wire.UserSearch.Types
 type Activated = Bool
 
 data WithWritetime a = WithWriteTime {value :: a, writetime :: Writetime a}
+  deriving (Eq, Show)
 
 data IndexUser = IndexUser
   { userId :: UserId,
@@ -35,8 +36,10 @@ data IndexUser = IndexUser
     serviceId :: Maybe (WithWritetime ServiceId),
     managedBy :: Maybe (WithWritetime ManagedBy),
     ssoId :: Maybe (WithWritetime UserSSOId),
-    unverifiedEmail :: Maybe (WithWritetime EmailAddress)
+    unverifiedEmail :: Maybe (WithWritetime EmailAddress),
+    writeTimeBumper :: Maybe (Writetime WriteTimeBumper)
   }
+  deriving (Eq, Show)
 
 {- ORMOLU_DISABLE -}
 type instance
@@ -52,12 +55,13 @@ type instance
       Maybe ServiceId, Maybe (Writetime ServiceId),
       Maybe ManagedBy, Maybe (Writetime ManagedBy),
       Maybe UserSSOId, Maybe (Writetime UserSSOId),
-      Maybe EmailAddress, Maybe (Writetime EmailAddress)
+      Maybe EmailAddress, Maybe (Writetime EmailAddress),
+      Maybe (Writetime WriteTimeBumper)
     )
 
 instance Record IndexUser where
   asTuple (IndexUser {..}) =
-    ( userId, 
+    ( userId,
       value <$> teamId, writetime <$> teamId,
       name.value, name.writetime,
       value <$> accountStatus, writetime <$> accountStatus,
@@ -68,11 +72,12 @@ instance Record IndexUser where
       value <$> serviceId, writetime <$> serviceId,
       value <$> managedBy, writetime <$> managedBy,
       value <$> ssoId, writetime <$> ssoId,
-      value <$> unverifiedEmail, writetime <$> unverifiedEmail
+      value <$> unverifiedEmail, writetime <$> unverifiedEmail,
+      writeTimeBumper
     )
 
   asRecord
-    ( u, 
+    ( u,
       mTeam, tTeam,
       name, tName,
       status, tStatus,
@@ -83,7 +88,8 @@ instance Record IndexUser where
       service, tService,
       managedBy, tManagedBy,
       ssoId, tSsoId,
-      emailUnvalidated, tEmailUnvalidated
+      emailUnvalidated, tEmailUnvalidated,
+      tWriteTimeBumper
     ) = IndexUser {
           userId = u,
           teamId =  WithWriteTime <$> mTeam <*> tTeam,
@@ -96,7 +102,8 @@ instance Record IndexUser where
           serviceId = WithWriteTime <$> service <*> tService,
           managedBy = WithWriteTime <$> managedBy <*> tManagedBy,
           ssoId = WithWriteTime <$> ssoId <*> tSsoId,
-          unverifiedEmail = WithWriteTime <$> emailUnvalidated <*> tEmailUnvalidated
+          unverifiedEmail = WithWriteTime <$> emailUnvalidated <*> tEmailUnvalidated,
+          writeTimeBumper = tWriteTimeBumper
         }
 {- ORMOLU_ENABLE -}
 
@@ -113,7 +120,8 @@ indexUserToVersion IndexUser {..} =
       const () <$$> fmap writetime serviceId,
       const () <$$> fmap writetime managedBy,
       const () <$$> fmap writetime ssoId,
-      const () <$$> fmap writetime unverifiedEmail
+      const () <$$> fmap writetime unverifiedEmail,
+      const () <$$> writeTimeBumper
     ]
 
 indexUserToDoc :: SearchVisibilityInbound -> IndexUser -> UserDoc

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

@@ -547,7 +547,7 @@ syncUserIndex ::
   ) =>
   UserId ->
   Sem r ()
-syncUserIndex uid =
+syncUserIndex uid = do
   getIndexUser uid
     >>= maybe deleteFromIndex upsert
   where

libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserStore.hs

@@ -83,7 +83,8 @@ storedUserToIndexUser storedUser =
           serviceId = withDefaultTime <$> storedUser.serviceId,
           managedBy = withDefaultTime <$> storedUser.managedBy,
           ssoId = withDefaultTime <$> storedUser.ssoId,
-          unverifiedEmail = Nothing
+          unverifiedEmail = Nothing,
+          writeTimeBumper = Nothing
         }
 
 lookupLocaleImpl :: (Member (State [StoredUser]) r) => UserId -> Sem r (Maybe ((Maybe Language, Maybe Country)))

services/brig/brig.cabal

@@ -183,6 +183,7 @@ library
     Brig.Schema.V83_AddTextStatus
     Brig.Schema.V84_DropTeamInvitationPhone
     Brig.Schema.V85_DropUserKeysHashed
+    Brig.Schema.V86_WriteTimeBumper
     Brig.Team.API
     Brig.Team.Email
     Brig.Team.Template

services/brig/src/Brig/API/User.hs

@@ -650,18 +650,11 @@ removeEmail uid = do
     Just (SSOIdentity (UserSSOId _) (Just e)) -> lift $ do
       liftSem $ deleteKey $ mkEmailKey e
       wrapClient $ Data.deleteEmail uid
-      -- FUTUREWORK: This doesn't delete user's email address from the index,
-      -- which is a bug, reported here:
-      -- https://wearezeta.atlassian.net/browse/WPB-11122.
-      --
-      -- Calling User.internalUpdateSearchIndex here wouldn't work as explained
-      -- in the ticket.
       liftSem $ Events.generateUserEvent uid Nothing (emailRemoved uid e)
+      liftSem $ User.internalUpdateSearchIndex uid
     Just _ -> throwE LastIdentity
     Nothing -> throwE NoIdentity
 
--------------------------------------------------------------------------------
-
 -------------------------------------------------------------------------------
 -- Forcefully revoke a verified identity
 

services/brig/src/Brig/Data/User.hs

@@ -559,7 +559,7 @@ userActivatedUpdate :: PrepQuery W (Maybe EmailAddress, UserId) ()
 userActivatedUpdate = {- `IF EXISTS`, but that requires benchmarking -} "UPDATE user SET activated = true, email = ? WHERE id = ?"
 
 userEmailDelete :: PrepQuery W (Identity UserId) ()
-userEmailDelete = {- `IF EXISTS`, but that requires benchmarking -} "UPDATE user SET email = null WHERE id = ?"
+userEmailDelete = {- `IF EXISTS`, but that requires benchmarking -} "UPDATE user SET email = null, write_time_bumper = 0 WHERE id = ?"
 
 userRichInfoUpdate :: PrepQuery W (RichInfoAssocList, UserId) ()
 userRichInfoUpdate = {- `IF EXISTS`, but that requires benchmarking -} "UPDATE rich_info SET json = ? WHERE user = ?"

services/brig/src/Brig/Schema/Run.hs

@@ -60,6 +60,7 @@ import Brig.Schema.V82_DropPhoneColumn qualified as V82_DropPhoneColumn
 import Brig.Schema.V83_AddTextStatus qualified as V83_AddTextStatus
 import Brig.Schema.V84_DropTeamInvitationPhone qualified as V84_DropTeamInvitationPhone
 import Brig.Schema.V85_DropUserKeysHashed qualified as V85_DropUserKeysHashed
+import Brig.Schema.V86_WriteTimeBumper qualified as V86_WriteTimeBumper
 import Cassandra.MigrateSchema (migrateSchema)
 import Cassandra.Schema
 import Control.Exception (finally)
@@ -126,7 +127,8 @@ migrations =
     V82_DropPhoneColumn.migration,
     V83_AddTextStatus.migration,
     V84_DropTeamInvitationPhone.migration,
-    V85_DropUserKeysHashed.migration
+    V85_DropUserKeysHashed.migration,
+    V86_WriteTimeBumper.migration
     -- FUTUREWORK: undo V41 (searchable flag); we stopped using it in
     -- https://github.com/wireapp/wire-server/pull/964
   ]

services/brig/src/Brig/Schema/V86_WriteTimeBumper.hs

@@ -0,0 +1,36 @@
+{-# LANGUAGE QuasiQuotes #-}
+
+-- This file is part of the Wire Server implementation.
+--
+-- Copyright (C) 2023 Wire Swiss GmbH <opensource@wire.com>
+--
+-- This program is free software: you can redistribute it and/or modify it under
+-- the terms of the GNU Affero General Public License as published by the Free
+-- Software Foundation, either version 3 of the License, or (at your option) any
+-- later version.
+--
+-- This program is distributed in the hope that it will be useful, but WITHOUT
+-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+-- details.
+--
+-- You should have received a copy of the GNU Affero General Public License along
+-- with this program. If not, see <https://www.gnu.org/licenses/>.
+
+module Brig.Schema.V86_WriteTimeBumper
+  ( migration,
+  )
+where
+
+import Cassandra.Schema
+import Imports
+import Text.RawString.QQ
+
+migration :: Migration
+migration =
+  -- See 'WriteTimeBumper' for more explanations.
+  Migration 86 "Add field for keeping track of time of nulling another field" $
+    schema'
+      [r| ALTER TABLE user ADD (
+            write_time_bumper int
+          ) |]