Notify a removed member that they were removed

src/chat.rs

@@ -31,6 +31,7 @@ use crate::debug_logging::maybe_set_logging_xdc;
 use crate::download::DownloadState;
 use crate::ephemeral::{Timer as EphemeralTimer, start_chat_ephemeral_timers};
 use crate::events::EventType;
+use crate::key::self_fingerprint;
 use crate::location;
 use crate::log::{LogExt, error, info, warn};
 use crate::logged_debug_assert;
@@ -4272,10 +4273,18 @@ pub async fn remove_contact_from_chat(
             // This allows to delete dangling references to deleted contacts
             // in case of the database becoming inconsistent due to a bug.
             if let Some(contact) = Contact::get_by_id_optional(context, contact_id).await? {
-                if chat.typ == Chattype::Group && chat.is_promoted() {
+                if chat.is_promoted() {
                     let addr = contact.get_addr();
+                    let fingerprint = contact.fingerprint().map(|f| f.hex());
 
-                    let res = send_member_removal_msg(context, chat_id, contact_id, addr).await;
+                    let res = send_member_removal_msg(
+                        context,
+                        chat_id,
+                        contact_id,
+                        addr,
+                        fingerprint.as_deref(),
+                    )
+                    .await;
 
                     if contact_id == ContactId::SELF {
                         res?;
@@ -4299,7 +4308,9 @@ pub async fn remove_contact_from_chat(
         // For incoming broadcast channels, it's not possible to remove members,
         // but it's possible to leave:
         let self_addr = context.get_primary_self_addr().await?;
-        send_member_removal_msg(context, chat_id, contact_id, &self_addr).await?;
+        let fingerprint = self_fingerprint(context).await?;
+        send_member_removal_msg(context, chat_id, contact_id, &self_addr, Some(fingerprint))
+            .await?;
     } else {
         bail!("Cannot remove members from non-group chats.");
     }
@@ -4312,6 +4323,7 @@ async fn send_member_removal_msg(
     chat_id: ChatId,
     contact_id: ContactId,
     addr: &str,
+    fingerprint: Option<&str>,
 ) -> Result<MsgId> {
     let mut msg = Message::new(Viewtype::Text);
 
@@ -4323,6 +4335,7 @@ async fn send_member_removal_msg(
 
     msg.param.set_cmd(SystemMessage::MemberRemovedFromGroup);
     msg.param.set(Param::Arg, addr.to_lowercase());
+    msg.param.set_optional(Param::Arg2, fingerprint);
     msg.param
         .set(Param::ContactAddedRemoved, contact_id.to_u32());
 

src/chat/chat_tests.rs

@@ -3909,13 +3909,18 @@ async fn test_sync_broadcast() -> Result<()> {
     let msg = alice0.recv_msg(&sent_msg).await;
     assert_eq!(msg.chat_id, a0_broadcast_id);
     remove_contact_from_chat(alice0, a0_broadcast_id, a0b_contact_id).await?;
-    sync(alice0, alice1).await;
+    let sent = alice0.pop_sent_msg().await;
+    alice1.recv_msg(&sent).await;
     assert!(get_chat_contacts(alice1, a1_broadcast_id).await?.is_empty());
-    assert!(
+    // TODO do we want to make sure that there is no trace of a member?
-        get_past_chat_contacts(alice1, a1_broadcast_id)
+    // assert!(
-            .await?
+    //     get_past_chat_contacts(alice1, a1_broadcast_id)
-            .is_empty()
+    //         .await?
-    );
+    //         .is_empty()
+    // );
+    bob.recv_msg(&sent).await;
+    let bob_chat = Chat::load_from_db(bob, bob_broadcast_id).await?;
+    assert!(!bob_chat.is_self_in_chat(bob).await?);
 
     a0_broadcast_id.delete(alice0).await?;
     sync(alice0, alice1).await;

src/headerdef.rs

@@ -63,6 +63,7 @@ pub enum HeaderDef {
     ChatUserAvatar,
     ChatVoiceMessage,
     ChatGroupMemberRemoved,
+    ChatGroupMemberRemovedFpr,
     ChatGroupMemberAdded,
     ChatContent,
 

src/mimefactory.rs

@@ -1433,6 +1433,7 @@ impl MimeFactory {
             match command {
                 SystemMessage::MemberRemovedFromGroup => {
                     let email_to_remove = msg.param.get(Param::Arg).unwrap_or_default();
+                    let fingerprint_to_remove = msg.param.get(Param::Arg2).unwrap_or_default();
 
                     if email_to_remove
                         == context
@@ -1453,6 +1454,14 @@ impl MimeFactory {
                                 .into(),
                         ));
                     }
+
+                    if !fingerprint_to_remove.is_empty() {
+                        headers.push((
+                            "Chat-Group-Member-Removed-Fpr",
+                            mail_builder::headers::raw::Raw::new(fingerprint_to_remove.to_string())
+                                .into(),
+                        ));
+                    }
                 }
                 SystemMessage::MemberAddedToGroup => {
                     // TODO: lookup the contact by ID rather than email address.

src/receive_imf.rs

@@ -15,8 +15,7 @@ use num_traits::FromPrimitive;
 use regex::Regex;
 
 use crate::chat::{
-    self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, remove_from_chat_contacts_table,
+    self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, save_broadcast_shared_secret,
-    save_broadcast_shared_secret,
 };
 use crate::config::Config;
 use crate::constants::{self, Blocked, Chattype, DC_CHAT_ID_TRASH, EDITED_PREFIX, ShowEmails};
@@ -28,8 +27,8 @@ use crate::ephemeral::{Timer as EphemeralTimer, stock_ephemeral_timer_changed};
 use crate::events::EventType;
 use crate::headerdef::{HeaderDef, HeaderDefMap};
 use crate::imap::{GENERATED_PREFIX, markseen_on_imap_table};
-use crate::key::self_fingerprint_opt;
 use crate::key::{DcKey, Fingerprint, SignedPublicKey};
+use crate::key::{self_fingerprint, self_fingerprint_opt};
 use crate::log::LogExt;
 use crate::log::{info, warn};
 use crate::logged_debug_assert;
@@ -2895,15 +2894,13 @@ async fn apply_group_changes(
     }
 
     if let Some(removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) {
-        // TODO: if address "alice@example.org" is a member of the group twice,
+        if let Some(removed_fpr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemovedFpr) {
-        // with old and new key,
+            removed_id = lookup_key_contact_by_fingerprint(context, removed_fpr).await?;
-        // and someone (maybe Alice's new contact) just removed Alice's old contact,
+        } else {
-        // we may lookup the wrong contact because we only look up by the address.
+            // Removal message sent by a legacy Delta Chat client.
-        // The result is that info message may contain the new Alice's display name
+            removed_id =
-        // rather than old display name.
+                lookup_key_contact_by_address(context, removed_addr, Some(chat.id)).await?;
-        // This could be fixed by looking up the contact with the highest
+        }
-        // `remove_timestamp` after applying Chat-Group-Member-Timestamps.
-        removed_id = lookup_key_contact_by_address(context, removed_addr, Some(chat.id)).await?;
         if let Some(id) = removed_id {
             better_msg = if id == from_id {
                 silent = true;
@@ -2920,6 +2917,8 @@ async fn apply_group_changes(
             // we may lookup the wrong contact.
             // This could be fixed by looking up the contact with
             // highest `add_timestamp` to disambiguate.
+            // Alternatively, this can be fixed by a header ChatGroupMemberAddedFpr,
+            // just like we have ChatGroupMemberRemovedFpr.
             // The result of the error is that info message
             // may contain display name of the wrong contact.
             let fingerprint = key.dc_fingerprint().hex();
@@ -3489,23 +3488,30 @@ async fn apply_out_broadcast_changes(
     )
     .await?;
 
-    if let Some(_removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) {
+    if let Some(removed_fpr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemovedFpr) {
-        // The sender of the message left the broadcast channel
+        let removed_id = lookup_key_contact_by_fingerprint(context, removed_fpr).await?;
-        remove_from_chat_contacts_table(context, chat.id, from_id).await?;
+        if removed_id == Some(from_id) {
+            // The sender of the message left the broadcast channel
+            chat::remove_from_chat_contacts_table(context, chat.id, from_id).await?;
 
-        return Ok(GroupChangesInfo {
+            return Ok(GroupChangesInfo {
-            better_msg: Some("".to_string()),
+                better_msg: Some("".to_string()),
-            added_removed_id: None,
+                added_removed_id: None,
-            silent: true,
+                silent: true,
-            extra_msgs: vec![],
+                extra_msgs: vec![],
-        });
+            });
+        } else if from_id == ContactId::SELF {
+            if let Some(removed_id) = removed_id {
+                chat::remove_from_chat_contacts_table(context, chat.id, removed_id).await?;
+
+                better_msg.get_or_insert(
+                    stock_str::msg_del_member_local(context, removed_id, ContactId::SELF).await,
+                );
+            }
+        }
     } else if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) {
-        // TODO this may lookup the wrong contact if multiple contacts have the same email addr.
+        // TODO this block can be removed,
-        // We can send sync messages instead,
+        // now that all of Alice's devices get to know about Bob joining via Bob's QR message.
-        // lookup the fingerprint by gossip header (like it's done for groups right now),
-        // add a header ChatGroupMemberAddedFpr,
-        // or only handle addition on receival of Bob's request message and solve the problem in a different way for member-removed.
-        // --> link2xt said to probably handle addition on receival of Bob's request message, and to add a header ChatGroupMemberRemovedFpr.
         let contact = lookup_key_contact_by_address(context, added_addr, None).await?;
         if let Some(contact) = contact {
             better_msg.get_or_insert(
@@ -3565,13 +3571,20 @@ async fn apply_in_broadcast_changes(
         }
     }
 
-    if let Some(_removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) {
+    if let Some(removed_fpr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemovedFpr) {
+        // We are not supposed to receive a notification when someone else than self is removed:
+        ensure!(removed_fpr == self_fingerprint(context).await?);
+
         if from_id == ContactId::SELF {
-            // The only member added/removed message that is ever sent is "I left.",
-            // so, this is the only case we need to handle here
             better_msg
                 .get_or_insert(stock_str::msg_group_left_local(context, ContactId::SELF).await);
-        } // TODO handle removed case
+        } else {
+            better_msg.get_or_insert(
+                stock_str::msg_del_member_local(context, ContactId::SELF, from_id).await,
+            );
+        }
+
+        chat::remove_from_chat_contacts_table(context, chat.id, ContactId::SELF).await?;
     } else if !chat.is_self_in_chat(context).await? {
         // Apparently, self is in the chat now, because we're receiving messages
         chat::add_to_chat_contacts_table(

test-data/golden/test_sync_broadcast_bob

@@ -1,6 +1,7 @@
-InBroadcast#Chat#11: Channel [2 member(s)] 
+InBroadcast#Chat#11: Channel [1 member(s)] 
 --------------------------------------------------------------------------------
 Msg#11: info (Contact#Contact#Info): Establishing guaranteed end-to-end encryption, please wait… [NOTICED][INFO]
 Msg#12🔒:  (Contact#Contact#10): Member Me added by alice@example.org. [FRESH][INFO]
 Msg#14🔒:  (Contact#Contact#10): hi [FRESH]
+Msg#15🔒:  (Contact#Contact#10): Member Me removed by alice@example.org. [FRESH][INFO]
 --------------------------------------------------------------------------------