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.

Sign up for GitHub

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

Remove client check for subconversations #3677

Merged
merged 1 commit into from
Oct 27, 2023
Merged

Remove client check for subconversations #3677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/3-bug-fixes/fix-subconv-client-check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove client check for subconversations
15 changes: 8 additions & 7 deletions integration/test/Test/MLS.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,13 +389,14 @@ testRemoteRemoveClient = do
testCreateSubConv :: HasCallStack => Ciphersuite -> App ()
testCreateSubConv suite = do
setMLSCiphersuite suite
alice <- randomUser OwnDomain def
alice1 <- createMLSClient def alice
(_, conv) <- createNewGroup alice1
bindResponse (getSubConversation alice conv "conference") $ \resp -> do
resp.status `shouldMatchInt` 200
let tm = resp.json %. "epoch_timestamp"
tm `shouldMatch` Null
[alice, bob] <- createAndConnectUsers [OwnDomain, OwnDomain]
aliceClients@(alice1 : _) <- replicateM 5 $ createMLSClient def alice
replicateM_ 3 $ traverse_ uploadNewKeyPackage aliceClients
[bob1, bob2] <- replicateM 2 $ createMLSClient def bob
replicateM_ 3 $ traverse_ uploadNewKeyPackage [bob1, bob2]
void $ createNewGroup alice1
void $ createAddCommit alice1 [alice, bob] >>= sendAndConsumeCommitBundle
createSubConv alice1 "conference"

testCreateSubConvProteus :: App ()
testCreateSubConvProteus = do
Expand Down
75 changes: 42 additions & 33 deletions services/galley/src/Galley/API/MLS/Commit/InternalCommit.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,39 +133,47 @@ processInternalCommit senderIdentity con lConvOrSub epoch action commit = do

pure qtarget

-- for each user, we compare their clients with the ones being added to the conversation
failedAddFetching <- fmap catMaybes . forM newUserClients $
\(qtarget, newclients) -> case Map.lookup qtarget cm of
-- user is already present, skip check in this case
Just _ -> do
-- new user
pure Nothing
Nothing -> do
-- final set of clients in the conversation
let clients = Map.keysSet (newclients <> Map.findWithDefault mempty qtarget cm)
-- get list of mls clients from Brig (local or remote)
getClientInfo lConvOrSub qtarget suite >>= \case
Left _e -> pure (Just qtarget)
Right clientInfo -> do
let allClients = Set.map ciId clientInfo
let allMLSClients = Set.map ciId (Set.filter ciMLS clientInfo)
-- We check the following condition:
-- allMLSClients ⊆ clients ⊆ allClients
-- i.e.
-- - if a client has at least 1 key package, it has to be added
-- - if a client is being added, it has to still exist
--
-- The reason why we can't simply check that clients == allMLSClients is
-- that a client with no remaining key packages might be added by a user
-- who just fetched its last key package.
unless
( Set.isSubsetOf allMLSClients clients
&& Set.isSubsetOf clients allClients
)
$ do
-- FUTUREWORK: turn this error into a proper response
throwS @'MLSClientMismatch
-- For each user, we compare their clients with the ones being added
-- to the conversation, and return a list of users for of which we
-- were unable to get a list of MLS-capable clients.
--
-- Again, for subconversations there is no need to check anything
-- here, so we simply return the empty list.
failedAddFetching <- case convOrSub.id of
SubConv _ _ -> pure []
Conv _ ->
fmap catMaybes . forM newUserClients $
\(qtarget, newclients) -> case Map.lookup qtarget cm of
-- user is already present, skip check in this case
Just _ -> do
-- new user
pure Nothing
Nothing -> do
-- final set of clients in the conversation
let clients = Map.keysSet (newclients <> Map.findWithDefault mempty qtarget cm)
-- get list of mls clients from Brig (local or remote)
getClientInfo lConvOrSub qtarget suite >>= \case
Left _e -> pure (Just qtarget)
Right clientInfo -> do
let allClients = Set.map ciId clientInfo
let allMLSClients = Set.map ciId (Set.filter ciMLS clientInfo)
-- We check the following condition:
-- allMLSClients ⊆ clients ⊆ allClients
-- i.e.
-- - if a client has at least 1 key package, it has to be added
-- - if a client is being added, it has to still exist
--
-- The reason why we can't simply check that clients == allMLSClients is
-- that a client with no remaining key packages might be added by a user
-- who just fetched its last key package.
unless
( Set.isSubsetOf allMLSClients clients
&& Set.isSubsetOf clients allClients
)
$
-- FUTUREWORK: turn this error into a proper response
throwS @'MLSClientMismatch
pure Nothing
for_
(unreachableFromList failedAddFetching)
(throw . unreachableUsersToUnreachableBackends)
Expand Down Expand Up @@ -225,7 +233,8 @@ processInternalCommit senderIdentity con lConvOrSub epoch action commit = do
cjRole = roleNameWireMember
}
pure [update]
_ -> do
SubConv _ _ -> pure []
Conv _ -> do
-- remove users from the conversation and send events
removeEvents <-
foldMap
Expand Down
Loading