diff --git a/src/chat.rs b/src/chat.rs index e4cb89057..98a618c28 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -3916,8 +3916,7 @@ pub async fn remove_contact_from_chat( if let Some(contact) = Contact::get_by_id_optional(context, contact_id).await? { if chat.typ == Chattype::Group && chat.is_promoted() { msg.viewtype = Viewtype::Text; - if contact.id == ContactId::SELF { - set_group_explicitly_left(context, &chat.grpid).await?; + if contact_id == ContactId::SELF { msg.text = stock_str::msg_group_left_local(context, ContactId::SELF).await; } else { msg.text = stock_str::msg_del_member_local( @@ -3929,17 +3928,24 @@ pub async fn remove_contact_from_chat( } msg.param.set_cmd(SystemMessage::MemberRemovedFromGroup); msg.param.set(Param::Arg, contact.get_addr().to_lowercase()); - msg.id = send_msg(context, chat_id, &mut msg).await?; + let res = send_msg(context, chat_id, &mut msg).await; + if contact_id == ContactId::SELF { + res?; + set_group_explicitly_left(context, &chat.grpid).await?; + } else if let Err(e) = res { + warn!(context, "remove_contact_from_chat({chat_id}, {contact_id}): send_msg() failed: {e:#}."); + } } else { 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 have to re-do 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. + // 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 @@ -5023,6 +5029,7 @@ mod tests { // Bob leaves the chat. remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?; + bob.pop_sent_msg().await; // Bob receives a msg about Alice adding Claire to the group. bob.recv_msg(&alice_sent_add_msg).await; @@ -5075,6 +5082,7 @@ mod tests { let sent_msg = alice.pop_sent_msg().await; bob.recv_msg(&sent_msg).await; 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; diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index 97e1040b9..5e7f8e90e 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -3923,6 +3923,7 @@ async fn test_dont_readd_with_normal_msg() -> Result<()> { bob_chat_id.accept(&bob).await?; 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); SystemTime::shift(Duration::from_secs(3600)); @@ -4085,6 +4086,7 @@ async fn test_mua_can_readd() -> Result<()> { // And leaves it. remove_contact_from_chat(&alice, alice_chat.id, ContactId::SELF).await?; + alice.pop_sent_msg().await; assert!(!is_contact_in_chat(&alice, alice_chat.id, ContactId::SELF).await?); // Bob uses a classical MUA to answer, adding Alice back. @@ -4159,6 +4161,7 @@ async fn test_recreate_member_list_on_missing_add_of_self() -> Result<()> { // But if Bob just left, they mustn't recreate the member list even after missing a message. bob_chat_id.accept(&bob).await?; remove_contact_from_chat(&bob, bob_chat_id, ContactId::SELF).await?; + bob.pop_sent_msg().await; send_text_msg(&alice, alice_chat_id, "3rd message".to_string()).await?; alice.pop_sent_msg().await; send_text_msg(&alice, alice_chat_id, "4th message".to_string()).await?; @@ -4498,6 +4501,33 @@ async fn test_leave_protected_group_missing_member_key() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_protected_group_remove_member_missing_key() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let bob_addr = bob.get_config(Config::Addr).await?.unwrap(); + mark_as_verified(alice, bob).await; + let group_id = create_group_chat(alice, ProtectionStatus::Protected, "Group").await?; + let alice_bob_id = alice.add_or_lookup_contact(bob).await.id; + add_contact_to_chat(alice, group_id, alice_bob_id).await?; + alice.send_text(group_id, "Hello!").await; + alice + .sql + .execute("DELETE FROM acpeerstates WHERE addr=?", (&bob_addr,)) + .await?; + remove_contact_from_chat(alice, group_id, alice_bob_id).await?; + assert!(!is_contact_in_chat(alice, group_id, alice_bob_id).await?); + let msg = alice.get_last_msg_in(group_id).await; + assert!(msg.is_info()); + assert_eq!( + msg.get_text(), + stock_str::msg_del_member_local(alice, &bob_addr, ContactId::SELF,).await + ); + assert!(msg.error().is_some()); + 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.rs b/src/securejoin.rs index 09ab9f93a..d643b2d68 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -1403,6 +1403,7 @@ First thread."#; let alice_bob_contact_id = Contact::create(&alice, "Bob", "bob@example.net").await?; remove_contact_from_chat(&alice, alice_chat_id, alice_bob_contact_id).await?; + alice.pop_sent_msg().await; // The message from Bob is delivered late, Bob is already removed. let msg = alice.recv_msg(&sent).await; diff --git a/src/webxdc.rs b/src/webxdc.rs index 78f5898d5..c864dc180 100644 --- a/src/webxdc.rs +++ b/src/webxdc.rs @@ -2588,6 +2588,7 @@ sth_for_the = "future""# ); remove_contact_from_chat(&alice, chat_id, contact_bob).await?; + alice.pop_sent_msg().await; let status = helper_send_receive_status_update(&bob, &alice, &bob_instance, &instance).await?; diff --git a/test-data/golden/chat_test_msg_with_implicit_member_add b/test-data/golden/chat_test_msg_with_implicit_member_add index 68760fa9f..0a3986231 100644 --- a/test-data/golden/chat_test_msg_with_implicit_member_add +++ b/test-data/golden/chat_test_msg_with_implicit_member_add @@ -2,7 +2,7 @@ Group#Chat#10: Group chat [3 member(s)] -------------------------------------------------------------------------------- Msg#10: (Contact#Contact#11): I created a group [FRESH] Msg#11: (Contact#Contact#11): Member Fiona (fiona@example.net) added by alice@example.org. [FRESH][INFO] -Msg#12: Me (Contact#Contact#Self): You removed member Fiona (fiona@example.net). [INFO] o +Msg#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 015a97937..a1e1e37d0 100644 --- a/test-data/golden/chat_test_parallel_member_remove +++ b/test-data/golden/chat_test_parallel_member_remove @@ -1,7 +1,7 @@ Group#Chat#10: Group chat [4 member(s)] -------------------------------------------------------------------------------- Msg#10: (Contact#Contact#10): Hi! I created a group. [FRESH] -Msg#11: Me (Contact#Contact#Self): You left the group. [INFO] o +Msg#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]