From d8eda42ff4d1eec59a140ae393ef457b25791a00 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Thu, 19 Dec 2024 17:13:35 -0300 Subject: [PATCH] feat: Delete vg-request-with-auth from IMAP after processing (#6208) In multi-device case `vg-request-with-auth` left on IMAP may result in situation when Bob joins the group, then leaves it, then second Alice device comes online and processes `vg-request-with-auth` again and adds Bob back. So we should IMAP-delete `vg-request-with-auth`. Another device will know the Bob's key from Autocrypt-Gossip. But also we should make sure that `vg-member-added` is sent before that. For this, add a new `imap.target_min_smtp_id` column and only move or delete emails when `smtp.id` reaches the `imap.target_min_smtp_id` value. --- deltachat-rpc-client/tests/test_securejoin.py | 6 ++++-- src/headerdef.rs | 1 + src/imap.rs | 9 ++++---- src/receive_imf.rs | 15 +++++++++++-- src/securejoin.rs | 7 ++++++- src/smtp.rs | 21 +++++++++++++++++++ src/sql/migrations.rs | 12 +++++++++++ 7 files changed, 62 insertions(+), 9 deletions(-) diff --git a/deltachat-rpc-client/tests/test_securejoin.py b/deltachat-rpc-client/tests/test_securejoin.py index 477eb16d9..14db4da59 100644 --- a/deltachat-rpc-client/tests/test_securejoin.py +++ b/deltachat-rpc-client/tests/test_securejoin.py @@ -73,6 +73,9 @@ def test_qr_securejoin(acfactory, protect, tmp_path): qr_code = alice_chat.get_qr_code() bob.secure_join(qr_code) + # Alice deletes "vg-member-added", SecureJoin succeeds before that. + alice.wait_for_securejoin_inviter_success() + # Check that at least some of the handshake messages are deleted. for ac in [alice, bob]: while True: @@ -80,13 +83,12 @@ def test_qr_securejoin(acfactory, protect, tmp_path): if event["kind"] == "ImapMessageDeleted": break - alice.wait_for_securejoin_inviter_success() - # Test that Alice verified Bob's profile. alice_contact_bob = alice.get_contact_by_addr(bob.get_config("addr")) alice_contact_bob_snapshot = alice_contact_bob.get_snapshot() assert alice_contact_bob_snapshot.is_verified + # Bob deletes "vg-request-with-auth", SecureJoin succeeds later. bob.wait_for_securejoin_joiner_success() snapshot = bob.get_message_by_id(bob.wait_for_incoming_msg_event().msg_id).get_snapshot() diff --git a/src/headerdef.rs b/src/headerdef.rs index b4865dd71..61d7746d8 100644 --- a/src/headerdef.rs +++ b/src/headerdef.rs @@ -73,6 +73,7 @@ pub enum HeaderDef { /// [Autocrypt](https://autocrypt.org/) header. Autocrypt, + AutocryptGossip, AutocryptSetupMessage, SecureJoin, diff --git a/src/imap.rs b/src/imap.rs index 55df079e3..0e4d24dfa 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -1020,10 +1020,11 @@ impl Session { .sql .query_map( "SELECT id, uid, target FROM imap - WHERE folder = ? - AND target != folder - ORDER BY target, uid", - (folder,), + WHERE folder = ? + AND target != folder + AND target_min_smtp_id <= (SELECT IFNULL((SELECT MIN(id) FROM smtp), ?)) + ORDER BY target, uid", + (folder, i64::MAX), |row| { let rowid: i64 = row.get(0)?; let uid: u32 = row.get(1)?; diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 27b27c7b8..5154b8397 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -60,7 +60,8 @@ pub struct ReceivedMsg { /// IDs of inserted rows in messages table. pub msg_ids: Vec, - /// Whether IMAP messages should be immediately deleted. + /// Whether IMAP messages should be deleted. Deletion is done after necessary auto-generated + /// messages are sent which is important for SecureJoin. pub needs_delete_job: bool, /// Whether the From address was repeated in the signed part @@ -587,10 +588,20 @@ pub(crate) async fn receive_imf_inner( Some(_) => "target=?1,", None => "", }; + let target_min_smtp_id_subst = match received_msg.needs_delete_job { + true => "target_min_smtp_id=(SELECT IFNULL((SELECT MAX(id)+1 FROM smtp),0)),", + false => "", + }; context .sql .execute( - &format!("UPDATE imap SET {target_subst} rfc724_mid=?2 WHERE rfc724_mid=?3"), + &format!( + "UPDATE imap SET + {target_subst} + {target_min_smtp_id_subst} + rfc724_mid=?2 + WHERE rfc724_mid=?3" + ), ( target.as_deref().unwrap_or_default(), rfc724_mid_orig, diff --git a/src/securejoin.rs b/src/securejoin.rs index 5779178f2..fd7208028 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -454,6 +454,9 @@ pub(crate) async fn handle_securejoin_handshake( .await?; inviter_progress(context, contact_id, 800); inviter_progress(context, contact_id, 1000); + // IMAP-delete the message to avoid handling it by another device and adding the + // member twice. Another device will know the member's key from Autocrypt-Gossip. + Ok(HandshakeMessage::Done) } else { // Setup verified contact. secure_connection_established( @@ -468,8 +471,8 @@ pub(crate) async fn handle_securejoin_handshake( .context("failed sending vc-contact-confirm message")?; inviter_progress(context, contact_id, 1000); + Ok(HandshakeMessage::Ignore) // "Done" would delete the message and break multi-device (the key from Autocrypt-header is needed) } - Ok(HandshakeMessage::Ignore) // "Done" would delete the message and break multi-device (the key from Autocrypt-header is needed) } /*======================================================= ==== Bob - the joiner's side ==== @@ -1353,6 +1356,8 @@ mod tests { // explicit user action, the Auto-Submitted header shouldn't be present. Otherwise it would // be strange to have it in "member-added" messages of verified groups only. assert!(msg.get_header(HeaderDef::AutoSubmitted).is_none()); + // This is a two-member group, but Alice must Autocrypt-gossip to her other devices. + assert!(msg.get_header(HeaderDef::AutocryptGossip).is_some()); { // Now Alice's chat with Bob should still be hidden, the verified message should diff --git a/src/smtp.rs b/src/smtp.rs index d50466d3c..980faa4fc 100644 --- a/src/smtp.rs +++ b/src/smtp.rs @@ -514,6 +514,27 @@ pub(crate) async fn send_smtp_messages(context: &Context, connection: &mut Smtp) .await .context("Failed to send message")?; } + let folders = context + .sql + .query_map( + "SELECT DISTINCT(folder) FROM imap + WHERE target_min_smtp_id <= (SELECT IFNULL((SELECT MIN(id) FROM smtp), ?))", + (i64::MAX,), + |row| { + let folder: String = row.get(0)?; + Ok(Some(folder)) + }, + |rows| rows.collect::, _>>().map_err(Into::into), + ) + .await?; + if folders.contains(&context.get_config(Config::ConfiguredInboxFolder).await?) { + context.scheduler.interrupt_inbox().await; + } + if folders.contains(&context.get_config(Config::ConfiguredMvboxFolder).await?) + || folders.contains(&context.get_config(Config::ConfiguredSentboxFolder).await?) + { + context.scheduler.interrupt_oboxes().await; + } // although by slow sending, ratelimit may have been expired meanwhile, // do not attempt to send MDNs if ratelimited happened before on status-updates/sync: diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index dcaafbbff..0aa7eb49f 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -1121,6 +1121,18 @@ CREATE INDEX msgs_status_updates_index2 ON msgs_status_updates (uid); .await?; } + inc_and_check(&mut migration_version, 127)?; + if dbversion < migration_version { + // Emails must be moved to `target` or deleted only when `smtp.id` reaches + // `target_min_smtp_id` to keep SecureJoin working for multi-device and in case of a local + // state loss. + sql.execute_migration( + "ALTER TABLE imap ADD COLUMN target_min_smtp_id INTEGER NOT NULL DEFAULT 0", + migration_version, + ) + .await?; + } + let new_version = sql .get_raw_config_int(VERSION_CFG) .await?