diff --git a/Cargo.lock b/Cargo.lock index 187a68a89..0e18940f5 100644 --- a/Cargo.lock +++ b/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", diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index decdd7faa..7e83e181f 100644 --- a/deltachat-ffi/deltachat.h +++ b/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. */ diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index 7789e386b..689300d23 100644 --- a/deltachat-ffi/src/lib.rs +++ b/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 diff --git a/python/src/deltachat/account.py b/python/src/deltachat/account.py index fde031564..b53bbe7a0 100644 --- a/python/src/deltachat/account.py +++ b/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: diff --git a/python/src/deltachat/contact.py b/python/src/deltachat/contact.py index 7b1ea4f7b..e28a31178 100644 --- a/python/src/deltachat/contact.py +++ b/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. diff --git a/python/src/deltachat/testplugin.py b/python/src/deltachat/testplugin.py index d736984b6..944982475 100644 --- a/python/src/deltachat/testplugin.py +++ b/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}") diff --git a/python/tests/test_0_complex_or_slow.py b/python/tests/test_0_complex_or_slow.py index dd49cf8b7..50ae55549 100644 --- a/python/tests/test_0_complex_or_slow.py +++ b/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") diff --git a/python/tests/test_3_offline.py b/python/tests/test_3_offline.py index ad30b7f55..4b040c65a 100644 --- a/python/tests/test_3_offline.py +++ b/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() diff --git a/src/key.rs b/src/key.rs index c03881c43..230f00f8c 100644 --- a/src/key.rs +++ b/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, diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 9fd04d18b..502a4fa10 100644 --- a/src/mimeparser.rs +++ b/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() diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 5e1f8782e..fc3896213 100644 --- a/src/receive_imf.rs +++ b/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 = + 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.", diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 616d27b52..89c21c2c8 100644 --- a/src/receive_imf/tests.rs +++ b/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: \r\n\ + To: , \r\n\ + Date: Mon, 12 Dec 2022 14:32:40 +0000\r\n\ + Message-ID: \r\n\ + In-Reply-To: \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(()) }