diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 64ee66c56..646f3ec24 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1718,7 +1718,8 @@ async fn apply_group_changes( false }; - let mut chat_contacts = HashSet::from_iter(chat::get_chat_contacts(context, chat_id).await?); + let mut chat_contacts = + HashSet::::from_iter(chat::get_chat_contacts(context, chat_id).await?); let is_from_in_chat = !chat_contacts.contains(&ContactId::SELF) || chat_contacts.contains(&from_id); @@ -1854,33 +1855,31 @@ async fn apply_group_changes( } if !recreate_member_list { - let diff: HashSet = - chat_contacts.difference(&new_members).copied().collect(); - // Only delete old contacts if the sender is not a classical MUA user: - // Classical MUA users usually don't intend to remove users from an email - // thread, so if they removed a recipient then it was probably by accident. - if mime_parser.has_chat_version() { - // This is what provides group membership consistency: we remove group members - // locally if we see a discrepancy with the "To" list in the received message as it - // is better for privacy than adding absent members locally. But it shouldn't be a - // big problem if somebody missed a member addition, because they will likely - // recreate the member list from the next received message. The problem occurs only - // if that "somebody" managed to reply earlier. Really, it's a problem for big - // groups with high message rate, but let it be for now. - if !diff.is_empty() { - warn!(context, "Implicit removal of {diff:?} from chat {chat_id}."); - } - new_members = chat_contacts.difference(&diff).copied().collect(); - } else { - new_members.extend(diff); + // 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. + 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); + } + if !diff.is_empty() { + warn!(context, "Implicit addition of {diff:?} to chat {chat_id}."); } } if let Some(removed_id) = removed_id { new_members.remove(&removed_id); } - if let Some(added_id) = added_id { - new_members.insert(added_id); - } if recreate_member_list { info!( context, diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 0f63a2c8b..cdfe479d3 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -3411,14 +3411,24 @@ async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> { alice.recv_msg(&bob.pop_sent_msg().await).await; - // Bob didn't receive the addition of Fiona, so Alice must remove Fiona from the members list - // back to make their group members view consistent. - assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3); + // Bob didn't receive the addition of Fiona, but Alice mustn't remove Fiona from the members + // list back. Instead, Bob must add Fiona from the next Alice's message to make their group + // members view consistent. + assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4); // Just a dumb check for remove_contact_from_chat(). Let's have it in this only place. remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?; alice.recv_msg(&bob.pop_sent_msg().await).await; - assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 2); + assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3); + + send_text_msg( + &alice, + alice_chat_id, + "Finally add Fiona please".to_string(), + ) + .await?; + bob.recv_msg(&alice.pop_sent_msg().await).await; + assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 3); Ok(()) } @@ -3502,8 +3512,11 @@ async fn test_dont_readd_with_normal_msg() -> Result<()> { bob.recv_msg(&alice.pop_sent_msg().await).await; - // Alice didn't receive Bobs leave message, but bob shouldn't readded himself just because of that. - assert!(!is_contact_in_chat(&bob, bob_chat_id, ContactId::SELF).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 + // 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?); Ok(()) }