From c4001cc3ff1f1eb9054a27ef0cf5ba1ebd9421ad Mon Sep 17 00:00:00 2001 From: Hocuri Date: Mon, 4 Aug 2025 17:17:06 +0200 Subject: [PATCH] fix: Let Alice send vb-member-added so that the chat is immediately shown on Bob's device --- src/chat.rs | 6 +--- src/chat/chat_tests.rs | 31 ++++++++++++++++ src/mimefactory.rs | 32 +++++++++++------ src/securejoin.rs | 35 ++++++++++--------- src/test_utils.rs | 15 +++++--- .../test_broadcast_joining_golden_alice | 6 ++++ .../golden/test_broadcast_joining_golden_bob | 5 +++ 7 files changed, 94 insertions(+), 36 deletions(-) create mode 100644 test-data/golden/test_broadcast_joining_golden_alice create mode 100644 test-data/golden/test_broadcast_joining_golden_bob diff --git a/src/chat.rs b/src/chat.rs index a26deb70c..8048a49e5 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -4004,7 +4004,7 @@ pub(crate) async fn add_contact_to_chat_ex( // this also makes sure, no contacts are added to special or normal chats let mut chat = Chat::load_from_db(context, chat_id).await?; ensure!( - chat.typ == Chattype::Group, + chat.typ == Chattype::Group || (from_handshake && chat.typ == Chattype::OutBroadcast), "{} is not a group where one can add members", chat_id ); @@ -4013,7 +4013,6 @@ pub(crate) async fn add_contact_to_chat_ex( "invalid contact_id {} for adding to group", contact_id ); - ensure!(!chat.is_mailing_list(), "Mailing lists can't be changed"); ensure!( chat.is_encrypted(context).await? == contact.is_key_contact(), "Only key-contacts can be added to encrypted chats" @@ -4060,9 +4059,6 @@ pub(crate) async fn add_contact_to_chat_ex( ); return Ok(false); } - if is_contact_in_chat(context, chat_id, contact_id).await? { - return Ok(false); - } add_to_chat_contacts_table(context, time(), chat_id, &[contact_id]).await?; } if chat.is_promoted() { diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index e67baf1e8..c217407e7 100644 --- a/src/chat/chat_tests.rs +++ b/src/chat/chat_tests.rs @@ -2873,6 +2873,37 @@ async fn test_broadcasts_name_and_avatar() -> Result<()> { Ok(()) } +/// Tests that directly after broadcast-securejoin, +/// the brodacast is shown correctly on both devices. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_broadcast_joining_golden() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + alice.set_config(Config::Displayname, Some("Alice")).await?; + + tcm.section("Create a broadcast channel with an avatar"); + let alice_chat_id = create_broadcast(alice, "My Channel".to_string()).await?; + let file = alice.get_blobdir().join("avatar.png"); + tokio::fs::write(&file, AVATAR_64x64_BYTES).await?; + set_chat_profile_image(alice, alice_chat_id, file.to_str().unwrap()).await?; + alice.pop_sent_msg().await; // TODO check if Alice wrongly sends out a message here + + let qr = get_securejoin_qr(alice, Some(alice_chat_id)).await.unwrap(); + let bob_chat_id = tcm.exec_securejoin_qr(bob, alice, &qr).await; + + // TODO it's not nice that it says 'you added member bob' + // and 'Secure-Join: vb-request-with-auth' + alice + .golden_test_chat(alice_chat_id, "test_broadcast_joining_golden_alice") + .await; + bob.golden_test_chat(bob_chat_id, "test_broadcast_joining_golden_bob") + .await; + + Ok(()) +} + /// - Create a broadcast channel /// - Block it /// - Check that the broadcast channel appears in the list of blocked contacts diff --git a/src/mimefactory.rs b/src/mimefactory.rs index a5af47063..ca8bf447c 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -3,7 +3,7 @@ use std::collections::{BTreeSet, HashSet}; use std::io::Cursor; -use anyhow::{Context as _, Result, anyhow, bail, ensure}; +use anyhow::{Context as _, Result, anyhow, bail}; use base64::Engine as _; use data_encoding::BASE32_NOPAD; use deltachat_contact_tools::sanitize_bidi_characters; @@ -415,8 +415,18 @@ impl MimeFactory { req_mdn = true; } - // TODO if hidden_recipients but email_to_remove is some, - // only send to email_to_remove + // If undisclosed_recipients, and this is a member-added/removed message, + // only send to the added/removed member + if undisclosed_recipients + && matches!( + msg.param.get_cmd(), + SystemMessage::MemberRemovedFromGroup | SystemMessage::MemberAddedToGroup + ) + { + if let Some(member) = msg.param.get(Param::Arg) { + recipients.retain(|addr| addr == member); + } + } encryption_keys = if !is_encrypted { None @@ -571,6 +581,7 @@ impl MimeFactory { || step == "vc-request-with-auth" || step == "vb-request-with-auth" || step == "vg-member-added" + || step == "vb-member-added" || step == "vc-contact-confirm" // TODO possibly add vb-member-added here } @@ -1396,7 +1407,6 @@ impl MimeFactory { match command { SystemMessage::MemberRemovedFromGroup => { - ensure!(chat.typ != Chattype::OutBroadcast); let email_to_remove = msg.param.get(Param::Arg).unwrap_or_default(); if email_to_remove @@ -1420,7 +1430,6 @@ impl MimeFactory { } } SystemMessage::MemberAddedToGroup => { - ensure!(chat.typ != Chattype::OutBroadcast); // TODO: lookup the contact by ID rather than email address. // We are adding key-contacts, the cannot be looked up by address. let email_to_add = msg.param.get(Param::Arg).unwrap_or_default(); @@ -1434,14 +1443,15 @@ impl MimeFactory { )); } if 0 != msg.param.get_int(Param::Arg2).unwrap_or_default() & DC_FROM_HANDSHAKE { - info!( - context, - "Sending secure-join message {:?}.", "vg-member-added", - ); + let step = match chat.typ { + Chattype::Group => "vg-member-added", + Chattype::OutBroadcast => "vb-member-added", + _ => bail!("Wrong chattype {}", chat.typ), + }; + info!(context, "Sending secure-join message {:?}.", step,); headers.push(( "Secure-Join", - mail_builder::headers::raw::Raw::new("vg-member-added".to_string()) - .into(), + mail_builder::headers::raw::Raw::new(step.to_string()).into(), )); } } diff --git a/src/securejoin.rs b/src/securejoin.rs index 69476639b..a52fae4ef 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -5,8 +5,8 @@ use deltachat_contact_tools::ContactAddress; use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode}; use crate::chat::{ - self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, add_to_chat_contacts_table, - get_chat_id_by_grpid, load_broadcast_shared_secret, + self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, get_chat_id_by_grpid, + load_broadcast_shared_secret, }; use crate::chatlist_events; use crate::config::Config; @@ -27,7 +27,6 @@ use crate::qr::check_qr; use crate::securejoin::bob::JoinerProgress; use crate::sync::Sync::*; use crate::token; -use crate::tools::time; mod bob; mod qrinvite; @@ -293,7 +292,10 @@ pub(crate) async fn handle_securejoin_handshake( // TODO talk with link2xt about whether we need to protect against this identity-misbinding attack, // and if so, how - if !matches!(step, "vg-request" | "vc-request" | "vb-request-with-auth") { + if !matches!( + step, + "vg-request" | "vc-request" | "vb-request-with-auth" | "vb-member-added" + ) { let mut self_found = false; let self_fingerprint = load_self_public_key(context).await?.dc_fingerprint(); for (addr, key) in &mime_message.gossiped_keys { @@ -438,14 +440,9 @@ pub(crate) async fn handle_securejoin_handshake( mime_message.timestamp_sent, ) .await?; - if step.starts_with("vb-") { - // TODO extract into variable - add_to_chat_contacts_table(context, time(), group_chat_id, &[contact_id]) - .await?; - } else { - chat::add_contact_to_chat_ex(context, Nosync, group_chat_id, contact_id, true) - .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); if step.starts_with("vb-") { @@ -486,7 +483,7 @@ pub(crate) async fn handle_securejoin_handshake( }); Ok(HandshakeMessage::Ignore) } - "vg-member-added" => { + "vg-member-added" | "vb-member-added" => { let Some(member_added) = mime_message.get_header(HeaderDef::ChatGroupMemberAdded) else { warn!( @@ -554,7 +551,11 @@ pub(crate) async fn observe_securejoin_on_other_device( if !matches!( step, - "vg-request-with-auth" | "vc-request-with-auth" | "vg-member-added" | "vc-contact-confirm" + "vg-request-with-auth" + | "vc-request-with-auth" + | "vg-member-added" + | "vb-member-added" + | "vc-contact-confirm" ) { return Ok(HandshakeMessage::Ignore); }; @@ -594,10 +595,12 @@ pub(crate) async fn observe_securejoin_on_other_device( if step == "vg-member-added" { inviter_progress(context, contact_id, 800); } - if step == "vg-member-added" || step == "vc-contact-confirm" { + if step == "vg-member-added" || step == "vb-member-added" || step == "vc-contact-confirm" { inviter_progress(context, contact_id, 1000); } + // TODO not sure if I should ad vb-request-with-auth here + // Actually, I'm not even sure why vg-request-with-auth is here - why do we create a 1:1 chat?? if step == "vg-request-with-auth" || step == "vc-request-with-auth" { // This actually reflects what happens on the first device (which does the secure // join) and causes a subsequent "vg-member-added" message to create an unblocked @@ -605,7 +608,7 @@ pub(crate) async fn observe_securejoin_on_other_device( ChatId::create_for_contact_with_blocked(context, contact_id, Blocked::Not).await?; } - if step == "vg-member-added" { + if step == "vg-member-added" || step == "vb-member-added" { Ok(HandshakeMessage::Propagate) } else { Ok(HandshakeMessage::Ignore) diff --git a/src/test_utils.rs b/src/test_utils.rs index c2aef7408..0a5c2aa97 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -222,10 +222,10 @@ impl TestContextManager { self.exec_securejoin_qr(scanner, scanned, &qr).await } - /// Executes SecureJoin initiated by `scanner` scanning `qr` generated by `scanned`. + /// Executes SecureJoin initiated by `joiner` scanning `qr` generated by `inviter`. /// /// The [`ChatId`] of the created chat is returned, for a SetupContact QR this is the 1:1 - /// chat with `scanned`, for a SecureJoin QR this is the group chat. + /// chat with `inviter`, for a SecureJoin QR this is the group chat. pub async fn exec_securejoin_qr( &self, joiner: &TestContext, @@ -236,16 +236,23 @@ impl TestContextManager { .await } - /// Executes SecureJoin initiated by `scanner` scanning `qr` generated by `scanned`. + /// Executes SecureJoin initiated by `joiner` + /// scanning `qr` generated by one of the `inviters` devices. + /// All of the `inviters` devices will get the messages and send replies. /// /// The [`ChatId`] of the created chat is returned, for a SetupContact QR this is the 1:1 - /// chat with `scanned`, for a SecureJoin QR this is the group chat. + /// chat with `inviter`, for a SecureJoin QR this is the group chat. pub async fn exec_securejoin_qr_multi_device( &self, joiner: &TestContext, inviters: &[&TestContext], qr: &str, ) -> ChatId { + assert!(joiner.pop_sent_msg_opt(Duration::ZERO).await.is_none()); + for inviter in inviters { + assert!(inviter.pop_sent_msg_opt(Duration::ZERO).await.is_none()); + } + let chat_id = join_securejoin(&joiner.ctx, qr).await.unwrap(); loop { diff --git a/test-data/golden/test_broadcast_joining_golden_alice b/test-data/golden/test_broadcast_joining_golden_alice new file mode 100644 index 000000000..3f71ff12f --- /dev/null +++ b/test-data/golden/test_broadcast_joining_golden_alice @@ -0,0 +1,6 @@ +OutBroadcast#Chat#10: My Channel [1 member(s)] Icon: e9b6c7a78aa2e4f415644f55a553e73.png +-------------------------------------------------------------------------------- +Msg#10πŸ”’: Me (Contact#Contact#Self): You changed the group image. [INFO] √ +Msg#12πŸ”’: Me (Contact#Contact#Self): You added member bob@example.net. [INFO] √ +Msg#13πŸ”’: (Contact#Contact#10): Secure-Join: vb-request-with-auth [FRESH] +-------------------------------------------------------------------------------- diff --git a/test-data/golden/test_broadcast_joining_golden_bob b/test-data/golden/test_broadcast_joining_golden_bob new file mode 100644 index 000000000..308762a74 --- /dev/null +++ b/test-data/golden/test_broadcast_joining_golden_bob @@ -0,0 +1,5 @@ +InBroadcast#Chat#11: My Channel [1 member(s)] Icon: e9b6c7a78aa2e4f415644f55a553e73.png +-------------------------------------------------------------------------------- +Msg#12: info (Contact#Contact#Info): You were invited to join this channel. Waiting for the channel owner's device to reply… [NOTICED][INFO] +Msg#13πŸ”’: (Contact#Contact#10): I added member bob@example.net. [FRESH][INFO] +--------------------------------------------------------------------------------