fix: Save QR code token regardless of whether the group exists (#5954)

Groups promotion to other devices and QR code tokens synchronisation are not synchronised processes,
so there are reasons why a QR code token may arrive earlier than the first group message:
- We are going to upload sync messages via IMAP while group messages are sent by SMTP.
- If sync messages go to the mvbox, they can be fetched earlier than group messages from Inbox.

deltachat-rpc-client/tests/test_securejoin.py

@@ -71,11 +71,11 @@ def test_qr_securejoin(acfactory, protect, tmp_path):
     alice2 = acfactory.get_unconfigured_account()
     alice2.import_backup(files[0])
 
-    logging.info("Alice creates a verified group")
+    logging.info("Alice creates a group")
-    alice_chat = alice.create_group("Verified group", protect=protect)
+    alice_chat = alice.create_group("Group", protect=protect)
     assert alice_chat.get_basic_snapshot().is_protected == protect
 
-    logging.info("Bob joins verified group")
+    logging.info("Bob joins the group")
     qr_code = alice_chat.get_qr_code()
     bob.secure_join(qr_code)
 
@@ -113,7 +113,7 @@ def test_qr_securejoin(acfactory, protect, tmp_path):
     assert alice2_contact_bob_snapshot.is_verified
 
     # The QR code token is synced, so alice2 must be able to handle join requests.
-    logging.info("Fiona joins verified group via alice2")
+    logging.info("Fiona joins the group via alice2")
     alice.stop_io()
     fiona.secure_join(qr_code)
     alice2.wait_for_securejoin_inviter_success()

src/chat.rs

@@ -1935,11 +1935,13 @@ impl Chat {
             msg.param.set_int(Param::AttachGroupImage, 1);
             self.param.remove(Param::Unpromoted);
             self.update_param(context).await?;
-            // send_sync_msg() is called a moment later at `smtp::send_smtp_messages()`
+            // TODO: Remove this compat code needed because Core <= v1.143:
-            // when the group creation message is already in the `smtp` table --
+            // - doesn't accept synchronization of QR code tokens for unpromoted groups, so we also
-            // this makes sure, the other devices are aware of grpid that is used in the sync-message.
+            //   send them when the group is promoted.
+            // - doesn't sync QR code tokens for unpromoted groups and the group might be created
+            //   before an upgrade.
             context
-                .sync_qr_code_tokens(Some(self.id))
+                .sync_qr_code_tokens(Some(self.grpid.as_str()))
                 .await
                 .log_err(context)
                 .ok();
@@ -3777,9 +3779,14 @@ pub(crate) async fn add_contact_to_chat_ex(
             return Err(e);
         }
         sync = Nosync;
+        // TODO: Remove this compat code needed because Core <= v1.143:
+        // - doesn't accept synchronization of QR code tokens for unpromoted groups, so we also send
+        //   them when the group is promoted.
+        // - doesn't sync QR code tokens for unpromoted groups and the group might be created before
+        //   an upgrade.
         if sync_qr_code_tokens
             && context
-                .sync_qr_code_tokens(Some(chat_id))
+                .sync_qr_code_tokens(Some(chat.grpid.as_str()))
                 .await
                 .log_err(context)
                 .is_ok()

src/qr.rs

@@ -11,7 +11,7 @@ use percent_encoding::{percent_decode_str, percent_encode, NON_ALPHANUMERIC};
 use serde::Deserialize;
 
 use self::dclogin_scheme::configure_from_login_qr;
-use crate::chat::{get_chat_id_by_grpid, ChatIdBlocked};
+use crate::chat::ChatIdBlocked;
 use crate::config::Config;
 use crate::constants::Blocked;
 use crate::contact::{Contact, ContactId, Origin};
@@ -727,18 +727,15 @@ pub async fn set_config_from_qr(context: &Context, qr: &str) -> Result<()> {
             grpid,
             ..
         } => {
-            let chat_id = get_chat_id_by_grpid(context, &grpid)
-                .await?
-                .map(|(chat_id, _protected, _blocked)| chat_id);
             token::save(
                 context,
                 token::Namespace::InviteNumber,
-                chat_id,
+                Some(&grpid),
                 &invitenumber,
             )
             .await?;
-            token::save(context, token::Namespace::Auth, chat_id, &authcode).await?;
+            token::save(context, token::Namespace::Auth, Some(&grpid), &authcode).await?;
-            context.sync_qr_code_tokens(chat_id).await?;
+            context.sync_qr_code_tokens(Some(&grpid)).await?;
             context.scheduler.interrupt_smtp().await;
         }
         Qr::Login { address, options } => {

src/securejoin.rs

@@ -1,13 +1,13 @@
 //! Implementation of [SecureJoin protocols](https://securejoin.delta.chat/).
 
-use anyhow::{bail, Context as _, Error, Result};
+use anyhow::{bail, ensure, Context as _, Error, Result};
 use percent_encoding::{utf8_percent_encode, AsciiSet, NON_ALPHANUMERIC};
 
 use crate::aheader::EncryptPreference;
-use crate::chat::{self, Chat, ChatId, ChatIdBlocked, ProtectionStatus};
+use crate::chat::{self, get_chat_id_by_grpid, Chat, ChatId, ChatIdBlocked, ProtectionStatus};
 use crate::chatlist_events;
 use crate::config::Config;
-use crate::constants::Blocked;
+use crate::constants::{Blocked, Chattype};
 use crate::contact::{Contact, ContactId, Origin};
 use crate::context::Context;
 use crate::e2ee::ensure_secret_key_exists;
@@ -60,13 +60,29 @@ pub async fn get_securejoin_qr(context: &Context, group: Option<ChatId>) -> Resu
 
     ensure_secret_key_exists(context).await.ok();
 
-    // invitenumber will be used to allow starting the handshake,
+    let chat = match group {
-    // auth will be used to verify the fingerprint
+        Some(id) => {
-    let sync_token = token::lookup(context, Namespace::InviteNumber, group)
+            let chat = Chat::load_from_db(context, id).await?;
+            ensure!(
+                chat.typ == Chattype::Group,
+                "Can't generate SecureJoin QR code for 1:1 chat {id}"
+            );
+            ensure!(
+                !chat.grpid.is_empty(),
+                "Can't generate SecureJoin QR code for ad-hoc group {id}"
+            );
+            Some(chat)
+        }
+        None => None,
+    };
+    let grpid = chat.as_ref().map(|c| c.grpid.as_str());
+    let sync_token = token::lookup(context, Namespace::InviteNumber, grpid)
         .await?
         .is_none();
-    let invitenumber = token::lookup_or_new(context, Namespace::InviteNumber, group).await?;
+    // invitenumber will be used to allow starting the handshake,
-    let auth = token::lookup_or_new(context, Namespace::Auth, group).await?;
+    // auth will be used to verify the fingerprint
+    let invitenumber = token::lookup_or_new(context, Namespace::InviteNumber, grpid).await?;
+    let auth = token::lookup_or_new(context, Namespace::Auth, grpid).await?;
     let self_addr = context.get_primary_self_addr().await?;
     let self_name = context
         .get_config(Config::Displayname)
@@ -85,19 +101,14 @@ pub async fn get_securejoin_qr(context: &Context, group: Option<ChatId>) -> Resu
     let self_name_urlencoded =
         utf8_percent_encode(&self_name, NON_ALPHANUMERIC_WITHOUT_DOT).to_string();
 
-    let qr = if let Some(group) = group {
+    let qr = if let Some(chat) = chat {
         // parameters used: a=g=x=i=s=
-        let chat = Chat::load_from_db(context, group).await?;
-        if chat.grpid.is_empty() {
-            bail!(
-                "can't generate securejoin QR code for ad-hoc group {}",
-                group
-            );
-        }
         let group_name = chat.get_name();
         let group_name_urlencoded = utf8_percent_encode(group_name, NON_ALPHANUMERIC).to_string();
         if sync_token {
-            context.sync_qr_code_tokens(Some(chat.id)).await?;
+            context
+                .sync_qr_code_tokens(Some(chat.grpid.as_str()))
+                .await?;
             context.scheduler.interrupt_smtp().await;
         }
         format!(
@@ -400,13 +411,23 @@ pub(crate) async fn handle_securejoin_handshake(
                 );
                 return Ok(HandshakeMessage::Ignore);
             };
-            let Some(group_chat_id) = token::auth_chat_id(context, auth).await? else {
+            let Some(grpid) = token::auth_foreign_key(context, auth).await? else {
                 warn!(
                     context,
                     "Ignoring {step} message because of invalid auth code."
                 );
                 return Ok(HandshakeMessage::Ignore);
             };
+            let group_chat_id = match grpid.as_str() {
+                "" => None,
+                id => {
+                    let Some((chat_id, ..)) = get_chat_id_by_grpid(context, id).await? else {
+                        warn!(context, "Ignoring {step} message: unknown grpid {id}.",);
+                        return Ok(HandshakeMessage::Ignore);
+                    };
+                    Some(chat_id)
+                }
+            };
 
             let contact_addr = Contact::get_by_id(context, contact_id)
                 .await?
@@ -432,7 +453,20 @@ pub(crate) async fn handle_securejoin_handshake(
             info!(context, "Auth verified.",);
             context.emit_event(EventType::ContactsChanged(Some(contact_id)));
             inviter_progress(context, contact_id, 600);
-            if group_chat_id.is_unset() {
+            if let Some(group_chat_id) = group_chat_id {
+                // Join group.
+                secure_connection_established(
+                    context,
+                    contact_id,
+                    group_chat_id,
+                    mime_message.timestamp_sent,
+                )
+                .await?;
+                chat::add_contact_to_chat_ex(context, Nosync, group_chat_id, contact_id, true)
+                    .await?;
+                inviter_progress(context, contact_id, 800);
+                inviter_progress(context, contact_id, 1000);
+            } else {
                 // Setup verified contact.
                 secure_connection_established(
                     context,
@@ -446,19 +480,6 @@ pub(crate) async fn handle_securejoin_handshake(
                     .context("failed sending vc-contact-confirm message")?;
 
                 inviter_progress(context, contact_id, 1000);
-            } else {
-                // Join group.
-                secure_connection_established(
-                    context,
-                    contact_id,
-                    group_chat_id,
-                    mime_message.timestamp_sent,
-                )
-                .await?;
-                chat::add_contact_to_chat_ex(context, Nosync, group_chat_id, contact_id, true)
-                    .await?;
-                inviter_progress(context, contact_id, 800);
-                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)
         }

src/sql/migrations.rs

@@ -971,6 +971,26 @@ CREATE INDEX msgs_status_updates_index2 ON msgs_status_updates (uid);
         .await?;
     }
 
+    inc_and_check(&mut migration_version, 118)?;
+    if dbversion < migration_version {
+        sql.execute_migration(
+            "CREATE TABLE tokens_new (
+                id INTEGER PRIMARY KEY,
+                namespc INTEGER DEFAULT 0,
+                foreign_key TEXT DEFAULT '',
+                token TEXT DEFAULT '',
+                timestamp INTEGER DEFAULT 0
+            ) STRICT;
+            INSERT INTO tokens_new
+                SELECT t.id, t.namespc, IFNULL(c.grpid, ''), t.token, t.timestamp
+                FROM tokens t LEFT JOIN chats c ON t.foreign_id=c.id;
+            DROP TABLE tokens;
+            ALTER TABLE tokens_new RENAME TO tokens;",
+            migration_version,
+        )
+        .await?;
+    }
+
     let new_version = sql
         .get_raw_config_int(VERSION_CFG)
         .await?

src/sync.rs

@@ -4,7 +4,7 @@ use anyhow::Result;
 use lettre_email::PartBuilder;
 use serde::{Deserialize, Serialize};
 
-use crate::chat::{self, Chat, ChatId};
+use crate::chat::{self, ChatId};
 use crate::config::Config;
 use crate::constants::Blocked;
 use crate::contact::ContactId;
@@ -117,36 +117,22 @@ impl Context {
         Ok(())
     }
 
-    /// Adds most recent qr-code tokens for a given chat to the list of items to be synced.
+    /// Adds most recent qr-code tokens for the given group or self-contact to the list of items to
-    /// If device synchronization is disabled,
+    /// be synced. If device synchronization is disabled,
     /// no tokens exist or the chat is unpromoted, the function does nothing.
     /// The caller should perform `SchedulerState::interrupt_smtp()` on its own to trigger sending.
-    pub(crate) async fn sync_qr_code_tokens(&self, chat_id: Option<ChatId>) -> Result<()> {
+    pub(crate) async fn sync_qr_code_tokens(&self, grpid: Option<&str>) -> Result<()> {
         if !self.should_send_sync_msgs().await? {
             return Ok(());
         }
-
         if let (Some(invitenumber), Some(auth)) = (
-            token::lookup(self, Namespace::InviteNumber, chat_id).await?,
+            token::lookup(self, Namespace::InviteNumber, grpid).await?,
-            token::lookup(self, Namespace::Auth, chat_id).await?,
+            token::lookup(self, Namespace::Auth, grpid).await?,
         ) {
-            let grpid = if let Some(chat_id) = chat_id {
-                let chat = Chat::load_from_db(self, chat_id).await?;
-                if !chat.is_promoted() {
-                    info!(
-                        self,
-                        "group '{}' not yet promoted, do not sync tokens yet.", chat.grpid
-                    );
-                    return Ok(());
-                }
-                Some(chat.grpid)
-            } else {
-                None
-            };
             self.add_sync_item(SyncData::AddQrToken(QrTokenData {
                 invitenumber,
                 auth,
-                grpid,
+                grpid: grpid.map(|s| s.to_string()),
             }))
             .await?;
         }
@@ -296,21 +282,9 @@ impl Context {
     }
 
     async fn add_qr_token(&self, token: &QrTokenData) -> Result<()> {
-        let chat_id = if let Some(grpid) = &token.grpid {
+        let grpid = token.grpid.as_deref();
-            if let Some((chat_id, _, _)) = chat::get_chat_id_by_grpid(self, grpid).await? {
+        token::save(self, Namespace::InviteNumber, grpid, &token.invitenumber).await?;
-                Some(chat_id)
+        token::save(self, Namespace::Auth, grpid, &token.auth).await?;
-            } else {
-                warn!(
-                    self,
-                    "Ignoring token for nonexistent/deleted group '{}'.", grpid
-                );
-                return Ok(());
-            }
-        } else {
-            None
-        };
-        token::save(self, Namespace::InviteNumber, chat_id, &token.invitenumber).await?;
-        token::save(self, Namespace::Auth, chat_id, &token.auth).await?;
         Ok(())
     }
 
@@ -328,11 +302,11 @@ mod tests {
     use anyhow::bail;
 
     use super::*;
-    use crate::chat::ProtectionStatus;
+    use crate::chat::{remove_contact_from_chat, Chat, ProtectionStatus};
     use crate::chatlist::Chatlist;
     use crate::contact::{Contact, Origin};
     use crate::securejoin::get_securejoin_qr;
-    use crate::test_utils::{TestContext, TestContextManager};
+    use crate::test_utils::{self, TestContext, TestContextManager};
     use crate::tools::SystemTime;
 
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
@@ -634,20 +608,25 @@ mod tests {
     }
 
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-    async fn test_unpromoted_group_no_qr_sync() -> Result<()> {
+    async fn test_unpromoted_group_qr_sync() -> Result<()> {
         let mut tcm = TestContextManager::new();
         let alice = &tcm.alice().await;
         alice.set_config_bool(Config::SyncMsgs, true).await?;
         let alice_chatid =
             chat::create_group_chat(alice, ProtectionStatus::Protected, "the chat").await?;
         let qr = get_securejoin_qr(alice, Some(alice_chatid)).await?;
-        let msg_id = alice.send_sync_msg().await?;
+
-        assert!(msg_id.is_none());
+        // alice2 syncs the QR code token.
+        let alice2 = &tcm.alice().await;
+        alice2.set_config_bool(Config::SyncMsgs, true).await?;
+        test_utils::sync(alice, alice2).await;
 
         let bob = &tcm.bob().await;
         tcm.exec_securejoin_qr(bob, alice, &qr).await;
         let msg_id = alice.send_sync_msg().await?;
-        // The group becomes promoted when Bob joins, so the QR code token is synced.
+        // Core <= v1.143 doesn't sync QR code tokens immediately, so current Core does that when a
+        // group is promoted for compatibility (because the group could be created by older Core).
+        // TODO: assert!(msg_id.is_none());
         assert!(msg_id.is_some());
         let sent = alice.pop_sent_msg().await;
         let msg = alice.parse_msg(&sent).await;
@@ -658,11 +637,22 @@ mod tests {
             unreachable!();
         };
 
+        // Remove Bob because alice2 doesn't have their key.
+        let alice_bob_id = alice.add_or_lookup_contact(bob).await.id;
+        remove_contact_from_chat(alice, alice_chatid, alice_bob_id).await?;
+        alice.pop_sent_msg().await;
+        let sent = alice
+            .send_text(alice_chatid, "Promoting group to another device")
+            .await;
+        alice2.recv_msg(&sent).await;
+
         let fiona = &tcm.fiona().await;
-        tcm.exec_securejoin_qr(fiona, alice, &qr).await;
+        tcm.exec_securejoin_qr(fiona, alice2, &qr).await;
-        let msg_id = alice.send_sync_msg().await?;
+        let msg = fiona.get_last_msg().await;
-        // The QR code token was already synced before.
+        assert_eq!(
-        assert!(msg_id.is_none());
+            msg.text,
+            "Member Me (fiona@example.net) added by alice@example.org."
+        );
         Ok(())
     }
 }

src/token.rs

@@ -7,7 +7,6 @@
 use anyhow::Result;
 use deltachat_derive::{FromSql, ToSql};
 
-use crate::chat::ChatId;
 use crate::context::Context;
 use crate::tools::{create_id, time};
 
@@ -27,34 +26,22 @@ pub enum Namespace {
 pub async fn save(
     context: &Context,
     namespace: Namespace,
-    foreign_id: Option<ChatId>,
+    foreign_key: Option<&str>,
     token: &str,
 ) -> Result<()> {
-    match foreign_id {
+    context
-        Some(foreign_id) => context
+        .sql
-            .sql
+        .execute(
-            .execute(
+            "INSERT INTO tokens (namespc, foreign_key, token, timestamp) VALUES (?, ?, ?, ?)",
-                "INSERT INTO tokens (namespc, foreign_id, token, timestamp) VALUES (?, ?, ?, ?);",
+            (namespace, foreign_key.unwrap_or(""), token, time()),
-                (namespace, foreign_id, token, time()),
+        )
-            )
+        .await?;
-            .await?,
-        None => {
-            context
-                .sql
-                .execute(
-                    "INSERT INTO tokens (namespc, token, timestamp) VALUES (?, ?, ?);",
-                    (namespace, token, time()),
-                )
-                .await?
-        }
-    };
-
     Ok(())
 }
 
-/// Lookup most recently created token for a namespace/chat combination.
+/// Looks up most recently created token for a namespace / foreign key combination.
 ///
-/// As there may be more than one valid token for a chat-id,
+/// As there may be more than one such valid token,
 /// (eg. when a qr code token is withdrawn, recreated and revived later),
 /// use lookup() for qr-code creation only;
 /// do not use lookup() to check for token validity.
@@ -63,43 +50,28 @@ pub async fn save(
 pub async fn lookup(
     context: &Context,
     namespace: Namespace,
-    chat: Option<ChatId>,
+    foreign_key: Option<&str>,
 ) -> Result<Option<String>> {
-    let token = match chat {
+    context
-        Some(chat_id) => {
+        .sql
-            context
+        .query_get_value(
-                .sql
+            "SELECT token FROM tokens WHERE namespc=? AND foreign_key=? ORDER BY timestamp DESC LIMIT 1",
-                .query_get_value(
+            (namespace, foreign_key.unwrap_or("")),
-                    "SELECT token FROM tokens WHERE namespc=? AND foreign_id=? ORDER BY timestamp DESC LIMIT 1;",
+        )
-                    (namespace, chat_id),
+        .await
-                )
-                .await?
-        }
-        // foreign_id is declared as `INTEGER DEFAULT 0` in the schema.
-        None => {
-            context
-                .sql
-                .query_get_value(
-                    "SELECT token FROM tokens WHERE namespc=? AND foreign_id=0 ORDER BY timestamp DESC LIMIT 1;",
-                    (namespace,),
-                )
-                .await?
-        }
-    };
-    Ok(token)
 }
 
 pub async fn lookup_or_new(
     context: &Context,
     namespace: Namespace,
-    foreign_id: Option<ChatId>,
+    foreign_key: Option<&str>,
 ) -> Result<String> {
-    if let Some(token) = lookup(context, namespace, foreign_id).await? {
+    if let Some(token) = lookup(context, namespace, foreign_key).await? {
         return Ok(token);
     }
 
     let token = create_id();
-    save(context, namespace, foreign_id, &token).await?;
+    save(context, namespace, foreign_key, &token).await?;
     Ok(token)
 }
 
@@ -114,23 +86,22 @@ pub async fn exists(context: &Context, namespace: Namespace, token: &str) -> Res
     Ok(exists)
 }
 
-/// Looks up ChatId by auth token.
+/// Looks up foreign key by auth token.
 ///
 /// Returns None if auth token is not valid.
-/// Returns zero/unset ChatId if the token corresponds to "setup contact" rather than group join.
+/// Returns an empty string if the token corresponds to "setup contact" rather than group join.
-pub async fn auth_chat_id(context: &Context, token: &str) -> Result<Option<ChatId>> {
+pub async fn auth_foreign_key(context: &Context, token: &str) -> Result<Option<String>> {
-    let chat_id: Option<ChatId> = context
+    context
         .sql
         .query_row_optional(
-            "SELECT foreign_id FROM tokens WHERE namespc=? AND token=?",
+            "SELECT foreign_key FROM tokens WHERE namespc=? AND token=?",
             (Namespace::Auth, token),
             |row| {
-                let chat_id: ChatId = row.get(0)?;
+                let foreign_key: String = row.get(0)?;
-                Ok(chat_id)
+                Ok(foreign_key)
             },
         )
-        .await?;
+        .await
-    Ok(chat_id)
 }
 
 pub async fn delete(context: &Context, namespace: Namespace, token: &str) -> Result<()> {