add test for group deletion and fix a few bugs

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,
         )

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,19 +64,27 @@ 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.
             let removed_clients: Vec<_> = staged_commit
                 .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);
         }
 

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(),

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<KeyStoreProvider = MemoryKeyStore>,
+    ) -> Result<DeleteGroupParamsOut, GroupOperationError> {
+        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::<Vec<_>>();
+        // 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.

coreclient/src/users/mod.rs

@@ -744,6 +744,38 @@ impl<T: Notifiable> SelfUser<T> {
             .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<QueueMessage> {
         let mut remaining_messages = 1;
         let mut messages: Vec<QueueMessage> = Vec::new();

coreclient/src/users/process.rs

@@ -219,7 +219,7 @@ impl<T: Notifiable> SelfUser<T> {
                                     .map(|user_name| user_name.to_string())
                                     .collect::<Vec<_>>();
                                 self.conversation_store
-                                    .set_inactive(&conversation_id, &past_members);
+                                    .set_inactive(conversation_id, &past_members);
                             }
                             group
                                 .merge_pending_commit(&self.crypto_backend, *staged_commit)

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]

server/tests/utils/setup.rs

@@ -70,7 +70,7 @@ impl TestUser {
 
 pub struct TestBackend {
     pub users: HashMap<String, TestUser>,
-    groups: HashMap<Uuid, HashSet<String>>,
+    pub groups: HashMap<Uuid, HashSet<String>>,
     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<String> = 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();
     }
 }