From 4e6f87e892c60c5052c9675bcac3a8fe03307934 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 | 14 +++++++++---- src/receive_imf.rs | 20 +++++++++++++++++-- src/securejoin.rs | 7 ++++++- src/sql/migrations.rs | 12 +++++++++++ 6 files changed, 51 insertions(+), 9 deletions(-) diff --git a/deltachat-rpc-client/tests/test_securejoin.py b/deltachat-rpc-client/tests/test_securejoin.py index 477eb16d93..14db4da597 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 b4865dd716..61d7746d86 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 50a159b387..67c5d39a44 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -1017,14 +1017,20 @@ impl Session { /// /// This is the only place where messages are moved or deleted on the IMAP server. async fn move_delete_messages(&mut self, context: &Context, folder: &str) -> Result<()> { + let target_min_smtp_id: i64 = context + .sql + .query_get_value("SELECT IFNULL((SELECT MIN(id) FROM smtp), 0)", ()) + .await? + .context("No value??")?; let rows = context .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 <= ? + ORDER BY target, uid", + (folder, target_min_smtp_id), |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 3a58c5473b..6a614445d7 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,12 +588,27 @@ pub(crate) async fn receive_imf_inner( Some(_) => "target=?1,", None => "", }; + let target_min_smtp_id: i64 = match received_msg.needs_delete_job { + true => context + .sql + .query_get_value("SELECT IFNULL((SELECT MAX(id) + 1 FROM smtp), 0)", ()) + .await? + .context("No value??")?, + false => 0, + }; 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=?2, + rfc724_mid=?3 + WHERE rfc724_mid=?4" + ), ( target.as_deref().unwrap_or_default(), + target_min_smtp_id, rfc724_mid_orig, rfc724_mid, ), diff --git a/src/securejoin.rs b/src/securejoin.rs index 5779178f2b..fd72080281 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/sql/migrations.rs b/src/sql/migrations.rs index dcaafbbff8..0aa7eb49fa 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?