fix: Let Alice send vb-member-added so that the chat is immediately shown on Bob's device

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

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

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,
+            // If undisclosed_recipients, and this is a member-added/removed message,
-            // only send to email_to_remove
+            // 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!(
+                        let step = match chat.typ {
-                            context,
+                            Chattype::Group => "vg-member-added",
-                            "Sending secure-join message {:?}.", "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())
+                            mail_builder::headers::raw::Raw::new(step.to_string()).into(),
-                                .into(),
                         ));
                     }
                 }

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,
+    self, Chat, ChatId, ChatIdBlocked, ProtectionStatus, get_chat_id_by_grpid,
-    get_chat_id_by_grpid, load_broadcast_shared_secret,
+    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?;
-                }
                 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)

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 {

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]
+--------------------------------------------------------------------------------

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]
+--------------------------------------------------------------------------------