From c7d6b6cfb71f7e7b3d5b16b634b408bc8c65525e Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 11 Apr 2024 05:39:51 +0000 Subject: [PATCH 1/2] fix: assign messages to chats based on not fully downloaded references --- deltachat-rpc-client/tests/test_something.py | 37 ++++++++++++++++++++ src/chat.rs | 15 +------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index fbf869d1ab..8ca9abad6d 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/deltachat-rpc-client/tests/test_something.py @@ -538,3 +538,40 @@ def test_reactions_for_a_reordering_move(acfactory): assert len(contacts) == 1 assert contacts[0].get_snapshot().address == ac1.get_config("addr") assert list(reactions.reactions_by_contact.values())[0] == [react_str] + + +def test_download_limit_chat_assignment(acfactory, tmp_path): + download_limit = 300000 + alice, bob, carol = acfactory.get_online_accounts(3) + + for account in bob, carol: + chat = account.create_chat(alice) + chat.send_text("Hello Alice!") + assert alice.get_message_by_id(alice.wait_for_incoming_msg_event().msg_id).get_snapshot().text == "Hello Alice!" + + bob_addr = bob.get_config("addr") + alice_contact_bob = alice.create_contact(bob_addr, "Bob") + + carol_addr = carol.get_config("addr") + alice_contact_carol = alice.create_contact(carol_addr, "Carol") + + alice_group = alice.create_group("test group") + alice_group.add_contact(alice_contact_bob) + alice_group.add_contact(alice_contact_carol) + + bob.set_config("download_limit", str(download_limit)) + + alice_group.send_text("hi") + snapshot = bob.get_message_by_id(bob.wait_for_incoming_msg_event().msg_id).get_snapshot() + assert snapshot.text == "hi" + bob_group = snapshot.chat + + path = tmp_path / "large" + path.write_bytes(os.urandom(download_limit + 1)) + + for i in range(10): + logging.info("Sending message %s", i) + alice_group.send_file(str(path)) + snapshot = bob.get_message_by_id(bob.wait_for_incoming_msg_event().msg_id).get_snapshot() + assert snapshot.download_state == DownloadState.AVAILABLE + assert snapshot.chat == bob_group diff --git a/src/chat.rs b/src/chat.rs index edb7d82097..b3616753d6 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -209,24 +209,11 @@ impl ChatId { } /// Returns [`ChatId`] of a chat that `msg` belongs to. - /// - /// Checks that `msg` is assigned to the right chat. pub(crate) fn lookup_by_message(msg: &Message) -> Option { if msg.chat_id == DC_CHAT_ID_TRASH { return None; } - if msg.download_state != DownloadState::Done - // TODO (2023-09-12): Added for backward compatibility with versions that did not have - // `DownloadState::Undecipherable`. Remove eventually with the comment in - // `MimeMessage::from_bytes()`. - || msg - .error - .as_ref() - .filter(|e| e.starts_with("Decrypting failed:")) - .is_some() - { - // If `msg` is not fully downloaded or undecipherable, it may have been assigned to the - // wrong chat (they often get assigned to the 1:1 chat with the sender). + if msg.download_state == DownloadState::Undecipherable { return None; } Some(msg.chat_id) From 18714eebe74bf7beb4f3b26c45b8baa97db4115d Mon Sep 17 00:00:00 2001 From: link2xt Date: Thu, 11 Apr 2024 19:02:55 +0000 Subject: [PATCH 2/2] fix: do not create ad-hoc groups from partial downloads --- deltachat-rpc-client/tests/test_something.py | 33 +++++++++++++------- src/receive_imf.rs | 17 +++++++--- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index 8ca9abad6d..9c53a078d5 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/deltachat-rpc-client/tests/test_something.py @@ -540,25 +540,26 @@ def test_reactions_for_a_reordering_move(acfactory): assert list(reactions.reactions_by_contact.values())[0] == [react_str] -def test_download_limit_chat_assignment(acfactory, tmp_path): +@pytest.mark.parametrize("n_accounts", [3, 2]) +def test_download_limit_chat_assignment(acfactory, tmp_path, n_accounts): download_limit = 300000 - alice, bob, carol = acfactory.get_online_accounts(3) - for account in bob, carol: + alice, *others = acfactory.get_online_accounts(n_accounts) + bob = others[0] + + alice_group = alice.create_group("test group") + for account in others: chat = account.create_chat(alice) chat.send_text("Hello Alice!") assert alice.get_message_by_id(alice.wait_for_incoming_msg_event().msg_id).get_snapshot().text == "Hello Alice!" - bob_addr = bob.get_config("addr") - alice_contact_bob = alice.create_contact(bob_addr, "Bob") + contact_addr = account.get_config("addr") + contact = alice.create_contact(contact_addr, "") - carol_addr = carol.get_config("addr") - alice_contact_carol = alice.create_contact(carol_addr, "Carol") - - alice_group = alice.create_group("test group") - alice_group.add_contact(alice_contact_bob) - alice_group.add_contact(alice_contact_carol) + alice_group.add_contact(contact) + if n_accounts == 2: + bob_chat_alice = bob.create_chat(alice) bob.set_config("download_limit", str(download_limit)) alice_group.send_text("hi") @@ -574,4 +575,12 @@ def test_download_limit_chat_assignment(acfactory, tmp_path): alice_group.send_file(str(path)) snapshot = bob.get_message_by_id(bob.wait_for_incoming_msg_event().msg_id).get_snapshot() assert snapshot.download_state == DownloadState.AVAILABLE - assert snapshot.chat == bob_group + if n_accounts > 2: + assert snapshot.chat == bob_group + else: + # Group contains only Alice and Bob, + # so partially downloaded messages are + # hard to distinguish from private replies to group messages. + # + # Message may be a private reply, so we assign it to 1:1 chat with Alice. + assert snapshot.chat == bob_chat_alice diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 10ba2bb7de..ac1b5e4c27 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1749,7 +1749,19 @@ async fn create_or_lookup_group( ) -> Result> { let grpid = if let Some(grpid) = try_getting_grpid(mime_parser) { grpid - } else if allow_creation { + } else if !allow_creation { + info!(context, "Creating ad-hoc group prevented from caller."); + return Ok(None); + } else if is_partial_download { + // Partial download may be an encrypted message with protected Subject header. + // + // We do not want to create a group with "..." or "Encrypted message" as a subject. + info!( + context, + "Ad-hoc group cannot be created from partial download." + ); + return Ok(None); + } else { let mut member_ids: Vec = to_ids.to_vec(); if !member_ids.contains(&(from_id)) { member_ids.push(from_id); @@ -1763,9 +1775,6 @@ async fn create_or_lookup_group( .context("could not create ad hoc group")? .map(|chat_id| (chat_id, create_blocked)); return Ok(res); - } else { - info!(context, "Creating ad-hoc group prevented from caller."); - return Ok(None); }; let mut chat_id;