diff --git a/src/chat.rs b/src/chat.rs index d5649b005..829b0d423 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1400,6 +1400,7 @@ impl Chat { } /// Returns true if the chat is promoted. + /// This means a message has been sent to it and it _not_ only exists on the users device. pub fn is_promoted(&self) -> bool { !self.is_unpromoted() } @@ -2969,6 +2970,7 @@ pub(crate) async fn remove_from_chat_contacts_table( } /// Adds a contact to the chat. +/// If the group is promoted, also sends out a system message to all group members pub async fn add_contact_to_chat( context: &Context, chat_id: ChatId, @@ -2990,7 +2992,7 @@ pub(crate) async fn add_contact_to_chat_ex( chat_id.reset_gossiped_timestamp(context).await?; - /*this also makes sure, not contacts are added to special or normal chats*/ + // this also makes sure, no contacts are added to special or normal chats let mut chat = Chat::load_from_db(context, chat_id).await?; ensure!( chat.typ == Chattype::Group || chat.typ == Chattype::Broadcast, @@ -3012,7 +3014,7 @@ pub(crate) async fn add_contact_to_chat_ex( context.emit_event(EventType::ErrorSelfNotInGroup( "Cannot add contact to group; self not in group.".into(), )); - bail!("can not add contact because our account is not part of it"); + bail!("can not add contact because the account is not part of the group/broadcast"); } if from_handshake && chat.param.get_int(Param::Unpromoted).unwrap_or_default() == 1 { diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 4e23d7698..c40389502 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -63,6 +63,8 @@ pub(crate) struct MimeMessage { /// Whether the From address was repeated in the signed part /// (and we know that the signer intended to send from this address) pub from_is_signed: bool, + /// The List-Post address is only set for mailing lists. Users can send + /// messages to this address to post them to the list. pub list_post: Option, pub chat_disposition_notification_to: Option, pub decryption_info: DecryptionInfo, @@ -768,6 +770,8 @@ impl MimeMessage { !self.signatures.is_empty() } + /// Returns whether the email contains a `chat-version` header. + /// This indicates that the email is a DC-email. pub(crate) fn has_chat_version(&self) -> bool { self.header.contains_key("chat-version") } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index b592fc3ba..4c38e3cf2 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1641,86 +1641,136 @@ async fn apply_group_changes( return Ok(None); } - let mut recreate_member_list = false; let mut send_event_chat_modified = false; - + let mut removed_id = None; let mut better_msg = None; - let removed_id; - if let Some(removed_addr) = mime_parser - .get_header(HeaderDef::ChatGroupMemberRemoved) - .cloned() - { - removed_id = Contact::lookup_id_by_addr(context, &removed_addr, Origin::Unknown).await?; - recreate_member_list = true; - match removed_id { - Some(contact_id) => { - better_msg = if contact_id == from_id { - Some(stock_str::msg_group_left(context, from_id).await) - } else { - Some(stock_str::msg_del_member(context, &removed_addr, from_id).await) - }; + + // True if a Delta Chat client has explicitly added our current primary address. + let self_added = + if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) { + context.get_primary_self_addr().await? == *added_addr + } else { + 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 { + // 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 rebuild the member list from scratch. + let recreate_member_list = if 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) { + // 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, } - None => warn!(context, "Removed {removed_addr:?} has no contact_id."), } } else { - removed_id = None; - if let Some(added_member) = mime_parser - .get_header(HeaderDef::ChatGroupMemberAdded) - .cloned() - { - better_msg = Some(stock_str::msg_add_member(context, &added_member, from_id).await); - recreate_member_list = true; - } else if let Some(old_name) = mime_parser - .get_header(HeaderDef::ChatGroupNameChanged) - // See create_or_lookup_group() for explanation - .map(|s| s.trim()) - { - if let Some(grpname) = mime_parser - .get_header(HeaderDef::ChatGroupName) - // See create_or_lookup_group() for explanation - .map(|grpname| grpname.trim()) - .filter(|grpname| grpname.len() < 200) - { - if chat_id - .update_timestamp(context, Param::GroupNameTimestamp, sent_timestamp) - .await? - { - info!(context, "Updating grpname for chat {chat_id}."); - context - .sql - .execute( - "UPDATE chats SET name=? WHERE id=?;", - (strip_rtlo_characters(grpname), chat_id), - ) - .await?; + false + }; + + if let Some(removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) { + removed_id = Contact::lookup_id_by_addr(context, removed_addr, Origin::Unknown).await?; + if let Some(contact_id) = removed_id { + if allow_member_list_changes { + // Remove a single member from the chat. + if !recreate_member_list { + chat::remove_from_chat_contacts_table(context, chat_id, contact_id).await?; send_event_chat_modified = true; } - better_msg = - Some(stock_str::msg_grp_name(context, old_name, grpname, from_id).await); + better_msg = if contact_id == from_id { + Some(stock_str::msg_group_left(context, from_id).await) + } else { + Some(stock_str::msg_del_member(context, removed_addr, from_id).await) + }; + } else { + info!( + context, + "Ignoring removal of {removed_addr:?} from {chat_id}." + ); } - } else if let Some(value) = mime_parser.get_header(HeaderDef::ChatContent) { - if value == "group-avatar-changed" { - if let Some(avatar_action) = &mime_parser.group_avatar { - // this is just an explicit message containing the group-avatar, - // apart from that, the group-avatar is send along with various other messages - better_msg = match avatar_action { - AvatarAction::Delete => { - Some(stock_str::msg_grp_img_deleted(context, from_id).await) - } - AvatarAction::Change(_) => { - Some(stock_str::msg_grp_img_changed(context, from_id).await) - } - }; + } else { + warn!(context, "Removed {removed_addr:?} has no contact id.") + } + } else if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) { + if allow_member_list_changes { + // Add a single member to the chat. + if !recreate_member_list { + if let Some(contact_id) = + Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await? + { + chat::add_to_chat_contacts_table(context, chat_id, &[contact_id]).await?; + send_event_chat_modified = true; + } else { + warn!(context, "Added {added_addr:?} has no contact id.") } } - } - } - if !mime_parser.has_chat_version() { - // If a classical MUA user adds someone to TO/CC, then the DC user shall - // see this addition and have the new recipient in the member list. - recreate_member_list = true; + better_msg = Some(stock_str::msg_add_member(context, added_addr, from_id).await); + } else { + info!(context, "Ignoring addition of {added_addr:?} to {chat_id}."); + } + } else if let Some(old_name) = mime_parser + .get_header(HeaderDef::ChatGroupNameChanged) + // See create_or_lookup_group() for explanation + .map(|s| s.trim()) + { + if let Some(grpname) = mime_parser + .get_header(HeaderDef::ChatGroupName) + // See create_or_lookup_group() for explanation + .map(|grpname| grpname.trim()) + .filter(|grpname| grpname.len() < 200) + { + if chat_id + .update_timestamp(context, Param::GroupNameTimestamp, sent_timestamp) + .await? + { + info!(context, "Updating grpname for chat {chat_id}."); + context + .sql + .execute( + "UPDATE chats SET name=? WHERE id=?;", + (strip_rtlo_characters(grpname), chat_id), + ) + .await?; + send_event_chat_modified = true; + } + + better_msg = Some(stock_str::msg_grp_name(context, old_name, grpname, from_id).await); + } + } else if let Some(value) = mime_parser.get_header(HeaderDef::ChatContent) { + if value == "group-avatar-changed" { + if let Some(avatar_action) = &mime_parser.group_avatar { + // this is just an explicit message containing the group-avatar, + // apart from that, the group-avatar is send along with various other messages + better_msg = match avatar_action { + AvatarAction::Delete => { + Some(stock_str::msg_grp_img_deleted(context, from_id).await) + } + AvatarAction::Change(_) => { + Some(stock_str::msg_grp_img_changed(context, from_id).await) + } + }; + } + } } if mime_parser.get_header(HeaderDef::ChatVerified).is_some() { @@ -1734,49 +1784,46 @@ async fn apply_group_changes( chat_id .inner_set_protection(context, ProtectionStatus::Protected) .await?; - recreate_member_list = true; } } - // add members to group/check members + // Recreate the member list. if recreate_member_list { - if chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await? - && !chat::is_contact_in_chat(context, chat_id, from_id).await? - { + if !chat::is_contact_in_chat(context, chat_id, from_id).await? { warn!( context, "Contact {from_id} attempts to modify group chat {chat_id} member list without being a member." ); - } else if chat_id - .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp) - .await? - { - let mut members_to_add = vec![]; - if removed_id.is_some() - || !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await? - { - // Members could have been removed while we were - // absent. We can't use existing member list and need to - // start from scratch. + } 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?; - - members_to_add.push(ContactId::SELF); } + 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.push(from_id); + members_to_add.insert(from_id); } - members_to_add.extend(to_ids); - if let Some(removed_id) = removed_id { - members_to_add.retain(|id| *id != removed_id); - } - members_to_add.dedup(); - info!(context, "Adding {members_to_add:?} to chat id={chat_id}."); - chat::add_to_chat_contacts_table(context, chat_id, &members_to_add).await?; + 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; } } diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 1fda38221..680f58bb6 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -2,7 +2,10 @@ use tokio::fs; use super::*; use crate::aheader::EncryptPreference; -use crate::chat::get_chat_contacts; +use crate::chat::{ + add_contact_to_chat, add_to_chat_contacts_table, create_group_chat, get_chat_contacts, + is_contact_in_chat, remove_contact_from_chat, send_text_msg, +}; use crate::chat::{get_chat_msgs, ChatItem, ChatVisibility}; use crate::chatlist::Chatlist; use crate::constants::DC_GCL_NO_SPECIALS; @@ -3302,3 +3305,268 @@ async fn test_mua_user_adds_recipient_to_single_chat() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_sync_member_list_on_rejoin() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = tcm.alice().await; + + let bob_id = Contact::create(&alice, "", "bob@example.net").await?; + let claire_id = Contact::create(&alice, "", "claire@example.de").await?; + + let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "foos").await?; + add_contact_to_chat(&alice, alice_chat_id, bob_id).await?; + add_contact_to_chat(&alice, alice_chat_id, claire_id).await?; + + send_text_msg(&alice, alice_chat_id, "populate".to_string()).await?; + let add = alice.pop_sent_msg().await; + let bob = tcm.bob().await; + bob.recv_msg(&add).await; + let bob_chat_id = bob.get_last_msg().await.chat_id; + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 3); + + // remove bob from chat + remove_contact_from_chat(&alice, alice_chat_id, bob_id).await?; + let remove_bob = alice.pop_sent_msg().await; + bob.recv_msg(&remove_bob).await; + + // remove any other member + remove_contact_from_chat(&alice, alice_chat_id, claire_id).await?; + alice.pop_sent_msg().await; + + // readd bob + add_contact_to_chat(&alice, alice_chat_id, bob_id).await?; + let add2 = alice.pop_sent_msg().await; + bob.recv_msg(&add2).await; + + // number of members in chat should have updated + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_dont_recreate_contacts_on_add_remove() -> 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@example.net").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; + bob_chat_id.accept(&bob).await?; + + // alice adds a member + add_contact_to_chat( + &alice, + alice_chat_id, + Contact::create(&alice, "fiona", "fiona@example.net").await?, + ) + .await?; + + // bob adds a member. + let bob_blue = Contact::create(&bob, "blue", "blue@example.net").await?; + add_contact_to_chat(&bob, bob_chat_id, bob_blue).await?; + + alice.recv_msg(&bob.pop_sent_msg().await).await; + + // bob didn't receive the addition of fiona, but alice doesn't overwrite her own + // contact list with the one from bob which only has three members instead of four. + assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4); + + // bob removes a member. + remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?; + + alice.recv_msg(&bob.pop_sent_msg().await).await; + + // Bobs chat only has two members after the removal of blue, because he still + // didn't receive the addition of fiona. But that doesn't overwrite alice' + // memberlist. + assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_recreate_contact_list_on_missing_message() -> Result<()> { + let alice = TestContext::new_alice().await; + let bob = TestContext::new_bob().await; + let chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?; + let alice_fiona = Contact::create(&alice, "fiona", "fiona@example.net").await?; + // create chat with three members + add_to_chat_contacts_table( + &alice, + chat_id, + &[ + Contact::create(&alice, "bob", "bob@example.net").await?, + alice_fiona, + ], + ) + .await?; + + send_text_msg(&alice, chat_id, "populate".to_string()).await?; + let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id; + bob_chat_id.accept(&bob).await?; + + // bob removes a member + let bob_contact_fiona = Contact::create(&bob, "fiona", "fiona@example.net").await?; + remove_contact_from_chat(&bob, bob_chat_id, bob_contact_fiona).await?; + let remove_msg = bob.pop_sent_msg().await; + + // bob adds a new member + let bob_blue = Contact::create(&bob, "blue", "blue@example.net").await?; + add_contact_to_chat(&bob, bob_chat_id, bob_blue).await?; + + let add_msg = bob.pop_sent_msg().await; + + // alice only receives the addition of the member + alice.recv_msg(&add_msg).await; + + // since we missed a message, a new contact list should be build + assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 3); + + // 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 + assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 4); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_dont_readd_with_normal_msg() -> 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@example.net").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; + bob_chat_id.accept(&bob).await?; + + remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?; + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 1); + + add_contact_to_chat( + &alice, + alice_chat_id, + Contact::create(&alice, "fiora", "fiora@example.net").await?, + ) + .await?; + + bob.recv_msg(&alice.pop_sent_msg().await).await; + + // Alice didn't receive Bobs leave message, but bob shouldn't readded himself just because of that. + assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_mua_cant_remove() -> Result<()> { + let alice = TestContext::new_alice().await; + + // Alice creates chat with 3 contacts + let msg = receive_imf( + &alice, + b"Subject: =?utf-8?q?Message_from_alice=40example=2Eorg?=\r\n\ + From: alice@example.org\r\n\ + To: , , \r\n\ + Date: Mon, 12 Dec 2022 14:30:39 +0000\r\n\ + Message-ID: \r\n\ + Chat-Version: 1.0\r\n\ + \r\n\ + tst\r\n", + false, + ) + .await? + .unwrap(); + let alice_chat = Chat::load_from_db(&alice, msg.chat_id).await?; + assert_eq!(alice_chat.typ, Chattype::Group); + + // Bob uses a classical MUA to answer, removing a recipient. + let bob_removes = receive_imf( + &alice, + b"Subject: Re: Message from alice\r\n\ + From: \r\n\ + To: , \r\n\ + Date: Mon, 12 Dec 2022 14:32:39 +0000\r\n\ + Message-ID: \r\n\ + In-Reply-To: \r\n\ + \r\n\ + Hi back!\r\n", + false, + ) + .await? + .unwrap(); + assert_eq!(bob_removes.chat_id, alice_chat.id); + let group_chat = Chat::load_from_db(&alice, bob_removes.chat_id).await?; + assert_eq!(group_chat.typ, Chattype::Group); + assert_eq!( + chat::get_chat_contacts(&alice, group_chat.id).await?.len(), + 4 + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_mua_can_add() -> Result<()> { + let alice = TestContext::new_alice().await; + + // Alice creates chat with 3 contacts + let msg = receive_imf( + &alice, + b"Subject: =?utf-8?q?Message_from_alice=40example=2Eorg?=\r\n\ + From: alice@example.org\r\n\ + To: , , \r\n\ + Date: Mon, 12 Dec 2022 14:30:39 +0000\r\n\ + Message-ID: \r\n\ + Chat-Version: 1.0\r\n\ + \r\n\ + Hi!\r\n", + false, + ) + .await? + .unwrap(); + let alice_chat = Chat::load_from_db(&alice, msg.chat_id).await?; + assert_eq!(alice_chat.typ, Chattype::Group); + + // Bob uses a classical MUA to answer, adding a recipient. + let bob_adds = receive_imf( + &alice, + b"Subject: Re: Message from alice\r\n\ + From: \r\n\ + To: , , , \r\n\ + Date: Mon, 12 Dec 2022 14:32:39 +0000\r\n\ + Message-ID: \r\n\ + In-Reply-To: \r\n\ + \r\n\ + Hi back!\r\n", + false, + ) + .await? + .unwrap(); + + let group_chat = Chat::load_from_db(&alice, bob_adds.chat_id).await?; + assert_eq!(group_chat.typ, Chattype::Group); + assert_eq!( + chat::get_chat_contacts(&alice, group_chat.id).await?.len(), + 5 + ); + Ok(()) +}