fix: prefer Chat-Group-ID over references for new groups

src/receive_imf.rs

@@ -782,29 +782,6 @@ async fn add_parts(
             info!(context, "Message is an MDN (TRASH).",);
         }
 
-        // Try to assign to a chat based on Chat-Group-ID.
-        if chat_id.is_none() {
-            if let Some(grpid) = mime_parser.get_chat_group_id() {
-                if let Some((id, _protected, blocked)) =
-                    chat::get_chat_id_by_grpid(context, grpid).await?
-                {
-                    chat_id = Some(id);
-                    chat_id_blocked = blocked;
-                }
-            }
-        }
-
-        if chat_id.is_none() {
-            // try to assign to a chat based on In-Reply-To/References:
-
-            if let Some((new_chat_id, new_chat_id_blocked)) =
-                lookup_chat_by_reply(context, mime_parser, &parent, to_ids, from_id).await?
-            {
-                chat_id = Some(new_chat_id);
-                chat_id_blocked = new_chat_id_blocked;
-            }
-        }
-
         // signals whether the current user is a bot
         let is_bot = context.get_config_bool(Config::Bot).await?;
 
@@ -834,10 +811,15 @@ async fn add_parts(
             create_blocked_default
         };
 
-        if chat_id.is_none() && (allow_creation || test_normal_chat.is_some()) {
+        // Try to assign to a chat based on Chat-Group-ID.
-            // try to create a group
+        if chat_id.is_none() {
-
             if let Some(grpid) = mime_parser.get_chat_group_id().map(|s| s.to_string()) {
+                if let Some((id, _protected, blocked)) =
+                    chat::get_chat_id_by_grpid(context, &grpid).await?
+                {
+                    chat_id = Some(id);
+                    chat_id_blocked = blocked;
+                } else if allow_creation || test_normal_chat.is_some() {
                     if let Some((new_chat_id, new_chat_id_blocked)) = create_group(
                         context,
                         mime_parser,
@@ -853,7 +835,24 @@ async fn add_parts(
                         chat_id = Some(new_chat_id);
                         chat_id_blocked = new_chat_id_blocked;
                     }
-            } else if let Some(new_chat_id) = create_adhoc_group(
+                }
+            }
+        }
+
+        if chat_id.is_none() {
+            // try to assign to a chat based on In-Reply-To/References:
+
+            if let Some((new_chat_id, new_chat_id_blocked)) =
+                lookup_chat_by_reply(context, mime_parser, &parent, to_ids, from_id).await?
+            {
+                chat_id = Some(new_chat_id);
+                chat_id_blocked = new_chat_id_blocked;
+            }
+        }
+
+        if chat_id.is_none() && (allow_creation || test_normal_chat.is_some()) {
+            // Try to create an ad hoc group.
+            if let Some(new_chat_id) = create_adhoc_group(
                 context,
                 mime_parser,
                 create_blocked,
@@ -1062,12 +1061,28 @@ async fn add_parts(
 
         // Try to assign to a chat based on Chat-Group-ID.
         if chat_id.is_none() {
-            if let Some(grpid) = mime_parser.get_chat_group_id() {
+            if let Some(grpid) = mime_parser.get_chat_group_id().map(|s| s.to_string()) {
                 if let Some((id, _protected, blocked)) =
-                    chat::get_chat_id_by_grpid(context, grpid).await?
+                    chat::get_chat_id_by_grpid(context, &grpid).await?
                 {
                     chat_id = Some(id);
                     chat_id_blocked = blocked;
+                } else if allow_creation {
+                    if let Some((new_chat_id, new_chat_id_blocked)) = create_group(
+                        context,
+                        mime_parser,
+                        is_partial_download.is_some(),
+                        Blocked::Not,
+                        from_id,
+                        to_ids,
+                        &verified_encryption,
+                        &grpid,
+                    )
+                    .await?
+                    {
+                        chat_id = Some(new_chat_id);
+                        chat_id_blocked = new_chat_id_blocked;
+                    }
                 }
             }
         }
@@ -1115,23 +1130,7 @@ async fn add_parts(
         }
 
         if chat_id.is_none() && allow_creation {
-            if let Some(grpid) = mime_parser.get_chat_group_id().map(|s| s.to_string()) {
+            if let Some(new_chat_id) = create_adhoc_group(
-                if let Some((new_chat_id, new_chat_id_blocked)) = create_group(
-                    context,
-                    mime_parser,
-                    is_partial_download.is_some(),
-                    Blocked::Not,
-                    from_id,
-                    to_ids,
-                    &verified_encryption,
-                    &grpid,
-                )
-                .await?
-                {
-                    chat_id = Some(new_chat_id);
-                    chat_id_blocked = new_chat_id_blocked;
-                }
-            } else if let Some(new_chat_id) = create_adhoc_group(
                 context,
                 mime_parser,
                 Blocked::Not,

src/receive_imf/tests.rs

@@ -4639,3 +4639,64 @@ async fn test_group_no_recipients() -> Result<()> {
 
     Ok(())
 }
+
+/// Tests that creating a group
+/// is preferred over assigning message to existing
+/// chat based on `In-Reply-To` and `References`.
+///
+/// Referenced message itself may be incorrectly assigned,
+/// but `Chat-Group-ID` uniquely identifies the chat.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_prefer_chat_group_id_over_references() -> Result<()> {
+    let t = &TestContext::new_alice().await;
+
+    // Alice receives 1:1 message from Bob.
+    let raw = b"From: bob@example.net\n\
+                To: alice@example.org\n\
+                Subject: Hi\n\
+                Message-ID: <oneoneone@localhost>\n\
+                \n\
+                Hello!";
+    receive_imf(t, raw, false).await?.unwrap();
+
+    // Alice receives a group message from Bob.
+    // This references 1:1 message,
+    // but should create a group.
+    let raw = b"From: bob@example.net\n\
+                To: alice@example.org\n\
+                Subject: Group\n\
+                Chat-Version: 1.0\n\
+                Chat-Group-Name: Group 1\n\
+                Chat-Group-ID: GePFDkwEj2K\n\
+                Message-ID: <incoming@localhost>\n\
+                References: <oneoneone@localhost>\n\
+                In-Reply-To: <oneoneone@localhost>\n\
+                \n\
+                Group 1";
+    let received1 = receive_imf(t, raw, false).await?.unwrap();
+    let msg1 = Message::load_from_db(t, *received1.msg_ids.last().unwrap()).await?;
+    let chat1 = Chat::load_from_db(t, msg1.chat_id).await?;
+    assert_eq!(chat1.typ, Chattype::Group);
+
+    // Alice receives outgoing group message.
+    // This references 1:1 message,
+    // but should create another group.
+    let raw = b"From: alice@example.org\n\
+                To: bob@example.net
+                Subject: Group\n\
+                Chat-Version: 1.0\n\
+                Chat-Group-Name: Group 2\n\
+                Chat-Group-ID: Abcdexyzfoo\n\
+                Message-ID: <outgoing@localhost>\n\
+                References: <oneoneone@localhost>\n\
+                In-Reply-To: <oneoneone@localhost>\n\
+                \n\
+                Group 2";
+    let received2 = receive_imf(t, raw, false).await?.unwrap();
+    let msg2 = Message::load_from_db(t, *received2.msg_ids.last().unwrap()).await?;
+    let chat2 = Chat::load_from_db(t, msg2.chat_id).await?;
+    assert_eq!(chat2.typ, Chattype::Group);
+
+    assert_ne!(chat1.id, chat2.id);
+    Ok(())
+}