fix broadcast list synchronization

Author: link2xt

src/chat.rs

@@ -2286,19 +2286,40 @@ impl Chat {
 
     /// Sends a `SyncAction` synchronising chat contacts to other devices.
     pub(crate) async fn sync_contacts(&self, context: &Context) -> Result<()> {
-        let addrs = context
-            .sql
-            .query_map(
-                "SELECT c.addr \
-                FROM contacts c INNER JOIN chats_contacts cc \
-                ON c.id=cc.contact_id \
-                WHERE cc.chat_id=? AND cc.add_timestamp >= cc.remove_timestamp",
-                (self.id,),
-                |row| row.get::<_, String>(0),
-                |addrs| addrs.collect::<Result<Vec<_>, _>>().map_err(Into::into),
-            )
-            .await?;
-        self.sync(context, SyncAction::SetContacts(addrs)).await
+        if self.is_encrypted(context).await? {
+            let fingerprint_addrs = context
+                .sql
+                .query_map(
+                    "SELECT c.fingerprint, c.addr
+                     FROM contacts c INNER JOIN chats_contacts cc
+                     ON c.id=cc.contact_id
+                     WHERE cc.chat_id=? AND cc.add_timestamp >= cc.remove_timestamp",
+                    (self.id,),
+                    |row| {
+                        let fingerprint = row.get(0)?;
+                        let addr = row.get(1)?;
+                        Ok((fingerprint, addr))
+                    },
+                    |addrs| addrs.collect::<Result<Vec<_>, _>>().map_err(Into::into),
+                )
+                .await?;
+            self.sync(context, SyncAction::SetPgpContacts(fingerprint_addrs)).await?;
+        } else {
+            let addrs = context
+                .sql
+                .query_map(
+                    "SELECT c.addr \
+                    FROM contacts c INNER JOIN chats_contacts cc \
+                    ON c.id=cc.contact_id \
+                    WHERE cc.chat_id=? AND cc.add_timestamp >= cc.remove_timestamp",
+                    (self.id,),
+                    |row| row.get::<_, String>(0),
+                    |addrs| addrs.collect::<Result<Vec<_>, _>>().map_err(Into::into),
+                )
+                .await?;
+            self.sync(context, SyncAction::SetContacts(addrs)).await?;
+        }
+        Ok(())
     }
 
     /// Returns chat id for the purpose of synchronisation across devices.
@@ -4808,6 +4829,10 @@ pub(crate) async fn update_msg_text_and_timestamp(
 /// Set chat contacts by their addresses creating the corresponding contacts if necessary.
 async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[String]) -> Result<()> {
     let chat = Chat::load_from_db(context, id).await?;
+    ensure!(
+        !chat.is_encrypted(context).await?,
+        "Cannot add email-contacts to encrypted chat {id}"
+    );
     ensure!(
         chat.typ == Chattype::Broadcast,
         "{id} is not a broadcast list",
@@ -4843,6 +4868,54 @@ async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[String])
     Ok(())
 }
 
+/// Set chat contacts by their fingerprints creating the corresponding contacts if necessary.
+///
+/// `fingerprint_addrs` is a list of pairs of fingerprint and address.
+async fn set_contacts_by_fingerprints(
+    context: &Context,
+    id: ChatId,
+    fingerprint_addrs: &[(String, String)],
+) -> Result<()> {
+    let chat = Chat::load_from_db(context, id).await?;
+    ensure!(
+        chat.is_encrypted(context).await?,
+        "Cannot add PGP-contacts to unencrypted chat {id}"
+    );
+    ensure!(
+        chat.typ == Chattype::Broadcast,
+        "{id} is not a broadcast list",
+    );
+    let mut contacts = HashSet::new();
+    for (fingerprint, addr) in fingerprint_addrs {
+        let contact_addr = ContactAddress::new(addr)?;
+        let contact = Contact::add_or_lookup_ex(context, "", &contact_addr, &fingerprint, Origin::Hidden)
+            .await?
+            .0;
+        contacts.insert(contact);
+    }
+    let contacts_old = HashSet::<ContactId>::from_iter(get_chat_contacts(context, id).await?);
+    if contacts == contacts_old {
+        return Ok(());
+    }
+    context
+        .sql
+        .transaction(move |transaction| {
+            transaction.execute("DELETE FROM chats_contacts WHERE chat_id=?", (id,))?;
+
+            // We do not care about `add_timestamp` column
+            // because timestamps are not used for broadcast lists.
+            let mut statement = transaction
+                .prepare("INSERT INTO chats_contacts (chat_id, contact_id) VALUES (?, ?)")?;
+            for contact_id in &contacts {
+                statement.execute((id, contact_id))?;
+            }
+            Ok(())
+        })
+        .await?;
+    context.emit_event(EventType::ChatModified(id));
+    Ok(())
+}
+
 /// A cross-device chat id used for synchronisation.
 #[derive(Debug, Serialize, Deserialize, PartialEq)]
 pub(crate) enum SyncId {
@@ -4873,6 +4946,10 @@ pub(crate) enum SyncAction {
     Rename(String),
     /// Set chat contacts by their addresses.
     SetContacts(Vec<String>),
+    /// Set chat contacts by their fingerprints.
+    ///
+    /// The list is a list of pairs of fingerprint and address.
+    SetPgpContacts(Vec<(String, String)>),
     Delete,
 }
 
@@ -4956,6 +5033,7 @@ impl Context {
             }
             SyncAction::Rename(to) => rename_ex(self, Nosync, chat_id, to).await,
             SyncAction::SetContacts(addrs) => set_contacts_by_addrs(self, chat_id, addrs).await,
+            SyncAction::SetPgpContacts(fingerprint_addrs) => set_contacts_by_fingerprints(self, chat_id, fingerprint_addrs).await,
             SyncAction::Delete => chat_id.delete_ex(self, Nosync).await,
         }
     }

