diff --git a/python/src/deltachat/account.py b/python/src/deltachat/account.py index 8b4e804ae..e2152c2c3 100644 --- a/python/src/deltachat/account.py +++ b/python/src/deltachat/account.py @@ -477,6 +477,16 @@ class Account: msg_ids = [msg.id for msg in messages] lib.dc_forward_msgs(self._dc_context, msg_ids, len(msg_ids), chat.id) + def resend_messages(self, messages: List[Message]) -> None: + """Resend list of messages. + + :param messages: list of :class:`deltachat.message.Message` object. + :returns: None + """ + msg_ids = [msg.id for msg in messages] + if lib.dc_resend_msgs(self._dc_context, msg_ids, len(msg_ids)) != 1: + raise ValueError(f"could not resend messages {msg_ids}") + def delete_messages(self, messages: List[Message]) -> None: """delete messages (local and remote). diff --git a/python/tests/test_1_online.py b/python/tests/test_1_online.py index e9d7ccb32..ac0fde685 100644 --- a/python/tests/test_1_online.py +++ b/python/tests/test_1_online.py @@ -543,6 +543,27 @@ def test_forward_own_message(acfactory, lp): assert msg_in.is_forwarded() +def test_resend_message(acfactory, lp): + ac1, ac2 = acfactory.get_online_accounts(2) + chat1 = ac1.create_chat(ac2) + + lp.sec("ac1: send message to ac2") + chat1.send_text("message") + + lp.sec("ac2: receive message") + msg_in = ac2._evtracker.wait_next_incoming_message() + assert msg_in.text == "message" + chat2 = msg_in.chat + chat2_msg_cnt = len(chat2.get_messages()) + + lp.sec("ac1: resend message") + ac1.resend_messages([msg_in]) + + lp.sec("ac2: check that message is deleted") + ac2._evtracker.get_matching("DC_EVENT_IMAP_MESSAGE_DELETED") + assert len(chat2.get_messages()) == chat2_msg_cnt + + def test_long_group_name(acfactory, lp): """See bug https://github.com/deltachat/deltachat-core-rust/issues/3650 "Space added before long group names after MIME serialization/deserialization". diff --git a/src/download.rs b/src/download.rs index 1f3e1d56b..b6905fa4f 100644 --- a/src/download.rs +++ b/src/download.rs @@ -136,23 +136,30 @@ pub(crate) async fn download_msg(context: &Context, msg_id: MsgId, imap: &mut Im let row = context .sql .query_row_optional( - "SELECT uid, folder FROM imap WHERE rfc724_mid=? AND target!=''", + "SELECT uid, folder, uidvalidity FROM imap WHERE rfc724_mid=? AND target!=''", (&msg.rfc724_mid,), |row| { let server_uid: u32 = row.get(0)?; let server_folder: String = row.get(1)?; - Ok((server_uid, server_folder)) + let uidvalidity: u32 = row.get(2)?; + Ok((server_uid, server_folder, uidvalidity)) }, ) .await?; - let Some((server_uid, server_folder)) = row else { + let Some((server_uid, server_folder, uidvalidity)) = row else { // No IMAP record found, we don't know the UID and folder. return Err(anyhow!("Call download_full() again to try over.")); }; match imap - .fetch_single_msg(context, &server_folder, server_uid, msg.rfc724_mid.clone()) + .fetch_single_msg( + context, + &server_folder, + uidvalidity, + server_uid, + msg.rfc724_mid.clone(), + ) .await { ImapActionResult::RetryLater | ImapActionResult::Failed => { @@ -171,6 +178,7 @@ impl Imap { &mut self, context: &Context, folder: &str, + uidvalidity: u32, uid: u32, rfc724_mid: String, ) -> ImapActionResult { @@ -187,7 +195,15 @@ impl Imap { let mut uid_message_ids: BTreeMap = BTreeMap::new(); uid_message_ids.insert(uid, rfc724_mid); let (last_uid, _received) = match self - .fetch_many_msgs(context, folder, vec![uid], &uid_message_ids, false, false) + .fetch_many_msgs( + context, + folder, + uidvalidity, + vec![uid], + &uid_message_ids, + false, + false, + ) .await { Ok(res) => res, @@ -247,7 +263,7 @@ mod tests { use crate::chat::{get_chat_msgs, send_msg}; use crate::ephemeral::Timer; use crate::message::Viewtype; - use crate::receive_imf::receive_imf_inner; + use crate::receive_imf::receive_imf_from_inbox; use crate::test_utils::TestContext; #[test] @@ -328,7 +344,7 @@ mod tests { Date: Sun, 22 Mar 2020 22:37:57 +0000\ Content-Type: text/plain"; - receive_imf_inner( + receive_imf_from_inbox( &t, "Mr.12345678901@example.com", header.as_bytes(), @@ -344,7 +360,7 @@ mod tests { .get_text() .contains(&stock_str::partial_download_msg_body(&t, 100000).await)); - receive_imf_inner( + receive_imf_from_inbox( &t, "Mr.12345678901@example.com", format!("{header}\n\n100k text...").as_bytes(), @@ -373,7 +389,7 @@ mod tests { .await?; // download message from bob partially, this must not change the ephemeral timer - receive_imf_inner( + receive_imf_from_inbox( &t, "first@example.org", b"From: Bob \n\ @@ -416,7 +432,7 @@ mod tests { let sent2_rfc724_mid = sent2.load_from_db().await.rfc724_mid; // not downloading the status update results in an placeholder - receive_imf_inner( + receive_imf_from_inbox( &bob, &sent2_rfc724_mid, sent2.payload().as_bytes(), @@ -432,7 +448,7 @@ mod tests { // downloading the status update afterwards expands to nothing and moves the placeholder to trash-chat // (usually status updates are too small for not being downloaded directly) - receive_imf_inner( + receive_imf_from_inbox( &bob, &sent2_rfc724_mid, sent2.payload().as_bytes(), @@ -483,7 +499,7 @@ mod tests { "; // not downloading the mdn results in an placeholder - receive_imf_inner( + receive_imf_from_inbox( &bob, "bar@example.org", raw, @@ -499,7 +515,7 @@ mod tests { // downloading the mdn afterwards expands to nothing and deletes the placeholder directly // (usually mdn are too small for not being downloaded directly) - receive_imf_inner(&bob, "bar@example.org", raw, false, None, false).await?; + receive_imf_from_inbox(&bob, "bar@example.org", raw, false, None, false).await?; assert_eq!(get_chat_msgs(&bob, chat_id).await?.len(), 0); assert!(Message::load_from_db(&bob, msg.id) .await? diff --git a/src/imap.rs b/src/imap.rs index 81a31d3d4..b637c2270 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -73,6 +73,7 @@ pub enum ImapActionResult { /// not necessarily sent by Delta Chat. const PREFETCH_FLAGS: &str = "(UID INTERNALDATE RFC822.SIZE BODY.PEEK[HEADER.FIELDS (\ MESSAGE-ID \ + DATE \ X-MICROSOFT-ORIGINAL-MESSAGE-ID \ FROM \ IN-REPLY-TO REFERENCES \ @@ -769,6 +770,7 @@ impl Imap { let mut uids_fetch = Vec::<(_, bool /* partially? */)>::with_capacity(msgs.len() + 1); let mut uid_message_ids = BTreeMap::new(); let mut largest_uid_skipped = None; + let delete_target = context.get_delete_msgs_target().await?; // Store the info about IMAP messages in the database. for (uid, ref fetch_response) in msgs { @@ -794,8 +796,24 @@ impl Imap { // Such move to the same folder results in the messages // getting a new UID, so the messages will be detected as new // in the `INBOX.DeltaChat` folder again. + let _target; let target = if let Some(message_id) = &message_id { - if context + let is_dup = if let Some((_, ts_sent_old)) = + message::rfc724_mid_exists(context, message_id).await? + { + let is_chat_msg = headers.get_header_value(HeaderDef::ChatVersion).is_some(); + let ts_sent = headers + .get_header_value(HeaderDef::Date) + .and_then(|v| mailparse::dateparse(&v).ok()) + .unwrap_or_default(); + is_dup_msg(is_chat_msg, ts_sent, ts_sent_old) + } else { + false + }; + if is_dup { + info!(context, "Deleting duplicate message {message_id}."); + &delete_target + } else if context .sql .exists( "SELECT COUNT (*) FROM imap WHERE rfc724_mid=?", @@ -807,9 +825,10 @@ impl Imap { context, "Not moving the message {} that we have seen before.", &message_id ); - folder.to_string() + folder } else { - target_folder(context, folder, folder_meaning, &headers).await? + _target = target_folder(context, folder, folder_meaning, &headers).await?; + &_target } } else { // Do not move the messages without Message-ID. @@ -819,7 +838,7 @@ impl Imap { context, "Not moving the message that does not have a Message-ID." ); - folder.to_string() + folder }; // Generate a fake Message-ID to identify the message in the database @@ -834,7 +853,7 @@ impl Imap { ON CONFLICT(folder, uid, uidvalidity) DO UPDATE SET rfc724_mid=excluded.rfc724_mid, target=excluded.target", - (&message_id, &folder, uid, uid_validity, &target), + (&message_id, &folder, uid, uid_validity, target), ) .await?; @@ -887,6 +906,7 @@ impl Imap { .fetch_many_msgs( context, folder, + uid_validity, uids_fetch_in_batch.split_off(0), &uid_message_ids, fetch_partially, @@ -1431,10 +1451,12 @@ impl Imap { /// Returns the last UID fetched successfully and the info about each downloaded message. /// If the message is incorrect or there is a failure to write a message to the database, /// it is skipped and the error is logged. + #[allow(clippy::too_many_arguments)] pub(crate) async fn fetch_many_msgs( &mut self, context: &Context, folder: &str, + uidvalidity: u32, request_uids: Vec, uid_message_ids: &BTreeMap, fetch_partially: bool, @@ -1568,6 +1590,9 @@ impl Imap { ); match receive_imf_inner( context, + folder, + uidvalidity, + request_uid, rfc724_mid, body, is_seen, @@ -2247,6 +2272,15 @@ pub(crate) async fn prefetch_should_download( Ok(should_download) } +/// Returns whether a message is a duplicate (resent message). +pub(crate) fn is_dup_msg(is_chat_msg: bool, ts_sent: i64, ts_sent_old: i64) -> bool { + // If the existing message has timestamp_sent == 0, that means we don't know its actual sent + // timestamp, so don't delete the new message. E.g. outgoing messages have zero timestamp_sent + // because they are stored to the db before sending. Also consider as duplicates only messages + // with greater timestamp to avoid deleting both messages in a multi-device setting. + is_chat_msg && ts_sent_old != 0 && ts_sent > ts_sent_old +} + /// Marks messages in `msgs` table as seen, searching for them by UID. /// /// Returns updated chat ID if any message was marked as seen. diff --git a/src/message.rs b/src/message.rs index 05f602be6..0ea6edcc4 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1130,7 +1130,7 @@ impl Message { /// `References` header is not taken into account. pub async fn parent(&self, context: &Context) -> Result> { if let Some(in_reply_to) = &self.in_reply_to { - if let Some(msg_id) = rfc724_mid_exists(context, in_reply_to).await? { + if let Some((msg_id, _ts_sent)) = rfc724_mid_exists(context, in_reply_to).await? { let msg = Message::load_from_db(context, msg_id).await?; return if msg.chat_id.is_trash() { // If message is already moved to trash chat, pretend it does not exist. @@ -1816,18 +1816,23 @@ pub async fn estimate_deletion_cnt( Ok(cnt) } +/// See [`rfc724_mid_exists_and()`]. pub(crate) async fn rfc724_mid_exists( context: &Context, rfc724_mid: &str, -) -> Result> { +) -> Result> { rfc724_mid_exists_and(context, rfc724_mid, "1").await } +/// Returns [MsgId] and "sent" timestamp of the message with given `rfc724_mid` (Message-ID header) +/// if it exists in the db. +/// +/// @param cond SQL subexpression for filtering messages. pub(crate) async fn rfc724_mid_exists_and( context: &Context, rfc724_mid: &str, cond: &str, -) -> Result> { +) -> Result> { let rfc724_mid = rfc724_mid.trim_start_matches('<').trim_end_matches('>'); if rfc724_mid.is_empty() { warn!(context, "Empty rfc724_mid passed to rfc724_mid_exists"); @@ -1837,12 +1842,13 @@ pub(crate) async fn rfc724_mid_exists_and( let res = context .sql .query_row_optional( - &("SELECT id FROM msgs WHERE rfc724_mid=? AND ".to_string() + cond), + &("SELECT id, timestamp_sent FROM msgs WHERE rfc724_mid=? AND ".to_string() + cond), (rfc724_mid,), |row| { let msg_id: MsgId = row.get(0)?; + let timestamp_sent: i64 = row.get(1)?; - Ok(msg_id) + Ok((msg_id, timestamp_sent)) }, ) .await?; @@ -1858,7 +1864,7 @@ pub(crate) async fn get_latest_by_rfc724_mids( mids: &[String], ) -> Result> { for id in mids.iter().rev() { - if let Some(msg_id) = rfc724_mid_exists(context, id).await? { + if let Some((msg_id, _)) = rfc724_mid_exists(context, id).await? { let msg = Message::load_from_db(context, msg_id).await?; if msg.chat_id != DC_CHAT_ID_TRASH { return Ok(Some(msg)); diff --git a/src/reaction.rs b/src/reaction.rs index 2fec8a0e8..892803322 100644 --- a/src/reaction.rs +++ b/src/reaction.rs @@ -252,7 +252,7 @@ pub(crate) async fn set_msg_reaction( contact_id: ContactId, reaction: Reaction, ) -> Result<()> { - if let Some(msg_id) = rfc724_mid_exists(context, in_reply_to).await? { + if let Some((msg_id, _)) = rfc724_mid_exists(context, in_reply_to).await? { set_msg_id_reaction(context, msg_id, chat_id, contact_id, reaction).await } else { info!( @@ -316,7 +316,7 @@ mod tests { use crate::contact::{Contact, ContactAddress, Origin}; use crate::download::DownloadState; use crate::message::MessageState; - use crate::receive_imf::{receive_imf, receive_imf_inner}; + use crate::receive_imf::{receive_imf, receive_imf_from_inbox}; use crate::test_utils::TestContext; use crate::test_utils::TestContextManager; @@ -568,7 +568,7 @@ Here's my footer -- bob@example.net" let msg_full = format!("{msg_header}\n\n100k text..."); // Alice downloads message from Bob partially. - let alice_received_message = receive_imf_inner( + let alice_received_message = receive_imf_from_inbox( &alice, "first@example.org", msg_header.as_bytes(), @@ -599,7 +599,7 @@ Here's my footer -- bob@example.net" assert_eq!(msg.download_state(), DownloadState::Available); // Alice downloads full message. - receive_imf_inner( + receive_imf_from_inbox( &alice, "first@example.org", msg_full.as_bytes(), diff --git a/src/receive_imf.rs b/src/receive_imf.rs index a4e9d516f..c0b3c1289 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -87,7 +87,7 @@ pub async fn receive_imf( .split("\r\n\r\n") .next() .context("No empty line in the message")?; - return receive_imf_inner( + return receive_imf_from_inbox( context, &rfc724_mid, head.as_bytes(), @@ -98,7 +98,32 @@ pub async fn receive_imf( .await; } } - receive_imf_inner(context, &rfc724_mid, imf_raw, seen, None, false).await + receive_imf_from_inbox(context, &rfc724_mid, imf_raw, seen, None, false).await +} + +/// Emulates reception of a message from "INBOX". +/// +/// Only used for tests and REPL tool, not actual message reception pipeline. +pub(crate) async fn receive_imf_from_inbox( + context: &Context, + rfc724_mid: &str, + imf_raw: &[u8], + seen: bool, + is_partial_download: Option, + fetching_existing_messages: bool, +) -> Result> { + receive_imf_inner( + context, + "INBOX", + 0, + 0, + rfc724_mid, + imf_raw, + seen, + is_partial_download, + fetching_existing_messages, + ) + .await } /// Inserts a tombstone into `msgs` table @@ -130,8 +155,12 @@ async fn insert_tombstone(context: &Context, rfc724_mid: &str) -> Result /// If `is_partial_download` is set, it contains the full message size in bytes. /// Do not confuse that with `replace_msg_id` that will be set when the full message is loaded /// later. +#[allow(clippy::too_many_arguments)] pub(crate) async fn receive_imf_inner( context: &Context, + folder: &str, + uidvalidity: u32, + uid: u32, rfc724_mid: &str, imf_raw: &[u8], seen: bool, @@ -193,48 +222,17 @@ pub(crate) async fn receive_imf_inner( ); let incoming = !context.is_self_addr(&mime_parser.from.addr).await?; - // For the case if we missed a successful SMTP response. Be optimistic that the message is - // delivered also. - let delivered = !incoming && { - let self_addr = context.get_primary_self_addr().await?; - context - .sql - .execute( - "DELETE FROM smtp \ - WHERE rfc724_mid=?1 AND (recipients LIKE ?2 OR recipients LIKE ('% ' || ?2))", - (rfc724_mid_orig, &self_addr), - ) - .await?; - !context - .sql - .exists( - "SELECT COUNT(*) FROM smtp WHERE rfc724_mid=?", - (rfc724_mid_orig,), - ) - .await? - }; - - async fn on_msg_in_db( - context: &Context, - msg_id: MsgId, - delivered: bool, - ) -> Result> { - if delivered { - msg_id.set_delivered(context).await?; - } - Ok(None) - } - // check, if the mail is already in our database. // make sure, this check is done eg. before securejoin-processing. let (replace_msg_id, replace_chat_id); - if let Some(old_msg_id) = message::rfc724_mid_exists(context, rfc724_mid).await? { + if let Some((old_msg_id, _)) = message::rfc724_mid_exists(context, rfc724_mid).await? { if is_partial_download.is_some() { + // Should never happen, see imap::prefetch_should_download(), but still. info!( context, "Got a partial download and message is already in DB." ); - return on_msg_in_db(context, old_msg_id, delivered).await; + return Ok(None); } let msg = Message::load_from_db(context, old_msg_id).await?; replace_msg_id = Some(old_msg_id); @@ -249,8 +247,27 @@ pub(crate) async fn receive_imf_inner( None }; } else { - replace_msg_id = if rfc724_mid_orig != rfc724_mid { + replace_msg_id = if rfc724_mid_orig == rfc724_mid { + None + } else if let Some((old_msg_id, old_ts_sent)) = message::rfc724_mid_exists(context, rfc724_mid_orig).await? + { + if imap::is_dup_msg( + mime_parser.has_chat_version(), + mime_parser.timestamp_sent, + old_ts_sent, + ) { + info!(context, "Deleting duplicate message {rfc724_mid_orig}."); + let target = context.get_delete_msgs_target().await?; + context + .sql + .execute( + "UPDATE imap SET target=? WHERE folder=? AND uidvalidity=? AND uid=?", + (target, folder, uidvalidity, uid), + ) + .await?; + } + Some(old_msg_id) } else { None }; @@ -261,7 +278,31 @@ pub(crate) async fn receive_imf_inner( // Need to update chat id in the db. } else if let Some(msg_id) = replace_msg_id { info!(context, "Message is already downloaded."); - return on_msg_in_db(context, msg_id, delivered).await; + if incoming { + return Ok(None); + } + // For the case if we missed a successful SMTP response. Be optimistic that the message is + // delivered also. + let self_addr = context.get_primary_self_addr().await?; + context + .sql + .execute( + "DELETE FROM smtp \ + WHERE rfc724_mid=?1 AND (recipients LIKE ?2 OR recipients LIKE ('% ' || ?2))", + (rfc724_mid_orig, &self_addr), + ) + .await?; + if !context + .sql + .exists( + "SELECT COUNT(*) FROM smtp WHERE rfc724_mid=?", + (rfc724_mid_orig,), + ) + .await? + { + msg_id.set_delivered(context).await?; + } + return Ok(None); }; let prevent_rename = @@ -2589,7 +2630,7 @@ async fn get_previous_message( ) -> Result> { if let Some(field) = mime_parser.get_header(HeaderDef::References) { if let Some(rfc724mid) = parse_message_ids(field).last() { - if let Some(msg_id) = rfc724_mid_exists(context, rfc724mid).await? { + if let Some((msg_id, _)) = rfc724_mid_exists(context, rfc724mid).await? { return Ok(Some(Message::load_from_db(context, msg_id).await?)); } } diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 85bc39555..d871531a1 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -1882,7 +1882,7 @@ async fn create_test_alias(chat_request: bool, group_request: bool) -> (TestCont .await .unwrap(); - let msg_id = rfc724_mid_exists(&claire, "non-dc-1@example.org") + let (msg_id, _) = rfc724_mid_exists(&claire, "non-dc-1@example.org") .await .unwrap() .unwrap(); @@ -4075,7 +4075,7 @@ async fn test_partial_group_consistency() -> Result<()> { .unwrap(); // Bob receives partial message. - let msg_id = receive_imf_inner( + let msg_id = receive_imf_from_inbox( &bob, "first@example.org", b"From: Alice \n\ @@ -4128,7 +4128,7 @@ Chat-Group-Member-Added: charlie@example.com", assert_eq!(contacts.len(), 3); // Bob fully reives the partial message. - let msg_id = receive_imf_inner( + let msg_id = receive_imf_from_inbox( &bob, "first@example.org", b"From: Alice \n\ diff --git a/src/webxdc.rs b/src/webxdc.rs index 12a51ed53..2e50756a2 100644 --- a/src/webxdc.rs +++ b/src/webxdc.rs @@ -883,7 +883,7 @@ mod tests { use crate::chatlist::Chatlist; use crate::config::Config; use crate::contact::Contact; - use crate::receive_imf::{receive_imf, receive_imf_inner}; + use crate::receive_imf::{receive_imf, receive_imf_from_inbox}; use crate::test_utils::TestContext; use crate::{message, sql}; @@ -1216,7 +1216,7 @@ mod tests { let sent2 = alice.pop_sent_msg().await; // Bob does not download instance but already receives update - receive_imf_inner( + receive_imf_from_inbox( &bob, &alice_instance.rfc724_mid, sent1.payload().as_bytes(), @@ -1231,7 +1231,7 @@ mod tests { assert_eq!(bob_instance.download_state, DownloadState::Available); // Bob downloads instance, updates should be assigned correctly - let received_msg = receive_imf_inner( + let received_msg = receive_imf_from_inbox( &bob, &alice_instance.rfc724_mid, sent1.payload().as_bytes(),