diff --git a/changelog.d/3-bug-fixes/WPB-22820 b/changelog.d/3-bug-fixes/WPB-22820 new file mode 100644 index 00000000000..3cdcb117e58 --- /dev/null +++ b/changelog.d/3-bug-fixes/WPB-22820 @@ -0,0 +1 @@ +Fixed a spurious `conversation.member-join` event being sent when an MLS 1:1 conversation is reset and re-established. Previously, re-establishing the group after a reset re-entered the 1:1 creation code path and reported both participants as newly joining, even though membership was unchanged, causing clients to show an incorrect "X added you" notification for a conversation they were already in. diff --git a/integration/test/Test/MLS/Reset.hs b/integration/test/Test/MLS/Reset.hs index 95cc774d9ac..fd47f868049 100644 --- a/integration/test/Test/MLS/Reset.hs +++ b/integration/test/Test/MLS/Reset.hs @@ -19,7 +19,7 @@ module Test.MLS.Reset where import API.Galley import MLS.Util -import Notifications (isConvResetNotif) +import Notifications (isConvResetNotif, isWelcomeNotif) import SetupHelpers import Testlib.Prelude import Testlib.VersionedFed @@ -114,6 +114,42 @@ testResetOne2OneConversation = do conv'' %. "epoch" `shouldMatchInt` 1 conv'' %. "group_id" `shouldMatch` (conv' %. "group_id") +-- | Regression test for WPB-22820: after an MLS 1:1 conversation is reset +-- (epoch back to 0, group id rotated, membership unchanged) and +-- re-established, the backend must not report either participant as newly +-- joining the conversation, since both were already members before the reset. +testResetOne2OneNoSpuriousMemberJoin :: (HasCallStack) => App () +testResetOne2OneNoSpuriousMemberJoin = do + [alice, bob] <- createAndConnectUsers [OwnDomain, OwnDomain] + [aliceClient, bobClient] <- traverse (createMLSClient def) [alice, bob] + void . for [aliceClient, bobClient] $ \cid -> replicateM 2 $ uploadNewKeyPackage def cid + conv <- getMLSOne2OneConversation alice bob >>= getJSON 200 + convId <- objConvId (conv %. "conversation") + + -- establish the 1:1 conversation at epoch 1 + resetOne2OneGroup def aliceClient conv + void $ createAddCommit aliceClient convId [bob] >>= sendAndConsumeCommitBundle + + -- reset back to epoch 0 (group id rotates, membership stays: alice + bob) + conv' <- resetMLSConversation aliceClient (conv %. "conversation") + convId' <- objConvId conv' + resetOne2OneGroupGeneric def aliceClient conv' (conv %. "public_keys") + + withWebSocket alice $ \wsAlice -> + withWebSocket bob $ \wsBob -> do + void $ createAddCommit aliceClient convId' [bob] >>= sendAndConsumeCommitBundle + + -- Bob rejoins the rotated group (legitimate welcome), but since he was + -- already a member of this 1:1 conversation, he must not additionally be + -- told (via member-join) that he was just added. + void $ awaitMatch isWelcomeNotif wsBob + mEvent <- awaitAnyEvent 2 wsBob + for_ mEvent $ \n -> n %. "payload.0.type" `shouldNotMatch` "conversation.member-join" + + -- Alice issued the commit and isn't newly joining anything either; + -- So, we expect no event for her. + awaitAnyEvent 2 wsAlice `shouldMatch` (Nothing :: Maybe Value) + testResetMixedConversation :: (HasCallStack) => Domain -> App () testResetMixedConversation domain = do -- create mixed conversation diff --git a/libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Commit/InternalCommit.hs b/libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Commit/InternalCommit.hs index d904f7396d5..afc4b0db1a0 100644 --- a/libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Commit/InternalCommit.hs +++ b/libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Commit/InternalCommit.hs @@ -197,29 +197,41 @@ processInternalCommit senderIdentity con lConvOrSub ciphersuite ciphersuiteUpdat ( mlsProtocolError "The first commit in a 1-1 conversation should add exactly 1 other user" ) - -- notify otherUser about being added to this 1-1 conversation + -- Only emit a member-join if someone is actually newly + -- joining. A reset leaves membership intact but re-enters + -- this branch (epoch back to 0), so on re-establish both + -- users are already members -- emitting a join then would be + -- a spurious "X added you" for a conversation they're + -- already in. let bm = convBotsAndMembers conv - members <- - note + newMembers = + filter + (flip Set.notMember (existingMembers lconv)) + (bmQualifiedMembers lconv bm) + void + . note ( InternalErrorWithDescription "Unexpected empty member list in MLS 1-1 conversation" ) - $ nonEmpty (bmQualifiedMembers lconv bm) - update <- - sendConversationActionNotifications - SConversationJoinTag - senderUser - False - con - lconv - bm - ConversationJoin - { users = members, - role = roleNameWireMember, - joinType = def - } - def - pure [update] + $ nonEmpty (bmQualifiedMembers lconv bm) + case nonEmpty newMembers of + Nothing -> pure [] + Just membersNE -> do + update <- + sendConversationActionNotifications + SConversationJoinTag + senderUser + False + con + lconv + bm + ConversationJoin + { users = membersNE, + role = roleNameWireMember, + joinType = def + } + def + pure [update] SubConv _ _ -> pure [] Conv _ -> do -- remove users from the conversation and send events