src/chat/chat_tests.rs

@@ -3213,7 +3213,7 @@ async fn test_sync_broadcast() -> Result<()> {
     for a in [alice0, alice1] {
         a.set_config_bool(Config::SyncMsgs, true).await?;
     }
-    let bob = TestContext::new_bob().await;
+    let bob = &tcm.bob().await;
     let a0b_contact_id = alice0.add_or_lookup_contact(&bob).await.id;
 
     let a0_broadcast_id = create_broadcast_list(alice0).await?;
@@ -3229,13 +3229,12 @@ async fn test_sync_broadcast() -> Result<()> {
     assert!(get_chat_contacts(alice1, a1_broadcast_id).await?.is_empty());
     add_contact_to_chat(alice0, a0_broadcast_id, a0b_contact_id).await?;
     sync(alice0, alice1).await;
-    let a1b_contact_id = Contact::lookup_id_by_addr(
-        alice1,
-        &bob.get_config(Config::Addr).await?.unwrap(),
-        Origin::Hidden,
-    )
-    .await?
-    .unwrap();
+
+    // This also imports Bob's key from the vCard.
+    // Otherwise it is possible that second device
+    // does not have Bob's key as only the fingerprint
+    // is transferred in the sync message.
+    let a1b_contact_id = alice1.add_or_lookup_contact(bob).await.id;
     assert_eq!(
         get_chat_contacts(alice1, a1_broadcast_id).await?,
         vec![a1b_contact_id]

src/contact.rs

@@ -1867,7 +1867,10 @@ pub(crate) async fn mark_contact_id_as_verified(
     contact_id: ContactId,
     verifier_id: ContactId,
 ) -> Result<()> {
-    debug_assert_ne!(contact_id, verifier_id, "Contact cannot be verified by self");
+    debug_assert_ne!(
+        contact_id, verifier_id,
+        "Contact cannot be verified by self"
+    );
     context
         .sql
         .transaction(|transaction| {
@@ -1886,7 +1889,9 @@ pub(crate) async fn mark_contact_id_as_verified(
                     |row| row.get(0),
                 )?;
                 if verifier_fingerprint.is_empty() {
-                    bail!("Contact {contact_id} cannot be verified by non-PGP contact {verifier_id}.");
+                    bail!(
+                        "Contact {contact_id} cannot be verified by non-PGP contact {verifier_id}."
+                    );
                 }
             }
             transaction.execute(

src/message/message_tests.rs

@@ -1,10 +1,7 @@
 use num_traits::FromPrimitive;
 
 use super::*;
-use crate::chat::{
-    self, add_contact_to_chat, forward_msgs, marknoticed_chat, save_msgs, send_text_msg, ChatItem,
-    ProtectionStatus,
-};
+use crate::chat::{self, forward_msgs, marknoticed_chat, save_msgs, send_text_msg, ChatItem};
 use crate::chatlist::Chatlist;
 use crate::config::Config;
 use crate::reaction::send_reaction;
@@ -197,7 +194,9 @@ async fn test_unencrypted_quote_encrypted_message() -> Result<()> {
 
     tcm.section("Bob sends encrypted message to Alice");
     let alice_chat = alice.create_chat(bob).await;
-    let sent = alice.send_text(alice_chat.id, "Hi! This is encrypted.").await;
+    let sent = alice
+        .send_text(alice_chat.id, "Hi! This is encrypted.")
+        .await;
 
     let bob_received_message = bob.recv_msg(&sent).await;
     assert_eq!(bob_received_message.get_showpadlock(), true);

src/receive_imf/receive_imf_tests.rs

@@ -4735,10 +4735,7 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> {
     let alice_bob_id = alice.add_or_lookup_contact(bob).await.id;
     add_contact_to_chat(alice, group_id, alice_bob_id).await?;
     alice.send_text(group_id, "Hello!").await;
-    alice
-        .sql
-        .execute("DELETE FROM public_keys", ())
-        .await?;
+    alice.sql.execute("DELETE FROM public_keys", ()).await?;
 
     let fiona = &tcm.fiona().await;
     let fiona_addr = fiona.get_config(Config::Addr).await?.unwrap();

src/securejoin/securejoin_tests.rs

@@ -1,17 +1,15 @@
-use deltachat_contact_tools::{ContactAddress, EmailAddress};
+use deltachat_contact_tools::EmailAddress;
 
 use super::*;
 use crate::chat::{remove_contact_from_chat, CantSendReason};
 use crate::chatlist::Chatlist;
 use crate::constants::{self, Chattype};
-use crate::imex::{imex, ImexMode};
 use crate::receive_imf::receive_imf;
 use crate::stock_str::{self, chat_protection_enabled};
 use crate::test_utils::{
     get_chat_msg, TestContext, TestContextManager, TimeShiftFalsePositiveNote,
 };
 use crate::tools::SystemTime;
-use std::collections::HashSet;
 use std::time::Duration;
 
 #[derive(PartialEq)]
@@ -234,9 +232,7 @@ async fn test_setup_contact_ex(case: SetupContactCase) {
     tcm.section("Step 5+6: Alice receives vc-request-with-auth, sends vc-contact-confirm");
     alice.recv_msg_trash(&sent).await;
     assert_eq!(contact_bob.is_verified(&alice).await.unwrap(), true);
-    let contact_bob = Contact::get_by_id(&alice, contact_bob_id)
-        .await
-        .unwrap();
+    let contact_bob = Contact::get_by_id(&alice, contact_bob_id).await.unwrap();
     assert_eq!(contact_bob.get_authname(), "Bob Examplenet");
     assert!(contact_bob.get_name().is_empty());
     assert_eq!(contact_bob.is_bot(), false);
@@ -465,9 +461,7 @@ async fn test_secure_join() -> Result<()> {
         chat::create_group_chat(&alice, ProtectionStatus::Protected, "the chat").await?;
 
     tcm.section("Step 1: Generate QR-code, secure-join implied by chatid");
-    let qr = get_securejoin_qr(&alice, Some(alice_chatid))
-        .await
-        .unwrap();
+    let qr = get_securejoin_qr(&alice, Some(alice_chatid)).await.unwrap();
 
     tcm.section("Step 2: Bob scans QR-code, sends vg-request");
     let bob_chatid = join_securejoin(&bob, &qr).await?;

src/tests/aeap.rs

@@ -1,9 +1,7 @@
 use anyhow::Result;
 
 use crate::chat::{self, Chat, ChatId, ProtectionStatus};
-use crate::contact;
-use crate::contact::Contact;
-use crate::contact::ContactId;
+use crate::contact::{Contact, ContactId};
 use crate::message::Message;
 use crate::receive_imf::receive_imf;
 use crate::securejoin::get_securejoin_qr;

src/tests/verified_chats.rs

@@ -4,9 +4,8 @@ use pretty_assertions::assert_eq;
 use crate::chat::{
     self, add_contact_to_chat, remove_contact_from_chat, send_msg, Chat, ProtectionStatus,
 };
-use crate::chatlist::Chatlist;
 use crate::config::Config;
-use crate::constants::{Chattype, DC_GCL_FOR_FORWARDING};
+use crate::constants::Chattype;
 use crate::contact::{Contact, ContactId};
 use crate::message::Message;
 use crate::mimefactory::MimeFactory;