From a21a053afda2c5b4faa5cfcd2efcfd206928728b Mon Sep 17 00:00:00 2001 From: Paolo Capriotti Date: Mon, 23 Oct 2023 11:04:28 +0200 Subject: [PATCH 1/3] Fix extra remove proposal bug We were sending external remove proposals for each client of a user that was kicked out of a conversation following a remove commit. This was caused by some overgeneralisation of the mechanism that removes clients from subconversations when a user is deleted from the main. --- services/galley/src/Galley/API/Action.hs | 4 +- services/galley/src/Galley/API/Federation.hs | 2 +- services/galley/src/Galley/API/Internal.hs | 2 +- services/galley/src/Galley/API/MLS/Removal.hs | 42 ++++++++++++++++++- 4 files changed, 44 insertions(+), 6 deletions(-) diff --git a/services/galley/src/Galley/API/Action.hs b/services/galley/src/Galley/API/Action.hs index d8d5c530cd..a2194ea8e1 100644 --- a/services/galley/src/Galley/API/Action.hs +++ b/services/galley/src/Galley/API/Action.hs @@ -425,14 +425,14 @@ performAction tag origUser lconv action = do let victims = [origUser] lconv' <- traverse (convDeleteMembers (toUserList lconv victims)) lconv -- send remove proposals in the MLS case - traverse_ (removeUser lconv') victims + traverse_ (removeUser lconv' RemoveUserIncludeMain) victims pure (mempty, action) SConversationRemoveMembersTag -> do let presentVictims = filter (isConvMemberL lconv) (toList action) when (null presentVictims) noChanges traverse_ (convDeleteMembers (toUserList lconv presentVictims)) lconv -- send remove proposals in the MLS case - traverse_ (removeUser lconv) presentVictims + traverse_ (removeUser lconv RemoveUserExcludeMain) presentVictims pure (mempty, action) -- FUTUREWORK: should we return the filtered action here? SConversationMemberUpdateTag -> do void $ ensureOtherMember lconv (cmuTarget action) conv diff --git a/services/galley/src/Galley/API/Federation.hs b/services/galley/src/Galley/API/Federation.hs index 3a22a033b2..23a7280bdf 100644 --- a/services/galley/src/Galley/API/Federation.hs +++ b/services/galley/src/Galley/API/Federation.hs @@ -414,7 +414,7 @@ onUserDeleted origDomain udcn = do Public.SelfConv -> pure () Public.RegularConv -> do let botsAndMembers = convBotsAndMembers conv - removeUser (qualifyAs lc conv) (tUntagged deletedUser) + removeUser (qualifyAs lc conv) RemoveUserIncludeMain (tUntagged deletedUser) outcome <- runError @FederationError $ notifyConversationAction diff --git a/services/galley/src/Galley/API/Internal.hs b/services/galley/src/Galley/API/Internal.hs index b33bab98ac..97d62a76c6 100644 --- a/services/galley/src/Galley/API/Internal.hs +++ b/services/galley/src/Galley/API/Internal.hs @@ -376,7 +376,7 @@ rmUser lusr conn = do ConnectConv -> E.deleteMembers (Data.convId c) (UserList [tUnqualified lusr] []) $> Nothing RegularConv | tUnqualified lusr `isMember` Data.convLocalMembers c -> do - runError (removeUser (qualifyAs lusr c) (tUntagged lusr)) >>= \case + runError (removeUser (qualifyAs lusr c) RemoveUserIncludeMain (tUntagged lusr)) >>= \case Left e -> P.err $ Log.msg ("failed to send remove proposal: " <> internalErrorDescription e) Right _ -> pure () E.deleteMembers (Data.convId c) (UserList [tUnqualified lusr] []) diff --git a/services/galley/src/Galley/API/MLS/Removal.hs b/services/galley/src/Galley/API/MLS/Removal.hs index 3a796a75c2..cc8ad52866 100644 --- a/services/galley/src/Galley/API/MLS/Removal.hs +++ b/services/galley/src/Galley/API/MLS/Removal.hs @@ -19,6 +19,7 @@ module Galley.API.MLS.Removal ( createAndSendRemoveProposals, removeExtraneousClients, removeClient, + RemoveUserIncludeMain (..), removeUser, ) where @@ -133,6 +134,31 @@ removeClientsWithClientMapRecursively lMlsConv getClients qusr = do planClientRemoval gid (fmap fst clients) createAndSendRemoveProposals mainConv (fmap snd clients) qusr cm + removeClientsFromSubConvs lMlsConv getClients qusr + +removeClientsFromSubConvs :: + ( Member (Input UTCTime) r, + Member TinyLog r, + Member BackendNotificationQueueAccess r, + Member ExternalAccess r, + Member GundeckAccess r, + Member MemberStore r, + Member ProposalStore r, + Member SubConversationStore r, + Member (Input Env) r, + Functor f, + Foldable f + ) => + Local MLSConversation -> + -- | A function returning the "list" of clients to be removed from either the + -- main conversation or each of its subconversations. + (ConvOrSubConv -> f (ClientIdentity, LeafIndex)) -> + -- | Originating user. The resulting proposals will appear to be sent by this user. + Qualified UserId -> + Sem r () +removeClientsFromSubConvs lMlsConv getClients qusr = do + let cm = mcMembers (tUnqualified lMlsConv) + -- remove this client from all subconversations subs <- listSubConversations' (mcId (tUnqualified lMlsConv)) for_ subs $ \sub -> do @@ -170,6 +196,10 @@ removeClient lc qusr c = do let getClients = fmap (cid,) . cmLookupIndex cid . (.members) removeClientsWithClientMapRecursively (qualifyAs lc mlsConv) getClients qusr +data RemoveUserIncludeMain + = RemoveUserIncludeMain + | RemoveUserExcludeMain + -- | Send remove proposals for all clients of the user to the local conversation. removeUser :: ( Member BackendNotificationQueueAccess r, @@ -183,9 +213,10 @@ removeUser :: Member TinyLog r ) => Local Data.Conversation -> + RemoveUserIncludeMain -> Qualified UserId -> Sem r () -removeUser lc qusr = do +removeUser lc includeMain qusr = do mMlsConv <- mkMLSConversation (tUnqualified lc) for_ mMlsConv $ \mlsConv -> do let getClients :: ConvOrSubConv -> [(ClientIdentity, LeafIndex)] @@ -194,7 +225,14 @@ removeUser lc qusr = do . Map.assocs . Map.findWithDefault mempty qusr . (.members) - removeClientsWithClientMapRecursively (qualifyAs lc mlsConv) getClients qusr + case includeMain of + RemoveUserIncludeMain -> + removeClientsWithClientMapRecursively + (qualifyAs lc mlsConv) + getClients + qusr + RemoveUserExcludeMain -> + removeClientsFromSubConvs (qualifyAs lc mlsConv) getClients qusr -- | Convert cassandra subconv maps into SubConversations listSubConversations' :: From c585e5a173c9f98b93ffb6a0370639b9dae981ef Mon Sep 17 00:00:00 2001 From: Paolo Capriotti Date: Mon, 23 Oct 2023 11:19:40 +0200 Subject: [PATCH 2/3] Add CHANGELOG entry --- changelog.d/3-bug-fixes/extra-remove-proposals | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3-bug-fixes/extra-remove-proposals diff --git a/changelog.d/3-bug-fixes/extra-remove-proposals b/changelog.d/3-bug-fixes/extra-remove-proposals new file mode 100644 index 0000000000..4aba01e42f --- /dev/null +++ b/changelog.d/3-bug-fixes/extra-remove-proposals @@ -0,0 +1 @@ +Extra remove proposals were being sent when a user was removed from a conversation From ebfd6e56ab302858547124a1bf0eccc5386233f6 Mon Sep 17 00:00:00 2001 From: Paolo Capriotti Date: Wed, 25 Oct 2023 12:22:47 +0200 Subject: [PATCH 3/3] Document RemoveUserIncludeMain type --- services/galley/src/Galley/API/MLS/Removal.hs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/services/galley/src/Galley/API/MLS/Removal.hs b/services/galley/src/Galley/API/MLS/Removal.hs index cc8ad52866..deb21228e5 100644 --- a/services/galley/src/Galley/API/MLS/Removal.hs +++ b/services/galley/src/Galley/API/MLS/Removal.hs @@ -196,9 +196,17 @@ removeClient lc qusr c = do let getClients = fmap (cid,) . cmLookupIndex cid . (.members) removeClientsWithClientMapRecursively (qualifyAs lc mlsConv) getClients qusr +-- | A flag to determine whether 'removeUser' should operate on the parent +-- conversation as well as all the subconversations. data RemoveUserIncludeMain - = RemoveUserIncludeMain - | RemoveUserExcludeMain + = -- | Remove user clients from all subconversations, including the parent. + RemoveUserIncludeMain + | -- | Remove user clients from all subconversations, but not the parent. + -- + -- This can be used when the clients are already in the process of being + -- removed from the main conversation, for example as a result of a commit + -- containing a remove proposal. + RemoveUserExcludeMain -- | Send remove proposals for all clients of the user to the local conversation. removeUser ::