fix: Switch to original Hocuri's group membership consistency algo (#3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But 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 need this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.

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.
+    // Whether to allow any changes to the member list at all. Just reject old group changes.
-    let allow_member_list_changes = if chat::is_contact_in_chat(context, chat_id, ContactId::SELF)
+    let allow_member_list_changes = chat_id
-        .await?
+        .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
-        || self_added
+        .await?;
-        || !mime_parser.has_chat_version()
+
-    {
+    let is_from_in_chat = chat::is_contact_in_chat(context, chat_id, from_id).await?;
-        // 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 {
+    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.
+        (is_from_in_chat && !mime_parser.has_chat_version())
-        if !mime_parser.has_chat_version() || self_added {
+            // Always recreate membership list if SELF has been added. The older versions of DC
-            true
+            // don't always set "In-Reply-To" to the latest message they sent, but to the latest
-        } else {
+            // delivered message (so it's a race), so we have this heuristic here.
-            match mime_parser.get_header(HeaderDef::InReplyTo) {
+            || 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 {

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(())
+}