fix: use different member added/removal messages locally and on the network

This commit adds new stock strings
"I added member ...",
"I removed member ..." and
"I left the group" that are sent over the network
and are visible in classic MUAs like Thunderbird.

Member name in these messages uses authname
instead of the display name,
so the name set locally does not get leaked when
a member is added or removed.

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;

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 {

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

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}.");
         }

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)."
         );
     }