diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index 22e6df1e2..fdc72f20f 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -420,11 +420,11 @@ char* dc_get_blobdir (const dc_context_t* context); * 0=watch all folders normally (default) * changes require restarting IO by calling dc_stop_io() and then dc_start_io(). * - `show_emails` = DC_SHOW_EMAILS_OFF (0)= - * show direct replies to chats only (default), + * show direct replies to chats only, * DC_SHOW_EMAILS_ACCEPTED_CONTACTS (1)= * also show all mails of confirmed contacts, * DC_SHOW_EMAILS_ALL (2)= - * also show mails of unconfirmed contacts. + * also show mails of unconfirmed contacts (default). * - `key_gen_type` = DC_KEY_GEN_DEFAULT (0)= * generate recommended key type (default), * DC_KEY_GEN_RSA2048 (1)= diff --git a/python/examples/group_tracking.py b/python/examples/group_tracking.py index 7ca003c0d..51f63cb74 100644 --- a/python/examples/group_tracking.py +++ b/python/examples/group_tracking.py @@ -32,25 +32,13 @@ class GroupTrackingPlugin: @account_hookimpl def ac_member_added(self, chat, contact, actor, message): - print( - "ac_member_added {} to chat {} from {}".format( - contact.addr, - chat.id, - actor or message.get_sender_contact().addr, - ), - ) + print(f"ac_member_added {contact.addr} to chat {chat.id} from {actor or message.get_sender_contact().addr}") for member in chat.get_contacts(): print(f"chat member: {member.addr}") @account_hookimpl def ac_member_removed(self, chat, contact, actor, message): - print( - "ac_member_removed {} from chat {} by {}".format( - contact.addr, - chat.id, - actor or message.get_sender_contact().addr, - ), - ) + print(f"ac_member_removed {contact.addr} from chat {chat.id} by {actor or message.get_sender_contact().addr}") def main(argv=None): diff --git a/python/tests/test_1_online.py b/python/tests/test_1_online.py index 10fbb77dd..798deda23 100644 --- a/python/tests/test_1_online.py +++ b/python/tests/test_1_online.py @@ -353,6 +353,41 @@ def test_move_works(acfactory): assert ev.data2 > dc.const.DC_CHAT_ID_LAST_SPECIAL +def test_move_avoids_loop(acfactory): + """Test that the message is only moved once. + + This is to avoid busy loop if moved message reappears in the Inbox + or some scanned folder later. + For example, this happens on servers that alias `INBOX.DeltaChat` to `DeltaChat` folder, + so the message moved to `DeltaChat` appears as a new message in the `INBOX.DeltaChat` folder. + We do not want to move this message from `INBOX.DeltaChat` to `DeltaChat` again. + """ + ac1 = acfactory.new_online_configuring_account() + ac2 = acfactory.new_online_configuring_account(mvbox_move=True) + acfactory.bring_accounts_online() + ac1_chat = acfactory.get_accepted_chat(ac1, ac2) + ac1_chat.send_text("Message 1") + + # Message is moved to the DeltaChat folder and downloaded. + ac2_msg1 = ac2._evtracker.wait_next_incoming_message() + assert ac2_msg1.text == "Message 1" + + # Move the message to the INBOX again. + ac2.direct_imap.select_folder("DeltaChat") + ac2.direct_imap.conn.move(["*"], "INBOX") + + ac1_chat.send_text("Message 2") + ac2_msg2 = ac2._evtracker.wait_next_incoming_message() + assert ac2_msg2.text == "Message 2" + + # Check that Message 1 is still in the INBOX folder + # and Message 2 is in the DeltaChat folder. + ac2.direct_imap.select_folder("INBOX") + assert len(ac2.direct_imap.get_all_messages()) == 1 + ac2.direct_imap.select_folder("DeltaChat") + assert len(ac2.direct_imap.get_all_messages()) == 1 + + def test_move_works_on_self_sent(acfactory): ac1 = acfactory.new_online_configuring_account(mvbox_move=True) ac2 = acfactory.new_online_configuring_account() diff --git a/src/imap.rs b/src/imap.rs index 0e63c291f..bb65c5e12 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -747,9 +747,51 @@ impl Imap { } }; - // Get the Message-ID or generate a fake one to identify the message in the database. - let message_id = prefetch_get_or_create_message_id(&headers); - let target = target_folder(context, folder, folder_meaning, &headers).await?; + let message_id = prefetch_get_message_id(&headers); + + // Determine the target folder where the message should be moved to. + // + // If we have seen the message on the IMAP server before, do not move it. + // This is required to avoid infinite MOVE loop on IMAP servers + // that alias `DeltaChat` folder to other names. + // For example, some Dovecot servers alias `DeltaChat` folder to `INBOX.DeltaChat`. + // In this case Delta Chat configured with `DeltaChat` as the destination folder + // would detect messages in the `INBOX.DeltaChat` folder + // and try to move them to the `DeltaChat` folder. + // 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 = if let Some(message_id) = &message_id { + if context + .sql + .exists( + "SELECT COUNT (*) FROM imap WHERE rfc724_mid=?", + (message_id,), + ) + .await? + { + info!( + context, + "Not moving the message {} that we have seen before.", &message_id + ); + folder.to_string() + } else { + target_folder(context, folder, folder_meaning, &headers).await? + } + } else { + // Do not move the messages without Message-ID. + // We cannot reliably determine if we have seen them before, + // so it is safer not to move them. + warn!( + context, + "Not moving the message that does not have a Message-ID." + ); + folder.to_string() + }; + + // Generate a fake Message-ID to identify the message in the database + // if the message has no real Message-ID. + let message_id = message_id.unwrap_or_else(create_message_id); context .sql @@ -2060,16 +2102,15 @@ fn get_fetch_headers(prefetch_msg: &Fetch) -> Result> } } -fn prefetch_get_message_id(headers: &[mailparse::MailHeader]) -> Option { +pub(crate) fn prefetch_get_message_id(headers: &[mailparse::MailHeader]) -> Option { headers .get_header_value(HeaderDef::XMicrosoftOriginalMessageId) .or_else(|| headers.get_header_value(HeaderDef::MessageId)) .and_then(|msgid| mimeparser::parse_message_id(&msgid).ok()) } -pub(crate) fn prefetch_get_or_create_message_id(headers: &[mailparse::MailHeader]) -> String { - prefetch_get_message_id(headers) - .unwrap_or_else(|| format!("{}{}", GENERATED_PREFIX, create_id())) +pub(crate) fn create_message_id() -> String { + format!("{}{}", GENERATED_PREFIX, create_id()) } /// Returns chat by prefetched headers. diff --git a/src/receive_imf.rs b/src/receive_imf.rs index da2a89985..010ba31df 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -74,7 +74,8 @@ pub async fn receive_imf( seen: bool, ) -> Result> { let mail = parse_mail(imf_raw).context("can't parse mail")?; - let rfc724_mid = imap::prefetch_get_or_create_message_id(&mail.headers); + let rfc724_mid = + imap::prefetch_get_message_id(&mail.headers).unwrap_or_else(imap::create_message_id); receive_imf_inner(context, &rfc724_mid, imf_raw, seen, None, false).await } @@ -1193,8 +1194,9 @@ SET rfc724_mid=excluded.rfc724_mid, chat_id=excluded.chat_id, mime_compressed=excluded.mime_compressed, mime_in_reply_to=excluded.mime_in_reply_to, mime_references=excluded.mime_references, mime_modified=excluded.mime_modified, error=excluded.error, ephemeral_timer=excluded.ephemeral_timer, ephemeral_timestamp=excluded.ephemeral_timestamp, download_state=excluded.download_state, hop_info=excluded.hop_info +RETURNING id "#)?; - stmt.execute(params![ + let row_id: MsgId = stmt.query_row(params![ replace_msg_id, rfc724_mid, if trash { DC_CHAT_ID_TRASH } else { chat_id }, @@ -1233,8 +1235,12 @@ SET rfc724_mid=excluded.rfc724_mid, chat_id=excluded.chat_id, DownloadState::Done }, mime_parser.hop_info - ])?; - let row_id = conn.last_insert_rowid(); + ], + |row| { + let msg_id: MsgId = row.get(0)?; + Ok(msg_id) + } + )?; Ok(row_id) }) .await?; @@ -1243,7 +1249,8 @@ SET rfc724_mid=excluded.rfc724_mid, chat_id=excluded.chat_id, // afterwards insert additional parts. replace_msg_id = None; - created_db_entries.push(MsgId::new(u32::try_from(row_id)?)); + debug_assert!(!row_id.is_special()); + created_db_entries.push(row_id); } // check all parts whether they contain a new logging webxdc diff --git a/src/webxdc.rs b/src/webxdc.rs index 0b6dbc1ca..1f3215365 100644 --- a/src/webxdc.rs +++ b/src/webxdc.rs @@ -1177,7 +1177,7 @@ mod tests { assert_eq!(bob_instance.download_state, DownloadState::Available); // Bob downloads instance, updates should be assigned correctly - receive_imf_inner( + let received_msg = receive_imf_inner( &bob, &alice_instance.rfc724_mid, sent1.payload().as_bytes(), @@ -1185,7 +1185,9 @@ mod tests { None, false, ) - .await?; + .await? + .unwrap(); + assert_eq!(*received_msg.msg_ids.get(0).unwrap(), bob_instance.id); let bob_instance = bob.get_last_msg().await; assert_eq!(bob_instance.viewtype, Viewtype::Webxdc); assert_eq!(bob_instance.download_state, DownloadState::Done);