Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid MLS messages in sender client's event stream #3379

Merged
merged 4 commits into from
Jun 27, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5-internal/dont-return-to-sender
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Avoid including MLS application messages in the sender client's event stream.
4 changes: 2 additions & 2 deletions services/galley/src/Galley/API/MLS/Message.hs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ postMLSCommitBundleToLocalConv qusr c conn bundle lConvOrSubId = do

storeGroupInfo (tUnqualified lConvOrSub).id bundle.groupInfo

propagateMessage qusr lConvOrSub conn bundle.rawMessage (tUnqualified lConvOrSub).members
propagateMessage qusr (Just c) lConvOrSub conn bundle.rawMessage (tUnqualified lConvOrSub).members
>>= mapM_ throwUnreachableUsers

for_ bundle.welcome $ \welcome ->
Expand Down Expand Up @@ -375,7 +375,7 @@ postMLSMessageToLocalConv qusr c con msg convOrSubId = do
when ((tUnqualified lConvOrSub).migrationState == MLSMigrationMixed) $
throwS @'MLSUnsupportedMessage

unreachables <- propagateMessage qusr lConvOrSub con msg.rawMessage (tUnqualified lConvOrSub).members
unreachables <- propagateMessage qusr (Just c) lConvOrSub con msg.rawMessage (tUnqualified lConvOrSub).members
pure ([], unreachables)

postMLSMessageToRemoteConv ::
Expand Down
9 changes: 6 additions & 3 deletions services/galley/src/Galley/API/MLS/Propagate.hs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import Wire.API.Event.Conversation
import Wire.API.Federation.API
import Wire.API.Federation.API.Galley
import Wire.API.Federation.Error
import Wire.API.MLS.Credential
import Wire.API.MLS.Message
import Wire.API.MLS.Serialisation
import Wire.API.MLS.SubConversation
Expand All @@ -58,12 +59,13 @@ propagateMessage ::
Member TinyLog r
) =>
Qualified UserId ->
Maybe ClientId ->
Local ConvOrSubConv ->
Maybe ConnId ->
RawMLS Message ->
ClientMap ->
Sem r (Maybe UnreachableUsers)
propagateMessage qusr lConvOrSub con msg cm = do
propagateMessage qusr mci lConvOrSub con msg cm = do
now <- input @UTCTime
let mlsConv = (.conv) <$> lConvOrSub
lmems = mcLocalMembers . tUnqualified $ mlsConv
Expand Down Expand Up @@ -102,21 +104,22 @@ propagateMessage qusr lConvOrSub con msg cm = do
rmmMessage = Base64ByteString msg.raw
}
where
cmWithoutSender = maybe cm (flip cmRemoveClient cm . mkClientIdentity qusr) mci
localMemberMLSClients :: Local x -> LocalMember -> [(UserId, ClientId)]
localMemberMLSClients loc lm =
let localUserQId = tUntagged (qualifyAs loc localUserId)
localUserId = lmId lm
in map
(\(c, _) -> (localUserId, c))
(Map.assocs (Map.findWithDefault mempty localUserQId cm))
(Map.assocs (Map.findWithDefault mempty localUserQId cmWithoutSender))

remoteMemberMLSClients :: RemoteMember -> [(UserId, ClientId)]
remoteMemberMLSClients rm =
let remoteUserQId = tUntagged (rmId rm)
remoteUserId = qUnqualified remoteUserQId
in map
(\(c, _) -> (remoteUserId, c))
(Map.assocs (Map.findWithDefault mempty remoteUserQId cm))
(Map.assocs (Map.findWithDefault mempty remoteUserQId cmWithoutSender))

remotesToQIds = fmap (tUntagged . rmId)

Expand Down
2 changes: 1 addition & 1 deletion services/galley/src/Galley/API/MLS/Removal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ createAndSendRemoveProposals lConvOrSubConv indices qusr cm = do
(publicMessageRef (cnvmlsCipherSuite meta) pmsg)
ProposalOriginBackend
proposal
propagateMessage qusr lConvOrSubConv Nothing msg cm
propagateMessage qusr Nothing lConvOrSubConv Nothing msg cm

