Skip to content

Commit

Permalink
Fix bug in MLS user removal from conversation
Browse files Browse the repository at this point in the history
The list of removed clients has to be compared with those in the
conversation, not the list of *all* clients of that user.
  • Loading branch information
pcapriotti committed Nov 2, 2022
1 parent a94d00e commit 49782f5
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 17 deletions.
28 changes: 11 additions & 17 deletions services/galley/src/Galley/API/MLS/Message.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ executeProposalAction qusr con lconv cm action = do
-- FUTUREWORK: turn this error into a proper response
throwS @'MLSClientMismatch

membersToRemove <- catMaybes <$> for removeUserClients (uncurry (checkRemoval lconv ss))
membersToRemove <- catMaybes <$> for removeUserClients (uncurry checkRemoval)

-- add users to the conversation and send events
addEvents <- foldMap addMembers . nonEmpty . map fst $ newUserClients
Expand All @@ -1083,25 +1083,19 @@ executeProposalAction qusr con lconv cm action = do
-- This also filters out client removals for clients that don't exist anymore
-- For these clients there is nothing left to do
checkRemoval ::
Local x ->
SignatureSchemeTag ->
Qualified UserId ->
Set (ClientId, KeyPackageRef) ->
Sem r (Maybe (Qualified UserId))
checkRemoval loc ss qtarget (Set.map fst -> clients) = do
allClients <- Set.map ciId <$> getMLSClients loc qtarget ss
let allClientsDontExist = Set.null (clients `Set.intersection` allClients)
if allClientsDontExist
then pure Nothing
else do
-- We only support removal of client for user. This is likely to change in the future.
-- See discussions here https://wearezeta.atlassian.net/wiki/spaces/CL/pages/612106259/Relax+constraint+between+users+and+clients+in+MLS+groups
when (clients /= allClients) $ do
-- FUTUREWORK: turn this error into a proper response
throwS @'MLSClientMismatch
when (qusr == qtarget) $
throwS @'MLSSelfRemovalNotAllowed
pure (Just qtarget)
checkRemoval qtarget (Set.map fst -> clients) = do
let clientsInConv = Set.map fst (Map.findWithDefault mempty qtarget cm)
-- We only support removal of client for user. This is likely to change in the future.
-- See discussions here https://wearezeta.atlassian.net/wiki/spaces/CL/pages/612106259/Relax+constraint+between+users+and+clients+in+MLS+groups
when (clients /= clientsInConv) $ do
-- FUTUREWORK: turn this error into a proper response
throwS @'MLSClientMismatch
when (qusr == qtarget) $
throwS @'MLSSelfRemovalNotAllowed
pure (Just qtarget)

existingLocalMembers :: Set (Qualified UserId)
existingLocalMembers =
Expand Down
1 change: 1 addition & 0 deletions services/galley/test/integration/API/MLS.hs
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,7 @@ testAdminRemovesUserFromConv = do
[alice, bob] <- createAndConnectUsers [Nothing, Nothing]
(qcnv, events) <- runMLSTest $ do
[alice1, bob1, bob2] <- traverse createMLSClient [alice, bob, bob]
void $ createWireClient bob -- also create one extra non-MLS client
traverse_ uploadNewKeyPackage [bob1, bob2]
(_, qcnv) <- setupMLSGroup alice1
void $ createAddCommit alice1 [bob] >>= sendAndConsumeCommit
Expand Down

0 comments on commit 49782f5

Please sign in to comment.