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

@@ -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()

src/headerdef.rs

@@ -73,6 +73,7 @@ pub enum HeaderDef {
 
     /// [Autocrypt](https://autocrypt.org/) header.
     Autocrypt,
+    AutocryptGossip,
     AutocryptSetupMessage,
     SecureJoin,
 

src/imap.rs

@@ -1022,8 +1022,9 @@ impl Session {
                 "SELECT id, uid, target FROM imap
                 WHERE folder = ?
                     AND target != folder
+                    AND target_min_smtp_id <= (SELECT IFNULL((SELECT MIN(id) FROM smtp), ?))
                 ORDER BY target, uid",
-                (folder,),
+                (folder, i64::MAX),
                 |row| {
                     let rowid: i64 = row.get(0)?;
                     let uid: u32 = row.get(1)?;

src/receive_imf.rs

@@ -60,7 +60,8 @@ pub struct ReceivedMsg {
     /// IDs of inserted rows in messages table.
     pub msg_ids: Vec<MsgId>,
 
-    /// 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,

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,9 +471,9 @@ 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)
             }
+        }
         /*=======================================================
         ====             Bob - the joiner's side             ====
         ====   Step 7 in "Setup verified contact" protocol   ====
@@ -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

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::<Result<Vec<_>, _>>().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:

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?