From 4781bd0c638a423ff7526a57ba37fd41832e2e80 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Thu, 21 Mar 2024 20:54:07 -0300 Subject: [PATCH] fix: Add tolerance to MemberListTimestamp (#5366) Let's add a 1-minute tolerance to `Params::MemberListTimestamp`. This adds to the group membership consistency algo the following properties: - If remote group membership changes were made by two members in parallel, both of them are applied, no matter in which order the messages are received. - If we remove a member locally, only explicit remote member additions/removals made in parallel are allowed, but not the synchronisation of the member list from "To". Before, if somebody managed to reply earlier than receiving our removal of a member, we added it back which doesn't look good. --- src/chat.rs | 45 ++++++++++---- src/constants.rs | 4 ++ src/mimefactory.rs | 6 -- src/mimeparser.rs | 6 +- src/receive_imf.rs | 60 +++++++++++++------ src/receive_imf/tests.rs | 17 +++++- .../chat_test_msg_with_implicit_member_add | 5 +- ...emove => chat_test_parallel_member_remove} | 5 +- 8 files changed, 104 insertions(+), 44 deletions(-) rename test-data/golden/{chat_test_simultaneous_member_remove => chat_test_parallel_member_remove} (67%) diff --git a/src/chat.rs b/src/chat.rs index 1af3feb281..0e33652cbf 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1172,6 +1172,15 @@ impl ChatId { Ok(self.get_param(context).await?.exists(Param::Devicetalk)) } + /// Returns chat member list timestamp. + pub(crate) async fn get_member_list_timestamp(self, context: &Context) -> Result { + Ok(self + .get_param(context) + .await? + .get_i64(Param::MemberListTimestamp) + .unwrap_or_default()) + } + async fn parent_query( self, context: &Context, @@ -2811,15 +2820,21 @@ pub(crate) async fn create_send_msg_jobs(context: &Context, msg: &mut Message) - ); } - let now = time(); + let now = smeared_time(context); if rendered_msg.is_gossiped { msg.chat_id.set_gossiped_timestamp(context, now).await?; } - if rendered_msg.is_group { + if msg.param.get_cmd() == SystemMessage::MemberRemovedFromGroup { + // Reject member list synchronisation from older messages. See also + // `receive_imf::apply_group_changes()`. msg.chat_id - .update_timestamp(context, Param::MemberListTimestamp, now) + .update_timestamp( + context, + Param::MemberListTimestamp, + now.saturating_add(constants::TIMESTAMP_SENT_TOLERANCE), + ) .await?; } @@ -4784,9 +4799,9 @@ mod tests { Ok(()) } - /// Test simultaneous removal of user from the chat and leaving the group. + /// Test parallel removal of user from the chat and leaving the group. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn test_simultaneous_member_remove() -> Result<()> { + async fn test_parallel_member_remove() -> Result<()> { let mut tcm = TestContextManager::new(); let alice = tcm.alice().await; @@ -4817,20 +4832,25 @@ mod tests { add_contact_to_chat(&alice, alice_chat_id, alice_claire_contact_id).await?; let alice_sent_add_msg = alice.pop_sent_msg().await; - // Alice removes Bob from the chat. - remove_contact_from_chat(&alice, alice_chat_id, alice_bob_contact_id).await?; - let alice_sent_remove_msg = alice.pop_sent_msg().await; - // Bob leaves the chat. remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?; // Bob receives a msg about Alice adding Claire to the group. bob.recv_msg(&alice_sent_add_msg).await; + SystemTime::shift(Duration::from_secs(3600)); + // This adds Bob because they left quite long ago. + let alice_sent_msg = alice.send_text(alice_chat_id, "What a silence!").await; + bob.recv_msg(&alice_sent_msg).await; + // Test that add message is rewritten. - bob.golden_test_chat(bob_chat_id, "chat_test_simultaneous_member_remove") + bob.golden_test_chat(bob_chat_id, "chat_test_parallel_member_remove") .await; + // Alice removes Bob from the chat. + remove_contact_from_chat(&alice, alice_chat_id, alice_bob_contact_id).await?; + let alice_sent_remove_msg = alice.pop_sent_msg().await; + // Bob receives a msg about Alice removing him from the group. let bob_received_remove_msg = bob.recv_msg(&alice_sent_remove_msg).await; @@ -4867,8 +4887,13 @@ mod tests { bob.recv_msg(&sent_msg).await; remove_contact_from_chat(&bob, bob_chat_id, bob_fiona_contact_id).await?; + // This doesn't add Fiona back because Bob just removed them. let sent_msg = alice.send_text(alice_chat_id, "Welcome, Fiona!").await; bob.recv_msg(&sent_msg).await; + + SystemTime::shift(Duration::from_secs(3600)); + let sent_msg = alice.send_text(alice_chat_id, "Welcome back, Fiona!").await; + bob.recv_msg(&sent_msg).await; bob.golden_test_chat(bob_chat_id, "chat_test_msg_with_implicit_member_add") .await; Ok(()) diff --git a/src/constants.rs b/src/constants.rs index a8747b9c6c..ce30520b70 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -219,6 +219,10 @@ pub(crate) const DEFAULT_MAX_SMTP_RCPT_TO: usize = 50; /// How far the last quota check needs to be in the past to be checked by the background function (in seconds). pub(crate) const DC_BACKGROUND_FETCH_QUOTA_CHECK_RATELIMIT: u64 = 12 * 60 * 60; // 12 hours +/// How far in the future the sender timestamp of a message is allowed to be, in seconds. Also used +/// in the group membership consistency algo to reject outdated membership changes. +pub(crate) const TIMESTAMP_SENT_TOLERANCE: i64 = 60; + #[cfg(test)] mod tests { use num_traits::FromPrimitive; diff --git a/src/mimefactory.rs b/src/mimefactory.rs index ddb69fadd0..1dd65ce36b 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -93,7 +93,6 @@ 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 @@ -596,8 +595,6 @@ 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); @@ -605,8 +602,6 @@ impl<'a> MimeFactory<'a> { "List-ID".into(), format!("{encoded_chat_name} <{}>", chat.grpid), )); - } else if chat.typ == Chattype::Group { - is_group = true; } } @@ -878,7 +873,6 @@ 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/mimeparser.rs b/src/mimeparser.rs index 177118f17e..48cf173e1e 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -17,7 +17,7 @@ use crate::aheader::{Aheader, EncryptPreference}; use crate::blob::BlobObject; use crate::chat::{add_info_msg, ChatId}; use crate::config::Config; -use crate::constants::{Chattype, DC_DESIRED_TEXT_LINES, DC_DESIRED_TEXT_LINE_LEN}; +use crate::constants::{self, Chattype, DC_DESIRED_TEXT_LINES, DC_DESIRED_TEXT_LINE_LEN}; use crate::contact::{addr_cmp, addr_normalize, Contact, ContactId, Origin}; use crate::context::Context; use crate::decrypt::{ @@ -214,7 +214,9 @@ impl MimeMessage { .headers .get_header_value(HeaderDef::Date) .and_then(|v| mailparse::dateparse(&v).ok()) - .map_or(timestamp_rcvd, |value| min(value, timestamp_rcvd + 60)); + .map_or(timestamp_rcvd, |value| { + min(value, timestamp_rcvd + constants::TIMESTAMP_SENT_TOLERANCE) + }); let mut hop_info = parse_receive_headers(&mail.get_headers()); let mut headers = Default::default(); diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 53497276ed..020fda18b4 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -11,7 +11,7 @@ use regex::Regex; use crate::aheader::EncryptPreference; use crate::chat::{self, Chat, ChatId, ChatIdBlocked, ProtectionStatus}; use crate::config::Config; -use crate::constants::{Blocked, Chattype, ShowEmails, DC_CHAT_ID_TRASH}; +use crate::constants::{self, Blocked, Chattype, ShowEmails, DC_CHAT_ID_TRASH}; use crate::contact::{ addr_cmp, may_be_valid_addr, normalize_name, Contact, ContactAddress, ContactId, Origin, }; @@ -1912,18 +1912,24 @@ async fn apply_group_changes( 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_partial_download - && is_from_in_chat - && chat_id - .update_timestamp( - context, - Param::MemberListTimestamp, - mime_parser.timestamp_sent, - ) - .await?; - + let member_list_ts = match !is_partial_download && is_from_in_chat { + true => Some(chat_id.get_member_list_timestamp(context).await?), + false => None, + }; + // When we remove a member locally, we shift `MemberListTimestamp` by `TIMESTAMP_SENT_TOLERANCE` + // into the future, so add some more tolerance here to allow remote membership changes as well. + let timestamp_sent_tolerance = constants::TIMESTAMP_SENT_TOLERANCE * 2; + let allow_member_list_changes = member_list_ts + .filter(|t| { + *t <= mime_parser + .timestamp_sent + .saturating_add(timestamp_sent_tolerance) + }) + .is_some(); + let sync_member_list = member_list_ts + .filter(|t| *t <= mime_parser.timestamp_sent) + .is_some(); // Whether to rebuild the member list from scratch. let recreate_member_list = { // Always recreate membership list if SELF has been added. The older versions of DC @@ -1938,15 +1944,16 @@ async fn apply_group_changes( .is_none(), None => false, } - } && { - if !allow_member_list_changes { + } && ( + // Don't allow the timestamp tolerance here for more reliable leaving of groups. + sync_member_list || { info!( context, "Ignoring a try to recreate member list of {chat_id} by {from_id}.", ); + false } - allow_member_list_changes - }; + ); if mime_parser.get_header(HeaderDef::ChatVerified).is_some() { if let VerifiedEncryption::NotVerified(err) = verified_encryption { @@ -2059,6 +2066,13 @@ async fn apply_group_changes( } if !recreate_member_list { + let mut diff = HashSet::::new(); + if sync_member_list { + diff = new_members.difference(&chat_contacts).copied().collect(); + } else if let Some(added_id) = added_id { + diff.insert(added_id); + } + new_members = chat_contacts.clone(); // Don't delete any members locally, but instead add absent ones to provide group // membership consistency for all members: // - Classical MUA users usually don't intend to remove users from an email thread, so @@ -2070,9 +2084,6 @@ async fn apply_group_changes( // 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. - let mut diff: HashSet = - new_members.difference(&chat_contacts).copied().collect(); - new_members = chat_contacts.clone(); new_members.extend(diff.clone()); if let Some(added_id) = added_id { diff.remove(&added_id); @@ -2108,6 +2119,17 @@ async fn apply_group_changes( chat_contacts = new_members; send_event_chat_modified = true; } + if sync_member_list { + let mut ts = mime_parser.timestamp_sent; + if recreate_member_list { + // Reject all older membership changes. See `allow_member_list_changes` to know how + // this works. + ts += timestamp_sent_tolerance; + } + chat_id + .update_timestamp(context, Param::MemberListTimestamp, ts) + .await?; + } } if let Some(avatar_action) = &mime_parser.group_avatar { diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index d4f19f71b9..24ae7e912d 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -3699,6 +3699,7 @@ async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> { alice.recv_msg(&bob.pop_sent_msg().await).await; assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3); + SystemTime::shift(Duration::from_secs(3600)); send_text_msg( &alice, alice_chat_id, @@ -3781,17 +3782,18 @@ async fn test_dont_readd_with_normal_msg() -> Result<()> { remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?; assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 1); + SystemTime::shift(Duration::from_secs(3600)); add_contact_to_chat( &alice, alice_chat_id, Contact::create(&alice, "fiora", "fiora@example.net").await?, ) .await?; - bob.recv_msg(&alice.pop_sent_msg().await).await; - // Alice didn't receive Bob's leave message, so Bob must readd themselves otherwise other - // members would think Bob is still here while they aren't, and then retry to leave if they + // Alice didn't receive Bob's leave message although a lot of time has + // passed, so Bob must readd themselves otherwise other members would think + // Bob is still here while they aren't. Bob should retry to leave if they // think that Alice didn't re-add them on purpose (which is possible if Alice uses a classical // MUA). assert!(is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); @@ -4011,6 +4013,15 @@ async fn test_recreate_member_list_on_missing_add_of_self() -> Result<()> { // Bob missed the message adding them, but must recreate the member list. assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); assert!(is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); + + // But if Bob just left, they mustn't recreate the member list even after missing a message. + bob_chat_id.accept(&bob).await?; + remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?; + send_text_msg(&alice, alice_chat_id, "3rd message".to_string()).await?; + alice.pop_sent_msg().await; + send_text_msg(&alice, alice_chat_id, "4th message".to_string()).await?; + bob.recv_msg(&alice.pop_sent_msg().await).await; + assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); Ok(()) } diff --git a/test-data/golden/chat_test_msg_with_implicit_member_add b/test-data/golden/chat_test_msg_with_implicit_member_add index 41f3677e34..68760fa9fb 100644 --- a/test-data/golden/chat_test_msg_with_implicit_member_add +++ b/test-data/golden/chat_test_msg_with_implicit_member_add @@ -3,6 +3,7 @@ Group#Chat#10: Group chat [3 member(s)] Msg#10: (Contact#Contact#11): I created a group [FRESH] Msg#11: (Contact#Contact#11): Member Fiona (fiona@example.net) added by alice@example.org. [FRESH][INFO] Msg#12: Me (Contact#Contact#Self): You removed member Fiona (fiona@example.net). [INFO] o -Msg#13: info (Contact#Contact#Info): Member Fiona (fiona@example.net) added. [NOTICED][INFO] -Msg#14: (Contact#Contact#11): Welcome, Fiona! [FRESH] +Msg#13: (Contact#Contact#11): Welcome, Fiona! [FRESH] +Msg#14: info (Contact#Contact#Info): Member Fiona (fiona@example.net) added. [NOTICED][INFO] +Msg#15: (Contact#Contact#11): Welcome back, Fiona! [FRESH] -------------------------------------------------------------------------------- diff --git a/test-data/golden/chat_test_simultaneous_member_remove b/test-data/golden/chat_test_parallel_member_remove similarity index 67% rename from test-data/golden/chat_test_simultaneous_member_remove rename to test-data/golden/chat_test_parallel_member_remove index ae35ed627d..015a979374 100644 --- a/test-data/golden/chat_test_simultaneous_member_remove +++ b/test-data/golden/chat_test_parallel_member_remove @@ -2,6 +2,7 @@ Group#Chat#10: Group chat [4 member(s)] -------------------------------------------------------------------------------- Msg#10: (Contact#Contact#10): Hi! I created a group. [FRESH] Msg#11: Me (Contact#Contact#Self): You left the group. [INFO] o -Msg#12: info (Contact#Contact#Info): Member Me (bob@example.net) added. [NOTICED][INFO] -Msg#13: (Contact#Contact#10): Member claire@example.net added by alice@example.org. [FRESH][INFO] +Msg#12: (Contact#Contact#10): Member claire@example.net added by alice@example.org. [FRESH][INFO] +Msg#13: info (Contact#Contact#Info): Member Me (bob@example.net) added. [NOTICED][INFO] +Msg#14: (Contact#Contact#10): What a silence! [FRESH] --------------------------------------------------------------------------------