fix: Don't send selfavatar in SecureJoin messages before contact verification (#5354)

Don't attach selfavatar in "v{c,g}-request" and "v{c,g}-auth-required" messages:
- These messages are deleted right after processing, so other devices won't see the avatar.
- It's also good for privacy because the contact isn't yet verified and these messages are auto-sent
  unlike usual unencrypted messages.

python/tests/test_0_complex_or_slow.py

@@ -547,13 +547,14 @@ def test_see_new_verified_member_after_going_online(acfactory, tmp_path, lp):
     assert msg_in.get_sender_contact().addr == ac2_addr
 
 
-def test_use_new_verified_group_after_going_online(acfactory, tmp_path, lp):
+def test_use_new_verified_group_after_going_online(acfactory, data, tmp_path, lp):
     """Another test for the bug #3836:
     - Bob has two devices, the second is offline.
     - Alice creates a verified group and sends a QR invitation to Bob.
     - Bob joins the group.
     - Bob's second devices goes online, but sees a contact request instead of the verified group.
     - The "member added" message is not a system message but a plain text message.
+    - Bob's second device doesn't display the Alice's avatar (bug #5354).
     - Sending a message fails as the key is missing -- message info says "proper enc-key for <Alice>
       missing, cannot encrypt".
     """
@@ -568,6 +569,10 @@ def test_use_new_verified_group_after_going_online(acfactory, tmp_path, lp):
     ac2_offl.import_self_keys(str(dir))
     ac2_offl.stop_io()
 
+    lp.sec("ac1: set avatar")
+    avatar_path = data.get_path("d.png")
+    ac1.set_avatar(avatar_path)
+
     lp.sec("ac1: create verified-group QR, ac2 scans and joins")
     chat = ac1.create_group_chat("hello", verified=True)
     assert chat.is_protected()
@@ -580,11 +585,13 @@ def test_use_new_verified_group_after_going_online(acfactory, tmp_path, lp):
     ac2_offl.start_io()
     # Receive "Member Me (<addr>) added by <addr>." message.
     msg_in = ac2_offl._evtracker.wait_next_incoming_message()
+    contact = msg_in.get_sender_contact()
     assert msg_in.is_system_message()
-    assert msg_in.get_sender_contact().addr == ac1.get_config("addr")
+    assert contact.addr == ac1.get_config("addr")
     chat2 = msg_in.chat
     assert chat2.is_protected()
     assert chat2.get_messages()[0].text == "Messages are guaranteed to be end-to-end encrypted from now on."
+    assert open(contact.get_profile_image(), "rb").read() == open(avatar_path, "rb").read()
 
     lp.sec("ac2_offl: sending message")
     msg_out = chat2.send_text("hello")

src/chat.rs

