From 0acc34a882457ab0d6688fcc303720028a830ac1 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Fri, 8 Aug 2025 15:00:32 +0200 Subject: [PATCH] Notify a removed member that they were removed --- src/chat.rs | 19 +++++- src/chat/chat_tests.rs | 17 ++++-- src/headerdef.rs | 1 + src/mimefactory.rs | 9 +++ src/receive_imf.rs | 75 ++++++++++++++---------- test-data/golden/test_sync_broadcast_bob | 3 +- 6 files changed, 83 insertions(+), 41 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index af9c9dcb1..d47f8e32c 100644 --- a/src/chat.rs +++ b/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 { 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()); diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index e251087fd..7a61c4fe7 100644 --- a/src/chat/chat_tests.rs +++ b/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!( - get_past_chat_contacts(alice1, a1_broadcast_id) - .await? - .is_empty() - ); + // TODO do we want to make sure that there is no trace of a member? + // assert!( + // get_past_chat_contacts(alice1, a1_broadcast_id) + // .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; diff --git a/src/headerdef.rs b/src/headerdef.rs index 4b32a904b..055b57b07 100644 --- a/src/headerdef.rs +++ b/src/headerdef.rs @@ -63,6 +63,7 @@ pub enum HeaderDef { ChatUserAvatar, ChatVoiceMessage, ChatGroupMemberRemoved, + ChatGroupMemberRemovedFpr, ChatGroupMemberAdded, ChatContent, diff --git a/src/mimefactory.rs b/src/mimefactory.rs index f03ef7c70..0f75b3bd5 100644 --- a/src/mimefactory.rs +++ b/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. diff --git a/src/receive_imf.rs b/src/receive_imf.rs index eb74cb0f4..f161fee0e 100644 --- a/src/receive_imf.rs +++ b/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, - save_broadcast_shared_secret, + self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, 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, - // with old and new key, - // and someone (maybe Alice's new contact) just removed Alice's old contact, - // we may lookup the wrong contact because we only look up by the address. - // The result is that info message may contain the new Alice's display name - // rather than old display name. - // 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(removed_fpr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemovedFpr) { + removed_id = lookup_key_contact_by_fingerprint(context, removed_fpr).await?; + } else { + // Removal message sent by a legacy Delta Chat client. + 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) { - // The sender of the message left the broadcast channel - remove_from_chat_contacts_table(context, chat.id, from_id).await?; + if let Some(removed_fpr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemovedFpr) { + let removed_id = lookup_key_contact_by_fingerprint(context, removed_fpr).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 { - better_msg: Some("".to_string()), - added_removed_id: None, - silent: true, - extra_msgs: vec![], - }); + return Ok(GroupChangesInfo { + better_msg: Some("".to_string()), + added_removed_id: None, + silent: true, + 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. - // We can send sync messages instead, - // 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. + // TODO this block can be removed, + // now that all of Alice's devices get to know about Bob joining via Bob's QR message. 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( diff --git a/test-data/golden/test_sync_broadcast_bob b/test-data/golden/test_sync_broadcast_bob index 568dd8648..e0a31122b 100644 --- a/test-data/golden/test_sync_broadcast_bob +++ b/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] --------------------------------------------------------------------------------