diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 79ee1efdb..2b6d8d979 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -773,6 +773,7 @@ pub(crate) async fn receive_imf_inner( chat_id, chat_id_blocked, is_dc_message, + is_created, ) .await .context("add_parts error")? @@ -1809,6 +1810,7 @@ async fn add_parts( mut chat_id: ChatId, mut chat_id_blocked: Blocked, is_dc_message: MessengerMessage, + is_chat_created: bool, ) -> Result { let to_id = if mime_parser.incoming { ContactId::SELF @@ -1872,7 +1874,16 @@ async fn add_parts( apply_out_broadcast_changes(context, mime_parser, &mut chat, from_id).await? } Chattype::Group => { - apply_group_changes(context, mime_parser, &mut chat, from_id, to_ids, past_ids).await? + apply_group_changes( + context, + mime_parser, + &mut chat, + from_id, + to_ids, + past_ids, + is_chat_created, + ) + .await? } Chattype::InBroadcast => { apply_in_broadcast_changes(context, mime_parser, &mut chat, from_id).await? @@ -2889,8 +2900,7 @@ async fn create_group( let mut chat_id = None; let mut chat_id_blocked = Default::default(); - if chat_id.is_none() - && !mime_parser.is_mailinglist_message() + if !mime_parser.is_mailinglist_message() && !grpid.is_empty() && mime_parser.get_header(HeaderDef::ChatGroupName).is_some() // otherwise, a pending "quit" message may pop up @@ -3082,6 +3092,7 @@ async fn apply_group_changes( from_id: ContactId, to_ids: &[Option], past_ids: &[Option], + is_chat_created: bool, ) -> Result { let from_is_key_contact = Contact::get_by_id(context, from_id).await?.is_key_contact(); ensure!(from_is_key_contact || chat.grpid.is_empty()); @@ -3295,19 +3306,15 @@ async fn apply_group_changes( .difference(&new_chat_contacts) .copied() .collect(); - - if let Some(added_id) = added_id - && !added_ids.remove(&added_id) - && added_id != ContactId::SELF - { - // No-op "Member added" message. An exception is self-addition messages because they at - // least must be shown when a chat is created on our side. - info!(context, "No-op 'Member added' message (TRASH)"); - better_msg = Some(String::new()); - } + let id_was_already_added = if let Some(added_id) = added_id { + !added_ids.remove(&added_id) + } else { + false + }; if let Some(removed_id) = removed_id { removed_ids.remove(&removed_id); } + let group_changes_msgs = if !chat_contacts.contains(&ContactId::SELF) && new_chat_contacts.contains(&ContactId::SELF) { @@ -3316,6 +3323,11 @@ async fn apply_group_changes( group_changes_msgs(context, &added_ids, &removed_ids, chat.id).await? }; + if id_was_already_added && group_changes_msgs.is_empty() && !is_chat_created { + info!(context, "No-op 'Member added' message (TRASH)"); + better_msg = Some(String::new()); + } + if send_event_chat_modified { context.emit_event(EventType::ChatModified(chat.id)); chatlist_events::emit_chatlist_item_changed(context, chat.id); diff --git a/src/securejoin/securejoin_tests.rs b/src/securejoin/securejoin_tests.rs index c8013cd9d..330d6716e 100644 --- a/src/securejoin/securejoin_tests.rs +++ b/src/securejoin/securejoin_tests.rs @@ -1584,3 +1584,51 @@ async fn test_auth_token_is_synchronized() -> Result<()> { Ok(()) } + +/// Tests that Bob deduplicates messages about being added to the group. +/// +/// If Alice (inviter) has multiple devices, +/// Bob may receive multiple "member added" messages. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_deduplicate_member_added() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice1 = &tcm.alice().await; + let alice2 = &tcm.alice().await; + let bob = &tcm.bob().await; + + // Enable sync messages so Alice QR code tokens + // are synchronized. + alice1.set_config_bool(Config::SyncMsgs, true).await?; + alice2.set_config_bool(Config::SyncMsgs, true).await?; + + tcm.section("Alice creates a group"); + let alice1_chat_id = chat::create_group(alice1, "Group").await?; + + tcm.section("Alice creates group QR code and synchronizes it"); + let qr = get_securejoin_qr(alice1, Some(alice1_chat_id)).await?; + sync(alice1, alice2).await; + + // Import Alice's key to skip key request step. + tcm.section("Bob imports Alice's vCard"); + bob.add_or_lookup_contact_id(alice1).await; + + tcm.section("Bob scans the QR code"); + let bob_chat_id = join_securejoin(bob, &qr).await?; + + tcm.section("Both Alice's devices add Bob to chat"); + let sent = bob.pop_sent_msg().await; + alice1.recv_msg_trash(&sent).await; + alice2.recv_msg_trash(&sent).await; + + let sent1 = alice1.pop_sent_msg().await; + let sent2 = alice2.pop_sent_msg().await; + + let bob_rcvd = bob.recv_msg(&sent1).await; + assert_eq!(bob_rcvd.chat_id, bob_chat_id); + assert_eq!(bob_rcvd.text, "Member Me added by alice@example.org."); + + // Second message is a no-op, so it is trashed. + bob.recv_msg_trash(&sent2).await; + + Ok(()) +} diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index 427cc3088..4f479ae0a 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -507,9 +507,8 @@ async fn test_verified_member_added_reordering() -> Result<()> { assert_eq!(fiona_received_message.get_text(), "Hi"); - // Fiona receives late "Member added" message - // and the chat becomes protected. - fiona.recv_msg(&alice_sent_member_added).await; + // Fiona receives late "Member added" message. + fiona.recv_msg_trash(&alice_sent_member_added).await; Ok(()) }