From c9cf2b7f2edfff3e83889e84ed38dd76ee22b457 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Wed, 6 Nov 2024 13:35:57 -0300 Subject: [PATCH] fix: Only add "member added/removed" messages if they actually do that (#5992) There were many cases in which "member added/removed" messages were added to chats even if they actually do nothing because a member is already added or removed. But primarily this fixes a scenario when Alice has several devices and shares an invite link somewhere, and both their devices handle the SecureJoin and issue `ChatGroupMemberAdded` messages so all other members see a duplicated group member addition. --- src/chat.rs | 2 +- src/receive_imf.rs | 66 +++++++++++++++++++++++++--------------- src/receive_imf/tests.rs | 29 ++++++++++++++++-- 3 files changed, 70 insertions(+), 27 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index e33a7d7a5..c1d2f3d1d 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -5371,7 +5371,7 @@ mod tests { // Eventually, first removal message arrives. // This has no effect. - bob.recv_msg(&remove1).await; + bob.recv_msg_trash(&remove1).await; assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); Ok(()) } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index e7fdb81c3..e4bbeac89 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1423,7 +1423,11 @@ async fn add_parts( if let Some(msg) = group_changes_msgs.1 { match &better_msg { None => better_msg = Some(msg), - Some(_) => group_changes_msgs.0.push(msg), + Some(_) => { + if !msg.is_empty() { + group_changes_msgs.0.push(msg) + } + } } } @@ -1506,6 +1510,9 @@ async fn add_parts( let mut txt_raw = "".to_string(); let (msg, typ): (&str, Viewtype) = if let Some(better_msg) = &better_msg { + if better_msg.is_empty() && is_partial_download.is_none() { + chat_id = DC_CHAT_ID_TRASH; + } (better_msg, Viewtype::Text) } else { (&part.msg, part.typ) @@ -2077,8 +2084,11 @@ async fn create_group( /// Apply group member list, name, avatar and protection status changes from the MIME message. /// -/// Optionally returns better message to replace the original system message. -/// is_partial_download: whether the message is not fully downloaded. +/// Returns `Vec` of group changes messages and, optionally, a better message to replace the +/// original system message. If the better message is empty, the original system message should be +/// just omitted. +/// +/// * `is_partial_download` - whether the message is not fully downloaded. #[allow(clippy::too_many_arguments)] async fn apply_group_changes( context: &Context, @@ -2180,39 +2190,47 @@ async fn apply_group_changes( if let Some(removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) { removed_id = Contact::lookup_id_by_addr(context, removed_addr, Origin::Unknown).await?; - - better_msg = if removed_id == Some(from_id) { - Some(stock_str::msg_group_left_local(context, from_id).await) - } else { - Some(stock_str::msg_del_member_local(context, removed_addr, from_id).await) - }; - - if removed_id.is_some() { - if !allow_member_list_changes { - info!( - context, - "Ignoring removal of {removed_addr:?} from {chat_id}." - ); + if let Some(id) = removed_id { + if allow_member_list_changes && chat_contacts.contains(&id) { + better_msg = if id == from_id { + Some(stock_str::msg_group_left_local(context, from_id).await) + } else { + Some(stock_str::msg_del_member_local(context, removed_addr, from_id).await) + }; } } else { warn!(context, "Removed {removed_addr:?} has no contact id.") } + better_msg.get_or_insert_with(Default::default); + if !allow_member_list_changes { + info!( + context, + "Ignoring removal of {removed_addr:?} from {chat_id}." + ); + } } else if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) { - better_msg = Some(stock_str::msg_add_member_local(context, added_addr, from_id).await); - if allow_member_list_changes { - if !recreate_member_list { - if let Some(contact_id) = - Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await? - { + let is_new_member; + if let Some(contact_id) = + Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await? + { + if !recreate_member_list { added_id = Some(contact_id); - } else { - warn!(context, "Added {added_addr:?} has no contact id.") } + is_new_member = !chat_contacts.contains(&contact_id); + } else { + warn!(context, "Added {added_addr:?} has no contact id."); + is_new_member = false; + } + + if is_new_member || self_added { + better_msg = + Some(stock_str::msg_add_member_local(context, added_addr, from_id).await); } } else { info!(context, "Ignoring addition of {added_addr:?} to {chat_id}."); } + better_msg.get_or_insert_with(Default::default); } else if let Some(old_name) = mime_parser .get_header(HeaderDef::ChatGroupNameChanged) .map(|s| s.trim()) diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index b9dd661cf..a049d6d9c 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -4185,9 +4185,8 @@ async fn test_recreate_contact_list_on_missing_message() -> Result<()> { // readd fiona add_contact_to_chat(&alice, chat_id, alice_fiona).await?; - alice.recv_msg(&remove_msg).await; - // delayed removal of fiona shouldn't remove her + alice.recv_msg_trash(&remove_msg).await; assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 4); Ok(()) @@ -4947,6 +4946,32 @@ async fn test_unarchive_on_member_removal() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_no_op_member_added_is_trash() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let alice_chat_id = alice + .create_group_with_members(ProtectionStatus::Unprotected, "foos", &[bob]) + .await; + send_text_msg(alice, alice_chat_id, "populate".to_string()).await?; + let msg = alice.pop_sent_msg().await; + bob.recv_msg(&msg).await; + let bob_chat_id = bob.get_last_msg().await.chat_id; + bob_chat_id.accept(bob).await?; + + let fiona_id = Contact::create(alice, "", "fiona@example.net").await?; + add_contact_to_chat(alice, alice_chat_id, fiona_id).await?; + let msg = alice.pop_sent_msg().await; + + let fiona_id = Contact::create(bob, "", "fiona@example.net").await?; + add_contact_to_chat(bob, bob_chat_id, fiona_id).await?; + bob.recv_msg_trash(&msg).await; + let contacts = get_chat_contacts(bob, bob_chat_id).await?; + assert_eq!(contacts.len(), 3); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_forged_from() -> Result<()> { let mut tcm = TestContextManager::new();