typos

This reverts commit 57ba2387e8.

CHANGELOG.md

@@ -57,13 +57,13 @@
 ### Fixes
 - Do not add an error if the message is encrypted but not signed #3860
 - Do not strip leading spaces from message lines #3867
+- Don't always rebuild group member lists #3872
 - Fix uncaught exception in JSON-RPC tests #3884
 - Fix STARTTLS connection and add a test for it #3907
 - Trigger reconnection when failing to fetch existing messages #3911
 - Do not retry fetching existing messages after failure, prevents infinite reconnection loop #3913
 - Ensure format=flowed formatting is always reversible on the receiver side #3880
 
-
 ## 1.104.0
 
 ### Changes

src/chat.rs

@@ -1363,6 +1363,8 @@ impl Chat {
         self.param.get_bool(Param::Unpromoted).unwrap_or_default()
     }
 
+    /// Returns whether the chat is promoted which means that a message has been
+    /// send to it and it not only exists on the users device.
     pub fn is_promoted(&self) -> bool {
         !self.is_unpromoted()
     }
@@ -2855,6 +2857,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,
@@ -2876,7 +2879,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,
@@ -2898,7 +2901,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 {
@@ -3679,7 +3682,7 @@ mod tests {
     use crate::constants::{DC_GCL_ARCHIVED_ONLY, DC_GCL_NO_SPECIALS};
     use crate::contact::{Contact, ContactAddress};
     use crate::receive_imf::receive_imf;
-    use crate::test_utils::TestContext;
+    use crate::test_utils::{TestContext, TestContextManager};
 
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_chat_info() {
@@ -4074,6 +4077,45 @@ mod tests {
         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_leave_group() -> Result<()> {
         let alice = TestContext::new_alice().await;

src/mimeparser.rs

@@ -57,6 +57,8 @@ pub 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<String>,
     pub chat_disposition_notification_to: Option<SingleInfo>,
     pub decryption_info: DecryptionInfo,
@@ -721,6 +723,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")
     }

src/receive_imf.rs

@@ -604,16 +604,6 @@ async fn add_parts(
                     }
                 }
             }
-
-            better_msg = better_msg.or(apply_group_changes(
-                context,
-                mime_parser,
-                sent_timestamp,
-                chat_id,
-                from_id,
-                to_ids,
-            )
-            .await?);
         }
 
         if chat_id.is_none() {
@@ -659,6 +649,21 @@ async fn add_parts(
             apply_mailinglist_changes(context, mime_parser, chat_id).await?;
         }
 
+        if let Some(chat_id) = chat_id {
+            if let Some(even_better_msg) = apply_group_changes(
+                context,
+                mime_parser,
+                sent_timestamp,
+                chat_id,
+                from_id,
+                to_ids,
+            )
+            .await?
+            {
+                better_msg = Some(even_better_msg)
+            }
+        }
+
         // if contact renaming is prevented (for mailinglists and bots),
         // we use name from From:-header as override name
         if prevent_rename {
@@ -1616,90 +1621,128 @@ async fn apply_group_changes(
     to_ids: &[ContactId],
 ) -> Result<Option<String>> {
     let mut chat = Chat::load_from_db(context, chat_id).await?;
+
     if chat.typ != Chattype::Group {
         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;
+    let mut apply_group_changes = false;
+
+    let mut recreate_member_list = if mime_parser
+        .get_header(HeaderDef::ChatGroupMemberRemoved)
+        .is_some()
+        || mime_parser
+            .get_header(HeaderDef::ChatGroupMemberAdded)
+            .is_some()
+    {
+        let self_added =
+            if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) {
+                if let Ok(self_addr) = context.get_primary_self_addr().await {
+                    self_addr == *added_addr
+                } else {
+                    false
+                }
+            } else {
+                false
+            };
+
+        if !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await? && !self_added {
+            false
+        } else {
+            apply_group_changes = chat_id
+                .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
+                .await?;
+
+            match mime_parser.get_header(HeaderDef::InReplyTo) {
+                // If we don't know the referenced message, we missed some messages which could be add/delete
+                Some(reply_to) if rfc724_mid_exists(context, reply_to).await?.is_none() => true,
+                Some(_) => self_added,
+                None => false,
+            }
+        }
+    } else {
+        false
+    };
+
     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)
+        if let Some(contact_id) = removed_id {
+            // remove a single member from the chat
+            if !recreate_member_list && apply_group_changes {
+                chat::remove_from_chat_contacts_table(context, chat_id, contact_id).await?;
+                send_event_chat_modified = true;
+            }
+
+            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 if let Some(added_addr) = mime_parser
+        .get_header(HeaderDef::ChatGroupMemberAdded)
+        .cloned()
+    {
+        better_msg = Some(stock_str::msg_add_member(context, &added_addr, from_id).await);
+
+        // add a single member to the chat
+        if !recreate_member_list && apply_group_changes {
+            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 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=?;",
+                        paramsv![grpname.to_string(), 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)
+                    }
                 };
             }
-            None => warn!(context, "removed {:?} has no contact_id", removed_addr),
         }
-    } 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=?;",
-                            paramsv![grpname.to_string(), 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.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;
     }
 
     if mime_parser.get_header(HeaderDef::ChatVerified).is_some() {
@@ -1713,32 +1756,30 @@ 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 member list if message is from a MUA
+    if !mime_parser.has_chat_version()
+        && chat_id
+            .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
+            .await?
+    {
+        recreate_member_list = true;
+    }
+
+    // recreate 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 {} attempts to modify group chat {} member list without being a member.",
                 from_id,
                 chat_id
             );
-        } 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 MUA
+            if mime_parser.has_chat_version() {
                 context
                     .sql
                     .execute(
@@ -1746,24 +1787,27 @@ async fn apply_group_changes(
                         paramsv![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.remove(&removed_id);
             }
-            members_to_add.dedup();
 
             info!(
                 context,
-                "adding {:?} to chat id={}", members_to_add, chat_id
+                "recreating chat {chat_id} with members {members_to_add:?}"
             );
-            chat::add_to_chat_contacts_table(context, chat_id, &members_to_add).await?;
+
+            chat::add_to_chat_contacts_table(context, chat_id, &Vec::from_iter(members_to_add))
+                .await?;
             send_event_chat_modified = true;
         }
     }

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;
@@ -3245,3 +3248,182 @@ async fn test_mua_user_adds_recipient_to_single_chat() -> Result<()> {
 
     Ok(())
 }
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_dont_rebuild_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;
+
+    // We don't rebuild the contactlist of ours just because we received a new addition
+    assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4);
+
+    // alice adds a member
+    add_contact_to_chat(
+        &alice,
+        alice_chat_id,
+        Contact::create(&alice, "daisy", "daisy@example.net").await?,
+    )
+    .await?;
+
+    // bob removes a member
+    remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?;
+
+    alice.recv_msg(&bob.pop_sent_msg().await).await;
+
+    // We don't rebuild the contactlist of ours just because we received a new removal
+    assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4);
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_rebuild_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?;
+
+    // create chat with three members
+    add_to_chat_contacts_table(
+        &alice,
+        chat_id,
+        &[
+            Contact::create(&alice, "bob", "bob@example.net").await?,
+            Contact::create(&alice, "fiona", "fiona@example.net").await?,
+        ],
+    )
+    .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?;
+    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);
+
+    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;
+
+    // even though the received message contains a header for member addition,
+    // you are not added to the group
+    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 2 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: <bob@example.net>, <claire@example.org>, <fiona@example.org> \r\n\
+            Date: Mon, 12 Dec 2022 14:30:39 +0000\r\n\
+            Message-ID: <Mr.alices_original_mail@example.org>\r\n\
+            Chat-Version: 1.0\r\n\
+            \r\n\
+            tst\r\n",
+        false,
+    )
+    .await?
+    .unwrap();
+    let single_chat = Chat::load_from_db(&alice, msg.chat_id).await?;
+    assert_eq!(single_chat.typ, Chattype::Group);
+
+    // Bob uses a classical MUA to answer again, removing a recipient.
+    let msg3 = receive_imf(
+        &alice,
+        b"Subject: Re: Message from alice\r\n\
+            From: <bob@example.net>\r\n\
+            To: <alice@example.org>, <claire@example.org>\r\n\
+            Date: Mon, 12 Dec 2022 14:32:39 +0000\r\n\
+            Message-ID: <bobs_answer_to_two_recipients@example.net>\r\n\
+            In-Reply-To: <Mr.alices_original_mail@example.org>\r\n\
+            \r\n\
+            Hi back!\r\n",
+        false,
+    )
+    .await?
+    .unwrap();
+    assert_eq!(msg3.chat_id, single_chat.id);
+    let group_chat = Chat::load_from_db(&alice, msg3.chat_id).await?;
+    assert_eq!(group_chat.typ, Chattype::Group);
+    assert_eq!(
+        chat::get_chat_contacts(&alice, group_chat.id).await?.len(),
+        4
+    );
+    Ok(())
+}