diff --git a/src/chat.rs b/src/chat.rs index f85d0ca2e..4db51c6c5 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -3054,9 +3054,10 @@ pub(crate) async fn add_contact_to_chat_ex( if chat.typ == Chattype::Group && chat.is_promoted() { msg.viewtype = Viewtype::Text; - msg.text = stock_str::msg_add_member(context, contact.get_addr(), ContactId::SELF).await; + let contact_addr = contact.get_addr(); + msg.text = stock_str::msg_add_member_local(context, contact_addr, ContactId::SELF).await; msg.param.set_cmd(SystemMessage::MemberAddedToGroup); - msg.param.set(Param::Arg, contact.get_addr()); + msg.param.set(Param::Arg, contact_addr); msg.param.set_int(Param::Arg2, from_handshake.into()); msg.id = send_msg(context, chat_id, &mut msg).await?; } @@ -3194,11 +3195,14 @@ pub async fn remove_contact_from_chat( msg.viewtype = Viewtype::Text; if contact.id == ContactId::SELF { set_group_explicitly_left(context, &chat.grpid).await?; - msg.text = stock_str::msg_group_left(context, ContactId::SELF).await; + msg.text = stock_str::msg_group_left_local(context, ContactId::SELF).await; } else { - msg.text = - stock_str::msg_del_member(context, contact.get_addr(), ContactId::SELF) - .await; + msg.text = stock_str::msg_del_member_local( + context, + contact.get_addr(), + ContactId::SELF, + ) + .await; } msg.param.set_cmd(SystemMessage::MemberRemovedFromGroup); msg.param.set(Param::Arg, contact.get_addr()); @@ -3785,7 +3789,7 @@ mod tests { use crate::contact::{Contact, ContactAddress}; use crate::message::delete_msgs; use crate::receive_imf::receive_imf; - use crate::test_utils::TestContext; + use crate::test_utils::{TestContext, TestContextManager}; use tokio::fs; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -4002,6 +4006,83 @@ mod tests { assert_eq!(added, false); } + /// Test adding and removing members in a group chat. + /// + /// Make sure messages sent outside contain authname + /// and displayed messages contain locally set name. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_member_add_remove() -> Result<()> { + let mut tcm = TestContextManager::new(); + + let alice = tcm.alice().await; + let bob = tcm.bob().await; + + // Disable encryption so we can inspect raw message contents. + alice.set_config(Config::E2eeEnabled, Some("0")).await?; + bob.set_config(Config::E2eeEnabled, Some("0")).await?; + + // Create contact for Bob on the Alice side with name "robert". + let alice_bob_contact_id = Contact::create(&alice, "robert", "bob@example.net").await?; + + // Set Bob authname to "Bob" and send it to Alice. + bob.set_config(Config::Displayname, Some("Bob")).await?; + tcm.send_recv(&bob, &alice, "Hello!").await; + + // Check that Alice has Bob's name set to "robert" and authname set to "Bob". + { + let alice_bob_contact = Contact::get_by_id(&alice, alice_bob_contact_id).await?; + assert_eq!(alice_bob_contact.get_name(), "robert"); + + // This is the name that will be sent outside. + assert_eq!(alice_bob_contact.get_authname(), "Bob"); + + assert_eq!(alice_bob_contact.get_display_name(), "robert"); + } + + // Create and promote a group. + let alice_chat_id = + create_group_chat(&alice, ProtectionStatus::Unprotected, "Group chat").await?; + let alice_fiona_contact_id = Contact::create(&alice, "Fiona", "fiona@example.net").await?; + add_contact_to_chat(&alice, alice_chat_id, alice_fiona_contact_id).await?; + let sent = alice + .send_text(alice_chat_id, "Hi! I created a group.") + .await; + assert!(sent.payload.contains("Hi! I created a group.")); + + // Alice adds Bob to the chat. + add_contact_to_chat(&alice, alice_chat_id, alice_bob_contact_id).await?; + let sent = alice.pop_sent_msg().await; + assert!(sent + .payload + .contains("I added member Bob (bob@example.net).")); + // Locally set name "robert" should not leak. + assert!(!sent.payload.contains("robert")); + assert_eq!( + sent.load_from_db().await.get_text(), + "You added member robert (bob@example.net)." + ); + + // Alice removes Bob from the chat. + remove_contact_from_chat(&alice, alice_chat_id, alice_bob_contact_id).await?; + let sent = alice.pop_sent_msg().await; + assert!(sent + .payload + .contains("I removed member Bob (bob@example.net).")); + assert!(!sent.payload.contains("robert")); + assert_eq!( + sent.load_from_db().await.get_text(), + "You removed member robert (bob@example.net)." + ); + + // Alice leaves the chat. + remove_contact_from_chat(&alice, alice_chat_id, ContactId::SELF).await?; + let sent = alice.pop_sent_msg().await; + assert!(sent.payload.contains("I left the group.")); + assert_eq!(sent.load_from_db().await.get_text(), "You left the group."); + + Ok(()) + } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_modify_chat_multi_device() -> Result<()> { let a1 = TestContext::new_alice().await; @@ -4198,6 +4279,7 @@ mod tests { Ok(()) } + /// Test that adding or removing contacts in 1:1 chat is not allowed. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_add_remove_contact_for_single() { let ctx = TestContext::new_alice().await; diff --git a/src/contact.rs b/src/contact.rs index 8bc8f6bdd..a5a400214 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1141,11 +1141,29 @@ impl Contact { &self.addr } + /// Get a summary of authorized name and address. + /// + /// The returned string is either "Name (email@domain.com)" or just + /// "email@domain.com" if the name is unset. + /// + /// This string is suitable for sending over email + /// as it does not leak the locally set name. + pub fn get_authname_n_addr(&self) -> String { + if !self.authname.is_empty() { + format!("{} ({})", self.authname, self.addr) + } else { + (&self.addr).into() + } + } + /// Get a summary of name and address. /// /// The returned string is either "Name (email@domain.com)" or just /// "email@domain.com" if the name is unset. /// + /// The result should only be used locally and never sent over the network + /// as it leaks the local contact name. + /// /// The summary is typically used when asking the user something about the contact. /// The attached email address makes the question unique, eg. "Chat with Alan Miller (am@uniquedomain.com)?" pub fn get_name_n_addr(&self) -> String { diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 34d8f8d4a..ce0dac303 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -918,6 +918,19 @@ impl<'a> MimeFactory<'a> { match command { SystemMessage::MemberRemovedFromGroup => { let email_to_remove = self.msg.param.get(Param::Arg).unwrap_or_default(); + + if email_to_remove + == context + .get_config(Config::ConfiguredAddr) + .await? + .unwrap_or_default() + { + placeholdertext = Some(stock_str::msg_group_left_remote(context).await); + } else { + placeholdertext = + Some(stock_str::msg_del_member_remote(context, email_to_remove).await); + }; + if !email_to_remove.is_empty() { headers.protected.push(Header::new( "Chat-Group-Member-Removed".into(), @@ -927,6 +940,9 @@ impl<'a> MimeFactory<'a> { } SystemMessage::MemberAddedToGroup => { let email_to_add = self.msg.param.get(Param::Arg).unwrap_or_default(); + placeholdertext = + Some(stock_str::msg_add_member_remote(context, email_to_add).await); + if !email_to_add.is_empty() { headers.protected.push(Header::new( "Chat-Group-Member-Added".into(), @@ -1138,11 +1154,8 @@ impl<'a> MimeFactory<'a> { } else { None }; - let final_text = if let Some(ref text) = placeholdertext { - text - } else { - &self.msg.text - }; + + let final_text = placeholdertext.as_deref().unwrap_or(&self.msg.text); let mut quoted_text = self .msg diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 6f4a726cc..a146cb416 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1691,9 +1691,9 @@ async fn apply_group_changes( } better_msg = if contact_id == from_id { - Some(stock_str::msg_group_left(context, from_id).await) + Some(stock_str::msg_group_left_local(context, from_id).await) } else { - Some(stock_str::msg_del_member(context, removed_addr, from_id).await) + Some(stock_str::msg_del_member_local(context, removed_addr, from_id).await) }; } else { info!( @@ -1718,7 +1718,7 @@ async fn apply_group_changes( } } - better_msg = Some(stock_str::msg_add_member(context, added_addr, from_id).await); + better_msg = Some(stock_str::msg_add_member_local(context, added_addr, from_id).await); } else { info!(context, "Ignoring addition of {added_addr:?} to {chat_id}."); } diff --git a/src/stock_str.rs b/src/stock_str.rs index 5c917b5a1..ea19a2a72 100644 --- a/src/stock_str.rs +++ b/src/stock_str.rs @@ -410,6 +410,15 @@ pub enum StockMessage { #[strum(props(fallback = "ℹ️ Account transferred to your second device."))] BackupTransferMsgBody = 163, + + #[strum(props(fallback = "I added member %1$s."))] + MsgIAddMember = 164, + + #[strum(props(fallback = "I removed member %1$s."))] + MsgIDelMember = 165, + + #[strum(props(fallback = "I left the group."))] + MsgILeftGroup = 166, } impl StockMessage { @@ -588,17 +597,35 @@ pub(crate) async fn msg_grp_img_changed(context: &Context, by_contact: ContactId } } -/// Stock string: `Member %1$s added.`. +/// Stock string: `I added member %1$s.`. +/// +/// The `added_member_addr` parameter should be an email address and is looked up in the +/// contacts to combine with the authorized display name. +pub(crate) async fn msg_add_member_remote(context: &Context, added_member_addr: &str) -> String { + let addr = added_member_addr; + let whom = &match Contact::lookup_id_by_addr(context, addr, Origin::Unknown).await { + Ok(Some(contact_id)) => Contact::get_by_id(context, contact_id) + .await + .map(|contact| contact.get_authname_n_addr()) + .unwrap_or_else(|_| addr.to_string()), + _ => addr.to_string(), + }; + translated(context, StockMessage::MsgIAddMember) + .await + .replace1(whom) +} + +/// Stock string: `You added member %1$s.` or `Member %1$s added by %2$s.`. /// /// The `added_member_addr` parameter should be an email address and is looked up in the /// contacts to combine with the display name. -pub(crate) async fn msg_add_member( +pub(crate) async fn msg_add_member_local( context: &Context, added_member_addr: &str, by_contact: ContactId, ) -> String { let addr = added_member_addr; - let who = &match Contact::lookup_id_by_addr(context, addr, Origin::Unknown).await { + let whom = &match Contact::lookup_id_by_addr(context, addr, Origin::Unknown).await { Ok(Some(contact_id)) => Contact::get_by_id(context, contact_id) .await .map(|contact| contact.get_name_n_addr()) @@ -608,26 +635,44 @@ pub(crate) async fn msg_add_member( if by_contact == ContactId::SELF { translated(context, StockMessage::MsgYouAddMember) .await - .replace1(who) + .replace1(whom) } else { translated(context, StockMessage::MsgAddMemberBy) .await - .replace1(who) + .replace1(whom) .replace2(&by_contact.get_stock_name(context).await) } } -/// Stock string: `Member %1$s removed.`. +/// Stock string: `I removed member %1$s.`. /// /// The `removed_member_addr` parameter should be an email address and is looked up in /// the contacts to combine with the display name. -pub(crate) async fn msg_del_member( +pub(crate) async fn msg_del_member_remote(context: &Context, removed_member_addr: &str) -> String { + let addr = removed_member_addr; + let whom = &match Contact::lookup_id_by_addr(context, addr, Origin::Unknown).await { + Ok(Some(contact_id)) => Contact::get_by_id(context, contact_id) + .await + .map(|contact| contact.get_authname_n_addr()) + .unwrap_or_else(|_| addr.to_string()), + _ => addr.to_string(), + }; + translated(context, StockMessage::MsgIDelMember) + .await + .replace1(whom) +} + +/// Stock string: `I added member %1$s.` or `Member %1$s removed by %2$s.`. +/// +/// The `removed_member_addr` parameter should be an email address and is looked up in +/// the contacts to combine with the display name. +pub(crate) async fn msg_del_member_local( context: &Context, removed_member_addr: &str, by_contact: ContactId, ) -> String { let addr = removed_member_addr; - let who = &match Contact::lookup_id_by_addr(context, addr, Origin::Unknown).await { + let whom = &match Contact::lookup_id_by_addr(context, addr, Origin::Unknown).await { Ok(Some(contact_id)) => Contact::get_by_id(context, contact_id) .await .map(|contact| contact.get_name_n_addr()) @@ -637,17 +682,22 @@ pub(crate) async fn msg_del_member( if by_contact == ContactId::SELF { translated(context, StockMessage::MsgYouDelMember) .await - .replace1(who) + .replace1(whom) } else { translated(context, StockMessage::MsgDelMemberBy) .await - .replace1(who) + .replace1(whom) .replace2(&by_contact.get_stock_name(context).await) } } -/// Stock string: `Group left.`. -pub(crate) async fn msg_group_left(context: &Context, by_contact: ContactId) -> String { +/// Stock string: `I left the group.`. +pub(crate) async fn msg_group_left_remote(context: &Context) -> String { + translated(context, StockMessage::MsgILeftGroup).await +} + +/// Stock string: `You left the group.` or `Group left by %1$s.`. +pub(crate) async fn msg_group_left_local(context: &Context, by_contact: ContactId) -> String { if by_contact == ContactId::SELF { translated(context, StockMessage::MsgYouLeftGroup).await } else { @@ -1409,7 +1459,11 @@ mod tests { async fn test_stock_system_msg_add_member_by_me() { let t = TestContext::new().await; assert_eq!( - msg_add_member(&t, "alice@example.org", ContactId::SELF).await, + msg_add_member_remote(&t, "alice@example.org").await, + "I added member alice@example.org." + ); + assert_eq!( + msg_add_member_local(&t, "alice@example.org", ContactId::SELF).await, "You added member alice@example.org." ) } @@ -1421,7 +1475,11 @@ mod tests { .await .expect("failed to create contact"); assert_eq!( - msg_add_member(&t, "alice@example.org", ContactId::SELF).await, + msg_add_member_remote(&t, "alice@example.org").await, + "I added member alice@example.org." + ); + assert_eq!( + msg_add_member_local(&t, "alice@example.org", ContactId::SELF).await, "You added member Alice (alice@example.org)." ); } @@ -1438,7 +1496,7 @@ mod tests { .expect("failed to create bob") }; assert_eq!( - msg_add_member(&t, "alice@example.org", contact_id,).await, + msg_add_member_local(&t, "alice@example.org", contact_id,).await, "Member Alice (alice@example.org) added by Bob (bob@example.com)." ); }