diff --git a/changelog.d/6-federation/optimize-user-deletion b/changelog.d/6-federation/optimize-user-deletion index a160e66fc0..e5e083c602 100644 --- a/changelog.d/6-federation/optimize-user-deletion +++ b/changelog.d/6-federation/optimize-user-deletion @@ -1 +1 @@ -When a user gets deleted, notify remotes about conversations and connections in chunks of 1000 \ No newline at end of file +When a user gets deleted, notify remotes about conversations and connections in chunks of 1000 (#1872, #1883) \ No newline at end of file diff --git a/docs/developer/federation-api-conventions.md b/docs/developer/federation-api-conventions.md index 6971d4b042..5703ffe3af 100644 --- a/docs/developer/federation-api-conventions.md +++ b/docs/developer/federation-api-conventions.md @@ -3,12 +3,14 @@ # Federation API Conventions - All endpoints must start with `/federation/` -- All endpoints must have exactly one path segment after federation, so - `/federation/foo` is valid `/fedeartion/foo/bar` is not. The path segments - must be in kebab-case. The name of the field in this record must be the - same name in camelCase. +- All path segments must be in kebab-case. The name the field in the record must + be the same name in camelCase. +- There can be either one or two path segments after `/federation/`, so + `/federation/foo` is valid, `/fedeartion/foo/bar` is valid, but + `/federation/foo/bar/baz` is not. - All endpoints must be `POST`. -- No query query params, all information that needs to go must go in body. +- No query query params or captured path params, all information that needs to + go must go in body. - All responses must be `200 OK`, domain specific failures (e.g. the conversation doesn't exist) must be indicated as a Sum type. Unhandled failures can be 5xx, an endpoint not being implemented will of course @@ -16,9 +18,11 @@ - Accept only json, respond with only json. Maybe we can think of changing this in future. But as of now, the federator hardcodes application/json as the content type of the body. -- Name of the last path segment must be either `-` or - `on--`, e.g. `get-conversations` or - `on-conversation-created`. +- Ensure that paths don't collide between brig and galley federation API, this + will be very helpful when we merge brig and galley. +- Name of the first path segment after `/federation/` must be either + `-` or `on--`, e.g. + `get-conversations` or `on-conversation-created`. How to decide which one to use: - If the request is supposed to ask for information/change from another @@ -29,3 +33,9 @@ this request has authority on, like a conversation got created, or a message is sent, then use the second format like `on-conversation-created` or `on-message-sent` +- Path segment number 3 (so `/federation/not-this/but-this-one`), must only be + used in exceptional circumstances, like when there needs to be the same path + in brig and galley, e.g. `on-user-deleted`. In this case use the third segment + to express the difference. For `on-user-deleted` we came up with + `on-user-deleted/connections`for brig and `on-user-deleted/conversations` for + galley. diff --git a/libs/wire-api-federation/src/Wire/API/Federation/API/Brig.hs b/libs/wire-api-federation/src/Wire/API/Federation/API/Brig.hs index c41324c411..ba8bf3f1a9 100644 --- a/libs/wire-api-federation/src/Wire/API/Federation/API/Brig.hs +++ b/libs/wire-api-federation/src/Wire/API/Federation/API/Brig.hs @@ -107,8 +107,9 @@ data Api routes = Api routes :- "federation" :> "on-user-deleted" + :> "connections" :> OriginDomainHeader - :> ReqBody '[JSON] UserDeletedNotification + :> ReqBody '[JSON] UserDeletedConnectionsNotification :> Post '[JSON] EmptyResponse } deriving (Generic) @@ -160,15 +161,17 @@ data NewConnectionResponse deriving (Arbitrary) via (GenericUniform NewConnectionResponse) deriving (FromJSON, ToJSON) via (CustomEncoded NewConnectionResponse) -data UserDeletedNotification = UserDeletedNotification +type UserDeletedNotificationMaxConnections = 1000 + +data UserDeletedConnectionsNotification = UserDeletedConnectionsNotification { -- | This is qualified implicitly by the origin domain - udnUser :: UserId, + udcnUser :: UserId, -- | These are qualified implicitly by the target domain - udnConnections :: Range 1 1000 [UserId] + udcnConnections :: Range 1 UserDeletedNotificationMaxConnections [UserId] } deriving stock (Eq, Show, Generic) - deriving (Arbitrary) via (GenericUniform UserDeletedNotification) - deriving (FromJSON, ToJSON) via (CustomEncoded UserDeletedNotification) + deriving (Arbitrary) via (GenericUniform UserDeletedConnectionsNotification) + deriving (FromJSON, ToJSON) via (CustomEncoded UserDeletedConnectionsNotification) clientRoutes :: (MonadError FederationClientFailure m, MonadIO m) => Api (AsClientT (FederatorClient 'Proto.Brig m)) clientRoutes = genericClient diff --git a/libs/wire-api-federation/src/Wire/API/Federation/API/Galley.hs b/libs/wire-api-federation/src/Wire/API/Federation/API/Galley.hs index ff9841544d..6e258cc242 100644 --- a/libs/wire-api-federation/src/Wire/API/Federation/API/Galley.hs +++ b/libs/wire-api-federation/src/Wire/API/Federation/API/Galley.hs @@ -109,8 +109,9 @@ data Api routes = Api routes :- "federation" :> "on-user-deleted" + :> "conversations" :> OriginDomainHeader - :> ReqBody '[JSON] UserDeletedNotification + :> ReqBody '[JSON] UserDeletedConversationsNotification :> Post '[JSON] EmptyResponse } deriving (Generic) @@ -262,15 +263,17 @@ newtype LeaveConversationResponse = LeaveConversationResponse (ToJSON, FromJSON) via (Either (CustomEncoded RemoveFromConversationError) ()) -data UserDeletedNotification = UserDeletedNotification +type UserDeletedNotificationMaxConvs = 1000 + +data UserDeletedConversationsNotification = UserDeletedConversationsNotification { -- | This is qualified implicitly by the origin domain - udnUser :: UserId, + udcnUser :: UserId, -- | These are qualified implicitly by the target domain - udnConversations :: Range 1 1000 [ConvId] + udcnConversations :: Range 1 UserDeletedNotificationMaxConvs [ConvId] } deriving stock (Eq, Show, Generic) - deriving (Arbitrary) via (GenericUniform UserDeletedNotification) - deriving (FromJSON, ToJSON) via (CustomEncoded UserDeletedNotification) + deriving (Arbitrary) via (GenericUniform UserDeletedConversationsNotification) + deriving (FromJSON, ToJSON) via (CustomEncoded UserDeletedConversationsNotification) clientRoutes :: (MonadError FederationClientFailure m, MonadIO m) => Api (AsClientT (FederatorClient 'Proto.Galley m)) clientRoutes = genericClient diff --git a/services/brig/src/Brig/API/Federation.hs b/services/brig/src/Brig/API/Federation.hs index 496b7e5caf..9ae84c6eee 100644 --- a/services/brig/src/Brig/API/Federation.hs +++ b/services/brig/src/Brig/API/Federation.hs @@ -123,10 +123,10 @@ searchUsers (SearchRequest searchTerm) = do getUserClients :: GetUserClients -> Handler (UserMap (Set PubClient)) getUserClients (GetUserClients uids) = API.lookupLocalPubClientsBulk uids !>> clientError -onUserDeleted :: Domain -> UserDeletedNotification -> Handler EmptyResponse -onUserDeleted origDomain udn = lift $ do - let deletedUser = toRemoteUnsafe origDomain (udnUser udn) - connections = udnConnections udn +onUserDeleted :: Domain -> UserDeletedConnectionsNotification -> Handler EmptyResponse +onUserDeleted origDomain udcn = lift $ do + let deletedUser = toRemoteUnsafe origDomain (udcnUser udcn) + connections = udcnConnections udcn event = pure . UserEvent $ UserDeleted (qUntagged deletedUser) acceptedLocals <- map csv2From diff --git a/services/brig/src/Brig/Federation/Client.hs b/services/brig/src/Brig/Federation/Client.hs index 3eb23f6662..951d67b4d4 100644 --- a/services/brig/src/Brig/Federation/Client.hs +++ b/services/brig/src/Brig/Federation/Client.hs @@ -104,5 +104,5 @@ notifyUserDeleted self remotes = do let remoteConnections = tUnqualified remotes let fedRPC = FederatedBrig.onUserDeleted clientRoutes (tDomain self) $ - UserDeletedNotification (tUnqualified self) remoteConnections + UserDeletedConnectionsNotification (tUnqualified self) remoteConnections void $ executeFederated (tDomain remotes) fedRPC diff --git a/services/brig/src/Brig/IO/Intra.hs b/services/brig/src/Brig/IO/Intra.hs index ff5617bff1..6ceed1c77f 100644 --- a/services/brig/src/Brig/IO/Intra.hs +++ b/services/brig/src/Brig/IO/Intra.hs @@ -98,9 +98,11 @@ import Data.Id import Data.Json.Util (UTCTimeMillis, (#)) import Data.List.Split (chunksOf) import Data.List1 (List1, list1, singleton) +import Data.Proxy import Data.Qualified import Data.Range import qualified Data.Set as Set +import GHC.TypeLits import Galley.Types (Connect (..), Conversation) import Galley.Types.Conversations.Intra (UpsertOne2OneConversationRequest, UpsertOne2OneConversationResponse) import qualified Galley.Types.Teams as Team @@ -114,6 +116,7 @@ import Network.HTTP.Types.Method import Network.HTTP.Types.Status import qualified Network.Wai.Utilities.Error as Wai import System.Logger.Class as Log hiding (name, (.=)) +import Wire.API.Federation.API.Brig import Wire.API.Federation.Client import Wire.API.Federation.Error (federationErrorToWai, federationNotImplemented) import Wire.API.Message (UserClients) @@ -256,7 +259,7 @@ notifyUserDeletionLocals deleted conn event = do notifyUserDeletionRemotes :: UserId -> AppIO () notifyUserDeletionRemotes deleted = do runConduit $ - Data.lookupRemoteConnectedUsersC deleted 1000 + Data.lookupRemoteConnectedUsersC deleted (fromInteger (natVal (Proxy @UserDeletedNotificationMaxConnections))) .| C.mapM_ fanoutNotifications where fanoutNotifications :: [Remote UserId] -> AppIO () @@ -264,8 +267,7 @@ notifyUserDeletionRemotes deleted = do notifyBackend :: Remote [UserId] -> AppIO () notifyBackend uids = do - let rangedMaybeUids = checked @_ @1 @1000 <$> uids - case tUnqualified rangedMaybeUids of + case tUnqualified (checked <$> uids) of Nothing -> -- The user IDs cannot be more than 1000, so we can assume the range -- check will only fail because there are 0 User Ids. diff --git a/services/brig/test/integration/API/Federation.hs b/services/brig/test/integration/API/Federation.hs index 72e7edf372..7f8960d7e6 100644 --- a/services/brig/test/integration/API/Federation.hs +++ b/services/brig/test/integration/API/Federation.hs @@ -42,7 +42,7 @@ import Test.Tasty import qualified Test.Tasty.Cannon as WS import Test.Tasty.HUnit (assertEqual, assertFailure) import Util -import Wire.API.Federation.API.Brig (GetUserClients (..), SearchRequest (SearchRequest), UserDeletedNotification (..)) +import Wire.API.Federation.API.Brig (GetUserClients (..), SearchRequest (SearchRequest), UserDeletedConnectionsNotification (..)) import qualified Wire.API.Federation.API.Brig as FedBrig import Wire.API.Message (UserClients (..)) import Wire.API.User.Client (mkUserClientPrekeyMap) @@ -67,7 +67,7 @@ tests m opts brig cannon fedBrigClient = test m "POST /federation/claim-multi-prekey-bundle : 200" (testClaimMultiPrekeyBundleSuccess brig fedBrigClient), test m "POST /federation/get-user-clients : 200" (testGetUserClients brig fedBrigClient), test m "POST /federation/get-user-clients : Not Found" (testGetUserClientsNotFound fedBrigClient), - test m "POST /federation/on-user-deleted : 200" (testRemoteUserGetsDeleted opts brig cannon fedBrigClient) + test m "POST /federation/on-user-deleted/connections : 200" (testRemoteUserGetsDeleted opts brig cannon fedBrigClient) ] testSearchSuccess :: Brig -> FedBrigClient -> Http () @@ -238,7 +238,7 @@ testRemoteUserGetsDeleted opts brig cannon fedBrigClient = do FedBrig.onUserDeleted fedBrigClient (qDomain remoteUser) - (UserDeletedNotification (qUnqualified remoteUser) (unsafeRange localUsers)) + (UserDeletedConnectionsNotification (qUnqualified remoteUser) (unsafeRange localUsers)) WS.assertMatchN_ (5 # Second) [cc] $ matchDeleteUserNotification remoteUser WS.assertNoEvent (1 # Second) [pc, bc, uc] diff --git a/services/brig/test/integration/API/User/Account.hs b/services/brig/test/integration/API/User/Account.hs index 1e71e967e2..fcc529fd2d 100644 --- a/services/brig/test/integration/API/User/Account.hs +++ b/services/brig/test/integration/API/User/Account.hs @@ -77,7 +77,7 @@ import UnliftIO (mapConcurrently_) import Util as Util import Util.AWS as Util import Web.Cookie (parseSetCookie) -import Wire.API.Federation.API.Brig (UserDeletedNotification (..)) +import Wire.API.Federation.API.Brig (UserDeletedConnectionsNotification (..)) import qualified Wire.API.Federation.API.Brig as FedBrig import Wire.API.Federation.API.Common (EmptyResponse (EmptyResponse)) import Wire.API.Federation.GRPC.Types (OutwardResponse (OutwardResponseBody)) @@ -1241,14 +1241,14 @@ testDeleteWithRemotes opts brig = do liftIO $ do remote1Call <- assertOne $ filter (\c -> F.domain c == domainText remote1Domain) rpcCalls remote1Udn <- assertRight $ parseFedRequest remote1Call - udnUser remote1Udn @?= userId localUser - sort (fromRange (udnConnections remote1Udn)) + udcnUser remote1Udn @?= userId localUser + sort (fromRange (udcnConnections remote1Udn)) @?= sort (map qUnqualified [remote1UserConnected, remote1UserPending]) remote2Call <- assertOne $ filter (\c -> F.domain c == domainText remote2Domain) rpcCalls remote2Udn <- assertRight $ parseFedRequest remote2Call - udnUser remote2Udn @?= userId localUser - fromRange (udnConnections remote2Udn) @?= [qUnqualified remote2UserBlocked] + udcnUser remote2Udn @?= userId localUser + fromRange (udcnConnections remote2Udn) @?= [qUnqualified remote2UserBlocked] where parseFedRequest :: FromJSON a => F.FederatedRequest -> Either String a parseFedRequest fr = diff --git a/services/galley/src/Galley/API/Federation.hs b/services/galley/src/Galley/API/Federation.hs index f802f256e3..b26697a00b 100644 --- a/services/galley/src/Galley/API/Federation.hs +++ b/services/galley/src/Galley/API/Federation.hs @@ -64,7 +64,7 @@ import Wire.API.Federation.API.Galley MessageSendResponse (..), NewRemoteConversation (..), RemoteMessage (..), - UserDeletedNotification, + UserDeletedConversationsNotification, ) import qualified Wire.API.Federation.API.Galley as FederationAPIGalley import Wire.API.Routes.Internal.Brig.Connection @@ -278,11 +278,11 @@ sendMessage originDomain msr = do where err = throwM . invalidPayload . LT.pack -onUserDeleted :: Domain -> UserDeletedNotification -> Galley EmptyResponse -onUserDeleted origDomain udn = do - let deletedUser = toRemoteUnsafe origDomain (FederationAPIGalley.udnUser udn) +onUserDeleted :: Domain -> UserDeletedConversationsNotification -> Galley EmptyResponse +onUserDeleted origDomain udcn = do + let deletedUser = toRemoteUnsafe origDomain (FederationAPIGalley.udcnUser udcn) untaggedDeletedUser = qUntagged deletedUser - convIds = FederationAPIGalley.udnConversations udn + convIds = FederationAPIGalley.udcnConversations udcn pooledForConcurrentlyN_ 16 (fromRange convIds) $ \c -> do lc <- qualifyLocal c mconv <- Data.conversation c diff --git a/services/galley/src/Galley/API/Internal.hs b/services/galley/src/Galley/API/Internal.hs index 546f09154f..400f7d775a 100644 --- a/services/galley/src/Galley/API/Internal.hs +++ b/services/galley/src/Galley/API/Internal.hs @@ -81,7 +81,7 @@ import System.Logger.Class hiding (Path, name) import qualified System.Logger.Class as Log import Wire.API.Conversation (ConvIdsPage, pattern GetPaginatedConversationIds) import Wire.API.ErrorDescription (MissingLegalholdConsent) -import Wire.API.Federation.API.Galley (UserDeletedNotification (UserDeletedNotification)) +import Wire.API.Federation.API.Galley (UserDeletedConversationsNotification (UserDeletedConversationsNotification)) import qualified Wire.API.Federation.API.Galley as FedGalley import Wire.API.Federation.Client (executeFederated) import Wire.API.Routes.MultiTablePaging (mtpHasMore, mtpPagingState, mtpResults) @@ -522,10 +522,10 @@ rmUser user conn = do (maybeList1 (catMaybes pp)) Intra.push - leaveRemoteConversations :: Local UserId -> Range 1 1000 [Remote ConvId] -> Galley () + leaveRemoteConversations :: Local UserId -> Range 1 FedGalley.UserDeletedNotificationMaxConvs [Remote ConvId] -> Galley () leaveRemoteConversations lusr cids = do for_ (bucketRemote (fromRange cids)) $ \remoteConvs -> do - let userDelete = UserDeletedNotification (tUnqualified lusr) (unsafeRange (tUnqualified remoteConvs)) + let userDelete = UserDeletedConversationsNotification (tUnqualified lusr) (unsafeRange (tUnqualified remoteConvs)) let rpc = FedGalley.onUserDeleted FedGalley.clientRoutes (tDomain lusr) userDelete res <- runExceptT (executeFederated (tDomain remoteConvs) rpc) case res of diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs index 7f1c7207f8..cd61c67eb4 100644 --- a/services/galley/test/integration/API.hs +++ b/services/galley/test/integration/API.hs @@ -3199,16 +3199,16 @@ removeUser = do cReq <- assertOne $ filter (\req -> F.domain req == domainText cDomain) fedRequests liftIO $ do fmap F.component (F.request bReq) @?= Just F.Galley - fmap F.path (F.request bReq) @?= Just "/federation/on-user-deleted" - Just (Right udnB) <- pure $ fmap (eitherDecode . LBS.fromStrict . F.body) (F.request bReq) - sort (fromRange (FederatedGalley.udnConversations udnB)) @?= sort [convB1, convB2] - FederatedGalley.udnUser udnB @?= qUnqualified alexDel + fmap F.path (F.request bReq) @?= Just "/federation/on-user-deleted/conversations" + Just (Right udcnB) <- pure $ fmap (eitherDecode . LBS.fromStrict . F.body) (F.request bReq) + sort (fromRange (FederatedGalley.udcnConversations udcnB)) @?= sort [convB1, convB2] + FederatedGalley.udcnUser udcnB @?= qUnqualified alexDel fmap F.component (F.request bReq) @?= Just F.Galley - fmap F.path (F.request cReq) @?= Just "/federation/on-user-deleted" - Just (Right udnC) <- pure $ fmap (eitherDecode . LBS.fromStrict . F.body) (F.request cReq) - sort (fromRange (FederatedGalley.udnConversations udnC)) @?= sort [convC1] - FederatedGalley.udnUser udnC @?= qUnqualified alexDel + fmap F.path (F.request cReq) @?= Just "/federation/on-user-deleted/conversations" + Just (Right udcnC) <- pure $ fmap (eitherDecode . LBS.fromStrict . F.body) (F.request cReq) + sort (fromRange (FederatedGalley.udcnConversations udcnC)) @?= sort [convC1] + FederatedGalley.udcnUser udcnC @?= qUnqualified alexDel WS.assertMatchN_ (5 # Second) [wsAlice, wsAlexDel] $ wsAssertMembersLeave qconvA1 alexDel [alexDel] diff --git a/services/galley/test/integration/API/Federation.hs b/services/galley/test/integration/API/Federation.hs index 82ef1ea967..2665106b2d 100644 --- a/services/galley/test/integration/API/Federation.hs +++ b/services/galley/test/integration/API/Federation.hs @@ -83,7 +83,7 @@ tests s = test s "POST /federation/leave-conversation : Success" leaveConversationSuccess, test s "POST /federation/on-message-sent : Receive a message from another backend" onMessageSent, test s "POST /federation/send-message : Post a message sent from another backend" sendMessage, - test s "POST /federation/on-user-deleted : Remove deleted remote user from local conversations" onUserDeleted + test s "POST /federation/on-user-deleted/conversations : Remove deleted remote user from local conversations" onUserDeleted ] getConversationsAllFound :: TestM () @@ -912,10 +912,10 @@ onUserDeleted = do WS.bracketR2 cannon (tUnqualified alice) (qUnqualified charlie) $ \(wsAlice, wsCharlie) -> do (resp, rpcCalls) <- withTempMockFederator (const ()) $ do - let udn = - FedGalley.UserDeletedNotification - { FedGalley.udnUser = tUnqualified bob, - FedGalley.udnConversations = + let udcn = + FedGalley.UserDeletedConversationsNotification + { FedGalley.udcnUser = tUnqualified bob, + FedGalley.udcnConversations = unsafeRange [ qUnqualified ooConvId, qUnqualified groupConvId, @@ -927,10 +927,10 @@ onUserDeleted = do responseJsonError =<< post ( g - . paths ["federation", "on-user-deleted"] + . paths ["federation", "on-user-deleted", "conversations"] . content "application/json" . header "Wire-Origin-Domain" (toByteString' (tDomain bob)) - . json udn + . json udcn )