Merge branch 'stable', resolving conflicts

Cargo.lock

@@ -5380,9 +5380,9 @@ dependencies = [
 
 [[package]]
 name = "webpki"
-version = "0.22.1"
+version = "0.22.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f0e74f82d49d545ad128049b7e88f6576df2da6b02e9ce565c6f533be576957e"
+checksum = "07ecc0cd7cac091bf682ec5efa18b1cff79d617b84181f38b3951dbe135f607f"
 dependencies = [
  "ring",
  "untrusted",

deltachat-ffi/deltachat.h

@@ -831,7 +831,7 @@ void            dc_maybe_network             (dc_context_t* context);
  * @param context The context as created by dc_context_new().
  * @param addr The e-mail address of the user. This must match the
  *    configured_addr setting of the context as well as the UID of the key.
- * @param public_data ASCII armored public key.
+ * @param public_data Ignored, actual public key is extracted from secret_data.
  * @param secret_data ASCII armored secret key.
  * @return 1 on success, 0 on failure.
  */

deltachat-ffi/src/lib.rs

@@ -805,7 +805,7 @@ pub unsafe extern "C" fn dc_maybe_network(context: *mut dc_context_t) {
 pub unsafe extern "C" fn dc_preconfigure_keypair(
     context: *mut dc_context_t,
     addr: *const libc::c_char,
-    public_data: *const libc::c_char,
+    _public_data: *const libc::c_char,
     secret_data: *const libc::c_char,
 ) -> i32 {
     if context.is_null() {
@@ -814,9 +814,8 @@ pub unsafe extern "C" fn dc_preconfigure_keypair(
     }
     let ctx = &*context;
     let addr = to_string_lossy(addr);
-    let public_data = to_string_lossy(public_data);
     let secret_data = to_string_lossy(secret_data);
-    block_on(preconfigure_keypair(ctx, &addr, &public_data, &secret_data))
+    block_on(preconfigure_keypair(ctx, &addr, &secret_data))
         .context("Failed to save keypair")
         .log_err(ctx)
         .is_ok() as libc::c_int

python/src/deltachat/account.py

@@ -195,7 +195,7 @@ class Account:
         assert res != ffi.NULL, f"config value not found for: {name!r}"
         return from_dc_charpointer(res)
 
-    def _preconfigure_keypair(self, addr: str, public: str, secret: str) -> None:
+    def _preconfigure_keypair(self, addr: str, secret: str) -> None:
         """See dc_preconfigure_keypair() in deltachat.h.
 
         In other words, you don't need this.
@@ -203,7 +203,7 @@ class Account:
         res = lib.dc_preconfigure_keypair(
             self._dc_context,
             as_dc_charpointer(addr),
-            as_dc_charpointer(public),
+            ffi.NULL,
             as_dc_charpointer(secret),
         )
         if res == 0:

python/src/deltachat/contact.py

@@ -75,9 +75,12 @@ class Contact:
         """Return True if the contact is verified."""
         return lib.dc_contact_is_verified(self._dc_contact) == 2
 
-    def get_verifier(self, contact):
+    def get_verifier(self, contact) -> Optional["Contact"]:
         """Return the address of the contact that verified the contact."""
-        return from_dc_charpointer(lib.dc_contact_get_verifier_addr(contact._dc_contact))
+        verifier_id = lib.dc_contact_get_verifier_id(contact._dc_contact)
+        if verifier_id == 0:
+            return None
+        return Contact(self.account, verifier_id)
 
     def get_profile_image(self) -> Optional[str]:
         """Get contact profile image.

python/src/deltachat/testplugin.py

@@ -478,10 +478,9 @@ class ACFactory:
         except IndexError:
             pass
         else:
-            fname_pub = self.data.read_path(f"key/{keyname}-public.asc")
             fname_sec = self.data.read_path(f"key/{keyname}-secret.asc")
-            if fname_pub and fname_sec:
-                account._preconfigure_keypair(addr, fname_pub, fname_sec)
+            if fname_sec:
+                account._preconfigure_keypair(addr, fname_sec)
                 return True
             print(f"WARN: could not use preconfigured keys for {addr!r}")
 

python/tests/test_0_complex_or_slow.py

@@ -1,6 +1,7 @@
 import sys
 
 import pytest
+import deltachat as dc
 
 
 class TestGroupStressTests:
@@ -150,9 +151,8 @@ def test_qr_verified_group_and_chatting(acfactory, lp):
     assert msg.is_encrypted()
 
     lp.sec("ac2: Check that ac2 verified ac1")
-    # If we verified the contact ourselves then verifier addr == contact addr
     ac2_ac1_contact = ac2.get_contacts()[0]
-    assert ac2.get_self_contact().get_verifier(ac2_ac1_contact) == ac1_addr
+    assert ac2.get_self_contact().get_verifier(ac2_ac1_contact).id == dc.const.DC_CONTACT_ID_SELF
 
     lp.sec("ac2: send message and let ac1 read it")
     chat2.send_text("world")
@@ -177,9 +177,9 @@ def test_qr_verified_group_and_chatting(acfactory, lp):
 
     lp.sec("ac2: Check that ac1 verified ac3 for ac2")
     ac2_ac1_contact = ac2.get_contacts()[0]
-    assert ac2.get_self_contact().get_verifier(ac2_ac1_contact) == ac1_addr
+    assert ac2.get_self_contact().get_verifier(ac2_ac1_contact).id == dc.const.DC_CONTACT_ID_SELF
     ac2_ac3_contact = ac2.get_contacts()[1]
-    assert ac2.get_self_contact().get_verifier(ac2_ac3_contact) == ac1_addr
+    assert ac2.get_self_contact().get_verifier(ac2_ac3_contact).addr == ac1_addr
 
     lp.sec("ac2: send message and let ac3 read it")
     chat2.send_text("hi")

python/tests/test_3_offline.py

@@ -67,10 +67,9 @@ class TestOfflineAccountBasic:
 
     def test_preconfigure_keypair(self, acfactory, data):
         ac = acfactory.get_unconfigured_account()
-        alice_public = data.read_path("key/alice-public.asc")
         alice_secret = data.read_path("key/alice-secret.asc")
-        assert alice_public and alice_secret
-        ac._preconfigure_keypair("alice@example.org", alice_public, alice_secret)
+        assert alice_secret
+        ac._preconfigure_keypair("alice@example.org", alice_secret)
 
     def test_getinfo(self, acfactory):
         ac1 = acfactory.get_unconfigured_account()

src/key.rs

@@ -312,15 +312,10 @@ pub async fn store_self_keypair(
 /// This API is used for testing purposes
 /// to avoid generating the key in tests.
 /// Use import/export APIs instead.
-pub async fn preconfigure_keypair(
-    context: &Context,
-    addr: &str,
-    public_data: &str,
-    secret_data: &str,
-) -> Result<()> {
+pub async fn preconfigure_keypair(context: &Context, addr: &str, secret_data: &str) -> Result<()> {
     let addr = EmailAddress::new(addr)?;
-    let public = SignedPublicKey::from_asc(public_data)?.0;
     let secret = SignedSecretKey::from_asc(secret_data)?.0;
+    let public = secret.split_public_key()?;
     let keypair = KeyPair {
         addr,
         public,

src/mimeparser.rs

@@ -804,18 +804,6 @@ impl MimeMessage {
 
         // Boxed future to deal with recursion
         async move {
-            if mail.ctype.params.get("protected-headers").is_some() {
-                if mail.ctype.mimetype == "text/rfc822-headers" {
-                    warn!(
-                        context,
-                        "Protected headers found in text/rfc822-headers attachment: Will be ignored.",
-                    );
-                    return Ok(false);
-                }
-
-                warn!(context, "Ignoring nested protected headers");
-            }
-
             enum MimeS {
                 Multiple,
                 Single,
@@ -852,7 +840,10 @@ impl MimeMessage {
 
                     self.parse_mime_recursive(context, &mail, is_related).await
                 }
-                MimeS::Single => self.add_single_part_if_known(context, mail, is_related).await,
+                MimeS::Single => {
+                    self.add_single_part_if_known(context, mail, is_related)
+                        .await
+                }
             }
         }
         .boxed()

src/receive_imf.rs

@@ -1708,7 +1708,7 @@ async fn apply_group_changes(
     }
 
     let mut send_event_chat_modified = false;
-    let mut removed_id = None;
+    let (mut removed_id, mut added_id) = (None, None);
     let mut better_msg = None;
 
     // True if a Delta Chat client has explicitly added our current primary address.
@@ -1719,8 +1719,9 @@ async fn apply_group_changes(
             false
         };
 
-    let is_from_in_chat = !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await?
-        || chat::is_contact_in_chat(context, chat_id, from_id).await?;
+    let mut chat_contacts = HashSet::from_iter(chat::get_chat_contacts(context, chat_id).await?);
+    let is_from_in_chat =
+        !chat_contacts.contains(&ContactId::SELF) || chat_contacts.contains(&from_id);
 
     // Reject group membership changes from non-members and old changes.
     let allow_member_list_changes = is_from_in_chat
@@ -1730,12 +1731,10 @@ async fn apply_group_changes(
 
     // Whether to rebuild the member list from scratch.
     let recreate_member_list = {
-        // Recreate member list if the message comes from a MUA as these messages do _not_ set add/remove headers.
-        !mime_parser.has_chat_version()
-            // Always recreate membership list if SELF has been added. The older versions of DC
-            // don't always set "In-Reply-To" to the latest message they sent, but to the latest
-            // delivered message (so it's a race), so we have this heuristic here.
-            || self_added
+        // Always recreate membership list if SELF has been added. The older versions of DC
+        // don't always set "In-Reply-To" to the latest message they sent, but to the latest
+        // delivered message (so it's a race), so we have this heuristic here.
+        self_added
             || match mime_parser.get_header(HeaderDef::InReplyTo) {
                 // If we don't know the referenced message, we missed some messages.
                 // Maybe they added/removed members, so we need to recreate our member list.
@@ -1777,14 +1776,8 @@ async fn apply_group_changes(
             Some(stock_str::msg_del_member_local(context, removed_addr, from_id).await)
         };
 
-        if let Some(contact_id) = removed_id {
-            if allow_member_list_changes {
-                // Remove a single member from the chat.
-                if !recreate_member_list {
-                    chat::remove_from_chat_contacts_table(context, chat_id, contact_id).await?;
-                    send_event_chat_modified = true;
-                }
-            } else {
+        if removed_id.is_some() {
+            if !allow_member_list_changes {
                 info!(
                     context,
                     "Ignoring removal of {removed_addr:?} from {chat_id}."
@@ -1797,13 +1790,11 @@ async fn apply_group_changes(
         better_msg = Some(stock_str::msg_add_member_local(context, added_addr, from_id).await);
 
         if allow_member_list_changes {
-            // Add a single member to the chat.
             if !recreate_member_list {
                 if let Some(contact_id) =
                     Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await?
                 {
-                    chat::add_to_chat_contacts_table(context, chat_id, &[contact_id]).await?;
-                    send_event_chat_modified = true;
+                    added_id = Some(contact_id);
                 } else {
                     warn!(context, "Added {added_addr:?} has no contact id.")
                 }
@@ -1856,46 +1847,76 @@ async fn apply_group_changes(
         }
     }
 
-    // Recreate the member list.
-    if recreate_member_list {
-        // Only delete old contacts if the sender is not a classical MUA user:
-        // Classical MUA users usually don't intend to remove users from an email
-        // thread, so if they removed a recipient then it was probably by accident.
-        if mime_parser.has_chat_version() {
+    if allow_member_list_changes {
+        let mut new_members = HashSet::from_iter(to_ids.iter().copied());
+        new_members.insert(ContactId::SELF);
+        if !from_id.is_special() {
+            new_members.insert(from_id);
+        }
+
+        if !recreate_member_list {
+            let diff: HashSet<ContactId> =
+                chat_contacts.difference(&new_members).copied().collect();
+            // Only delete old contacts if the sender is not a classical MUA user:
+            // Classical MUA users usually don't intend to remove users from an email
+            // thread, so if they removed a recipient then it was probably by accident.
+            if mime_parser.has_chat_version() {
+                // This is what provides group membership consistency: we remove group members
+                // locally if we see a discrepancy with the "To" list in the received message as it
+                // is better for privacy than adding absent members locally. But it shouldn't be a
+                // big problem if somebody missed a member addition, because they will likely
+                // recreate the member list from the next received message. The problem occurs only
+                // if that "somebody" managed to reply earlier. Really, it's a problem for big
+                // groups with high message rate, but let it be for now.
+                if !diff.is_empty() {
+                    warn!(context, "Implicit removal of {diff:?} from chat {chat_id}.");
+                }
+                new_members = chat_contacts.difference(&diff).copied().collect();
+            } else {
+                new_members.extend(diff);
+            }
+        }
+        if let Some(removed_id) = removed_id {
+            new_members.remove(&removed_id);
+        }
+        if let Some(added_id) = added_id {
+            new_members.insert(added_id);
+        }
+        if recreate_member_list {
+            info!(
+                context,
+                "Recreating chat {chat_id} member list with {new_members:?}.",
+            );
+        }
+
+        if new_members != chat_contacts {
+            let new_members_ref = &new_members;
             context
                 .sql
-                .execute("DELETE FROM chats_contacts WHERE chat_id=?;", (chat_id,))
+                .transaction(move |transaction| {
+                    transaction
+                        .execute("DELETE FROM chats_contacts WHERE chat_id=?", (chat_id,))?;
+                    for contact_id in new_members_ref {
+                        transaction.execute(
+                            "INSERT INTO chats_contacts (chat_id, contact_id) VALUES(?, ?)",
+                            (chat_id, contact_id),
+                        )?;
+                    }
+                    Ok(())
+                })
                 .await?;
+            chat_contacts = new_members;
+            send_event_chat_modified = true;
         }
-
-        let mut members_to_add = HashSet::new();
-        members_to_add.extend(to_ids);
-        members_to_add.insert(ContactId::SELF);
-
-        if !from_id.is_special() {
-            members_to_add.insert(from_id);
-        }
-
-        if let Some(removed_id) = removed_id {
-            members_to_add.remove(&removed_id);
-        }
-
-        info!(
-            context,
-            "Recreating chat {chat_id} with members {members_to_add:?}."
-        );
-
-        chat::add_to_chat_contacts_table(context, chat_id, &Vec::from_iter(members_to_add)).await?;
-        send_event_chat_modified = true;
     }
 
     if let Some(avatar_action) = &mime_parser.group_avatar {
-        if !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await? {
+        if !chat_contacts.contains(&ContactId::SELF) {
             warn!(
                 context,
                 "Received group avatar update for group chat {chat_id} we are not a member of."
             );
-        } else if !chat::is_contact_in_chat(context, chat_id, from_id).await? {
+        } else if !chat_contacts.contains(&from_id) {
             warn!(
                 context,
                 "Contact {from_id} attempts to modify group chat {chat_id} avatar without being a member.",

src/receive_imf/tests.rs

@@ -3369,20 +3369,15 @@ async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> {
 
     alice.recv_msg(&bob.pop_sent_msg().await).await;
 
-    // bob didn't receive the addition of fiona, but alice doesn't overwrite her own
-    // contact list with the one from bob which only has three members instead of four.
-    assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4);
-
-    // bob removes a member.
-    remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?;
-
-    alice.recv_msg(&bob.pop_sent_msg().await).await;
-
-    // Bobs chat only has two members after the removal of blue, because he still
-    // didn't receive the addition of fiona. But that doesn't overwrite alice'
-    // memberlist.
+    // Bob didn't receive the addition of Fiona, so Alice must remove Fiona from the members list
+    // back to make their group members view consistent.
     assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3);
 
+    // Just a dumb check for remove_contact_from_chat(). Let's have it in this only place.
+    remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?;
+    alice.recv_msg(&bob.pop_sent_msg().await).await;
+    assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 2);
+
     Ok(())
 }
 
@@ -3514,6 +3509,29 @@ async fn test_mua_cant_remove() -> Result<()> {
         chat::get_chat_contacts(&alice, group_chat.id).await?.len(),
         4
     );
+
+    // But if the parent message is missing, the message must goto a new ad-hoc group.
+    let bob_removes = receive_imf(
+        &alice,
+        b"Subject: Re: Message from alice\r\n\
+            From: <bob@example.net>\r\n\
+            To: <alice@example.org>, <claire@example.org>\r\n\
+            Date: Mon, 12 Dec 2022 14:32:40 +0000\r\n\
+            Message-ID: <bobs_answer_to_two_recipients_1@example.net>\r\n\
+            In-Reply-To: <Mr.missing@example.org>\r\n\
+            \r\n\
+            Hi back!\r\n",
+        false,
+    )
+    .await?
+    .unwrap();
+    assert_ne!(bob_removes.chat_id, alice_chat.id);
+    let group_chat = Chat::load_from_db(&alice, bob_removes.chat_id).await?;
+    assert_eq!(group_chat.typ, Chattype::Group);
+    assert_eq!(
+        chat::get_chat_contacts(&alice, group_chat.id).await?.len(),
+        3,
+    );
     Ok(())
 }