From 33777d87596691d2264c57cf1fc915071a70514d Mon Sep 17 00:00:00 2001 From: iequidoo Date: Wed, 13 Mar 2024 21:59:10 -0300 Subject: [PATCH] fix: Update MemberListTimestamp when sending a group message `Param::MemberListTimestamp` was updated only from `receive_imf::apply_group_changes()` i.e. for received messages. If we sent a message, that timestamp wasn't updated, so remote group membership changes always overrode local ones. Especially that was a problem when a message is sent offline so that it doesn't incorporate recent group membership changes. --- src/chat.rs | 6 +++++ src/mimefactory.rs | 6 +++++ src/receive_imf/tests.rs | 54 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index c3b7326a53..fc709e5131 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2805,6 +2805,12 @@ pub(crate) async fn create_send_msg_jobs(context: &Context, msg: &mut Message) - msg.chat_id.set_gossiped_timestamp(context, now).await?; } + if rendered_msg.is_group { + msg.chat_id + .update_timestamp(context, Param::MemberListTimestamp, now) + .await?; + } + if let Some(last_added_location_id) = rendered_msg.last_added_location_id { if let Err(err) = location::set_kml_sent_timestamp(context, msg.chat_id, now).await { error!(context, "Failed to set kml sent_timestamp: {err:#}."); diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 6eebceafa2..0c7620223a 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -85,6 +85,7 @@ pub struct RenderedEmail { // pub envelope: Envelope, pub is_encrypted: bool, pub is_gossiped: bool, + pub is_group: bool, pub last_added_location_id: Option, /// A comma-separated string of sync-IDs that are used by the rendered email @@ -587,6 +588,8 @@ impl<'a> MimeFactory<'a> { )); } + let mut is_group = false; + if let Loaded::Message { chat } = &self.loaded { if chat.typ == Chattype::Broadcast { let encoded_chat_name = encode_words(&chat.name); @@ -594,6 +597,8 @@ impl<'a> MimeFactory<'a> { "List-ID".into(), format!("{encoded_chat_name} <{}>", chat.grpid), )); + } else if chat.typ == Chattype::Group { + is_group = true; } } @@ -865,6 +870,7 @@ impl<'a> MimeFactory<'a> { // envelope: Envelope::new, is_encrypted, is_gossiped, + is_group, last_added_location_id, sync_ids_to_delete: self.sync_ids_to_delete, rfc724_mid, diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index fd60bd7e90..50d0daaeba 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + use tokio::fs; use super::*; @@ -12,6 +14,7 @@ use crate::download::MIN_DOWNLOAD_LIMIT; use crate::imap::prefetch_should_download; use crate::imex::{imex, ImexMode}; use crate::test_utils::{get_chat_msg, TestContext, TestContextManager}; +use crate::tools::SystemTime; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_grpid_simple() { @@ -3608,6 +3611,52 @@ async fn test_sync_member_list_on_rejoin() -> Result<()> { Ok(()) } +/// Test for the bug when remote group membership changes from outdated messages overrode local +/// ones. Especially that was a problem when a message is sent offline so that it doesn't +/// incorporate recent group membership changes. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_ignore_outdated_membership_changes() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let alice_bob_id = + Contact::create(alice, "", &bob.get_config(Config::Addr).await?.unwrap()).await?; + let alice_chat_id = create_group_chat(alice, ProtectionStatus::Unprotected, "grp").await?; + + // Alice creates a group chat. Bob accepts it. + add_contact_to_chat(alice, alice_chat_id, alice_bob_id).await?; + send_text_msg(alice, alice_chat_id, "populate".to_string()).await?; + let msg = &alice.pop_sent_msg().await; + bob.recv_msg(msg).await; + let bob_chat_id = bob.get_last_msg().await.chat_id; + bob_chat_id.accept(bob).await?; + + // Bob replies. + send_text_msg(bob, bob_chat_id, "i'm bob".to_string()).await?; + let msg = &bob.pop_sent_msg().await; + + SystemTime::shift(Duration::from_secs(3600)); + + // Alice leaves. + remove_contact_from_chat(alice, alice_chat_id, ContactId::SELF).await?; + alice.pop_sent_msg().await; + assert!(!is_contact_in_chat(alice, alice_chat_id, ContactId::SELF).await?); + + // Alice receives Bob's message, but it's outdated to add Alice back. + alice.recv_msg(msg).await; + assert!(!is_contact_in_chat(alice, alice_chat_id, ContactId::SELF).await?); + + SystemTime::shift(Duration::from_secs(3600)); + + // Bob replies again adding Alice back. + send_text_msg(bob, bob_chat_id, "i'm bob".to_string()).await?; + let msg = &bob.pop_sent_msg().await; + alice.recv_msg(msg).await; + assert!(is_contact_in_chat(alice, alice_chat_id, ContactId::SELF).await?); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> { let alice = TestContext::new_alice().await; @@ -3891,7 +3940,6 @@ async fn test_mua_can_readd() -> Result<()> { // And leaves it. remove_contact_from_chat(&alice, alice_chat.id, ContactId::SELF).await?; - let alice_chat = Chat::load_from_db(&alice, alice_chat.id).await?; assert!(!is_contact_in_chat(&alice, alice_chat.id, ContactId::SELF).await?); // Bob uses a classical MUA to answer, adding Alice back. @@ -3900,7 +3948,7 @@ async fn test_mua_can_readd() -> Result<()> { b"Subject: Re: Message from alice\r\n\ From: \r\n\ To: , , \r\n\ - Date: Mon, 12 Dec 2022 14:32:39 +0000\r\n\ + Date: Mon, 12 Dec 3000 14:32:39 +0000\r\n\ Message-ID: \r\n\ In-Reply-To: \r\n\ \r\n\ @@ -3909,8 +3957,6 @@ async fn test_mua_can_readd() -> Result<()> { ) .await? .unwrap(); - - let alice_chat = Chat::load_from_db(&alice, alice_chat.id).await?; assert!(is_contact_in_chat(&alice, alice_chat.id, ContactId::SELF).await?); Ok(()) }