removeClientsWithClientMapRecursively ::
( Members
Expand Down
25 changes: 14 additions & 11 deletions services/galley/test/integration/API/MLS.hs
Original file line number Diff line number Diff line change
Expand Up @@ -967,9 +967,10 @@ testAppMessage = do
mlsBracket clients $ \wss -> do
(events, _) <- sendAndConsumeMessage message
liftIO $ events @?= []
liftIO $
WS.assertMatchN_ (5 # WS.Second) wss $
liftIO $ do
WS.assertMatchN_ (5 # WS.Second) (tail wss) $
wsAssertMLSMessage (fmap Conv qcnv) alice (mpMessage message)
WS.assertNoEvent (2 # WS.Second) [head wss]

testAppMessage2 :: TestM ()
testAppMessage2 = do
Expand All @@ -992,15 +993,16 @@ testAppMessage2 = do

message <- createApplicationMessage bob1 "some text"

mlsBracket (alice1 : clients) $ \wss -> do
mlsBracket (alice1 : clients) $ \[wsAlice1, wsBob1, wsBob2, wsCharlie1] -> do
(events, _) <- sendAndConsumeMessage message
liftIO $ events @?= []

-- check that the corresponding event is received

liftIO $
WS.assertMatchN_ (5 # WS.Second) wss $
-- check that the corresponding event is received by everyone except bob1
-- (the sender) and no message is received by bob1
liftIO $ do
WS.assertMatchN_ (5 # WS.Second) [wsAlice1, wsBob2, wsCharlie1] $
wsAssertMLSMessage (fmap Conv conversation) bob (mpMessage message)
WS.assertNoEvent (2 # WS.Second) [wsBob1]

testAppMessageSomeReachable :: TestM ()
testAppMessageSomeReachable = do
Expand Down Expand Up @@ -1829,7 +1831,7 @@ testBackendRemoveProposalLocalConvLocalClient = do
WS.assertMatch_ (5 # WS.Second) wsB $
wsAssertClientRemoved (ciClient bob1)

msg <- WS.assertMatch (5 # WS.Second) wsA $ \notification -> do
(msg : _) <- WS.assertMatchN (5 # WS.Second) [wsA, wsC] $ \notification -> do
wsAssertBackendRemoveProposal bob (Conv <$> qcnv) idxBob1 notification

for_ [alice1, bob2, charlie1] $
Expand All @@ -1838,8 +1840,9 @@ testBackendRemoveProposalLocalConvLocalClient = do
mp <- createPendingProposalCommit charlie1
events <- sendAndConsumeCommitBundle mp
liftIO $ events @?= []
WS.assertMatchN_ (5 # WS.Second) [wsA, wsC] $ \n -> do
WS.assertMatchN_ (5 # WS.Second) [wsA] $ \n -> do
wsAssertMLSMessage (Conv <$> qcnv) charlie (mpMessage mp) n
WS.assertNoEvent (2 # WS.Second) [wsC]

testBackendRemoveProposalLocalConvRemoteClient :: TestM ()
testBackendRemoveProposalLocalConvRemoteClient = do
Expand Down Expand Up @@ -2700,7 +2703,7 @@ testLeaveSubConv isSubConvCreator = do
-- a member commits the pending proposal
do
leaveCommit <- createPendingProposalCommit (head others)
mlsBracket (firstLeaver : others) $ \(wsLeaver : wss) -> do
mlsBracket (firstLeaver : tail others) $ \(wsLeaver : wss) -> do
events <-
fst
<$$> withTempMockFederator' ("on-mls-message-sent" ~> RemoteMLSMessageOk)
Expand All @@ -2713,7 +2716,7 @@ testLeaveSubConv isSubConvCreator = do
-- send an application message
do
message <- createApplicationMessage (head others) "some text"
mlsBracket (firstLeaver : others) $ \(wsLeaver : wss) -> do
mlsBracket (firstLeaver : tail others) $ \(wsLeaver : wss) -> do
(events, _) <- sendAndConsumeMessage message
liftIO $ events @?= []
WS.assertMatchN_ (5 # WS.Second) wss $ \n -> do
Expand Down