From 1f52b8af2f5d689359cf483690a4cfa1dcffba35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Kl=C3=A4hn?= <39526136+Septias@users.noreply.github.com> Date: Mon, 6 Nov 2023 21:01:55 +0100 Subject: [PATCH] fix: Partial messages do not change group state (#4900) This message makes that partial messages do not change the group state. A simple fix and a comprehensive test is added. This is a follow up to the former #4841 which took a different approach. --- src/message.rs | 10 +++- src/mimeparser.rs | 2 +- src/receive_imf.rs | 15 ++++-- src/receive_imf/tests.rs | 110 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 6 deletions(-) diff --git a/src/message.rs b/src/message.rs index 8e7e8e3db..295f38a9d 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1818,6 +1818,14 @@ pub async fn estimate_deletion_cnt( pub(crate) async fn rfc724_mid_exists( context: &Context, rfc724_mid: &str, +) -> Result> { + rfc724_mid_exists_and(context, rfc724_mid, "1").await +} + +pub(crate) async fn rfc724_mid_exists_and( + context: &Context, + rfc724_mid: &str, + cond: &str, ) -> Result> { let rfc724_mid = rfc724_mid.trim_start_matches('<').trim_end_matches('>'); if rfc724_mid.is_empty() { @@ -1828,7 +1836,7 @@ pub(crate) async fn rfc724_mid_exists( let res = context .sql .query_row_optional( - "SELECT id FROM msgs WHERE rfc724_mid=?", + &("SELECT id FROM msgs WHERE rfc724_mid=? AND ".to_string() + cond), (rfc724_mid,), |row| { let msg_id: MsgId = row.get(0)?; diff --git a/src/mimeparser.rs b/src/mimeparser.rs index d4ffc69e0..655e0a23f 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -59,7 +59,7 @@ pub(crate) struct MimeMessage { /// Message headers. headers: HashMap, - /// Addresses are normalized and lowercased: + /// Addresses are normalized and lowercase pub recipients: Vec, /// `From:` address. diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 646f3ec24..fd33eb4b2 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -26,7 +26,8 @@ use crate::imap::{markseen_on_imap_table, GENERATED_PREFIX}; use crate::location; use crate::log::LogExt; use crate::message::{ - self, rfc724_mid_exists, Message, MessageState, MessengerMessage, MsgId, Viewtype, + self, rfc724_mid_exists, rfc724_mid_exists_and, Message, MessageState, MessengerMessage, MsgId, + Viewtype, }; use crate::mimeparser::{parse_message_ids, AvatarAction, MimeMessage, SystemMessage}; use crate::param::{Param, Params}; @@ -503,7 +504,6 @@ async fn add_parts( // - incoming messages introduce a chat only for known contacts if they are sent by a messenger // (of course, the user can add other chats manually later) let to_id: ContactId; - let state: MessageState; let mut needs_delete_job = false; if incoming { @@ -656,6 +656,7 @@ async fn add_parts( group_chat_id, from_id, to_ids, + is_partial_download.is_some(), ) .await?); } @@ -898,6 +899,7 @@ async fn add_parts( chat_id, from_id, to_ids, + is_partial_download.is_some(), ) .await?); } @@ -1693,6 +1695,7 @@ async fn create_or_lookup_group( /// Apply group member list, name, avatar and protection status changes from the MIME message. /// /// Optionally returns better message to replace the original system message. +/// is_partial_download: whether the message is not fully downloaded. async fn apply_group_changes( context: &Context, mime_parser: &mut MimeMessage, @@ -1700,6 +1703,7 @@ async fn apply_group_changes( chat_id: ChatId, from_id: ContactId, to_ids: &[ContactId], + is_partial_download: bool, ) -> Result> { let mut chat = Chat::load_from_db(context, chat_id).await?; if chat.typ != Chattype::Group { @@ -1724,7 +1728,8 @@ async fn apply_group_changes( !chat_contacts.contains(&ContactId::SELF) || chat_contacts.contains(&from_id); // Reject group membership changes from non-members and old changes. - let allow_member_list_changes = is_from_in_chat + let allow_member_list_changes = !is_partial_download + && is_from_in_chat && chat_id .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp) .await?; @@ -1738,7 +1743,9 @@ async fn apply_group_changes( || 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(), + Some(reply_to) => rfc724_mid_exists_and(context, reply_to, "download_state=0") + .await? + .is_none(), None => false, } } && { diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index cdfe479d3..850469634 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -3863,3 +3863,113 @@ async fn test_create_group_with_big_msg() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_partial_group_consistency() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = tcm.alice().await; + let bob_id = Contact::create(&alice, "", "bob@example.net").await?; + let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "foos").await?; + add_contact_to_chat(&alice, alice_chat_id, bob_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; + let contacts = get_chat_contacts(&bob, bob_chat_id).await?; + assert_eq!(contacts.len(), 2); + + // Get initial timestamp. + let timestamp = bob_chat_id + .get_param(&bob) + .await? + .get_i64(Param::MemberListTimestamp) + .unwrap(); + + // Bob receives partial message. + let msg_id = receive_imf_inner( + &bob, + "first@example.org", + b"From: Alice \n\ +To: , \n\ +Chat-Version: 1.0\n\ +Subject: subject\n\ +Message-ID: \n\ +Date: Sun, 14 Nov 2021 00:10:00 +0000\ +Content-Type: text/plain +Chat-Group-Member-Added: charlie@example.com", + false, + Some(100000), + false, + ) + .await? + .context("no received message")?; + + let msg = Message::load_from_db(&bob, msg_id.msg_ids[0]).await?; + let timestamp2 = bob_chat_id + .get_param(&bob) + .await? + .get_i64(Param::MemberListTimestamp) + .unwrap(); + + // Partial download does not change the member list. + assert_eq!(msg.download_state, DownloadState::Available); + assert_eq!(timestamp, timestamp2); + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?, contacts); + + // Alice sends normal message to bob, adding fiona. + add_contact_to_chat( + &alice, + alice_chat_id, + Contact::create(&alice, "fiona", "fiona@example.net").await?, + ) + .await?; + + bob.recv_msg(&alice.pop_sent_msg().await).await; + + let timestamp3 = bob_chat_id + .get_param(&bob) + .await? + .get_i64(Param::MemberListTimestamp) + .unwrap(); + + // Receiving a message after a partial download recreates the member list because we treat + // such messages as if we have not seen them. + assert_ne!(timestamp, timestamp3); + let contacts = get_chat_contacts(&bob, bob_chat_id).await?; + assert_eq!(contacts.len(), 3); + + // Bob fully reives the partial message. + let msg_id = receive_imf_inner( + &bob, + "first@example.org", + b"From: Alice \n\ +To: Bob \n\ +Chat-Version: 1.0\n\ +Subject: subject\n\ +Message-ID: \n\ +Date: Sun, 14 Nov 2021 00:10:00 +0000\ +Content-Type: text/plain +Chat-Group-Member-Added: charlie@example.com", + false, + None, + false, + ) + .await? + .context("no received message")?; + + let msg = Message::load_from_db(&bob, msg_id.msg_ids[0]).await?; + let timestamp4 = bob_chat_id + .get_param(&bob) + .await? + .get_i64(Param::MemberListTimestamp) + .unwrap(); + + // After full download, the old message should not change group state. + assert_eq!(msg.download_state, DownloadState::Done); + assert_eq!(timestamp3, timestamp4); + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?, contacts); + + Ok(()) +}