diff --git a/changelog.d/3-bug-fixes/pr-2758 b/changelog.d/3-bug-fixes/pr-2758 new file mode 100644 index 00000000000..6297e958351 --- /dev/null +++ b/changelog.d/3-bug-fixes/pr-2758 @@ -0,0 +1 @@ +Clients without any prekeys are not deleted completely diff --git a/services/brig/src/Brig/API.hs b/services/brig/src/Brig/API.hs index 6ce411f724f..f5cef376774 100644 --- a/services/brig/src/Brig/API.hs +++ b/services/brig/src/Brig/API.hs @@ -32,15 +32,17 @@ import Brig.Effects.UserPendingActivationStore (UserPendingActivationStore) import qualified Data.Swagger.Build.Api as Doc import Network.Wai.Routing (Routes) import Polysemy +import Wire.Sem.Concurrency sitemap :: forall r p. Members - '[ CodeStore, - PasswordResetStore, + '[ BlacklistPhonePrefixStore, BlacklistStore, - BlacklistPhonePrefixStore, GalleyProvider, + CodeStore, + Concurrency 'Unsafe, + PasswordResetStore, UserPendingActivationStore p ] r => diff --git a/services/brig/src/Brig/API/Client.hs b/services/brig/src/Brig/API/Client.hs index 3da92199446..9d0a5b87757 100644 --- a/services/brig/src/Brig/API/Client.hs +++ b/services/brig/src/Brig/API/Client.hs @@ -46,8 +46,6 @@ module Brig.API.Client ) where -import Bilge.IO -import Bilge.RPC import Brig.API.Types import Brig.API.Util import Brig.App @@ -76,7 +74,6 @@ import Brig.User.Email import Cassandra (MonadClient) import Control.Error import Control.Lens (view) -import Control.Monad.Catch import Data.ByteString.Conversion import Data.Code as Code import Data.Domain (Domain) @@ -96,7 +93,6 @@ import Polysemy (Member, Members) import Servant (Link, ToHttpApiData (toUrlPiece)) import System.Logger.Class (field, msg, val, (~~)) import qualified System.Logger.Class as Log -import UnliftIO.Async (Concurrently (Concurrently, runConcurrently)) import Wire.API.Federation.API.Brig (GetUserClients (GetUserClients)) import Wire.API.Federation.Error import Wire.API.MLS.Credential (ClientIdentity (..)) @@ -109,6 +105,7 @@ import Wire.API.User.Client import Wire.API.User.Client.DPoPAccessToken import Wire.API.User.Client.Prekey import Wire.API.UserMap (QualifiedUserMap (QualifiedUserMap, qualifiedUserMap), UserMap (userMap)) +import Wire.Sem.Concurrency import Wire.Sem.FromUTC (FromUTC (fromUTCTime)) import Wire.Sem.Now as Now @@ -241,40 +238,26 @@ rmClient u con clt pw = lift $ execDelete u (Just con) client claimPrekey :: - ( MonadReader Env m, - MonadMask m, - MonadHttp m, - HasRequestId m, - Log.MonadLogger m, - MonadClient m - ) => LegalholdProtectee -> UserId -> Domain -> ClientId -> - ExceptT ClientError m (Maybe ClientPrekey) + ExceptT ClientError (AppT r) (Maybe ClientPrekey) claimPrekey protectee u d c = do isLocalDomain <- (d ==) <$> viewFederationDomain if isLocalDomain then claimLocalPrekey protectee u c - else claimRemotePrekey (Qualified u d) c + else wrapClientE $ claimRemotePrekey (Qualified u d) c claimLocalPrekey :: - ( MonadClient m, - MonadReader Env m, - MonadMask m, - MonadHttp m, - HasRequestId m, - Log.MonadLogger m - ) => LegalholdProtectee -> UserId -> ClientId -> - ExceptT ClientError m (Maybe ClientPrekey) + ExceptT ClientError (AppT r) (Maybe ClientPrekey) claimLocalPrekey protectee user client = do guardLegalhold protectee (mkUserClients [(user, [client])]) lift $ do - prekey <- Data.claimPrekey user client + prekey <- wrapHttpClient $ Data.claimPrekey user client when (isNothing prekey) (noPrekeys user client) pure prekey @@ -298,14 +281,19 @@ claimPrekeyBundle protectee domain uid = do claimLocalPrekeyBundle :: LegalholdProtectee -> UserId -> ExceptT ClientError (AppT r) PrekeyBundle claimLocalPrekeyBundle protectee u = do clients <- map clientId <$> lift (wrapClient (Data.lookupClients u)) - mapExceptT wrapHttp $ guardLegalhold protectee (mkUserClients [(u, clients)]) + guardLegalhold protectee (mkUserClients [(u, clients)]) PrekeyBundle u . catMaybes <$> lift (mapM (wrapHttp . Data.claimPrekey u) clients) claimRemotePrekeyBundle :: Qualified UserId -> ExceptT ClientError (AppT r) PrekeyBundle claimRemotePrekeyBundle quser = do Federation.claimPrekeyBundle quser !>> ClientFederationError -claimMultiPrekeyBundles :: LegalholdProtectee -> QualifiedUserClients -> ExceptT ClientError (AppT r) QualifiedUserClientPrekeyMap +claimMultiPrekeyBundles :: + forall r. + Members '[Concurrency 'Unsafe] r => + LegalholdProtectee -> + QualifiedUserClients -> + ExceptT ClientError (AppT r) QualifiedUserClientPrekeyMap claimMultiPrekeyBundles protectee quc = do loc <- qualifyLocal () let (locals, remotes) = @@ -341,11 +329,13 @@ claimMultiPrekeyBundles protectee quc = do <$> claimLocalMultiPrekeyBundles protectee (tUnqualified luc) claimLocalMultiPrekeyBundles :: + forall r. + Members '[Concurrency 'Unsafe] r => LegalholdProtectee -> UserClients -> ExceptT ClientError (AppT r) UserClientPrekeyMap claimLocalMultiPrekeyBundles protectee userClients = do - mapExceptT wrapHttp $ guardLegalhold protectee userClients + guardLegalhold protectee userClients lift . fmap mkUserClientPrekeyMap . foldMap (getChunk . Map.fromList) @@ -355,34 +345,27 @@ claimLocalMultiPrekeyBundles protectee userClients = do $ userClients where getChunk :: Map UserId (Set ClientId) -> AppT r (Map UserId (Map ClientId (Maybe Prekey))) - getChunk = - wrapHttpClient . runConcurrently . Map.traverseWithKey (\u -> Concurrently . getUserKeys u) + getChunk m = do + e <- ask + AppT $ + lift $ + fmap (Map.fromListWith (<>)) $ + unsafePooledMapConcurrentlyN + 16 + (\(u, cids) -> (u,) <$> lowerAppT e (getUserKeys u cids)) + (Map.toList m) getUserKeys :: - ( MonadClient m, - Log.MonadLogger m, - MonadMask m, - MonadReader Env m, - MonadHttp m, - HasRequestId m - ) => UserId -> Set ClientId -> - m (Map ClientId (Maybe Prekey)) + (AppT r) (Map ClientId (Maybe Prekey)) getUserKeys u = sequenceA . Map.fromSet (getClientKeys u) getClientKeys :: - ( MonadClient m, - Log.MonadLogger m, - MonadMask m, - MonadReader Env m, - MonadHttp m, - HasRequestId m - ) => UserId -> ClientId -> - m (Maybe Prekey) + (AppT r) (Maybe Prekey) getClientKeys u c = do - key <- fmap prekeyData <$> Data.claimPrekey u c + key <- fmap prekeyData <$> wrapHttpClient (Data.claimPrekey u c) when (isNothing key) $ noPrekeys u c pure key @@ -402,28 +385,23 @@ execDelete u con c = do -- thus repairing any inconsistencies related to distributed -- (and possibly duplicated) client data. noPrekeys :: - ( MonadReader Env m, - MonadMask m, - MonadHttp m, - HasRequestId m, - Log.MonadLogger m, - MonadClient m - ) => UserId -> ClientId -> - m () + (AppT r) () noPrekeys u c = do - Log.info $ - field "user" (toByteString u) - ~~ field "client" (toByteString c) - ~~ msg (val "No prekey found. Ensuring client does not exist.") - Intra.rmClient u c - client <- Data.lookupClient u c - for_ client $ \_ -> - Log.err $ - field "user" (toByteString u) - ~~ field "client" (toByteString c) - ~~ msg (val "Client exists without prekeys.") + mclient <- wrapClient $ Data.lookupClient u c + case mclient of + Nothing -> do + Log.warn $ + field "user" (toByteString u) + ~~ field "client" (toByteString c) + ~~ msg (val "No prekey found. Client is missing, so doing nothing.") + Just client -> do + Log.warn $ + field "user" (toByteString u) + ~~ field "client" (toByteString c) + ~~ msg (val "No prekey found. Deleting client.") + execDelete u Nothing client pubClient :: Client -> PubClient pubClient c = diff --git a/services/brig/src/Brig/API/Federation.hs b/services/brig/src/Brig/API/Federation.hs index 5f721785834..80100b8d8b3 100644 --- a/services/brig/src/Brig/API/Federation.hs +++ b/services/brig/src/Brig/API/Federation.hs @@ -47,7 +47,7 @@ import Data.Range import qualified Gundeck.Types.Push as Push import Imports import Network.Wai.Utilities.Error ((!>>)) -import Polysemy (Members) +import Polysemy import Servant (ServerT) import Servant.API import UnliftIO.Async (pooledForConcurrentlyN_) @@ -64,12 +64,14 @@ import Wire.API.User.Client import Wire.API.User.Client.Prekey import Wire.API.User.Search import Wire.API.UserMap (UserMap) +import Wire.Sem.Concurrency type FederationAPI = "federation" :> BrigApi federationSitemap :: Members - '[ GalleyProvider + '[ GalleyProvider, + Concurrency 'Unsafe ] r => ServerT FederationAPI (Handler r) @@ -138,13 +140,17 @@ getUsersByIds _ uids = claimPrekey :: Domain -> (UserId, ClientId) -> (Handler r) (Maybe ClientPrekey) claimPrekey _ (user, client) = do - wrapHttpClientE (API.claimLocalPrekey LegalholdPlusFederationNotImplemented user client) !>> clientError + API.claimLocalPrekey LegalholdPlusFederationNotImplemented user client !>> clientError claimPrekeyBundle :: Domain -> UserId -> (Handler r) PrekeyBundle claimPrekeyBundle _ user = API.claimLocalPrekeyBundle LegalholdPlusFederationNotImplemented user !>> clientError -claimMultiPrekeyBundle :: Domain -> UserClients -> (Handler r) UserClientPrekeyMap +claimMultiPrekeyBundle :: + Members '[Concurrency 'Unsafe] r => + Domain -> + UserClients -> + Handler r UserClientPrekeyMap claimMultiPrekeyBundle _ uc = API.claimLocalMultiPrekeyBundles LegalholdPlusFederationNotImplemented uc !>> clientError fedClaimKeyPackages :: Domain -> ClaimKeyPackageRequest -> Handler r (Maybe KeyPackageBundle) diff --git a/services/brig/src/Brig/API/MLS/KeyPackages.hs b/services/brig/src/Brig/API/MLS/KeyPackages.hs index 49cd60de864..9748e920f29 100644 --- a/services/brig/src/Brig/API/MLS/KeyPackages.hs +++ b/services/brig/src/Brig/API/MLS/KeyPackages.hs @@ -76,10 +76,9 @@ claimLocalKeyPackages qusr skipOwn target = do foldQualified target ( \lusr -> - wrapHttpClientE $ - guardLegalhold - (ProtectedUser (tUnqualified lusr)) - (mkUserClients [(tUnqualified target, clients)]) + guardLegalhold + (ProtectedUser (tUnqualified lusr)) + (mkUserClients [(tUnqualified target, clients)]) ) (\_ -> pure ()) qusr diff --git a/services/brig/src/Brig/API/Public.hs b/services/brig/src/Brig/API/Public.hs index 6620bf1f9fb..029594b6e41 100644 --- a/services/brig/src/Brig/API/Public.hs +++ b/services/brig/src/Brig/API/Public.hs @@ -163,16 +163,17 @@ swaggerDocsAPI Nothing = swaggerDocsAPI (Just maxBound) servantSitemap :: forall r p. Members - '[ BlacklistStore, - BlacklistPhonePrefixStore, - GalleyProvider, - UserPendingActivationStore p, - PasswordResetStore, + '[ BlacklistPhonePrefixStore, + BlacklistStore, CodeStore, + Concurrency 'Unsafe, + Concurrency 'Unsafe, + GalleyProvider, JwtTools, + Now, + PasswordResetStore, PublicKeyBundle, - Concurrency 'Unsafe, - Now + UserPendingActivationStore p ] r => ServerT BrigAPI (Handler r) @@ -293,11 +294,12 @@ servantSitemap = userAPI :<|> selfAPI :<|> accountAPI :<|> clientAPI :<|> prekey sitemap :: Members - '[ CodeStore, - PasswordResetStore, + '[ BlacklistPhonePrefixStore, BlacklistStore, - BlacklistPhonePrefixStore, - GalleyProvider + CodeStore, + Concurrency 'Unsafe, + GalleyProvider, + PasswordResetStore ] r => Routes Doc.ApiBuilder (Handler r) () @@ -310,11 +312,12 @@ sitemap = do apiDocs :: forall r. Members - '[ CodeStore, - PasswordResetStore, + '[ BlacklistPhonePrefixStore, BlacklistStore, - BlacklistPhonePrefixStore, - GalleyProvider + CodeStore, + Concurrency 'Unsafe, + GalleyProvider, + PasswordResetStore ] r => Routes Doc.ApiBuilder (Handler r) () @@ -394,7 +397,7 @@ getPrekeyUnqualifiedH zusr user client = do getPrekeyH :: UserId -> Qualified UserId -> ClientId -> (Handler r) Public.ClientPrekey getPrekeyH zusr (Qualified user domain) client = do - mPrekey <- wrapHttpClientE $ API.claimPrekey (ProtectedUser zusr) user domain client !>> clientError + mPrekey <- API.claimPrekey (ProtectedUser zusr) user domain client !>> clientError ifNothing (notFound "prekey not found") mPrekey getPrekeyBundleUnqualifiedH :: UserId -> UserId -> (Handler r) Public.PrekeyBundle @@ -406,14 +409,22 @@ getPrekeyBundleH :: UserId -> Qualified UserId -> (Handler r) Public.PrekeyBundl getPrekeyBundleH zusr (Qualified uid domain) = API.claimPrekeyBundle (ProtectedUser zusr) domain uid !>> clientError -getMultiUserPrekeyBundleUnqualifiedH :: UserId -> Public.UserClients -> (Handler r) Public.UserClientPrekeyMap +getMultiUserPrekeyBundleUnqualifiedH :: + Members '[Concurrency 'Unsafe] r => + UserId -> + Public.UserClients -> + Handler r Public.UserClientPrekeyMap getMultiUserPrekeyBundleUnqualifiedH zusr userClients = do maxSize <- fromIntegral . setMaxConvSize <$> view settings when (Map.size (Public.userClients userClients) > maxSize) $ throwStd (errorToWai @'E.TooManyClients) API.claimLocalMultiPrekeyBundles (ProtectedUser zusr) userClients !>> clientError -getMultiUserPrekeyBundleH :: UserId -> Public.QualifiedUserClients -> (Handler r) Public.QualifiedUserClientPrekeyMap +getMultiUserPrekeyBundleH :: + Members '[Concurrency 'Unsafe] r => + UserId -> + Public.QualifiedUserClients -> + (Handler r) Public.QualifiedUserClientPrekeyMap getMultiUserPrekeyBundleH zusr qualUserClients = do maxSize <- fromIntegral . setMaxConvSize <$> view settings let Sum (size :: Int) = diff --git a/services/brig/src/Brig/CanonicalInterpreter.hs b/services/brig/src/Brig/CanonicalInterpreter.hs index 32141859174..e57229eeef6 100644 --- a/services/brig/src/Brig/CanonicalInterpreter.hs +++ b/services/brig/src/Brig/CanonicalInterpreter.hs @@ -27,8 +27,8 @@ import Imports import Polysemy (Embed, Final, embedToFinal, runFinal) import Polysemy.Error (Error, mapError, runError) import Polysemy.TinyLog (TinyLog) -import Wire.Sem.Concurrency (Concurrency, ConcurrencySafety (Unsafe)) -import Wire.Sem.Concurrency.IO (unsafelyPerformConcurrency) +import Wire.Sem.Concurrency +import Wire.Sem.Concurrency.IO import Wire.Sem.Logger.TinyLog (loggerToTinyLog) import Wire.Sem.Now (Now) import Wire.Sem.Now.IO (nowToIOAction) diff --git a/services/brig/src/Brig/IO/Intra.hs b/services/brig/src/Brig/IO/Intra.hs index 88c1894a972..4c0bedd91a4 100644 --- a/services/brig/src/Brig/IO/Intra.hs +++ b/services/brig/src/Brig/IO/Intra.hs @@ -986,17 +986,11 @@ getTeamContacts u = do . expect [status200, status404] guardLegalhold :: - ( MonadReader Env m, - MonadIO m, - MonadMask m, - MonadHttp m, - HasRequestId m - ) => LegalholdProtectee -> UserClients -> - ExceptT ClientError m () + ExceptT ClientError (AppT r) () guardLegalhold protectee userClients = do - res <- lift $ galleyRequest PUT req + res <- lift . wrapHttp $ galleyRequest PUT req case Bilge.statusCode res of 200 -> pure () 403 -> throwE ClientMissingLegalholdConsent diff --git a/services/brig/src/Brig/Provider/API.hs b/services/brig/src/Brig/Provider/API.hs index 3b216f787c0..f8c1c4ad0f4 100644 --- a/services/brig/src/Brig/Provider/API.hs +++ b/services/brig/src/Brig/Provider/API.hs @@ -92,7 +92,7 @@ import qualified OpenSSL.EVP.PKey as SSL import qualified OpenSSL.PEM as SSL import qualified OpenSSL.RSA as SSL import OpenSSL.Random (randBytes) -import Polysemy (Members) +import Polysemy import qualified Ssl.Util as SSL import System.Logger.Class (MonadLogger) import UnliftIO.Async (pooledMapConcurrentlyN_) @@ -122,8 +122,11 @@ import Wire.API.User.Client import qualified Wire.API.User.Client as Public (Client, ClientCapability (ClientSupportsLegalholdImplicitConsent), PubClient (..), UserClientPrekeyMap, UserClients, userClients) import qualified Wire.API.User.Client.Prekey as Public (PrekeyId) import qualified Wire.API.User.Identity as Public (Email) +import Wire.Sem.Concurrency (Concurrency, ConcurrencySafety (Unsafe)) -routesPublic :: Members '[GalleyProvider] r => Routes Doc.ApiBuilder (Handler r) () +routesPublic :: + Members '[GalleyProvider, Concurrency 'Unsafe] r => + Routes Doc.ApiBuilder (Handler r) () routesPublic = do -- Public API (Unauthenticated) -------------------------------------------- @@ -1024,12 +1027,18 @@ botUpdatePrekeys bot upd = do let pks = updateBotPrekeyList upd wrapClientE (User.updatePrekeys (botUserId bot) (clientId c) pks) !>> clientDataError -botClaimUsersPrekeysH :: Members '[GalleyProvider] r => JsonRequest Public.UserClients -> (Handler r) Response +botClaimUsersPrekeysH :: + Members '[GalleyProvider, Concurrency 'Unsafe] r => + JsonRequest Public.UserClients -> + Handler r Response botClaimUsersPrekeysH req = do guardSecondFactorDisabled Nothing json <$> (botClaimUsersPrekeys =<< parseJsonBody req) -botClaimUsersPrekeys :: Public.UserClients -> (Handler r) Public.UserClientPrekeyMap +botClaimUsersPrekeys :: + Members '[Concurrency 'Unsafe] r => + Public.UserClients -> + Handler r Public.UserClientPrekeyMap botClaimUsersPrekeys body = do maxSize <- fromIntegral . setMaxConvSize <$> view settings when (Map.size (Public.userClients body) > maxSize) $ diff --git a/services/brig/test/integration/API/User/Client.hs b/services/brig/test/integration/API/User/Client.hs index a942cc5739e..1123a894815 100644 --- a/services/brig/test/integration/API/User/Client.hs +++ b/services/brig/test/integration/API/User/Client.hs @@ -38,6 +38,7 @@ import Data.Aeson hiding (json) import Data.Aeson.Lens import Data.ByteString.Conversion import Data.Default +import Data.Domain (Domain (..)) import Data.Id hiding (client) import qualified Data.List1 as List1 import qualified Data.Map as Map @@ -87,6 +88,7 @@ tests _cl _at opts p db b c g = test p "post /users/list-prekeys" $ testMultiUserGetPrekeysQualified b opts, test p "post /users/list-clients - 200" $ testListClientsBulk opts b, test p "post /users/list-clients/v2 - 200" $ testListClientsBulkV2 opts b, + test p "post /users/list-prekeys - clients without prekeys" $ testClientsWithoutPrekeys b c db opts, test p "post /clients - 201 (pwd)" $ testAddGetClient def {addWithPassword = True} b c, test p "post /clients - 201 (no pwd)" $ testAddGetClient def {addWithPassword = False} b c, testGroup @@ -399,6 +401,94 @@ testListClientsBulk opts brig = do const 200 === statusCode const (Just expectedResponse) === responseJsonMaybe +testClientsWithoutPrekeys :: Brig -> Cannon -> DB.ClientState -> Opt.Opts -> Http () +testClientsWithoutPrekeys brig cannon db opts = do + uid1 <- userId <$> randomUser brig + let (pk11, lk11) = (somePrekeys !! 0, someLastPrekeys !! 0) + c11 <- responseJsonError =<< addClient brig uid1 (defNewClient PermanentClientType [pk11] lk11) + let (pk12, lk12) = (somePrekeys !! 1, someLastPrekeys !! 1) + c12 <- responseJsonError =<< addClient brig uid1 (defNewClient PermanentClientType [pk12] lk12) + + -- Simulating loss of all prekeys from c11 (due e.g. DB problems, business + -- logic prevents this from happening) + let removeClientKeys :: DB.PrepQuery DB.W (UserId, ClientId) () + removeClientKeys = "DELETE FROM prekeys where user = ? and client = ?" + liftIO $ + DB.runClient db $ DB.write removeClientKeys (DB.params DB.LocalQuorum (uid1, clientId c11)) + + uid2 <- userId <$> randomUser brig + + let domain = opts ^. Opt.optionSettings & Opt.setFederationDomain + + let userClients = + QualifiedUserClients $ + Map.singleton domain $ + Map.singleton uid1 $ + Set.fromList [clientId c11, clientId c12] + + WS.bracketR cannon uid1 $ \ws -> do + getClient brig uid1 (clientId c11) !!! do + const 200 === statusCode + + post + ( brig + . paths ["users", "list-prekeys"] + . contentJson + . body (RequestBodyLBS $ encode userClients) + . zUser uid2 + ) + !!! do + const 200 === statusCode + const + ( Right $ + ( expectedClientMap + domain + uid1 + [ (clientId c11, Nothing), + (clientId c12, Just pk12) + ] + ) + ) + === responseJsonEither + + getClient brig uid1 (clientId c11) !!! do + const 404 === statusCode + + liftIO . WS.assertMatch (5 # Second) ws $ \n -> do + let ob = Object $ List1.head (ntfPayload n) + ob ^? key "type" . _String + @?= Just "user.client-remove" + fmap ClientId (ob ^? key "client" . key "id" . _String) + @?= Just (clientId c11) + + post + ( brig + . paths ["users", "list-prekeys"] + . contentJson + . body (RequestBodyLBS $ encode userClients) + . zUser uid2 + ) + !!! do + const 200 === statusCode + const + ( Right $ + expectedClientMap + domain + uid1 + [ (clientId c11, Nothing), + (clientId c12, Just (unpackLastPrekey lk12)) + ] + ) + === responseJsonEither + where + expectedClientMap :: Domain -> UserId -> [(ClientId, Maybe Prekey)] -> QualifiedUserClientPrekeyMap + expectedClientMap domain u xs = + mkQualifiedUserClientPrekeyMap $ + Map.singleton domain $ + mkUserClientPrekeyMap $ + Map.singleton u $ + Map.fromList xs + testListClientsBulkV2 :: Opt.Opts -> Brig -> Http () testListClientsBulkV2 opts brig = do uid1 <- userId <$> randomUser brig