diff --git a/deltachat-rpc-client/tests/test_securejoin.py b/deltachat-rpc-client/tests/test_securejoin.py index 7a0fdbaae..57dc06d9d 100644 --- a/deltachat-rpc-client/tests/test_securejoin.py +++ b/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") - alice_chat = alice.create_group("Verified group", protect=protect) + logging.info("Alice creates a group") + 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() diff --git a/src/chat.rs b/src/chat.rs index b8084008c..93f8cb8b6 100644 --- a/src/chat.rs +++ b/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()` - // when the group creation message is already in the `smtp` table -- - // this makes sure, the other devices are aware of grpid that is used in the sync-message. + // 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. 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() diff --git a/src/qr.rs b/src/qr.rs index 8d4995d62..2ad2f52ad 100644 --- a/src/qr.rs +++ b/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?; - context.sync_qr_code_tokens(chat_id).await?; + token::save(context, token::Namespace::Auth, Some(&grpid), &authcode).await?; + context.sync_qr_code_tokens(Some(&grpid)).await?; context.scheduler.interrupt_smtp().await; } Qr::Login { address, options } => { diff --git a/src/securejoin.rs b/src/securejoin.rs index 7f7487670..93877540e 100644 --- a/src/securejoin.rs +++ b/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) -> Resu ensure_secret_key_exists(context).await.ok(); - // invitenumber will be used to allow starting the handshake, - // auth will be used to verify the fingerprint - let sync_token = token::lookup(context, Namespace::InviteNumber, group) + let chat = match group { + Some(id) => { + 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?; - let auth = token::lookup_or_new(context, Namespace::Auth, group).await?; + // invitenumber will be used to allow starting the handshake, + // 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) -> 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) } diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index ec5e86b9e..f357de8b3 100644 --- a/src/sql/migrations.rs +++ b/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? diff --git a/src/sync.rs b/src/sync.rs index a60ec40a0..23b8ae5e5 100644 --- a/src/sync.rs +++ b/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. - /// If device synchronization is disabled, + /// Adds most recent qr-code tokens for the given group or self-contact to the list of items to + /// 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) -> 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::Auth, chat_id).await?, + token::lookup(self, Namespace::InviteNumber, grpid).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 { - if let Some((chat_id, _, _)) = chat::get_chat_id_by_grpid(self, grpid).await? { - Some(chat_id) - } 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?; + let grpid = token.grpid.as_deref(); + token::save(self, Namespace::InviteNumber, grpid, &token.invitenumber).await?; + token::save(self, Namespace::Auth, grpid, &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; - let msg_id = alice.send_sync_msg().await?; - // The QR code token was already synced before. - assert!(msg_id.is_none()); + tcm.exec_securejoin_qr(fiona, alice2, &qr).await; + let msg = fiona.get_last_msg().await; + assert_eq!( + msg.text, + "Member Me (fiona@example.net) added by alice@example.org." + ); Ok(()) } } diff --git a/src/token.rs b/src/token.rs index 63f97175e..a5bdfc068 100644 --- a/src/token.rs +++ b/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, + foreign_key: Option<&str>, token: &str, ) -> Result<()> { - match foreign_id { - Some(foreign_id) => context - .sql - .execute( - "INSERT INTO tokens (namespc, foreign_id, token, timestamp) VALUES (?, ?, ?, ?);", - (namespace, foreign_id, token, time()), - ) - .await?, - None => { - context - .sql - .execute( - "INSERT INTO tokens (namespc, token, timestamp) VALUES (?, ?, ?);", - (namespace, token, time()), - ) - .await? - } - }; - + context + .sql + .execute( + "INSERT INTO tokens (namespc, foreign_key, token, timestamp) VALUES (?, ?, ?, ?)", + (namespace, foreign_key.unwrap_or(""), 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, + foreign_key: Option<&str>, ) -> Result> { - let token = match chat { - Some(chat_id) => { - context - .sql - .query_get_value( - "SELECT token FROM tokens WHERE namespc=? AND foreign_id=? ORDER BY timestamp DESC LIMIT 1;", - (namespace, chat_id), - ) - .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) + context + .sql + .query_get_value( + "SELECT token FROM tokens WHERE namespc=? AND foreign_key=? ORDER BY timestamp DESC LIMIT 1", + (namespace, foreign_key.unwrap_or("")), + ) + .await } pub async fn lookup_or_new( context: &Context, namespace: Namespace, - foreign_id: Option, + foreign_key: Option<&str>, ) -> Result { - 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. -pub async fn auth_chat_id(context: &Context, token: &str) -> Result> { - let chat_id: Option = context +/// Returns an empty string if the token corresponds to "setup contact" rather than group join. +pub async fn auth_foreign_key(context: &Context, token: &str) -> Result> { + 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)?; - Ok(chat_id) + let foreign_key: String = row.get(0)?; + Ok(foreign_key) }, ) - .await?; - Ok(chat_id) + .await } pub async fn delete(context: &Context, namespace: Namespace, token: &str) -> Result<()> {