From de63527d940b0304d3095b4cf940eec4ef02c28d Mon Sep 17 00:00:00 2001 From: link2xt Date: Sat, 4 Jan 2025 06:19:28 +0000 Subject: [PATCH] feat: new group consistency algorithm This implements new group consistency algorithm described in New `Chat-Group-Member-Timestamps` header is added to send timestamps of member additions and removals. Member is part of the chat if its addition timestamp is greater or equal to the removal timestamp. --- src/chat.rs | 266 +++++++---- src/chatlist.rs | 4 +- src/contact.rs | 3 +- src/headerdef.rs | 9 + src/mimefactory.rs | 239 +++++++--- src/mimeparser.rs | 36 +- src/param.rs | 2 + src/peerstate.rs | 22 +- src/receive_imf.rs | 432 ++++++++++-------- src/receive_imf/tests.rs | 169 +++---- src/securejoin/bob.rs | 9 +- src/sql/migrations.rs | 18 + src/test_utils.rs | 3 +- .../chat_test_msg_with_implicit_member_add | 9 - .../golden/chat_test_parallel_member_remove | 5 +- ...=> receive_imf_delayed_removal_is_ignored} | 1 + ...ecreate_member_list_on_missing_add_of_self | 9 - 17 files changed, 750 insertions(+), 486 deletions(-) delete mode 100644 test-data/golden/chat_test_msg_with_implicit_member_add rename test-data/golden/{receive_imf_recreate_contact_list_on_missing_messages => receive_imf_delayed_removal_is_ignored} (84%) delete mode 100644 test-data/golden/receive_imf_recreate_member_list_on_missing_add_of_self diff --git a/src/chat.rs b/src/chat.rs index 99769fbcf0..986debcab4 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -364,8 +364,8 @@ impl ChatId { .sql .execute( "UPDATE contacts - SET selfavatar_sent=? - WHERE id IN(SELECT contact_id FROM chats_contacts WHERE chat_id=?);", + SET selfavatar_sent=? + WHERE id IN(SELECT contact_id FROM chats_contacts WHERE chat_id=? AND add_timestamp >= remove_timestamp)", (timestamp, self), ) .await?; @@ -1080,6 +1080,8 @@ impl ChatId { JOIN chats_contacts as y WHERE x.contact_id > 9 AND y.contact_id > 9 + AND x.add_timestamp >= x.remove_timestamp + AND y.add_timestamp >= y.remove_timestamp AND x.chat_id=? AND y.chat_id<>x.chat_id AND y.chat_id>? @@ -1104,6 +1106,7 @@ impl ChatId { "SELECT chat_id, count(*) AS n FROM chats_contacts WHERE contact_id > ? AND chat_id > ? + AND add_timestamp >= remove_timestamp GROUP BY chat_id", (ContactId::LAST_SPECIAL, DC_CHAT_ID_LAST_SPECIAL), |row| { @@ -1218,15 +1221,6 @@ 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, @@ -2232,7 +2226,7 @@ impl Chat { "SELECT c.addr \ FROM contacts c INNER JOIN chats_contacts cc \ ON c.id=cc.contact_id \ - WHERE cc.chat_id=?", + WHERE cc.chat_id=? AND cc.add_timestamp >= cc.remove_timestamp", (self.id,), |row| row.get::<_, String>(0), |addrs| addrs.collect::, _>>().map_err(Into::into), @@ -2780,7 +2774,9 @@ pub async fn is_contact_in_chat( let exists = context .sql .exists( - "SELECT COUNT(*) FROM chats_contacts WHERE chat_id=? AND contact_id=?;", + "SELECT COUNT(*) FROM chats_contacts + WHERE chat_id=? AND contact_id=? + AND add_timestamp >= remove_timestamp", (chat_id, contact_id), ) .await?; @@ -2854,19 +2850,26 @@ async fn prepare_send_msg( ) -> Result> { let mut chat = Chat::load_from_db(context, chat_id).await?; - // Check if the chat can be sent to. - if let Some(reason) = chat.why_cant_send(context).await? { - if matches!( - reason, - CantSendReason::ProtectionBroken - | CantSendReason::ContactRequest - | CantSendReason::SecurejoinWait - ) && msg.param.get_cmd() == SystemMessage::SecurejoinMessage - { - // Send out the message, the securejoin message is supposed to repair the verification. - // If the chat is a contact request, let the user accept it later. - } else { - bail!("cannot send to {chat_id}: {reason}"); + // Check if the chat can be sent to, + // but always allow to send "Member removed" messages + // so we can leave the group. + // + // Necessary checks should be made anyway before removing contact + // from the chat. + if msg.param.get_cmd() != SystemMessage::MemberRemovedFromGroup { + if let Some(reason) = chat.why_cant_send(context).await? { + if matches!( + reason, + CantSendReason::ProtectionBroken + | CantSendReason::ContactRequest + | CantSendReason::SecurejoinWait + ) && msg.param.get_cmd() == SystemMessage::SecurejoinMessage + { + // Send out the message, the securejoin message is supposed to repair the verification. + // If the chat is a contact request, let the user accept it later. + } else { + bail!("cannot send to {chat_id}: {reason}"); + } } } @@ -2996,18 +2999,6 @@ pub(crate) async fn create_send_msg_jobs(context: &Context, msg: &mut Message) - msg.chat_id.set_gossiped_timestamp(context, now).await?; } - 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.saturating_add(constants::TIMESTAMP_SENT_TOLERANCE), - ) - .await?; - } - if rendered_msg.last_added_location_id.is_some() { if let Err(err) = location::set_kml_sent_timestamp(context, msg.chat_id, now).await { error!(context, "Failed to set kml sent_timestamp: {err:#}."); @@ -3435,7 +3426,7 @@ pub async fn get_chat_contacts(context: &Context, chat_id: ChatId) -> Result= cc.remove_timestamp ORDER BY c.id=1, c.last_seen DESC, c.id DESC;", (chat_id,), |row| row.get::<_, ContactId>(0), @@ -3446,6 +3437,26 @@ pub async fn get_chat_contacts(context: &Context, chat_id: ChatId) -> Result Result> { + let list = context + .sql + .query_map( + "SELECT cc.contact_id + FROM chats_contacts cc + LEFT JOIN contacts c + ON c.id=cc.contact_id + WHERE cc.chat_id=? AND cc.add_timestamp < cc.remove_timestamp + ORDER BY c.id=1, c.last_seen DESC, c.id DESC", + (chat_id,), + |row| row.get::<_, ContactId>(0), + |ids| ids.collect::, _>>().map_err(Into::into), + ) + .await?; + + Ok(list) +} + /// Creates a group chat with a given `name`. pub async fn create_group_chat( context: &Context, @@ -3470,7 +3481,7 @@ pub async fn create_group_chat( let chat_id = ChatId::new(u32::try_from(row_id)?); if !is_contact_in_chat(context, chat_id, ContactId::SELF).await? { - add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?; + add_to_chat_contacts_table(context, timestamp, chat_id, &[ContactId::SELF]).await?; } context.emit_msgs_changed_without_ids(); @@ -3577,18 +3588,37 @@ pub(crate) async fn create_broadcast_list_ex( /// Set chat contacts in the `chats_contacts` table. pub(crate) async fn update_chat_contacts_table( context: &Context, + timestamp: i64, id: ChatId, contacts: &HashSet, ) -> Result<()> { context .sql .transaction(move |transaction| { - transaction.execute("DELETE FROM chats_contacts WHERE chat_id=?", (id,))?; - for contact_id in contacts { - transaction.execute( - "INSERT INTO chats_contacts (chat_id, contact_id) VALUES(?, ?)", - (id, contact_id), + // Bump `remove_timestamp` to at least `now` + // even for members from `contacts`. + // We add members from `contacts` back below. + transaction.execute( + "UPDATE chats_contacts + SET remove_timestamp=MAX(add_timestamp+1, ?) + WHERE chat_id=?", + (timestamp, id), + )?; + + if !contacts.is_empty() { + let mut statement = transaction.prepare( + "INSERT INTO chats_contacts (chat_id, contact_id, add_timestamp) + VALUES (?1, ?2, ?3) + ON CONFLICT (chat_id, contact_id) + DO UPDATE SET add_timestamp=remove_timestamp", )?; + + for contact_id in contacts { + // We bumped `add_timestamp` for existing rows above, + // so on conflict it is enough to set `add_timestamp = remove_timestamp` + // and this guarantees that `add_timestamp` is no less than `timestamp`. + statement.execute((id, contact_id, timestamp))?; + } } Ok(()) }) @@ -3599,17 +3629,21 @@ pub(crate) async fn update_chat_contacts_table( /// Adds contacts to the `chats_contacts` table. pub(crate) async fn add_to_chat_contacts_table( context: &Context, + timestamp: i64, chat_id: ChatId, contact_ids: &[ContactId], ) -> Result<()> { context .sql .transaction(move |transaction| { + let mut add_statement = transaction.prepare( + "INSERT INTO chats_contacts (chat_id, contact_id, add_timestamp) VALUES(?1, ?2, ?3) + ON CONFLICT (chat_id, contact_id) + DO UPDATE SET add_timestamp=MAX(remove_timestamp, ?3)", + )?; + for contact_id in contact_ids { - transaction.execute( - "INSERT OR IGNORE INTO chats_contacts (chat_id, contact_id) VALUES(?, ?)", - (chat_id, contact_id), - )?; + add_statement.execute((chat_id, contact_id, timestamp))?; } Ok(()) }) @@ -3618,17 +3652,21 @@ pub(crate) async fn add_to_chat_contacts_table( Ok(()) } -/// remove a contact from the chats_contact table +/// Removes a contact from the chat +/// by updating the `remove_timestamp`. pub(crate) async fn remove_from_chat_contacts_table( context: &Context, chat_id: ChatId, contact_id: ContactId, ) -> Result<()> { + let now = time(); context .sql .execute( - "DELETE FROM chats_contacts WHERE chat_id=? AND contact_id=?", - (chat_id, contact_id), + "UPDATE chats_contacts + SET remove_timestamp=MAX(add_timestamp+1, ?) + WHERE chat_id=? AND contact_id=?", + (now, chat_id, contact_id), ) .await?; Ok(()) @@ -3718,7 +3756,7 @@ pub(crate) async fn add_contact_to_chat_ex( if is_contact_in_chat(context, chat_id, contact_id).await? { return Ok(false); } - add_to_chat_contacts_table(context, chat_id, &[contact_id]).await?; + add_to_chat_contacts_table(context, time(), chat_id, &[contact_id]).await?; } if chat.typ == Chattype::Group && chat.is_promoted() { msg.viewtype = Viewtype::Text; @@ -3728,10 +3766,8 @@ pub(crate) async fn add_contact_to_chat_ex( msg.param.set_cmd(SystemMessage::MemberAddedToGroup); msg.param.set(Param::Arg, contact_addr); msg.param.set_int(Param::Arg2, from_handshake.into()); - if let Err(e) = send_msg(context, chat_id, &mut msg).await { - remove_from_chat_contacts_table(context, chat_id, contact_id).await?; - return Err(e); - } + send_msg(context, chat_id, &mut msg).await?; + sync = Nosync; // TODO: Remove this compat code needed because Core <= v1.143: // - doesn't accept synchronization of QR code tokens for unpromoted groups, so we also send @@ -3766,9 +3802,9 @@ pub(crate) async fn shall_attach_selfavatar(context: &Context, chat_id: ChatId) .sql .query_map( "SELECT c.selfavatar_sent - FROM chats_contacts cc - LEFT JOIN contacts c ON c.id=cc.contact_id - WHERE cc.chat_id=? AND cc.contact_id!=?;", + FROM chats_contacts cc + LEFT JOIN contacts c ON c.id=cc.contact_id + WHERE cc.chat_id=? AND cc.contact_id!=? AND cc.add_timestamp >= cc.remove_timestamp", (chat_id, ContactId::SELF), |row| Ok(row.get::<_, i64>(0)), |rows| { @@ -3895,6 +3931,9 @@ pub async fn remove_contact_from_chat( bail!("{}", err_msg); } else { let mut sync = Nosync; + + remove_from_chat_contacts_table(context, chat_id, contact_id).await?; + // We do not return an error if the contact does not exist in the database. // This allows to delete dangling references to deleted contacts // in case of the database becoming inconsistent due to a bug. @@ -3924,18 +3963,6 @@ pub async fn remove_contact_from_chat( sync = Sync; } } - // we remove the member from the chat after constructing the - // to-be-send message. If between send_msg() and here the - // process dies, the user will be able to redo the action. It's better than the other - // way round: you removed someone from DB but no peer or device gets to know about it - // and group membership is thus different on different devices. But if send_msg() - // failed, we still remove the member locally, otherwise it would be impossible to - // remove a member with missing key from a protected group. - // Note also that sending a message needs all recipients - // in order to correctly determine encryption so if we - // removed it first, it would complicate the - // check/encryption logic. - remove_from_chat_contacts_table(context, chat_id, contact_id).await?; context.emit_event(EventType::ChatModified(chat_id)); if sync.into() { chat.sync_contacts(context).await.log_err(context).ok(); @@ -4555,7 +4582,7 @@ async fn set_contacts_by_addrs(context: &Context, id: ChatId, addrs: &[String]) if contacts == contacts_old { return Ok(()); } - update_chat_contacts_table(context, id, &contacts).await?; + update_chat_contacts_table(context, time(), id, &contacts).await?; context.emit_event(EventType::ChatModified(id)); Ok(()) } @@ -5063,11 +5090,11 @@ mod tests { bob.recv_msg(&alice_sent_add_msg).await; SystemTime::shift(Duration::from_secs(3600)); - // This adds Bob because they left quite long ago. + + // Alice sends a message to Bob because the message about leaving is lost. 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_parallel_member_remove") .await; @@ -5087,9 +5114,9 @@ mod tests { Ok(()) } - /// Test that if a message implicitly adds a member, both messages appear. + /// Test that member removal is synchronized eventually even if the message is lost. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn test_msg_with_implicit_member_add() -> Result<()> { + async fn test_msg_with_implicit_member_removed() -> Result<()> { let mut tcm = TestContextManager::new(); let alice = tcm.alice().await; let bob = tcm.bob().await; @@ -5105,22 +5132,35 @@ mod tests { let bob_received_msg = bob.recv_msg(&sent_msg).await; let bob_chat_id = bob_received_msg.get_chat_id(); bob_chat_id.accept(&bob).await?; + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); add_contact_to_chat(&alice, alice_chat_id, alice_fiona_contact_id).await?; let sent_msg = alice.pop_sent_msg().await; bob.recv_msg(&sent_msg).await; + + // Bob removed Fiona, but the message is lost. remove_contact_from_chat(&bob, bob_chat_id, bob_fiona_contact_id).await?; bob.pop_sent_msg().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; + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); + // Even after some time Fiona is not added back. 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") + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); + + // If Bob sends a message to Alice now, Fiona is removed. + assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3); + let sent_msg = bob + .send_text(alice_chat_id, "I have removed Fiona some time ago.") .await; + alice.recv_msg(&sent_msg).await; + assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 2); + Ok(()) } @@ -5163,6 +5203,8 @@ mod tests { assert_eq!(a2_msg.get_info_type(), SystemMessage::MemberAddedToGroup); assert_eq!(get_chat_contacts(&a1, a1_chat_id).await?.len(), 2); assert_eq!(get_chat_contacts(&a2, a2_chat_id).await?.len(), 2); + assert_eq!(get_past_chat_contacts(&a1, a1_chat_id).await?.len(), 0); + assert_eq!(get_past_chat_contacts(&a2, a2_chat_id).await?.len(), 0); // rename the group set_chat_name(&a1, a1_chat_id, "bar").await?; @@ -5195,6 +5237,8 @@ mod tests { ); assert_eq!(get_chat_contacts(&a1, a1_chat_id).await?.len(), 1); assert_eq!(get_chat_contacts(&a2, a2_chat_id).await?.len(), 1); + assert_eq!(get_past_chat_contacts(&a1, a1_chat_id).await?.len(), 1); + assert_eq!(get_past_chat_contacts(&a2, a2_chat_id).await?.len(), 1); Ok(()) } @@ -5204,7 +5248,7 @@ mod tests { let _n = TimeShiftFalsePositiveNote; // Alice creates a group with Bob, Claire and Daisy and then removes Claire and Daisy - // (sleep() is needed as otherwise smeared time from Alice looks to Bob like messages from the future which are all set to "now" then) + // (time shift is needed as otherwise smeared time from Alice looks to Bob like messages from the future which are all set to "now" then) let alice = TestContext::new_alice().await; let bob_id = Contact::create(&alice, "", "bob@example.net").await?; @@ -5219,17 +5263,17 @@ mod tests { add_contact_to_chat(&alice, alice_chat_id, claire_id).await?; let add2 = alice.pop_sent_msg().await; - tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + SystemTime::shift(Duration::from_millis(1100)); add_contact_to_chat(&alice, alice_chat_id, daisy_id).await?; let add3 = alice.pop_sent_msg().await; - tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + SystemTime::shift(Duration::from_millis(1100)); assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4); remove_contact_from_chat(&alice, alice_chat_id, claire_id).await?; let remove1 = alice.pop_sent_msg().await; - tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + SystemTime::shift(Duration::from_millis(1100)); remove_contact_from_chat(&alice, alice_chat_id, daisy_id).await?; let remove2 = alice.pop_sent_msg().await; @@ -5279,7 +5323,8 @@ mod tests { /// Test that group updates are robust to lost messages and eventual out of order arrival. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_modify_chat_lost() -> Result<()> { - let alice = TestContext::new_alice().await; + let mut tcm = TestContextManager::new(); + let alice = tcm.alice().await; let bob_id = Contact::create(&alice, "", "bob@example.net").await?; let claire_id = Contact::create(&alice, "", "claire@foo.de").await?; @@ -5292,16 +5337,16 @@ mod tests { send_text_msg(&alice, alice_chat_id, "populate".to_string()).await?; let add = alice.pop_sent_msg().await; - tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + SystemTime::shift(Duration::from_millis(1100)); remove_contact_from_chat(&alice, alice_chat_id, claire_id).await?; let remove1 = alice.pop_sent_msg().await; - tokio::time::sleep(std::time::Duration::from_millis(1100)).await; + SystemTime::shift(Duration::from_millis(1100)); remove_contact_from_chat(&alice, alice_chat_id, daisy_id).await?; let remove2 = alice.pop_sent_msg().await; - let bob = TestContext::new_bob().await; + let bob = tcm.bob().await; bob.recv_msg(&add).await; let bob_chat_id = bob.get_last_msg().await.chat_id; @@ -5314,7 +5359,7 @@ mod tests { // Eventually, first removal message arrives. // This has no effect. - bob.recv_msg_trash(&remove1).await; + bob.recv_msg(&remove1).await; assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); Ok(()) } @@ -7716,6 +7761,53 @@ mod tests { self_chat.set_draft(&alice, Some(&mut msg)).await.unwrap(); let draft2 = self_chat.get_draft(&alice).await?.unwrap(); assert_eq!(draft1.timestamp_sort, draft2.timestamp_sort); + + Ok(()) + } + + /// Test group consistency. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_add_member_bug() -> Result<()> { + let mut tcm = TestContextManager::new(); + + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + let alice_bob_contact_id = Contact::create(alice, "Bob", "bob@example.net").await?; + let alice_fiona_contact_id = Contact::create(alice, "Fiona", "fiona@example.net").await?; + + // Create a group. + let alice_chat_id = + create_group_chat(alice, ProtectionStatus::Unprotected, "Group chat").await?; + add_contact_to_chat(alice, alice_chat_id, alice_bob_contact_id).await?; + add_contact_to_chat(alice, alice_chat_id, alice_fiona_contact_id).await?; + + // Promote the group. + let alice_sent_msg = alice + .send_text(alice_chat_id, "Hi! I created a group.") + .await; + let bob_received_msg = bob.recv_msg(&alice_sent_msg).await; + + let bob_chat_id = bob_received_msg.get_chat_id(); + bob_chat_id.accept(bob).await?; + + // Alice removes Fiona from the chat. + remove_contact_from_chat(alice, alice_chat_id, alice_fiona_contact_id).await?; + let _alice_sent_add_msg = alice.pop_sent_msg().await; + + SystemTime::shift(Duration::from_secs(3600)); + + // Bob sends a message + // to Alice and Fiona because he still has not received + // a message about Fiona being removed. + let bob_sent_msg = bob.send_text(bob_chat_id, "Hi Alice!").await; + + // Alice receives a message. + // This should not add Fiona back. + let _alice_received_msg = alice.recv_msg(&bob_sent_msg).await; + + assert_eq!(get_chat_contacts(alice, alice_chat_id).await?.len(), 2); + Ok(()) } } diff --git a/src/chatlist.rs b/src/chatlist.rs index 8f1c495379..85a6067fa6 100644 --- a/src/chatlist.rs +++ b/src/chatlist.rs @@ -144,7 +144,7 @@ impl Chatlist { ORDER BY timestamp DESC, id DESC LIMIT 1) WHERE c.id>9 AND c.blocked!=1 - AND c.id IN(SELECT chat_id FROM chats_contacts WHERE contact_id=?2) + AND c.id IN(SELECT chat_id FROM chats_contacts WHERE contact_id=?2 AND add_timestamp >= remove_timestamp) GROUP BY c.id ORDER BY c.archived=?3 DESC, IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", (MessageState::OutDraft, query_contact_id, ChatVisibility::Pinned), @@ -261,7 +261,7 @@ impl Chatlist { WHERE c.id>9 AND c.id!=? AND c.blocked=0 AND NOT c.archived=? - AND (c.type!=? OR c.id IN(SELECT chat_id FROM chats_contacts WHERE contact_id=?)) + AND (c.type!=? OR c.id IN(SELECT chat_id FROM chats_contacts WHERE contact_id=? AND add_timestamp >= remove_timestamp)) GROUP BY c.id ORDER BY c.id=? DESC, c.archived=? DESC, IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", ( diff --git a/src/contact.rs b/src/contact.rs index 06f153457c..c9270fe46a 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -114,7 +114,8 @@ impl ContactId { SET gossiped_timestamp=0 WHERE EXISTS (SELECT 1 FROM chats_contacts WHERE chats_contacts.chat_id=chats.id - AND chats_contacts.contact_id=?)", + AND chats_contacts.contact_id=? + AND chats_contacts.add_timestamp >= chats_contacts.remove_timestamp)", (self,), ) .await?; diff --git a/src/headerdef.rs b/src/headerdef.rs index 61d7746d86..1434b53232 100644 --- a/src/headerdef.rs +++ b/src/headerdef.rs @@ -65,6 +65,15 @@ pub enum HeaderDef { ChatGroupMemberAdded, ChatContent, + /// Past members of the group. + ChatGroupPastMembers, + + /// Space-separated timestamps of member addition + /// for members listed in the `To` field + /// followed by timestamps of member removal + /// for members listed in the `Chat-Group-Past-Members` field. + ChatGroupMemberTimestamps, + /// Duration of the attached media file. ChatDuration, diff --git a/src/mimefactory.rs b/src/mimefactory.rs index a1e2876f95..6a78a668ff 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -66,8 +66,36 @@ pub struct MimeFactory { selfstatus: String, - /// Vector of pairs of recipient name and address - recipients: Vec<(String, String)>, + /// Vector of actual recipient addresses. + /// + /// This is the list of addresses the message should be sent to. + /// It is not the same as the `To` header, + /// because in case of "member removed" message + /// removed member is in the recipient list, + /// but not in the `To` header. + /// In case of broadcast lists there are multiple recipients, + /// but the `To` header has no members. + /// + /// If `bcc_self` configuration is enabled, + /// this list will be extended with own address later, + /// but `MimeFactory` is not responsible for this. + recipients: Vec, + + /// Vector of pairs of recipient name and address that goes into the `To` field. + /// + /// The list of actual message recipient addresses may be different, + /// e.g. if members are hidden for broadcast lists. + to: Vec<(String, String)>, + + /// Vector of pairs of past group member names and addresses. + past_members: Vec<(String, String)>, + + /// Timestamps of the members in the same order as in the `recipients` + /// followed by `past_members`. + /// + /// If this is not empty, its length + /// should be the sum of `recipients` and `past_members` length. + member_timestamps: Vec, timestamp: i64, loaded: Loaded, @@ -128,6 +156,7 @@ impl MimeFactory { pub async fn from_msg(context: &Context, msg: Message) -> Result { let chat = Chat::load_from_db(context, msg.chat_id).await?; let attach_profile_data = Self::should_attach_profile_data(&msg); + let undisclosed_recipients = chat.typ == Chattype::Broadcast; let from_addr = context.get_primary_self_addr().await?; let config_displayname = context @@ -145,47 +174,101 @@ impl MimeFactory { (name, None) }; - let mut recipients = Vec::with_capacity(5); + let mut recipients = Vec::new(); + let mut to = Vec::new(); + let mut past_members = Vec::new(); + let mut member_timestamps = Vec::new(); let mut recipient_ids = HashSet::new(); let mut req_mdn = false; if chat.is_self_talk() { if msg.param.get_cmd() == SystemMessage::AutocryptSetupMessage { - recipients.push((from_displayname.to_string(), from_addr.to_string())); + recipients.push(from_addr.to_string()); + to.push((from_displayname.to_string(), from_addr.to_string())); } } else if chat.is_mailing_list() { let list_post = chat .param .get(Param::ListPost) .context("Can't write to mailinglist without ListPost param")?; - recipients.push(("".to_string(), list_post.to_string())); + to.push(("".to_string(), list_post.to_string())); + recipients.push(list_post.to_string()); } else { + let email_to_remove = if msg.param.get_cmd() == SystemMessage::MemberRemovedFromGroup { + msg.param.get(Param::Arg) + } else { + None + }; + context .sql .query_map( - "SELECT c.authname, c.addr, c.id \ - FROM chats_contacts cc \ - LEFT JOIN contacts c ON cc.contact_id=c.id \ - WHERE cc.chat_id=? AND cc.contact_id>9;", - (msg.chat_id,), + "SELECT c.authname, c.addr, c.id, cc.add_timestamp, cc.remove_timestamp + FROM chats_contacts cc + LEFT JOIN contacts c ON cc.contact_id=c.id + WHERE cc.chat_id=? AND cc.contact_id>9 OR (cc.contact_id=1 AND ?)", + (msg.chat_id, chat.typ == Chattype::Group), |row| { let authname: String = row.get(0)?; let addr: String = row.get(1)?; let id: ContactId = row.get(2)?; - Ok((authname, addr, id)) + let add_timestamp: i64 = row.get(3)?; + let remove_timestamp: i64 = row.get(4)?; + Ok((authname, addr, id, add_timestamp, remove_timestamp)) }, |rows| { + let mut past_member_timestamps = Vec::new(); + for row in rows { - let (authname, addr, id) = row?; - if !recipients_contain_addr(&recipients, &addr) { - let name = match attach_profile_data { - true => authname, - false => "".to_string(), - }; - recipients.push((name, addr)); + let (authname, addr, id, add_timestamp, remove_timestamp) = row?; + let addr = if id == ContactId::SELF { + from_addr.to_string() + } else { + addr + }; + let name = match attach_profile_data { + true => authname, + false => "".to_string(), + }; + if add_timestamp >= remove_timestamp { + if !recipients_contain_addr(&to, &addr) { + recipients.push(addr.clone()); + if !undisclosed_recipients { + to.push((name, addr)); + member_timestamps.push(add_timestamp); + } + } + recipient_ids.insert(id); + } else { + // Row is a tombstone, + // member is not actually part of the group. + if !recipients_contain_addr(&past_members, &addr) { + if let Some(email_to_remove) = email_to_remove { + if email_to_remove == addr { + // This is a "member removed" message, + // we need to notify removed member + // that it was removed. + recipients.push(addr.clone()); + } + } + if !undisclosed_recipients { + past_members.push((name, addr)); + past_member_timestamps.push(remove_timestamp); + } + } } - recipient_ids.insert(id); } + + debug_assert!(member_timestamps.len() >= to.len()); + + if to.len() > 1 { + if let Some(position) = to.iter().position(|(_, x)| x == &from_addr) { + to.remove(position); + member_timestamps.remove(position); + } + } + + member_timestamps.extend(past_member_timestamps); Ok(()) }, ) @@ -226,12 +309,19 @@ impl MimeFactory { }; let attach_selfavatar = Self::should_attach_selfavatar(context, &msg).await; + debug_assert!( + member_timestamps.is_empty() + || to.len() + past_members.len() == member_timestamps.len() + ); let factory = MimeFactory { from_addr, from_displayname, sender_displayname, selfstatus, recipients, + to, + past_members, + member_timestamps, timestamp: msg.timestamp_sort, loaded: Loaded::Message { msg, chat }, in_reply_to, @@ -259,7 +349,10 @@ impl MimeFactory { from_displayname: "".to_string(), sender_displayname: None, selfstatus: "".to_string(), - recipients: vec![("".to_string(), contact.get_addr().to_string())], + recipients: vec![contact.get_addr().to_string()], + to: vec![("".to_string(), contact.get_addr().to_string())], + past_members: vec![], + member_timestamps: vec![], timestamp, loaded: Loaded::Mdn { rfc724_mid, @@ -283,11 +376,7 @@ impl MimeFactory { let self_addr = context.get_primary_self_addr().await?; let mut res = Vec::new(); - for (_, addr) in self - .recipients - .iter() - .filter(|(_, addr)| addr != &self_addr) - { + for addr in self.recipients.iter().filter(|&addr| *addr != self_addr) { res.push((Peerstate::from_addr(context, addr).await?, addr.clone())); } @@ -475,10 +564,7 @@ impl MimeFactory { } pub fn recipients(&self) -> Vec { - self.recipients - .iter() - .map(|(_, addr)| addr.clone()) - .collect() + self.recipients.clone() } /// Consumes a `MimeFactory` and renders it into a message which is then stored in @@ -488,46 +574,33 @@ impl MimeFactory { let from = new_address_with_name(&self.from_displayname, self.from_addr.clone()); - let undisclosed_recipients = match &self.loaded { - Loaded::Message { chat, .. } => chat.typ == Chattype::Broadcast, - Loaded::Mdn { .. } => false, - }; - let mut to = Vec::new(); - if undisclosed_recipients { + for (name, addr) in &self.to { + if name.is_empty() { + to.push(Address::new_mailbox(addr.clone())); + } else { + to.push(new_address_with_name(name, addr.clone())); + } + } + + let mut past_members = Vec::new(); // Contents of `Chat-Group-Past-Members` header. + for (name, addr) in &self.past_members { + if name.is_empty() { + past_members.push(Address::new_mailbox(addr.clone())); + } else { + past_members.push(new_address_with_name(name, addr.clone())); + } + } + + debug_assert!( + self.member_timestamps.is_empty() + || to.len() + past_members.len() == self.member_timestamps.len() + ); + if to.is_empty() { to.push(Address::new_group( "hidden-recipients".to_string(), Vec::new(), )); - } else { - let email_to_remove = match &self.loaded { - Loaded::Message { msg, .. } => { - if msg.param.get_cmd() == SystemMessage::MemberRemovedFromGroup { - msg.param.get(Param::Arg) - } else { - None - } - } - Loaded::Mdn { .. } => None, - }; - - for (name, addr) in &self.recipients { - if let Some(email_to_remove) = email_to_remove { - if email_to_remove == addr { - continue; - } - } - - if name.is_empty() { - to.push(Address::new_mailbox(addr.clone())); - } else { - to.push(new_address_with_name(name, addr.clone())); - } - } - - if to.is_empty() { - to.push(from.clone()); - } } // Start with Internet Message Format headers in the order of the standard example @@ -540,6 +613,26 @@ impl MimeFactory { headers.push(Header::new_with_value("Sender".into(), vec![sender]).unwrap()); } headers.push(Header::new_with_value("To".into(), to.clone()).unwrap()); + if !past_members.is_empty() { + headers.push( + Header::new_with_value("Chat-Group-Past-Members".into(), past_members.clone()) + .unwrap(), + ); + } + + if !self.member_timestamps.is_empty() { + headers.push( + Header::new_with_value( + "Chat-Group-Member-Timestamps".into(), + self.member_timestamps + .iter() + .map(|ts| ts.to_string()) + .collect::>() + .join(" "), + ) + .unwrap(), + ); + } let subject_str = self.subject_str(context).await?; let encoded_subject = if subject_str @@ -2461,8 +2554,9 @@ mod tests { // Alice creates a group with Bob and Claire and then removes Bob. let alice = TestContext::new_alice().await; + let claire_addr = "claire@foo.de"; let bob_id = Contact::create(&alice, "Bob", "bob@example.net").await?; - let claire_id = Contact::create(&alice, "Claire", "claire@foo.de").await?; + let claire_id = Contact::create(&alice, "Claire", claire_addr).await?; let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "foo").await?; add_contact_to_chat(&alice, alice_chat_id, bob_id).await?; @@ -2478,10 +2572,17 @@ mod tests { .get_first_header("To") .context("no To: header parsed")?; let to = addrparse_header(to)?; - let mailbox = to - .extract_single_info() - .context("to: field does not contain exactly one address")?; - assert_eq!(mailbox.addr, "bob@example.net"); + for to_addr in to.iter() { + match to_addr { + mailparse::MailAddr::Single(ref info) => { + // Addresses should be of existing members (Alice and Bob) and not Claire. + assert_ne!(info.addr, claire_addr); + } + mailparse::MailAddr::Group(_) => { + panic!("Group addresses are not expected here"); + } + } + } Ok(()) } diff --git a/src/mimeparser.rs b/src/mimeparser.rs index c027b57664..8e0625957d 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -35,6 +35,7 @@ use crate::param::{Param, Params}; use crate::peerstate::Peerstate; use crate::simplify::{simplify, SimplifiedText}; use crate::sync::SyncItems; +use crate::tools::time; use crate::tools::{ get_filemeta, parse_receive_headers, smeared_time, truncate_msg_text, validate_id, }; @@ -57,9 +58,14 @@ pub(crate) struct MimeMessage { /// Message headers. headers: HashMap, - /// Addresses are normalized and lowercase + /// List of addresses from the `To` and `Cc` headers. + /// + /// Addresses are normalized and lowercase. pub recipients: Vec, + /// List of addresses from the `Chat-Group-Past-Members` header. + pub past_members: Vec, + /// `From:` address. pub from: SingleInfo, @@ -232,6 +238,7 @@ impl MimeMessage { let mut headers = Default::default(); let mut recipients = Default::default(); + let mut past_members = Default::default(); let mut from = Default::default(); let mut list_post = Default::default(); let mut chat_disposition_notification_to = None; @@ -241,6 +248,7 @@ impl MimeMessage { context, &mut headers, &mut recipients, + &mut past_members, &mut from, &mut list_post, &mut chat_disposition_notification_to, @@ -261,6 +269,7 @@ impl MimeMessage { context, &mut headers, &mut recipients, + &mut past_members, &mut from, &mut list_post, &mut chat_disposition_notification_to, @@ -438,6 +447,8 @@ impl MimeMessage { HeaderDef::ChatGroupAvatar, HeaderDef::ChatGroupMemberRemoved, HeaderDef::ChatGroupMemberAdded, + HeaderDef::ChatGroupMemberTimestamps, + HeaderDef::ChatGroupPastMembers, ] { headers.remove(h.get_headername()); } @@ -454,6 +465,7 @@ impl MimeMessage { context, &mut headers, &mut recipients, + &mut past_members, &mut inner_from, &mut list_post, &mut chat_disposition_notification_to, @@ -511,6 +523,7 @@ impl MimeMessage { parts: Vec::new(), headers, recipients, + past_members, list_post, from, from_is_signed, @@ -1530,10 +1543,12 @@ impl MimeMessage { } } + #[allow(clippy::too_many_arguments)] fn merge_headers( context: &Context, headers: &mut HashMap, recipients: &mut Vec, + past_members: &mut Vec, from: &mut Option, list_post: &mut Option, chat_disposition_notification_to: &mut Option, @@ -1562,6 +1577,11 @@ impl MimeMessage { if !recipients_new.is_empty() { *recipients = recipients_new; } + let past_members_addresses = + get_all_addresses_from_header(fields, "chat-group-past-members"); + if !past_members_addresses.is_empty() { + *past_members = past_members_addresses; + } let from_new = get_from(fields); if from_new.is_some() { *from = from_new; @@ -1828,6 +1848,20 @@ impl MimeMessage { }; Ok(parent_timestamp) } + + /// Returns parsed `Chat-Group-Member-Timestamps` header contents. + /// + /// Returns `None` if there is no such header. + pub fn chat_group_member_timestamps(&self) -> Option> { + let now = time() + constants::TIMESTAMP_SENT_TOLERANCE; + self.get_header(HeaderDef::ChatGroupMemberTimestamps) + .map(|h| { + h.split_ascii_whitespace() + .filter_map(|ts| ts.parse::().ok()) + .map(|ts| std::cmp::min(now, ts)) + .collect() + }) + } } /// Parses `Autocrypt-Gossip` headers from the email and applies them to peerstates. diff --git a/src/param.rs b/src/param.rs index 9200357284..e9255d69f4 100644 --- a/src/param.rs +++ b/src/param.rs @@ -183,6 +183,8 @@ pub enum Param { GroupNameTimestamp = b'g', /// For Chats: timestamp of member list update. + /// + /// Deprecated 2025-01-07. MemberListTimestamp = b'k', /// For Webxdc Message Instances: Current document name diff --git a/src/peerstate.rs b/src/peerstate.rs index cf1e30e9a6..9d3f526e11 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -711,9 +711,25 @@ impl Peerstate { Origin::IncomingUnknownFrom, ) .await?; - chat::remove_from_chat_contacts_table(context, *chat_id, contact_id) - .await?; - chat::add_to_chat_contacts_table(context, *chat_id, &[new_contact_id]) + context + .sql + .transaction(|transaction| { + transaction.execute( + "UPDATE chats_contacts + SET remove_timestamp=MAX(add_timestamp+1, ?) + WHERE chat_id=? AND contact_id=?", + (timestamp, chat_id, contact_id), + )?; + transaction.execute( + "INSERT INTO chats_contacts + (chat_id, contact_id, add_timestamp) + VALUES (?1, ?2, ?3) + ON CONFLICT (chat_id, contact_id) + DO UPDATE SET add_timestamp=MAX(remove_timestamp, ?3)", + (chat_id, new_contact_id, timestamp), + )?; + Ok(()) + }) .await?; context.emit_event(EventType::ChatModified(*chat_id)); diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 006151306f..e2000957bb 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1,6 +1,7 @@ //! Internet Message Format reception pipeline. use std::collections::HashSet; +use std::iter; use std::str::FromStr; use anyhow::{Context as _, Result}; @@ -14,7 +15,7 @@ use regex::Regex; use crate::aheader::EncryptPreference; use crate::chat::{self, Chat, ChatId, ChatIdBlocked, ProtectionStatus}; use crate::config::Config; -use crate::constants::{self, Blocked, Chattype, ShowEmails, DC_CHAT_ID_TRASH}; +use crate::constants::{Blocked, Chattype, ShowEmails, DC_CHAT_ID_TRASH}; use crate::contact::{Contact, ContactId, Origin}; use crate::context::Context; use crate::debug_logging::maybe_set_logging_xdc_inner; @@ -25,8 +26,7 @@ use crate::headerdef::{HeaderDef, HeaderDefMap}; use crate::imap::{markseen_on_imap_table, GENERATED_PREFIX}; use crate::log::LogExt; use crate::message::{ - self, rfc724_mid_exists, rfc724_mid_exists_ex, Message, MessageState, MessengerMessage, MsgId, - Viewtype, + self, rfc724_mid_exists, Message, MessageState, MessengerMessage, MsgId, Viewtype, }; use crate::mimeparser::{parse_message_ids, AvatarAction, MimeMessage, SystemMessage}; use crate::param::{Param, Params}; @@ -345,6 +345,18 @@ pub(crate) async fn receive_imf_inner( }, ) .await?; + let past_ids = add_or_lookup_contacts_by_address_list( + context, + &mime_parser.past_members, + if !mime_parser.incoming { + Origin::OutgoingTo + } else if incoming_origin.is_known() { + Origin::IncomingTo + } else { + Origin::IncomingUnknownTo + }, + ) + .await?; update_verified_keys(context, &mut mime_parser, from_id).await?; @@ -418,6 +430,7 @@ pub(crate) async fn receive_imf_inner( &mut mime_parser, imf_raw, &to_ids, + &past_ids, rfc724_mid_orig, from_id, seen, @@ -440,10 +453,10 @@ pub(crate) async fn receive_imf_inner( // and waste traffic. let chat_id = received_msg.chat_id; if !chat_id.is_special() - && mime_parser - .recipients - .iter() - .all(|recipient| mime_parser.gossiped_keys.contains_key(&recipient.addr)) + && mime_parser.recipients.iter().all(|recipient| { + recipient.addr == mime_parser.from.addr + || mime_parser.gossiped_keys.contains_key(&recipient.addr) + }) { info!( context, @@ -689,6 +702,7 @@ async fn add_parts( mime_parser: &mut MimeMessage, imf_raw: &[u8], to_ids: &[ContactId], + past_ids: &[ContactId], rfc724_mid: &str, from_id: ContactId, seen: bool, @@ -836,6 +850,7 @@ async fn add_parts( create_blocked, from_id, to_ids, + past_ids, &verified_encryption, &grpid, ) @@ -906,7 +921,7 @@ async fn add_parts( group_chat_id, from_id, to_ids, - is_partial_download.is_some(), + past_ids, &verified_encryption, ) .await?; @@ -1075,6 +1090,7 @@ async fn add_parts( Blocked::Not, from_id, to_ids, + past_ids, &verified_encryption, &grpid, ) @@ -1175,7 +1191,7 @@ async fn add_parts( chat_id, from_id, to_ids, - is_partial_download.is_some(), + past_ids, &verified_encryption, ) .await?; @@ -1512,9 +1528,6 @@ async fn add_parts( let mut txt_raw = "".to_string(); let (msg, typ): (&str, Viewtype) = if let Some(better_msg) = &better_msg { - if better_msg.is_empty() && is_partial_download.is_none() { - chat_id = DC_CHAT_ID_TRASH; - } (better_msg, Viewtype::Text) } else { (&part.msg, part.typ) @@ -1879,10 +1892,12 @@ async fn lookup_chat_or_create_adhoc_group( FROM chats c INNER JOIN msgs m ON c.id=m.chat_id WHERE m.hidden=0 AND c.grpid='' AND c.name=? AND (SELECT COUNT(*) FROM chats_contacts - WHERE chat_id=c.id)=? + WHERE chat_id=c.id + AND add_timestamp >= remove_timestamp)=? AND (SELECT COUNT(*) FROM chats_contacts - WHERE chat_id=c.id - AND contact_id NOT IN ({}))=0 + WHERE chat_id=c.id + AND contact_id NOT IN ({}) + AND add_timestamp >= remove_timestamp)=0 ORDER BY m.timestamp DESC", sql::repeat_vars(contact_ids.len()), ), @@ -1970,6 +1985,7 @@ async fn create_group( create_blocked: Blocked, from_id: ContactId, to_ids: &[ContactId], + past_ids: &[ContactId], verified_encryption: &VerifiedEncryption, grpid: &str, ) -> Result> { @@ -2043,14 +2059,37 @@ async fn create_group( chat_id_blocked = create_blocked; // Create initial member list. - let mut members = vec![ContactId::SELF]; - if !from_id.is_special() { - members.push(from_id); + if let Some(mut chat_group_member_timestamps) = mime_parser.chat_group_member_timestamps() { + let mut new_to_ids = to_ids.to_vec(); + if !new_to_ids.contains(&from_id) { + new_to_ids.insert(0, from_id); + chat_group_member_timestamps.insert(0, mime_parser.timestamp_sent); + } + + update_chats_contacts_timestamps( + context, + new_chat_id, + None, + &new_to_ids, + past_ids, + &chat_group_member_timestamps, + ) + .await?; + } else { + let mut members = vec![ContactId::SELF]; + if !from_id.is_special() { + members.push(from_id); + } + members.extend(to_ids); + + chat::add_to_chat_contacts_table( + context, + mime_parser.timestamp_sent, + new_chat_id, + &members, + ) + .await?; } - members.extend(to_ids); - members.sort_unstable(); - members.dedup(); - chat::add_to_chat_contacts_table(context, new_chat_id, &members).await?; context.emit_event(EventType::ChatModified(new_chat_id)); chatlist_events::emit_chatlist_changed(context); @@ -2075,13 +2114,83 @@ async fn create_group( } } +async fn update_chats_contacts_timestamps( + context: &Context, + chat_id: ChatId, + ignored_id: Option, + to_ids: &[ContactId], + past_ids: &[ContactId], + chat_group_member_timestamps: &[i64], +) -> Result { + let expected_timestamps_count = to_ids.len() + past_ids.len(); + + if chat_group_member_timestamps.len() != expected_timestamps_count { + warn!( + context, + "Chat-Group-Member-Timestamps has wrong number of timestamps, got {}, expected {}.", + chat_group_member_timestamps.len(), + expected_timestamps_count + ); + return Ok(false); + } + + let mut modified = false; + + context + .sql + .transaction(|transaction| { + let mut add_statement = transaction.prepare( + "INSERT INTO chats_contacts (chat_id, contact_id, add_timestamp) + VALUES (?1, ?2, ?3) + ON CONFLICT (chat_id, contact_id) + DO + UPDATE SET add_timestamp=?3 + WHERE ?3>add_timestamp AND ?3>=remove_timestamp", + )?; + + for (contact_id, ts) in iter::zip( + to_ids.iter(), + chat_group_member_timestamps.iter().take(to_ids.len()), + ) { + if Some(*contact_id) != ignored_id { + // It could be that member was already added, + // but updated addition timestamp + // is also a modification worth notifying about. + modified |= add_statement.execute((chat_id, contact_id, ts))? > 0; + } + } + + let mut remove_statement = transaction.prepare( + "UPDATE chats_contacts + SET remove_timestamp=?1 + WHERE chat_id=?2 AND contact_id=?3 + AND ?1>remove_timestamp AND ?1>add_timestamp", + )?; + + for (contact_id, ts) in iter::zip( + past_ids.iter(), + chat_group_member_timestamps.iter().skip(to_ids.len()), + ) { + // It could be that member was already removed, + // but updated removal timestamp + // is also a modification worth notifying about. + modified |= remove_statement.execute((ts, chat_id, contact_id))? > 0; + } + + Ok(()) + }) + .await?; + + Ok(modified) +} + /// Apply group member list, name, avatar and protection status changes from the MIME message. /// /// Returns `Vec` of group changes messages and, optionally, a better message to replace the -/// original system message. If the better message is empty, the original system message should be -/// just omitted. +/// original system message. /// -/// * `is_partial_download` - whether the message is not fully downloaded. +/// * `to_ids` - contents of the `To` and `Cc` headers. +/// * `past_ids` - contents of the `Chat-Group-Past-Members` header. #[allow(clippy::too_many_arguments)] async fn apply_group_changes( context: &Context, @@ -2089,7 +2198,7 @@ async fn apply_group_changes( chat_id: ChatId, from_id: ContactId, to_ids: &[ContactId], - is_partial_download: bool, + past_ids: &[ContactId], verified_encryption: &VerifiedEncryption, ) -> Result<(Vec, Option)> { if chat_id.is_special() { @@ -2118,49 +2227,6 @@ 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 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 - // 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. - Some(reply_to) => rfc724_mid_exists_ex(context, reply_to, "download_state=0") - .await? - .filter(|(_, _, downloaded)| *downloaded) - .is_none(), - None => false, - } - } && ( - // 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 - } - ); if mime_parser.get_header(HeaderDef::ChatVerified).is_some() { if let VerifiedEncryption::NotVerified(err) = verified_encryption { @@ -2184,44 +2250,24 @@ async fn apply_group_changes( if let Some(removed_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberRemoved) { removed_id = Contact::lookup_id_by_addr(context, removed_addr, Origin::Unknown).await?; if let Some(id) = removed_id { - if allow_member_list_changes && chat_contacts.contains(&id) { - better_msg = if id == from_id { - Some(stock_str::msg_group_left_local(context, from_id).await) - } else { - Some(stock_str::msg_del_member_local(context, removed_addr, from_id).await) - }; - } + better_msg = if id == from_id { + Some(stock_str::msg_group_left_local(context, from_id).await) + } else { + Some(stock_str::msg_del_member_local(context, removed_addr, from_id).await) + }; } else { warn!(context, "Removed {removed_addr:?} has no contact id.") } - better_msg.get_or_insert_with(Default::default); - if !allow_member_list_changes { - info!( - context, - "Ignoring removal of {removed_addr:?} from {chat_id}." - ); - } } else if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) { - if allow_member_list_changes { - let is_new_member; - if let Some(contact_id) = - Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await? - { - added_id = Some(contact_id); - is_new_member = !chat_contacts.contains(&contact_id); - } else { - warn!(context, "Added {added_addr:?} has no contact id."); - is_new_member = false; - } - - if is_new_member || self_added { - better_msg = - Some(stock_str::msg_add_member_local(context, added_addr, from_id).await); - } + if let Some(contact_id) = + Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await? + { + added_id = Some(contact_id); } else { - info!(context, "Ignoring addition of {added_addr:?} to {chat_id}."); + warn!(context, "Added {added_addr:?} has no contact id."); } - better_msg.get_or_insert_with(Default::default); + + better_msg = Some(stock_str::msg_add_member_local(context, added_addr, from_id).await); } else if let Some(old_name) = mime_parser .get_header(HeaderDef::ChatGroupNameChanged) .map(|s| s.trim()) @@ -2268,111 +2314,105 @@ async fn apply_group_changes( } } - if allow_member_list_changes { + // These are for adding info messages about implicit membership changes, so they are only + // filled when such messages are needed. + let mut added_ids = HashSet::::new(); + let mut removed_ids = HashSet::::new(); + + if let Some(ref chat_group_member_timestamps) = mime_parser.chat_group_member_timestamps() { + send_event_chat_modified |= update_chats_contacts_timestamps( + context, + chat_id, + Some(from_id), + to_ids, + past_ids, + chat_group_member_timestamps, + ) + .await?; + let new_chat_contacts = HashSet::::from_iter( + chat::get_chat_contacts(context, chat_id) + .await? + .iter() + .copied(), + ); + added_ids = new_chat_contacts + .difference(&chat_contacts) + .copied() + .collect(); + removed_ids = chat_contacts + .difference(&new_chat_contacts) + .copied() + .collect(); + } else if is_from_in_chat { 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); } - // These are for adding info messages about implicit membership changes, so they are only - // filled when such messages are needed. - let mut added_ids = HashSet::::new(); - let mut removed_ids = HashSet::::new(); - - if !recreate_member_list { - if sync_member_list { + if !self_added { + if mime_parser.get_header(HeaderDef::ChatVersion).is_none() { + // Allow non-Delta Chat MUAs to add members. added_ids = new_members.difference(&chat_contacts).copied().collect(); - } else if let Some(added_id) = added_id { + } + + if let Some(added_id) = added_id { added_ids.insert(added_id); } new_members.clone_from(&chat_contacts); // 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 - // if they removed a recipient then it was probably by accident. - // - DC users could miss new member additions and then better to handle this in the same - // way as for classical MUA messages. Moreover, if we remove a member implicitly, they - // will never know that and continue to think they're still here. - // But it shouldn't be a big problem if somebody missed a member removal, 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. new_members.extend(added_ids.clone()); } if let Some(removed_id) = removed_id { new_members.remove(&removed_id); } - if recreate_member_list { - if self_added { - // ... then `better_msg` is already set. - } else if chat.blocked == Blocked::Request || !chat_contacts.contains(&ContactId::SELF) - { - warn!(context, "Implicit addition of SELF to chat {chat_id}."); - group_changes_msgs.push( - stock_str::msg_add_member_local( - context, - &context.get_primary_self_addr().await?, - ContactId::UNDEFINED, - ) - .await, - ); - } else { - added_ids = new_members.difference(&chat_contacts).copied().collect(); - removed_ids = chat_contacts.difference(&new_members).copied().collect(); - } - } - - if let Some(added_id) = added_id { - added_ids.remove(&added_id); - } - if let Some(removed_id) = removed_id { - removed_ids.remove(&removed_id); - } - if !added_ids.is_empty() { - warn!( - context, - "Implicit addition of {added_ids:?} to chat {chat_id}." - ); - } - if !removed_ids.is_empty() { - warn!( - context, - "Implicit removal of {removed_ids:?} from chat {chat_id}." - ); - } - group_changes_msgs.reserve(added_ids.len() + removed_ids.len()); - for contact_id in added_ids { - let contact = Contact::get_by_id(context, contact_id).await?; - group_changes_msgs.push( - stock_str::msg_add_member_local(context, contact.get_addr(), ContactId::UNDEFINED) - .await, - ); - } - for contact_id in removed_ids { - let contact = Contact::get_by_id(context, contact_id).await?; - group_changes_msgs.push( - stock_str::msg_del_member_local(context, contact.get_addr(), ContactId::UNDEFINED) - .await, - ); - } if new_members != chat_contacts { - chat::update_chat_contacts_table(context, chat_id, &new_members).await?; + chat::update_chat_contacts_table( + context, + mime_parser.timestamp_sent, + chat_id, + &new_members, + ) + .await?; 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(added_id) = added_id { + added_ids.remove(&added_id); + } + if let Some(removed_id) = removed_id { + removed_ids.remove(&removed_id); + } + if !added_ids.is_empty() { + warn!( + context, + "Implicit addition of {added_ids:?} to chat {chat_id}." + ); + } + if !removed_ids.is_empty() { + warn!( + context, + "Implicit removal of {removed_ids:?} from chat {chat_id}." + ); + } + group_changes_msgs.reserve(added_ids.len() + removed_ids.len()); + for contact_id in added_ids { + let contact = Contact::get_by_id(context, contact_id).await?; + group_changes_msgs.push( + stock_str::msg_add_member_local(context, contact.get_addr(), ContactId::UNDEFINED) + .await, + ); + } + for contact_id in removed_ids { + let contact = Contact::get_by_id(context, contact_id).await?; + group_changes_msgs.push( + stock_str::msg_del_member_local(context, contact.get_addr(), ContactId::UNDEFINED) + .await, + ); } if let Some(avatar_action) = &mime_parser.group_avatar { @@ -2481,7 +2521,13 @@ async fn create_or_lookup_mailinglist( ) })?; - chat::add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?; + chat::add_to_chat_contacts_table( + context, + mime_parser.timestamp_sent, + chat_id, + &[ContactId::SELF], + ) + .await?; Ok(Some((chat_id, blocked))) } else { info!(context, "Creating list forbidden by caller."); @@ -2677,7 +2723,13 @@ async fn create_adhoc_group( context, "Created ad-hoc group id={new_chat_id}, name={grpname:?}." ); - chat::add_to_chat_contacts_table(context, new_chat_id, &member_ids).await?; + chat::add_to_chat_contacts_table( + context, + mime_parser.timestamp_sent, + new_chat_id, + &member_ids, + ) + .await?; context.emit_event(EventType::ChatModified(new_chat_id)); chatlist_events::emit_chatlist_changed(context); @@ -2939,14 +2991,12 @@ pub(crate) async fn get_prefetch_parent_message( } /// Looks up contact IDs from the database given the list of recipients. -/// -/// Returns vector of IDs guaranteed to be unique. async fn add_or_lookup_contacts_by_address_list( context: &Context, address_list: &[SingleInfo], origin: Origin, ) -> Result> { - let mut contact_ids = HashSet::new(); + let mut contact_ids = Vec::new(); for info in address_list { let addr = &info.addr; if !may_be_valid_addr(addr) { @@ -2957,13 +3007,13 @@ async fn add_or_lookup_contacts_by_address_list( let (contact_id, _) = Contact::add_or_lookup(context, display_name.unwrap_or_default(), &addr, origin) .await?; - contact_ids.insert(contact_id); + contact_ids.push(contact_id); } else { warn!(context, "Contact with address {:?} cannot exist.", addr); } } - Ok(contact_ids.into_iter().collect::>()) + Ok(contact_ids) } #[cfg(test)] diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 398471719e..60bbeb6801 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -566,6 +566,8 @@ async fn test_escaped_recipients() { .unwrap() .0; + // We test with non-chat message here + // because chat messages are not expected to have `Cc` header. receive_imf( &t, b"From: Foobar \n\ @@ -573,8 +575,6 @@ async fn test_escaped_recipients() { Cc: =?utf-8?q?=3Ch2=3E?= \n\ Subject: foo\n\ Message-ID: \n\ - Chat-Version: 1.0\n\ - Chat-Disposition-Notification-To: \n\ Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ \n\ hello\n", @@ -590,11 +590,12 @@ async fn test_escaped_recipients() { let msg = Message::load_from_db(&t, chats.get_msg_id(0).unwrap().unwrap()) .await .unwrap(); - assert_eq!(msg.is_dc_message, MessengerMessage::Yes); - assert_eq!(msg.text, "hello"); - assert_eq!(msg.param.get_int(Param::WantsMdn).unwrap(), 1); + assert_eq!(msg.is_dc_message, MessengerMessage::No); + assert_eq!(msg.text, "foo – hello"); } +/// Tests that `Cc` header updates display name +/// if existing contact has low enough origin. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_cc_to_contact() { let t = TestContext::new_alice().await; @@ -612,6 +613,8 @@ async fn test_cc_to_contact() { .unwrap() .0; + // We use non-chat message here + // because chat messages are not expected to have `Cc` header. receive_imf( &t, b"Received: (Postfix, from userid 1000); Mon, 4 Dec 2006 14:51:39 +0100 (CET)\n\ @@ -620,8 +623,6 @@ async fn test_cc_to_contact() { Cc: Carl \n\ Subject: foo\n\ Message-ID: \n\ - Chat-Version: 1.0\n\ - Chat-Disposition-Notification-To: \n\ Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ \n\ hello\n", @@ -3333,6 +3334,7 @@ async fn test_outgoing_private_reply_multidevice() -> Result<()> { let group_id = chat::create_group_chat(&bob, ProtectionStatus::Unprotected, "Group").await?; chat::add_to_chat_contacts_table( &bob, + time(), group_id, &[ bob.add_or_lookup_contact(&alice1).await.id, @@ -3542,26 +3544,27 @@ async fn test_no_private_reply_to_blocked_account() -> Result<()> { let alice = tcm.alice().await; let bob = tcm.bob().await; - // =============== Bob creates a group =============== + tcm.section("Bob creates a group"); let group_id = chat::create_group_chat(&bob, ProtectionStatus::Unprotected, "Group").await?; chat::add_to_chat_contacts_table( &bob, + time(), group_id, &[bob.add_or_lookup_contact(&alice).await.id], ) .await?; - // =============== Bob sends the first message to the group =============== + tcm.section("Bob sends the first message to the group"); let sent = bob.send_text(group_id, "Hello all!").await; alice.recv_msg(&sent).await; let chats = Chatlist::try_load(&bob, 0, None, None).await?; assert_eq!(chats.len(), 1); - // =============== Bob blocks Alice ================ + tcm.section("Bob blocks Alice"); Contact::block(&bob, bob.add_or_lookup_contact(&alice).await.id).await?; - // =============== Alice replies private to Bob ============== + tcm.section("Alice replies private to Bob"); let received = alice.get_last_msg().await; assert_eq!(received.text, "Hello all!"); @@ -3575,7 +3578,7 @@ async fn test_no_private_reply_to_blocked_account() -> Result<()> { let sent2 = alice.send_msg(alice_bob_chat.id, &mut msg_out).await; bob.recv_msg(&sent2).await; - // ========= check that no contact request was created ============ + // check that no contact request was created let chats = Chatlist::try_load(&bob, 0, None, None).await.unwrap(); assert_eq!(chats.len(), 1); let chat_id = chats.get_chat_id(0).unwrap(); @@ -3586,7 +3589,7 @@ async fn test_no_private_reply_to_blocked_account() -> Result<()> { let received = bob.get_last_msg().await; assert_eq!(received.text, "Hello all!"); - // =============== Bob unblocks Alice ================ + tcm.section("Bob unblocks Alice"); // test if the blocked chat is restored correctly Contact::unblock(&bob, bob.add_or_lookup_contact(&alice).await.id).await?; let chats = Chatlist::try_load(&bob, 0, None, None).await.unwrap(); @@ -4151,11 +4154,15 @@ async fn test_ignore_outdated_membership_changes() -> Result<()> { SystemTime::shift(Duration::from_secs(3600)); - // Bob replies again adding Alice back. + // Bob replies again, even after some time this does not add Alice back. + // + // Bob cannot learn from Alice that Alice has left the group + // because Alice is not going to send more messages to the group. 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?); + + assert!(!is_contact_in_chat(alice, alice_chat_id, ContactId::SELF).await?); Ok(()) } @@ -4216,7 +4223,7 @@ async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn test_recreate_contact_list_on_missing_messages() -> Result<()> { +async fn test_delayed_removal_is_ignored() -> Result<()> { let alice = TestContext::new_alice().await; let bob = TestContext::new_bob().await; let chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?; @@ -4224,6 +4231,7 @@ async fn test_recreate_contact_list_on_missing_messages() -> Result<()> { // create chat with three members add_to_chat_contacts_table( &alice, + time(), chat_id, &[ Contact::create(&alice, "bob", "bob@example.net").await?, @@ -4236,12 +4244,12 @@ async fn test_recreate_contact_list_on_missing_messages() -> Result<()> { let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id; bob_chat_id.accept(&bob).await?; - // bob removes a member + // Bob removes Fiona. let bob_contact_fiona = Contact::create(&bob, "fiona", "fiona@example.net").await?; remove_contact_from_chat(&bob, bob_chat_id, bob_contact_fiona).await?; let remove_msg = bob.pop_sent_msg().await; - // bob adds new members + // Bob adds new members "blue" and "orange", but first addition message is lost. let bob_blue = Contact::create(&bob, "blue", "blue@example.net").await?; add_contact_to_chat(&bob, bob_chat_id, bob_blue).await?; bob.pop_sent_msg().await; @@ -4249,32 +4257,32 @@ async fn test_recreate_contact_list_on_missing_messages() -> Result<()> { add_contact_to_chat(&bob, bob_chat_id, bob_orange).await?; let add_msg = bob.pop_sent_msg().await; - // alice only receives the second member addition + // Alice only receives the second member addition, + // but this results in addition of both members + // and removal of Fiona. alice.recv_msg(&add_msg).await; - - // since we missed messages, a new contact list should be build assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 4); - // re-add fiona + // Alice re-adds Fiona. add_contact_to_chat(&alice, chat_id, alice_fiona).await?; + assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 5); - // delayed removal of fiona shouldn't remove her - alice.recv_msg_trash(&remove_msg).await; + // Delayed removal of Fiona by Bob shouldn't remove her. + alice.recv_msg(&remove_msg).await; assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 5); alice - .golden_test_chat( - chat_id, - "receive_imf_recreate_contact_list_on_missing_messages", - ) + .golden_test_chat(chat_id, "receive_imf_delayed_removal_is_ignored") .await; + Ok(()) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_dont_readd_with_normal_msg() -> Result<()> { - let alice = TestContext::new_alice().await; - let bob = TestContext::new_bob().await; + let mut tcm = TestContextManager::new(); + let alice = tcm.alice().await; + let bob = tcm.bob().await; let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?; @@ -4289,6 +4297,7 @@ async fn test_dont_readd_with_normal_msg() -> Result<()> { let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id; bob_chat_id.accept(&bob).await?; + // Bob leaves, but Alice didn't receive Bob's leave message. remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?; bob.pop_sent_msg().await; assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 1); @@ -4302,12 +4311,11 @@ async fn test_dont_readd_with_normal_msg() -> Result<()> { .await?; bob.recv_msg(&alice.pop_sent_msg().await).await; - // Alice didn't receive Bob's leave message although a lot of time has - // passed, so Bob must re-add 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?); + // Bob received a message from Alice, but this should not re-add him to the group. + assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); + + // Bob got an update that fiora is added nevertheless. + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2); Ok(()) } @@ -4535,19 +4543,14 @@ async fn test_recreate_member_list_on_missing_add_of_self() -> Result<()> { bob.recv_msg(&alice.pop_sent_msg().await).await; assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); - // But if Bob left a long time ago, they must recreate the member list after missing a message. + // Even if some time passed, Bob must not be re-added back. SystemTime::shift(Duration::from_secs(3600)); send_text_msg(&alice, alice_chat_id, "5th message".to_string()).await?; alice.pop_sent_msg().await; send_text_msg(&alice, alice_chat_id, "6th 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?); + assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).await?); - bob.golden_test_chat( - bob_chat_id, - "receive_imf_recreate_member_list_on_missing_add_of_self", - ) - .await; Ok(()) } @@ -4781,13 +4784,6 @@ async fn test_partial_group_consistency() -> Result<()> { let contacts = get_chat_contacts(&bob, bob_chat_id).await?; assert_eq!(contacts.len(), 2); - // Get initial timestamp. - let timestamp = bob_chat_id - .get_param(&bob) - .await? - .get_i64(Param::MemberListTimestamp) - .unwrap(); - // Bob receives partial message. let msg_id = receive_imf_from_inbox( &bob, @@ -4808,15 +4804,9 @@ Chat-Group-Member-Added: charlie@example.com", .context("no received message")?; let msg = Message::load_from_db(&bob, msg_id.msg_ids[0]).await?; - let timestamp2 = bob_chat_id - .get_param(&bob) - .await? - .get_i64(Param::MemberListTimestamp) - .unwrap(); // Partial download does not change the member list. assert_eq!(msg.download_state, DownloadState::Available); - assert_eq!(timestamp, timestamp2); assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?, contacts); // Alice sends normal message to bob, adding fiona. @@ -4829,15 +4819,6 @@ Chat-Group-Member-Added: charlie@example.com", bob.recv_msg(&alice.pop_sent_msg().await).await; - let timestamp3 = bob_chat_id - .get_param(&bob) - .await? - .get_i64(Param::MemberListTimestamp) - .unwrap(); - - // Receiving a message after a partial download recreates the member list because we treat - // such messages as if we have not seen them. - assert_ne!(timestamp, timestamp3); let contacts = get_chat_contacts(&bob, bob_chat_id).await?; assert_eq!(contacts.len(), 3); @@ -4861,15 +4842,9 @@ Chat-Group-Member-Added: charlie@example.com", .context("no received message")?; let msg = Message::load_from_db(&bob, msg_id.msg_ids[0]).await?; - let timestamp4 = bob_chat_id - .get_param(&bob) - .await? - .get_i64(Param::MemberListTimestamp) - .unwrap(); // After full download, the old message should not change group state. assert_eq!(msg.download_state, DownloadState::Done); - assert_eq!(timestamp3, timestamp4); assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?, contacts); Ok(()) @@ -4892,19 +4867,13 @@ async fn test_leave_protected_group_missing_member_key() -> Result<()> { ("b@b", "bob@example.net"), ) .await?; + + // We fail to send the message. assert!(remove_contact_from_chat(alice, group_id, ContactId::SELF) .await .is_err()); - assert!(is_contact_in_chat(alice, group_id, ContactId::SELF).await?); - alice - .sql - .execute( - "UPDATE acpeerstates SET addr=? WHERE addr=?", - ("bob@example.net", "b@b"), - ) - .await?; - remove_contact_from_chat(alice, group_id, ContactId::SELF).await?; - alice.pop_sent_msg().await; + + // The contact is already removed anyway. assert!(!is_contact_in_chat(alice, group_id, ContactId::SELF).await?); Ok(()) } @@ -4926,12 +4895,22 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> { .await?; let fiona = &tcm.fiona().await; + let fiona_addr = fiona.get_config(Config::Addr).await?.unwrap(); mark_as_verified(alice, fiona).await; let alice_fiona_id = alice.add_or_lookup_contact(fiona).await.id; assert!(add_contact_to_chat(alice, group_id, alice_fiona_id) .await .is_err()); - assert!(!is_contact_in_chat(alice, group_id, alice_fiona_id).await?); + // Sending the message failed, + // but member is added to the chat locally already. + assert!(is_contact_in_chat(alice, group_id, alice_fiona_id).await?); + let msg = alice.get_last_msg_in(group_id).await; + assert!(msg.is_info()); + assert_eq!( + msg.get_text(), + stock_str::msg_add_member_local(alice, &fiona_addr, ContactId::SELF).await + ); + // Now the chat has a message "You added member fiona@example.net. [INFO] !!" (with error) that // may be confusing, but if the error is displayed in UIs, it's more or less ok. This is not a // normal scenario anyway. @@ -5070,32 +5049,6 @@ async fn test_unarchive_on_member_removal() -> Result<()> { Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn test_no_op_member_added_is_trash() -> Result<()> { - let mut tcm = TestContextManager::new(); - let alice = &tcm.alice().await; - let bob = &tcm.bob().await; - let alice_chat_id = alice - .create_group_with_members(ProtectionStatus::Unprotected, "foos", &[bob]) - .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?; - - let fiona_id = Contact::create(alice, "", "fiona@example.net").await?; - add_contact_to_chat(alice, alice_chat_id, fiona_id).await?; - let msg = alice.pop_sent_msg().await; - - let fiona_id = Contact::create(bob, "", "fiona@example.net").await?; - add_contact_to_chat(bob, bob_chat_id, fiona_id).await?; - bob.recv_msg_trash(&msg).await; - let contacts = get_chat_contacts(bob, bob_chat_id).await?; - assert_eq!(contacts.len(), 3); - Ok(()) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_forged_from() -> Result<()> { let mut tcm = TestContextManager::new(); diff --git a/src/securejoin/bob.rs b/src/securejoin/bob.rs index 24c2191a46..8782365185 100644 --- a/src/securejoin/bob.rs +++ b/src/securejoin/bob.rs @@ -59,8 +59,13 @@ pub(super) async fn start_protocol(context: &Context, invite: QrInvite) -> Resul // only become usable once the protocol is finished. let group_chat_id = state.joining_chat_id(context).await?; if !is_contact_in_chat(context, group_chat_id, invite.contact_id()).await? { - chat::add_to_chat_contacts_table(context, group_chat_id, &[invite.contact_id()]) - .await?; + chat::add_to_chat_contacts_table( + context, + time(), + group_chat_id, + &[invite.contact_id()], + ) + .await?; } let msg = stock_str::secure_join_started(context, invite.contact_id()).await; chat::add_info_msg(context, group_chat_id, &msg, time()).await?; diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 1ee96f073e..27172ef337 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -1138,6 +1138,24 @@ CREATE INDEX msgs_status_updates_index2 ON msgs_status_updates (uid); .await?; } + inc_and_check(&mut migration_version, 128)?; + if dbversion < migration_version { + // Add the timestamps of addition and removal. + // + // If `add_timestamp >= remove_timestamp`, + // then the member is currently a member of the chat. + // Otherwise the member is a past member. + sql.execute_migration( + "ALTER TABLE chats_contacts + ADD COLUMN add_timestamp NOT NULL DEFAULT 0; + ALTER TABLE chats_contacts + ADD COLUMN remove_timestamp NOT NULL DEFAULT 0; + ", + migration_version, + ) + .await?; + } + let new_version = sql .get_raw_config_int(VERSION_CFG) .await? diff --git a/src/test_utils.rs b/src/test_utils.rs index e680d2025a..8beba66335 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -41,6 +41,7 @@ use crate::pgp::KeyPair; use crate::receive_imf::receive_imf; use crate::securejoin::{get_securejoin_qr, join_securejoin}; use crate::stock_str::StockStrings; +use crate::tools::time; #[allow(non_upper_case_globals)] pub const AVATAR_900x900_BYTES: &[u8] = include_bytes!("../test-data/image/avatar900x900.png"); @@ -880,7 +881,7 @@ impl TestContext { let contact = self.add_or_lookup_contact(member).await; to_add.push(contact.id); } - add_to_chat_contacts_table(self, chat_id, &to_add) + add_to_chat_contacts_table(self, time(), chat_id, &to_add) .await .unwrap(); diff --git a/test-data/golden/chat_test_msg_with_implicit_member_add b/test-data/golden/chat_test_msg_with_implicit_member_add deleted file mode 100644 index 0a3986231d..0000000000 --- a/test-data/golden/chat_test_msg_with_implicit_member_add +++ /dev/null @@ -1,9 +0,0 @@ -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] √ -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_parallel_member_remove b/test-data/golden/chat_test_parallel_member_remove index a1e1e37d06..86e0998578 100644 --- a/test-data/golden/chat_test_parallel_member_remove +++ b/test-data/golden/chat_test_parallel_member_remove @@ -1,8 +1,7 @@ -Group#Chat#10: Group chat [4 member(s)] +Group#Chat#10: Group chat [3 member(s)] -------------------------------------------------------------------------------- Msg#10: (Contact#Contact#10): Hi! I created a group. [FRESH] Msg#11: Me (Contact#Contact#Self): You left the group. [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] +Msg#13: (Contact#Contact#10): What a silence! [FRESH] -------------------------------------------------------------------------------- diff --git a/test-data/golden/receive_imf_recreate_contact_list_on_missing_messages b/test-data/golden/receive_imf_delayed_removal_is_ignored similarity index 84% rename from test-data/golden/receive_imf_recreate_contact_list_on_missing_messages rename to test-data/golden/receive_imf_delayed_removal_is_ignored index 4dfcb87a66..697d5086fe 100644 --- a/test-data/golden/receive_imf_recreate_contact_list_on_missing_messages +++ b/test-data/golden/receive_imf_delayed_removal_is_ignored @@ -5,4 +5,5 @@ Msg#11: info (Contact#Contact#Info): Member blue@example.net added. [NOTICED][IN Msg#12: info (Contact#Contact#Info): Member fiona (fiona@example.net) removed. [NOTICED][INFO] Msg#13: bob (Contact#Contact#11): Member orange@example.net added by bob (bob@example.net). [FRESH][INFO] Msg#14: Me (Contact#Contact#Self): You added member fiona (fiona@example.net). [INFO] o +Msg#15: bob (Contact#Contact#11): Member fiona (fiona@example.net) removed by bob (bob@example.net). [FRESH][INFO] -------------------------------------------------------------------------------- diff --git a/test-data/golden/receive_imf_recreate_member_list_on_missing_add_of_self b/test-data/golden/receive_imf_recreate_member_list_on_missing_add_of_self deleted file mode 100644 index d1995d0a82..0000000000 --- a/test-data/golden/receive_imf_recreate_member_list_on_missing_add_of_self +++ /dev/null @@ -1,9 +0,0 @@ -Group#Chat#10: Group [2 member(s)] --------------------------------------------------------------------------------- -Msg#10: info (Contact#Contact#Info): Member Me (bob@example.net) added. [NOTICED][INFO] -Msg#11: (Contact#Contact#10): second message [FRESH] -Msg#12🔒: Me (Contact#Contact#Self): You left the group. [INFO] √ -Msg#13: (Contact#Contact#10): 4th message [FRESH] -Msg#14: info (Contact#Contact#Info): Member Me (bob@example.net) added. [NOTICED][INFO] -Msg#15: (Contact#Contact#10): 6th message [FRESH] ---------------------------------------------------------------------------------