fix: Don't send removal message to contact that hasn't been a chat member (#8298)

src/chat.rs

@@ -3800,26 +3800,31 @@ pub(crate) async fn add_to_chat_contacts_table(
 
 /// Removes a contact from the chat
 /// by updating the `remove_timestamp`.
+/// Returns whether the contact has been a chat member recently. If so, a removal message should be
+/// sent.
 pub(crate) async fn remove_from_chat_contacts_table(
     context: &Context,
     chat_id: ChatId,
     contact_id: ContactId,
-) -> Result<()> {
+) -> Result<bool> {
     let now = time();
-    context
+    let is_past_member = context
         .sql
         .execute(
             "UPDATE chats_contacts
              SET remove_timestamp=MAX(add_timestamp+1, ?)
              WHERE chat_id=? AND contact_id=?",
             (now, chat_id, contact_id),
         )
-        .await?;
-    Ok(())
+        .await?
+        > 0;
+    Ok(is_past_member)
 }
 
 /// Removes a contact from the chat
-/// without leaving a trace.
+/// without leaving a trace in the db.
+/// Returns whether the contact was removed, even if it was a past contact. If so, a removal message
+/// should be sent if the removal is issued by this device.
 ///
 /// Note that if we call this function,
 /// and then receive a message from another device
@@ -3829,17 +3834,17 @@ pub(crate) async fn remove_from_chat_contacts_table_without_trace(
     context: &Context,
     chat_id: ChatId,
     contact_id: ContactId,
-) -> Result<()> {
-    context
+) -> Result<bool> {
+    let removed = context
         .sql
         .execute(
             "DELETE FROM chats_contacts
             WHERE chat_id=? AND contact_id=?",
             (chat_id, contact_id),
         )
-        .await?;
-
-    Ok(())
+        .await?
+        > 0;
+    Ok(removed)
 }
 
 /// Adds a contact to the chat.
@@ -4159,10 +4164,13 @@ pub async fn remove_contact_from_chat(
 
     let mut sync = Nosync;
 
-    if chat.is_promoted() && chat.typ != Chattype::OutBroadcast {
-        remove_from_chat_contacts_table(context, chat_id, contact_id).await?;
+    let removed = if chat.is_promoted() && chat.typ != Chattype::OutBroadcast {
+        remove_from_chat_contacts_table(context, chat_id, contact_id).await?
     } else {
-        remove_from_chat_contacts_table_without_trace(context, chat_id, contact_id).await?;
+        remove_from_chat_contacts_table_without_trace(context, chat_id, contact_id).await?
+    };
+    if !removed {
+        return Ok(());
     }
 
     // We do not return an error if the contact does not exist in the database.

src/chat/chat_tests.rs

@@ -2800,6 +2800,30 @@ async fn test_can_send_group() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_cant_remove_nonmember() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = &tcm.alice().await;
+    let bob = &tcm.bob().await;
+    let charlie = &tcm.charlie().await;
+
+    let alice_broadcast_id = create_broadcast(alice, "Channel".to_string()).await?;
+    let qr = get_securejoin_qr(alice, Some(alice_broadcast_id))
+        .await
+        .unwrap();
+    tcm.exec_securejoin_qr(bob, alice, &qr).await;
+
+    let alice_charlie_id = alice.add_or_lookup_contact_id(charlie).await;
+    remove_contact_from_chat(alice, alice_broadcast_id, alice_charlie_id).await?;
+    assert!(alice.pop_sent_msg_opt(Duration::ZERO).await.is_none());
+    assert!(!remove_from_chat_contacts_table(alice, alice_broadcast_id, alice_charlie_id).await?);
+    assert!(
+        !remove_from_chat_contacts_table_without_trace(alice, alice_broadcast_id, alice_charlie_id)
+            .await?
+    );
+    Ok(())
+}
+
 /// Tests that in a broadcast channel,
 /// the recipients can't see the identity of their fellow recipients.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]

src/receive_imf.rs

@@ -3790,13 +3790,17 @@ async fn apply_out_broadcast_changes(
         } else if from_id == ContactId::SELF
             && let Some(removed_id) = removed_id
         {
-            chat::remove_from_chat_contacts_table_without_trace(context, chat.id, removed_id)
-                .await?;
-
-            better_msg.get_or_insert(
-                stock_str::msg_del_member_local(context, removed_id, ContactId::SELF).await,
-            );
-            added_removed_id = Some(removed_id);
+            if chat::remove_from_chat_contacts_table_without_trace(context, chat.id, removed_id)
+                .await?
+            {
+                better_msg.get_or_insert(
+                    stock_str::msg_del_member_local(context, removed_id, ContactId::SELF).await,
+                );
+                added_removed_id = Some(removed_id);
+            } else {
+                info!(context, "No-op broadcast member removal message (TRASH).");
+                better_msg = Some("".to_string());
+            }
         }
     }
 
@@ -3870,17 +3874,20 @@ async fn apply_in_broadcast_changes(
         }
         chat::delete_broadcast_secret(context, chat.id).await?;
 
-        if from_id == ContactId::SELF {
+        let removed =
+            chat::remove_from_chat_contacts_table_without_trace(context, chat.id, ContactId::SELF)
+                .await?;
+        if !removed {
+            info!(context, "No-op broadcast SELF-removal message (TRASH).");
+            better_msg = Some("".to_string());
+        } else if from_id == ContactId::SELF {
             better_msg.get_or_insert(stock_str::msg_you_left_broadcast(context));
         } else {
             better_msg.get_or_insert(
                 stock_str::msg_del_member_local(context, ContactId::SELF, from_id).await,
             );
         }
-
-        chat::remove_from_chat_contacts_table_without_trace(context, chat.id, ContactId::SELF)
-            .await?;
-        send_event_chat_modified = true;
+        send_event_chat_modified |= removed;
     } else if !chat.is_self_in_chat(context).await? {
         chat::add_to_chat_contacts_table(
             context,