feat: new group membership update algorithm

New algorithm improves group consistency
in cases of missing messages,
restored old backups and replies from classic MUAs.

Co-authored-by: Hocuri <hocuri@gmx.de>
Co-authored-by: link2xt <link2xt@testrun.org>
This commit is contained in:
Sebastian Klähn
2023-03-20 14:48:37 +01:00
committed by link2xt
parent 91bf87fa80
commit 6eb8abe535
4 changed files with 418 additions and 97 deletions

View File

@@ -1400,6 +1400,7 @@ impl Chat {
}
/// Returns true if the chat is promoted.
/// This means a message has been sent to it and it _not_ only exists on the users device.
pub fn is_promoted(&self) -> bool {
!self.is_unpromoted()
}
@@ -2969,6 +2970,7 @@ pub(crate) async fn remove_from_chat_contacts_table(
}
/// Adds a contact to the chat.
/// If the group is promoted, also sends out a system message to all group members
pub async fn add_contact_to_chat(
context: &Context,
chat_id: ChatId,
@@ -2990,7 +2992,7 @@ pub(crate) async fn add_contact_to_chat_ex(
chat_id.reset_gossiped_timestamp(context).await?;
/*this also makes sure, not contacts are added to special or normal chats*/
// this also makes sure, no contacts are added to special or normal chats
let mut chat = Chat::load_from_db(context, chat_id).await?;
ensure!(
chat.typ == Chattype::Group || chat.typ == Chattype::Broadcast,
@@ -3012,7 +3014,7 @@ pub(crate) async fn add_contact_to_chat_ex(
context.emit_event(EventType::ErrorSelfNotInGroup(
"Cannot add contact to group; self not in group.".into(),
));
bail!("can not add contact because our account is not part of it");
bail!("can not add contact because the account is not part of the group/broadcast");
}
if from_handshake && chat.param.get_int(Param::Unpromoted).unwrap_or_default() == 1 {

View File

@@ -63,6 +63,8 @@ pub(crate) struct MimeMessage {
/// Whether the From address was repeated in the signed part
/// (and we know that the signer intended to send from this address)
pub from_is_signed: bool,
/// The List-Post address is only set for mailing lists. Users can send
/// messages to this address to post them to the list.
pub list_post: Option<String>,
pub chat_disposition_notification_to: Option<SingleInfo>,
pub decryption_info: DecryptionInfo,
@@ -768,6 +770,8 @@ impl MimeMessage {
!self.signatures.is_empty()
}
/// Returns whether the email contains a `chat-version` header.
/// This indicates that the email is a DC-email.
pub(crate) fn has_chat_version(&self) -> bool {
self.header.contains_key("chat-version")
}

View File

@@ -1641,86 +1641,136 @@ async fn apply_group_changes(
return Ok(None);
}
let mut recreate_member_list = false;
let mut send_event_chat_modified = false;
let mut removed_id = None;
let mut better_msg = None;
let removed_id;
if let Some(removed_addr) = mime_parser
.get_header(HeaderDef::ChatGroupMemberRemoved)
.cloned()
{
removed_id = Contact::lookup_id_by_addr(context, &removed_addr, Origin::Unknown).await?;
recreate_member_list = true;
match removed_id {
Some(contact_id) => {
better_msg = if contact_id == from_id {
Some(stock_str::msg_group_left(context, from_id).await)
} else {
Some(stock_str::msg_del_member(context, &removed_addr, from_id).await)
};
// True if a Delta Chat client has explicitly added our current primary address.
let self_added =
if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) {
context.get_primary_self_addr().await? == *added_addr
} else {
false
};
// Whether to allow any changes to the member list at all.
let allow_member_list_changes =
if chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await? || self_added {
// Reject old group changes.
chat_id
.update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
.await?
} else {
// Member list changes are not allowed if we're not in the group
// and are not explicitly added.
// This message comes from a Delta Chat that restored an old backup
// or the message is a MUA reply to an old message.
false
};
// Whether to rebuild the member list from scratch.
let recreate_member_list = if allow_member_list_changes {
// Recreate member list if the message comes from a MUA as these messages do _not_ set add/remove headers.
// Always recreate membership list if self has been added.
if !mime_parser.has_chat_version() || self_added {
true
} else {
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(context, reply_to).await?.is_none(),
None => false,
}
None => warn!(context, "Removed {removed_addr:?} has no contact_id."),
}
} else {
removed_id = None;
if let Some(added_member) = mime_parser
.get_header(HeaderDef::ChatGroupMemberAdded)
.cloned()
{
better_msg = Some(stock_str::msg_add_member(context, &added_member, from_id).await);
recreate_member_list = true;
} else if let Some(old_name) = mime_parser
.get_header(HeaderDef::ChatGroupNameChanged)
// See create_or_lookup_group() for explanation
.map(|s| s.trim())
{
if let Some(grpname) = mime_parser
.get_header(HeaderDef::ChatGroupName)
// See create_or_lookup_group() for explanation
.map(|grpname| grpname.trim())
.filter(|grpname| grpname.len() < 200)
{
if chat_id
.update_timestamp(context, Param::GroupNameTimestamp, sent_timestamp)
.await?
{
info!(context, "Updating grpname for chat {chat_id}.");
context
.sql
.execute(
"UPDATE chats SET name=? WHERE id=?;",
(strip_rtlo_characters(grpname), chat_id),
)
.await?;
false
};
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(contact_id) = removed_id {
if allow_member_list_changes {
// Remove a single member from the chat.
if !recreate_member_list {
chat::remove_from_chat_contacts_table(context, chat_id, contact_id).await?;
send_event_chat_modified = true;
}
better_msg =
Some(stock_str::msg_grp_name(context, old_name, grpname, from_id).await);
better_msg = if contact_id == from_id {
Some(stock_str::msg_group_left(context, from_id).await)
} else {
Some(stock_str::msg_del_member(context, removed_addr, from_id).await)
};
} else {
info!(
context,
"Ignoring removal of {removed_addr:?} from {chat_id}."
);
}
} else if let Some(value) = mime_parser.get_header(HeaderDef::ChatContent) {
if value == "group-avatar-changed" {
if let Some(avatar_action) = &mime_parser.group_avatar {
// this is just an explicit message containing the group-avatar,
// apart from that, the group-avatar is send along with various other messages
better_msg = match avatar_action {
AvatarAction::Delete => {
Some(stock_str::msg_grp_img_deleted(context, from_id).await)
}
AvatarAction::Change(_) => {
Some(stock_str::msg_grp_img_changed(context, from_id).await)
}
};
} else {
warn!(context, "Removed {removed_addr:?} has no contact id.")
}
} else if let Some(added_addr) = mime_parser.get_header(HeaderDef::ChatGroupMemberAdded) {
if allow_member_list_changes {
// Add a single member to the chat.
if !recreate_member_list {
if let Some(contact_id) =
Contact::lookup_id_by_addr(context, added_addr, Origin::Unknown).await?
{
chat::add_to_chat_contacts_table(context, chat_id, &[contact_id]).await?;
send_event_chat_modified = true;
} else {
warn!(context, "Added {added_addr:?} has no contact id.")
}
}
}
}
if !mime_parser.has_chat_version() {
// If a classical MUA user adds someone to TO/CC, then the DC user shall
// see this addition and have the new recipient in the member list.
recreate_member_list = true;
better_msg = Some(stock_str::msg_add_member(context, added_addr, from_id).await);
} else {
info!(context, "Ignoring addition of {added_addr:?} to {chat_id}.");
}
} else if let Some(old_name) = mime_parser
.get_header(HeaderDef::ChatGroupNameChanged)
// See create_or_lookup_group() for explanation
.map(|s| s.trim())
{
if let Some(grpname) = mime_parser
.get_header(HeaderDef::ChatGroupName)
// See create_or_lookup_group() for explanation
.map(|grpname| grpname.trim())
.filter(|grpname| grpname.len() < 200)
{
if chat_id
.update_timestamp(context, Param::GroupNameTimestamp, sent_timestamp)
.await?
{
info!(context, "Updating grpname for chat {chat_id}.");
context
.sql
.execute(
"UPDATE chats SET name=? WHERE id=?;",
(strip_rtlo_characters(grpname), chat_id),
)
.await?;
send_event_chat_modified = true;
}
better_msg = Some(stock_str::msg_grp_name(context, old_name, grpname, from_id).await);
}
} else if let Some(value) = mime_parser.get_header(HeaderDef::ChatContent) {
if value == "group-avatar-changed" {
if let Some(avatar_action) = &mime_parser.group_avatar {
// this is just an explicit message containing the group-avatar,
// apart from that, the group-avatar is send along with various other messages
better_msg = match avatar_action {
AvatarAction::Delete => {
Some(stock_str::msg_grp_img_deleted(context, from_id).await)
}
AvatarAction::Change(_) => {
Some(stock_str::msg_grp_img_changed(context, from_id).await)
}
};
}
}
}
if mime_parser.get_header(HeaderDef::ChatVerified).is_some() {
@@ -1734,49 +1784,46 @@ async fn apply_group_changes(
chat_id
.inner_set_protection(context, ProtectionStatus::Protected)
.await?;
recreate_member_list = true;
}
}
// add members to group/check members
// Recreate the member list.
if recreate_member_list {
if chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await?
&& !chat::is_contact_in_chat(context, chat_id, from_id).await?
{
if !chat::is_contact_in_chat(context, chat_id, from_id).await? {
warn!(
context,
"Contact {from_id} attempts to modify group chat {chat_id} member list without being a member."
);
} else if chat_id
.update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
.await?
{
let mut members_to_add = vec![];
if removed_id.is_some()
|| !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await?
{
// Members could have been removed while we were
// absent. We can't use existing member list and need to
// start from scratch.
} else {
// 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() {
context
.sql
.execute("DELETE FROM chats_contacts WHERE chat_id=?;", (chat_id,))
.await?;
members_to_add.push(ContactId::SELF);
}
let mut members_to_add = HashSet::new();
members_to_add.extend(to_ids);
members_to_add.insert(ContactId::SELF);
if !from_id.is_special() {
members_to_add.push(from_id);
members_to_add.insert(from_id);
}
members_to_add.extend(to_ids);
if let Some(removed_id) = removed_id {
members_to_add.retain(|id| *id != removed_id);
}
members_to_add.dedup();
info!(context, "Adding {members_to_add:?} to chat id={chat_id}.");
chat::add_to_chat_contacts_table(context, chat_id, &members_to_add).await?;
if let Some(removed_id) = removed_id {
members_to_add.remove(&removed_id);
}
info!(
context,
"Recreating chat {chat_id} with members {members_to_add:?}."
);
chat::add_to_chat_contacts_table(context, chat_id, &Vec::from_iter(members_to_add))
.await?;
send_event_chat_modified = true;
}
}

View File

@@ -2,7 +2,10 @@ use tokio::fs;
use super::*;
use crate::aheader::EncryptPreference;
use crate::chat::get_chat_contacts;
use crate::chat::{
add_contact_to_chat, add_to_chat_contacts_table, create_group_chat, get_chat_contacts,
is_contact_in_chat, remove_contact_from_chat, send_text_msg,
};
use crate::chat::{get_chat_msgs, ChatItem, ChatVisibility};
use crate::chatlist::Chatlist;
use crate::constants::DC_GCL_NO_SPECIALS;
@@ -3302,3 +3305,268 @@ async fn test_mua_user_adds_recipient_to_single_chat() -> Result<()> {
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_sync_member_list_on_rejoin() -> Result<()> {
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@example.de").await?;
let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "foos").await?;
add_contact_to_chat(&alice, alice_chat_id, bob_id).await?;
add_contact_to_chat(&alice, alice_chat_id, claire_id).await?;
send_text_msg(&alice, alice_chat_id, "populate".to_string()).await?;
let add = alice.pop_sent_msg().await;
let bob = tcm.bob().await;
bob.recv_msg(&add).await;
let bob_chat_id = bob.get_last_msg().await.chat_id;
assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 3);
// remove bob from chat
remove_contact_from_chat(&alice, alice_chat_id, bob_id).await?;
let remove_bob = alice.pop_sent_msg().await;
bob.recv_msg(&remove_bob).await;
// remove any other member
remove_contact_from_chat(&alice, alice_chat_id, claire_id).await?;
alice.pop_sent_msg().await;
// readd bob
add_contact_to_chat(&alice, alice_chat_id, bob_id).await?;
let add2 = alice.pop_sent_msg().await;
bob.recv_msg(&add2).await;
// number of members in chat should have updated
assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 2);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_dont_recreate_contacts_on_add_remove() -> Result<()> {
let alice = TestContext::new_alice().await;
let bob = TestContext::new_bob().await;
let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?;
add_contact_to_chat(
&alice,
alice_chat_id,
Contact::create(&alice, "bob", "bob@example.net").await?,
)
.await?;
send_text_msg(&alice, alice_chat_id, "populate".to_string()).await?;
let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id;
bob_chat_id.accept(&bob).await?;
// alice adds a member
add_contact_to_chat(
&alice,
alice_chat_id,
Contact::create(&alice, "fiona", "fiona@example.net").await?,
)
.await?;
// bob adds a member.
let bob_blue = Contact::create(&bob, "blue", "blue@example.net").await?;
add_contact_to_chat(&bob, bob_chat_id, bob_blue).await?;
alice.recv_msg(&bob.pop_sent_msg().await).await;
// bob didn't receive the addition of fiona, but alice doesn't overwrite her own
// contact list with the one from bob which only has three members instead of four.
assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 4);
// bob removes a member.
remove_contact_from_chat(&bob, bob_chat_id, bob_blue).await?;
alice.recv_msg(&bob.pop_sent_msg().await).await;
// Bobs chat only has two members after the removal of blue, because he still
// didn't receive the addition of fiona. But that doesn't overwrite alice'
// memberlist.
assert_eq!(get_chat_contacts(&alice, alice_chat_id).await?.len(), 3);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_recreate_contact_list_on_missing_message() -> Result<()> {
let alice = TestContext::new_alice().await;
let bob = TestContext::new_bob().await;
let chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?;
let alice_fiona = Contact::create(&alice, "fiona", "fiona@example.net").await?;
// create chat with three members
add_to_chat_contacts_table(
&alice,
chat_id,
&[
Contact::create(&alice, "bob", "bob@example.net").await?,
alice_fiona,
],
)
.await?;
send_text_msg(&alice, chat_id, "populate".to_string()).await?;
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
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 a new member
let bob_blue = Contact::create(&bob, "blue", "blue@example.net").await?;
add_contact_to_chat(&bob, bob_chat_id, bob_blue).await?;
let add_msg = bob.pop_sent_msg().await;
// alice only receives the addition of the member
alice.recv_msg(&add_msg).await;
// since we missed a message, a new contact list should be build
assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 3);
// readd fiona
add_contact_to_chat(&alice, chat_id, alice_fiona).await?;
alice.recv_msg(&remove_msg).await;
// delayed removal of fiona shouldn't remove her
assert_eq!(get_chat_contacts(&alice, chat_id).await?.len(), 4);
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 alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?;
add_contact_to_chat(
&alice,
alice_chat_id,
Contact::create(&alice, "bob", "bob@example.net").await?,
)
.await?;
send_text_msg(&alice, alice_chat_id, "populate".to_string()).await?;
let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id;
bob_chat_id.accept(&bob).await?;
remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?;
assert_eq!(get_chat_contacts(&bob, bob_chat_id).await?.len(), 1);
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 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?);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_mua_cant_remove() -> Result<()> {
let alice = TestContext::new_alice().await;
// Alice creates chat with 3 contacts
let msg = receive_imf(
&alice,
b"Subject: =?utf-8?q?Message_from_alice=40example=2Eorg?=\r\n\
From: alice@example.org\r\n\
To: <bob@example.net>, <claire@example.org>, <fiona@example.org> \r\n\
Date: Mon, 12 Dec 2022 14:30:39 +0000\r\n\
Message-ID: <Mr.alices_original_mail@example.org>\r\n\
Chat-Version: 1.0\r\n\
\r\n\
tst\r\n",
false,
)
.await?
.unwrap();
let alice_chat = Chat::load_from_db(&alice, msg.chat_id).await?;
assert_eq!(alice_chat.typ, Chattype::Group);
// Bob uses a classical MUA to answer, removing a recipient.
let bob_removes = receive_imf(
&alice,
b"Subject: Re: Message from alice\r\n\
From: <bob@example.net>\r\n\
To: <alice@example.org>, <claire@example.org>\r\n\
Date: Mon, 12 Dec 2022 14:32:39 +0000\r\n\
Message-ID: <bobs_answer_to_two_recipients@example.net>\r\n\
In-Reply-To: <Mr.alices_original_mail@example.org>\r\n\
\r\n\
Hi back!\r\n",
false,
)
.await?
.unwrap();
assert_eq!(bob_removes.chat_id, alice_chat.id);
let group_chat = Chat::load_from_db(&alice, bob_removes.chat_id).await?;
assert_eq!(group_chat.typ, Chattype::Group);
assert_eq!(
chat::get_chat_contacts(&alice, group_chat.id).await?.len(),
4
);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_mua_can_add() -> Result<()> {
let alice = TestContext::new_alice().await;
// Alice creates chat with 3 contacts
let msg = receive_imf(
&alice,
b"Subject: =?utf-8?q?Message_from_alice=40example=2Eorg?=\r\n\
From: alice@example.org\r\n\
To: <bob@example.net>, <claire@example.org>, <fiona@example.org> \r\n\
Date: Mon, 12 Dec 2022 14:30:39 +0000\r\n\
Message-ID: <Mr.alices_original_mail@example.org>\r\n\
Chat-Version: 1.0\r\n\
\r\n\
Hi!\r\n",
false,
)
.await?
.unwrap();
let alice_chat = Chat::load_from_db(&alice, msg.chat_id).await?;
assert_eq!(alice_chat.typ, Chattype::Group);
// Bob uses a classical MUA to answer, adding a recipient.
let bob_adds = receive_imf(
&alice,
b"Subject: Re: Message from alice\r\n\
From: <bob@example.net>\r\n\
To: <alice@example.org>, <claire@example.org>, <fiona@example.org>, <greg@example.host>\r\n\
Date: Mon, 12 Dec 2022 14:32:39 +0000\r\n\
Message-ID: <bobs_answer_to_two_recipients@example.net>\r\n\
In-Reply-To: <Mr.alices_original_mail@example.org>\r\n\
\r\n\
Hi back!\r\n",
false,
)
.await?
.unwrap();
let group_chat = Chat::load_from_db(&alice, bob_adds.chat_id).await?;
assert_eq!(group_chat.typ, Chattype::Group);
assert_eq!(
chat::get_chat_contacts(&alice, group_chat.id).await?.len(),
5
);
Ok(())
}