diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 62029dc03..ddff89833 100644 --- a/src/receive_imf.rs +++ b/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,26 +811,48 @@ async fn add_parts( create_blocked_default }; - if chat_id.is_none() && (allow_creation || test_normal_chat.is_some()) { - // try to create a group - + // 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().map(|s| s.to_string()) { - if let Some((new_chat_id, new_chat_id_blocked)) = create_group( - context, - mime_parser, - is_partial_download.is_some(), - create_blocked, - from_id, - to_ids, - &verified_encryption, - &grpid, - ) - .await? + if let Some((id, _protected, blocked)) = + chat::get_chat_id_by_grpid(context, &grpid).await? { - chat_id = Some(new_chat_id); - chat_id_blocked = new_chat_id_blocked; + 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, + is_partial_download.is_some(), + create_blocked, + 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( + } + } + + 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, 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( + if let Some(new_chat_id) = create_adhoc_group( context, mime_parser, Blocked::Not, diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 88900100a..33dcec546 100644 --- a/src/receive_imf/tests.rs +++ b/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: \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: \n\ + References: \n\ + In-Reply-To: \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: \n\ + References: \n\ + In-Reply-To: \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(()) +}