diff --git a/src/receive_imf.rs b/src/receive_imf.rs index b575529d1..755301ae5 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1654,40 +1654,27 @@ async fn apply_group_changes( false }; - // Whether to allow any changes to the member list at all. - let allow_member_list_changes = if chat::is_contact_in_chat(context, chat_id, ContactId::SELF) - .await? - || self_added - || !mime_parser.has_chat_version() - { - // Reject old group changes. - chat_id - .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp) - .await? - } else { - // Member list changes are not allowed if we're not in the group - // and are not explicitly added. - // This message comes from a Delta Chat that restored an old backup - // or the message is a MUA reply to an old message. - false - }; + // Whether to allow any changes to the member list at all. Just reject old group changes. + let allow_member_list_changes = chat_id + .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp) + .await?; + + let is_from_in_chat = chat::is_contact_in_chat(context, chat_id, from_id).await?; // Whether to rebuild the member list from scratch. - let recreate_member_list = if allow_member_list_changes { + let recreate_member_list = allow_member_list_changes && { // Recreate member list if the message comes from a MUA as these messages do _not_ set add/remove headers. - // Always recreate membership list if self has been added. - if !mime_parser.has_chat_version() || self_added { - true - } else { - match mime_parser.get_header(HeaderDef::InReplyTo) { + (is_from_in_chat && !mime_parser.has_chat_version()) + // Always recreate membership list if SELF has been added. The older versions of DC + // don't always set "In-Reply-To" to the latest message they sent, but to the latest + // delivered message (so it's a race), so we have this heuristic here. + || self_added + || match mime_parser.get_header(HeaderDef::InReplyTo) { // If we don't know the referenced message, we missed some messages. // Maybe they added/removed members, so we need to recreate our member list. Some(reply_to) => rfc724_mid_exists(context, reply_to).await?.is_none(), None => false, } - } - } else { - false }; if let Some(removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) { @@ -1794,43 +1781,41 @@ async fn apply_group_changes( // Recreate the member list. if recreate_member_list { - if !chat::is_contact_in_chat(context, chat_id, from_id).await? { + if !is_from_in_chat { warn!( context, - "Contact {from_id} attempts to modify group chat {chat_id} member list without being a member." + "Contact {from_id} modifies group chat {chat_id} member list possibly not being a member." ); - } else { - // Only delete old contacts if the sender is not a classical MUA user: - // Classical MUA users usually don't intend to remove users from an email - // thread, so if they removed a recipient then it was probably by accident. - if mime_parser.has_chat_version() { - context - .sql - .execute("DELETE FROM chats_contacts WHERE chat_id=?;", (chat_id,)) - .await?; - } - - let mut members_to_add = HashSet::new(); - members_to_add.extend(to_ids); - members_to_add.insert(ContactId::SELF); - - if !from_id.is_special() { - members_to_add.insert(from_id); - } - - if let Some(removed_id) = removed_id { - members_to_add.remove(&removed_id); - } - - info!( - context, - "Recreating chat {chat_id} with members {members_to_add:?}." - ); - - chat::add_to_chat_contacts_table(context, chat_id, &Vec::from_iter(members_to_add)) - .await?; - send_event_chat_modified = true; } + // Only delete old contacts if the sender is not a classical MUA user: + // Classical MUA users usually don't intend to remove users from an email + // thread, so if they removed a recipient then it was probably by accident. + if mime_parser.has_chat_version() { + context + .sql + .execute("DELETE FROM chats_contacts WHERE chat_id=?;", (chat_id,)) + .await?; + } + + let mut members_to_add = HashSet::new(); + members_to_add.extend(to_ids); + members_to_add.insert(ContactId::SELF); + + if !from_id.is_special() { + members_to_add.insert(from_id); + } + + if let Some(removed_id) = removed_id { + members_to_add.remove(&removed_id); + } + + info!( + context, + "Recreating chat {chat_id} with members {members_to_add:?}." + ); + + chat::add_to_chat_contacts_table(context, chat_id, &Vec::from_iter(members_to_add)).await?; + send_event_chat_modified = true; } if let Some(avatar_action) = &mime_parser.group_avatar { diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index c610961e1..dc9f91696 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -8,6 +8,7 @@ use crate::chat::{ }; use crate::chat::{get_chat_msgs, ChatItem, ChatVisibility}; use crate::chatlist::Chatlist; +use crate::config::Config; use crate::constants::{DC_GCL_FOR_FORWARDING, DC_GCL_NO_SPECIALS}; use crate::imap::prefetch_should_download; use crate::message::Message; @@ -3611,3 +3612,62 @@ async fn test_mua_can_readd() -> Result<()> { assert!(is_contact_in_chat(&alice, alice_chat.id, ContactId::SELF).await?); Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_recreate_member_list_on_missing_add_of_self() -> Result<()> { + let alice = TestContext::new_alice().await; + let bob = TestContext::new_bob().await; + let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?; + add_contact_to_chat( + &alice, + alice_chat_id, + Contact::create(&alice, "bob", &bob.get_config(Config::Addr).await?.unwrap()).await?, + ) + .await?; + send_text_msg(&alice, alice_chat_id, "populate".to_string()).await?; + alice.pop_sent_msg().await; + remove_contact_from_chat(&alice, alice_chat_id, ContactId::SELF).await?; + let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id; + + // Bob missed the message adding them, but must recreate the member list. + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 1); + assert!(is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_recreate_member_list_on_missing_add_of_others() -> Result<()> { + let alice = TestContext::new_alice().await; + let bob = TestContext::new_bob().await; + let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?; + add_contact_to_chat( + &alice, + alice_chat_id, + Contact::create(&alice, "bob", &bob.get_config(Config::Addr).await?.unwrap()).await?, + ) + .await?; + send_text_msg(&alice, alice_chat_id, "populate".to_string()).await?; + let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id; + + let fiona = TestContext::new_fiona().await; + add_contact_to_chat( + &alice, + alice_chat_id, + Contact::create( + &alice, + "fiona", + &fiona.get_config(Config::Addr).await?.unwrap(), + ) + .await?, + ) + .await?; + let fiona_chat_id = fiona.recv_msg(&alice.pop_sent_msg().await).await.chat_id; + fiona_chat_id.accept(&fiona).await?; + + send_text_msg(&fiona, fiona_chat_id, "hi".to_string()).await?; + bob.recv_msg(&fiona.pop_sent_msg().await).await; + + // Bob missed the message adding fiona, but must recreate the member list. + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 3); + Ok(()) +}