diff --git a/cassandra-schema.cql b/cassandra-schema.cql index fbc45dc57bb..7d520e33b1f 100644 --- a/cassandra-schema.cql +++ b/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 = '' diff --git a/changelog.d/3-bug-fixes/WPB-11122-disallow-searching-user-by-old-email b/changelog.d/3-bug-fixes/WPB-11122-disallow-searching-user-by-old-email new file mode 100644 index 00000000000..85b54569f90 --- /dev/null +++ b/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. diff --git a/integration/test/API/Brig.hs b/integration/test/API/Brig.hs index a40dddd3bd9..a82046379a3 100644 --- a/integration/test/API/Brig.hs +++ b/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 diff --git a/integration/test/API/Spar.hs b/integration/test/API/Spar.hs index f040889f6c4..ee57ef581aa 100644 --- a/integration/test/API/Spar.hs +++ b/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" diff --git a/integration/test/Test/Brig.hs b/integration/test/Test/Brig.hs index 4839e36b286..f55fc952b00 100644 --- a/integration/test/Test/Brig.hs +++ b/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 diff --git a/libs/cassandra-util/src/Cassandra/Util.hs b/libs/cassandra-util/src/Cassandra/Util.hs index 4331da819c5..378c6b6f2f0 100644 --- a/libs/cassandra-util/src/Cassandra/Util.hs +++ b/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 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 f55d8a83dea..ec853e7f933 100644 --- a/libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs +++ b/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" diff --git a/libs/wire-subsystems/src/Wire/UserSearch/Types.hs b/libs/wire-subsystems/src/Wire/UserSearch/Types.hs index fc4d15e434e..61fab5fe704 100644 --- a/libs/wire-subsystems/src/Wire/UserSearch/Types.hs +++ b/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 diff --git a/libs/wire-subsystems/src/Wire/UserStore/Cassandra.hs b/libs/wire-subsystems/src/Wire/UserStore/Cassandra.hs index ee7f51bab8c..f6c71536c65 100644 --- a/libs/wire-subsystems/src/Wire/UserStore/Cassandra.hs +++ b/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 |] diff --git a/libs/wire-subsystems/src/Wire/UserStore/IndexUser.hs b/libs/wire-subsystems/src/Wire/UserStore/IndexUser.hs index 2334260f447..b5a005036ce 100644 --- a/libs/wire-subsystems/src/Wire/UserStore/IndexUser.hs +++ b/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 diff --git a/libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs b/libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs index 769be28ee74..a824fe73c92 100644 --- a/libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs +++ b/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 diff --git a/libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserStore.hs b/libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserStore.hs index a1e0e5d96e1..db318e5366b 100644 --- a/libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserStore.hs +++ b/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))) diff --git a/services/brig/brig.cabal b/services/brig/brig.cabal index 52642552923..9b116cb9a2b 100644 --- a/services/brig/brig.cabal +++ b/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 diff --git a/services/brig/src/Brig/API/User.hs b/services/brig/src/Brig/API/User.hs index 982c907e338..897bc3a4700 100644 --- a/services/brig/src/Brig/API/User.hs +++ b/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 diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index b71bdd02cfe..e406f84afb2 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/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 = ?" diff --git a/services/brig/src/Brig/Schema/Run.hs b/services/brig/src/Brig/Schema/Run.hs index f833b8d97b1..72bbff2b1f2 100644 --- a/services/brig/src/Brig/Schema/Run.hs +++ b/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 ] diff --git a/services/brig/src/Brig/Schema/V86_WriteTimeBumper.hs b/services/brig/src/Brig/Schema/V86_WriteTimeBumper.hs new file mode 100644 index 00000000000..e579517ac94 --- /dev/null +++ b/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 +-- +-- 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 . + +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 + ) |]