@@ -2742,17 +2742,8 @@ async fn prepare_send_msg(
 /// The caller has to interrupt SMTP loop or otherwise process new rows.
 pub(crate) async fn create_send_msg_jobs(context: &Context, msg: &mut Message) -> Result<Vec<i64>> {
     let needs_encryption = msg.param.get_bool(Param::GuaranteeE2ee).unwrap_or_default();
-
+    let mimefactory = MimeFactory::from_msg(context, msg).await?;
-    let attach_selfavatar = match shall_attach_selfavatar(context, msg.chat_id).await {
+    let attach_selfavatar = mimefactory.attach_selfavatar;
-        Ok(attach_selfavatar) => attach_selfavatar,
-        Err(err) => {
-            warn!(context, "SMTP job cannot get selfavatar-state: {err:#}.");
-            false
-        }
-    };
-
-    let mimefactory = MimeFactory::from_msg(context, msg, attach_selfavatar).await?;
-
     let mut recipients = mimefactory.recipients();
 
     let from = context.get_primary_self_addr().await?;

src/mimefactory.rs

@@ -10,7 +10,7 @@ use lettre_email::{Address, Header, MimeMultipartType, PartBuilder};
 use tokio::fs;
 
 use crate::blob::BlobObject;
-use crate::chat::Chat;
+use crate::chat::{self, Chat};
 use crate::config::Config;
 use crate::constants::{Chattype, DC_FROM_HANDSHAKE};
 use crate::contact::Contact;
@@ -83,7 +83,7 @@ pub struct MimeFactory<'a> {
     sync_ids_to_delete: Option<String>,
 
     /// True if the avatar should be attached.
-    attach_selfavatar: bool,
+    pub attach_selfavatar: bool,
 }
 
 /// Result of rendering a message, ready to be submitted to a send job.
@@ -138,11 +138,7 @@ struct MessageHeaders {
 }
 
 impl<'a> MimeFactory<'a> {
-    pub async fn from_msg(
+    pub async fn from_msg(context: &Context, msg: &'a Message) -> Result<MimeFactory<'a>> {
-        context: &Context,
-        msg: &'a Message,
-        attach_selfavatar: bool,
-    ) -> Result<MimeFactory<'a>> {
         let chat = Chat::load_from_db(context, msg.chat_id).await?;
 
         let from_addr = context.get_primary_self_addr().await?;
@@ -217,6 +213,7 @@ impl<'a> MimeFactory<'a> {
                 },
             )
             .await?;
+        let attach_selfavatar = Self::should_attach_selfavatar(context, msg).await;
 
         let factory = MimeFactory {
             from_addr,
@@ -392,6 +389,31 @@ impl<'a> MimeFactory<'a> {
         }
     }
 
+    async fn should_attach_selfavatar(context: &Context, msg: &Message) -> bool {
+        let cmd = msg.param.get_cmd();
+        (cmd != SystemMessage::SecurejoinMessage || {
+            let step = msg.param.get(Param::Arg).unwrap_or_default();
+            // Don't attach selfavatar at the earlier SecureJoin steps:
+            // - The corresponding messages, i.e. "v{c,g}-request" and "v{c,g}-auth-required" are
+            //   deleted right after processing, so other devices won't see the avatar.
+            // - It's also good for privacy because the contact isn't yet verified and these
+            //   messages are auto-sent unlike usual unencrypted messages.
+            step == "vg-request-with-auth"
+                || step == "vc-request-with-auth"
+                || step == "vg-member-added"
+                || step == "vc-contact-confirm"
+        }) && match chat::shall_attach_selfavatar(context, msg.chat_id).await {
+            Ok(should) => should,
+            Err(err) => {
+                warn!(
+                    context,
+                    "should_attach_selfavatar: cannot get selfavatar state: {err:#}."
+                );
+                false
+            }
+        }
+    }
+
     fn grpimage(&self) -> Option<String> {
         match &self.loaded {
             Loaded::Message { chat } => {
@@ -1836,7 +1858,7 @@ mod tests {
                  Original-Message-ID: <2893@example.com>\n\
                  Disposition: manual-action/MDN-sent-automatically; displayed\n\
                  \n", &t).await;
-        let mf = MimeFactory::from_msg(&t, &new_msg, false).await.unwrap();
+        let mf = MimeFactory::from_msg(&t, &new_msg).await.unwrap();
         // The subject string should not be "Re: message opened"
         assert_eq!("Re: Hello, Bob", mf.subject_str(&t).await.unwrap());
     }
@@ -1971,7 +1993,7 @@ mod tests {
         new_msg.chat_id = chat_id;
         chat::prepare_msg(&t, chat_id, &mut new_msg).await.unwrap();
 
-        let mf = MimeFactory::from_msg(&t, &new_msg, false).await.unwrap();
+        let mf = MimeFactory::from_msg(&t, &new_msg).await.unwrap();
 
         mf.subject_str(&t).await.unwrap()
     }
@@ -2056,7 +2078,7 @@ mod tests {
             new_msg.set_quote(&t, Some(&incoming_msg)).await.unwrap();
         }
 
-        let mf = MimeFactory::from_msg(&t, &new_msg, false).await.unwrap();
+        let mf = MimeFactory::from_msg(&t, &new_msg).await.unwrap();
         mf.subject_str(&t).await.unwrap()
     }
 
@@ -2104,7 +2126,7 @@ mod tests {
         )
         .await;
 
-        let mimefactory = MimeFactory::from_msg(&t, &msg, false).await.unwrap();
+        let mimefactory = MimeFactory::from_msg(&t, &msg).await.unwrap();
 
         let recipients = mimefactory.recipients();
         assert_eq!(recipients, vec!["charlie@example.com"]);