From 5ff43bc260ae3ef83580f7989519e8ccdc276801 Mon Sep 17 00:00:00 2001 From: Konrad Kohbrok Date: Tue, 18 Jul 2023 11:33:26 +0200 Subject: [PATCH] add test for group deletion and fix a few bugs --- apiclient/src/ds_api/mod.rs | 8 +-- backend/src/ds/delete_group.rs | 18 +++++- coreclient/src/conversations/store.rs | 4 +- coreclient/src/groups/mod.rs | 57 ++++++++++++++++-- coreclient/src/users/mod.rs | 32 ++++++++++ coreclient/src/users/process.rs | 2 +- server/tests/mod.rs | 18 +++++- server/tests/utils/setup.rs | 87 ++++++++++++++++++++++++++- 8 files changed, 206 insertions(+), 20 deletions(-) diff --git a/apiclient/src/ds_api/mod.rs b/apiclient/src/ds_api/mod.rs index e9c6dc89..c2a49513 100644 --- a/apiclient/src/ds_api/mod.rs +++ b/apiclient/src/ds_api/mod.rs @@ -525,16 +525,12 @@ impl ApiClient { /// Delete the given group. pub async fn ds_delete_group( &self, - commit: AssistedMessageOut, + params: DeleteGroupParamsOut, signing_key: &UserAuthSigningKey, group_state_ear_key: &GroupStateEarKey, ) -> Result<(), DsRequestError> { - let payload = DeleteGroupParamsOut { - commit, - sender: signing_key.verifying_key().hash(), - }; self.prepare_and_send_ds_message( - DsRequestParamsOut::DeleteGroup(payload), + DsRequestParamsOut::DeleteGroup(params), signing_key, group_state_ear_key, ) diff --git a/backend/src/ds/delete_group.rs b/backend/src/ds/delete_group.rs index e52918c4..4d2e7cfb 100644 --- a/backend/src/ds/delete_group.rs +++ b/backend/src/ds/delete_group.rs @@ -33,11 +33,12 @@ impl DsGroupState { processed_message } else { // This should be a commit. + tracing::warn!("Received non-commit message for delete_group operation"); return Err(GroupDeletionError::InvalidMessage); }; // Check if sender index and user profile match. - if let Sender::Member(leaf_index) = processed_message.sender() { + let sender_index = if let Sender::Member(leaf_index) = processed_message.sender() { // There should be a user profile. If there wasn't, verification should have failed. if !self .user_profiles @@ -46,12 +47,15 @@ impl DsGroupState { .clients .contains(leaf_index) { + tracing::warn!("Missing user profile"); return Err(GroupDeletionError::InvalidMessage); }; + leaf_index } else { // Remove users should be a regular commit + tracing::warn!("Invalid sender"); return Err(GroupDeletionError::InvalidMessage); - } + }; if let ProcessedMessageContent::StagedCommitMessage(staged_commit) = processed_message.content() @@ -60,6 +64,7 @@ impl DsGroupState { if staged_commit.add_proposals().count() > 0 || staged_commit.update_proposals().count() > 0 { + tracing::warn!("Found add or update proposals in delete group commit"); return Err(GroupDeletionError::InvalidMessage); } // Process remove proposals, but only non-inline ones. @@ -67,12 +72,19 @@ impl DsGroupState { .remove_proposals() .map(|remove_proposal| remove_proposal.remove_proposal().removed()) .collect(); - let existing_clients: Vec<_> = self.client_profiles.keys().copied().collect(); + let existing_clients: Vec<_> = self + .client_profiles + .keys() + .filter(|index| index != &sender_index) + .copied() + .collect(); // Check that we're indeed removing all the clients. if removed_clients != existing_clients { + tracing::warn!("Incomplete remove proposals in delete group commit"); return Err(GroupDeletionError::InvalidMessage); } } else { + tracing::warn!("Invalid message content"); return Err(GroupDeletionError::InvalidMessage); } diff --git a/coreclient/src/conversations/store.rs b/coreclient/src/conversations/store.rs index 67387e18..1a4d4f94 100644 --- a/coreclient/src/conversations/store.rs +++ b/coreclient/src/conversations/store.rs @@ -88,9 +88,9 @@ impl ConversationStore { self.conversations.get(&conversation_id) } - pub(crate) fn set_inactive(&mut self, conversation_id: &Uuid, past_members: &[String]) { + pub(crate) fn set_inactive(&mut self, conversation_id: Uuid, past_members: &[String]) { self.conversations - .get_mut(conversation_id) + .get_mut(&conversation_id) .map(|conversation| { conversation.status = ConversationStatus::Inactive(InactiveConversation { past_members: past_members.iter().map(|m| m.to_owned()).collect(), diff --git a/coreclient/src/groups/mod.rs b/coreclient/src/groups/mod.rs index 4f1009ea..caefac79 100644 --- a/coreclient/src/groups/mod.rs +++ b/coreclient/src/groups/mod.rs @@ -44,7 +44,7 @@ use phnxbackend::{ UpdateClientParamsAad, WelcomeBundle, }, client_ds_out::{ - AddUsersParamsOut, ExternalCommitInfoIn, RemoveUsersParamsOut, + AddUsersParamsOut, DeleteGroupParamsOut, ExternalCommitInfoIn, RemoveUsersParamsOut, SelfRemoveClientParamsOut, SendMessageParamsOut, UpdateClientParamsOut, }, }, @@ -872,8 +872,6 @@ impl Group { ); } InfraAadPayload::DeleteGroup => { - // After processing the message, the MLS Group should already be marked as inactive. - debug_assert!(!self.mls_group.is_active()); we_were_removed = true; // There is nothing else to do at this point. } @@ -1029,7 +1027,7 @@ impl Group { let group_info = group_info_option.unwrap(); let assisted_group_info = AssistedGroupInfo::Full(group_info.into()); let commit = AssistedMessageOut { - mls_message: mls_message, + mls_message, group_info_option: Some(assisted_group_info), }; @@ -1047,6 +1045,57 @@ impl Group { Ok(params) } + pub(crate) fn delete( + &mut self, + backend: &impl OpenMlsCryptoProvider, + ) -> Result { + let Some(user_auth_key) = &self.user_auth_signing_key_option else { + return Err(GroupOperationError::NoUserAuthKey); + }; + let remove_indices = self + .client_information + .keys() + .filter_map(|&index| { + if index != self.mls_group.own_leaf_index().usize() { + Some(LeafNodeIndex::new(index as u32)) + } else { + None + } + }) + .collect::>(); + // There shouldn't be a welcome + let aad_payload = InfraAadPayload::DeleteGroup; + let aad = InfraAadMessage::from(aad_payload) + .tls_serialize_detached() + .unwrap(); + self.mls_group.set_aad(aad.as_slice()); + let (mls_message, _welcome_option, group_info_option) = self + .mls_group + .remove_members(backend, &self.leaf_signer, remove_indices.as_slice()) + .unwrap(); + self.mls_group.set_aad(&[]); + debug_assert!(_welcome_option.is_none()); + let group_info = group_info_option.unwrap(); + let assisted_group_info = AssistedGroupInfo::Full(group_info.into()); + let commit = AssistedMessageOut { + mls_message, + group_info_option: Some(assisted_group_info), + }; + + let mut diff = GroupDiff::new(&self); + diff.apply_pending_removes(self.mls_group().pending_commit().unwrap()); + for index in remove_indices { + diff.remove_client_credential(index); + } + self.pending_diff = Some(diff); + + let params = DeleteGroupParamsOut { + commit, + sender: user_auth_key.verifying_key().hash(), + }; + Ok(params) + } + /// If a [`StagedCommit`] is given, merge it and apply the pending group /// diff. If no [`StagedCommit`] is given, merge any pending commit and /// apply the pending group diff. diff --git a/coreclient/src/users/mod.rs b/coreclient/src/users/mod.rs index f4d3ae7e..21587c4e 100644 --- a/coreclient/src/users/mod.rs +++ b/coreclient/src/users/mod.rs @@ -744,6 +744,38 @@ impl SelfUser { .unwrap(); } + pub async fn delete_group(&mut self, conversation_id: Uuid) { + let conversation = self + .conversation_store + .conversation(conversation_id) + .unwrap(); + let group = self + .group_store + .get_group_mut(&conversation.group_id.as_group_id()) + .unwrap(); + let past_members: Vec<_> = group.members().into_iter().map(|m| m.to_string()).collect(); + // No need to send a message to the server if we are the only member. + // TODO: Make sure this is what we want. + if past_members.len() != 1 { + let params = group.delete(&self.crypto_backend).unwrap(); + self.api_client + .ds_delete_group( + params, + group.user_auth_key().unwrap(), + group.group_state_ear_key(), + ) + .await + .unwrap(); + let conversation_messages = group + .merge_pending_commit(&self.crypto_backend, None) + .unwrap(); + self.send_off_notifications(conversation_id, conversation_messages) + .unwrap(); + } + self.conversation_store + .set_inactive(conversation_id, &past_members); + } + pub async fn as_fetch_messages(&mut self) -> Vec { let mut remaining_messages = 1; let mut messages: Vec = Vec::new(); diff --git a/coreclient/src/users/process.rs b/coreclient/src/users/process.rs index 645793a9..32a66b72 100644 --- a/coreclient/src/users/process.rs +++ b/coreclient/src/users/process.rs @@ -219,7 +219,7 @@ impl SelfUser { .map(|user_name| user_name.to_string()) .collect::>(); self.conversation_store - .set_inactive(&conversation_id, &past_members); + .set_inactive(conversation_id, &past_members); } group .merge_pending_commit(&self.crypto_backend, *staged_commit) diff --git a/server/tests/mod.rs b/server/tests/mod.rs index ee28ca84..07e4465b 100644 --- a/server/tests/mod.rs +++ b/server/tests/mod.rs @@ -119,6 +119,20 @@ async fn leave_group() { setup.leave_group(conversation_id, "alice").await; } +#[actix_rt::test] +#[tracing::instrument(name = "Invite to group test", skip_all)] +async fn delete_group() { + let mut setup = TestBackend::new().await; + setup.add_user("alice").await; + setup.add_user("bob").await; + setup.connect_users("alice", "bob").await; + let conversation_id = setup.create_group("alice").await; + setup + .invite_to_group(conversation_id, "alice", &["bob"]) + .await; + setup.delete_group(conversation_id, "bob").await; +} + #[actix_rt::test] #[tracing::instrument(name = "Create user", skip_all)] async fn create_user() { @@ -189,7 +203,9 @@ async fn full_cycle() { .remove_from_group(conversation_id, "dave", &["alice", "bob"]) .await; - setup.leave_group(conversation_id, "charlie").await + setup.leave_group(conversation_id, "charlie").await; + + setup.delete_group(conversation_id, "dave").await } #[actix_rt::test] diff --git a/server/tests/utils/setup.rs b/server/tests/utils/setup.rs index dee36d08..ba949995 100644 --- a/server/tests/utils/setup.rs +++ b/server/tests/utils/setup.rs @@ -70,7 +70,7 @@ impl TestUser { pub struct TestBackend { pub users: HashMap, - groups: HashMap>, + pub groups: HashMap>, pub address: SocketAddr, } @@ -570,7 +570,7 @@ impl TestBackend { } } - /// Has the inviter invite the invitees to the given group and has everyone + /// Has the remover remove the removed from the given group and has everyone /// send and process their messages. pub async fn remove_from_group( &mut self, @@ -606,7 +606,7 @@ impl TestBackend { remover .remove_users(conversation_id, removed_names) .await - .expect("Error inviting users."); + .expect("Error removing users."); let remover_group_members_after: HashSet = remover .group_members(conversation_id) @@ -742,6 +742,87 @@ impl TestBackend { self.commit_to_proposals(conversation_id, random_member_name) .await; + let group_members = self.groups.get_mut(&conversation_id).unwrap(); + group_members.remove(leaver_name); + + self.flush_notifications(); + } + + pub async fn delete_group(&mut self, conversation_id: Uuid, deleter_name: &str) { + tracing::info!( + "{} deletes the group with id {}", + deleter_name, + conversation_id + ); + let test_deleter = self.users.get_mut(deleter_name).unwrap(); + let deleter = &mut test_deleter.user; + + // Before removing anyone from a group, the remover must first fetch and + // process its QS messages. + let qs_messages = deleter.qs_fetch_messages().await; + + deleter + .process_qs_messages(qs_messages) + .await + .expect("Error processing qs messages."); + + // Perform the remove operation and check that the removed are not in + // the group anymore. + let deleter_conversation_before = deleter.conversation(conversation_id).unwrap().clone(); + assert_eq!( + deleter_conversation_before.status, + ConversationStatus::Active + ); + let past_members = deleter.group_members(conversation_id).unwrap(); + + deleter.delete_group(conversation_id).await; + + let deleter_conversation_after = deleter.conversation(conversation_id).unwrap(); + if let ConversationStatus::Inactive(inactive_status) = &deleter_conversation_after.status { + let inactive_status_members: HashSet<_> = + inactive_status.past_members.clone().into_iter().collect(); + assert_eq!(inactive_status_members, past_members); + } else { + panic!("Conversation should be inactive.") + } + + for group_member_name in self.groups.get(&conversation_id).unwrap().iter() { + // Skip the deleter + if group_member_name == deleter_name { + continue; + } + let test_group_member = self.users.get_mut(group_member_name).unwrap(); + let group_member = &mut test_group_member.user; + + let group_member_conversation_before = + group_member.conversation(conversation_id).unwrap(); + assert_eq!( + group_member_conversation_before.status, + ConversationStatus::Active + ); + let past_members: HashSet<_> = group_member.group_members(conversation_id).unwrap(); + + let qs_messages = group_member.qs_fetch_messages().await; + + group_member + .process_qs_messages(qs_messages) + .await + .expect("Error processing qs messages."); + + let group_member_conversation_after = + group_member.conversation(conversation_id).unwrap(); + if let ConversationStatus::Inactive(inactive_status) = + &group_member_conversation_after.status + { + let inactive_status_members: HashSet<_> = + inactive_status.past_members.clone().into_iter().collect(); + assert_eq!(inactive_status_members, past_members); + } else { + panic!("Conversation should be inactive.") + } + } + self.groups.remove(&conversation_id); + self.flush_notifications(); } }