fix: do not fail to send the message if some keys are missing

Author: link2xt

src/e2ee.rs

@@ -1,8 +1,9 @@
 //! End-to-end encryption support.
 
+use std::collections::BTreeSet;
 use std::io::Cursor;
 
-use anyhow::{format_err, Context as _, Result};
+use anyhow::{bail, Result};
 use mail_builder::mime::MimePart;
 use num_traits::FromPrimitive;
 
@@ -42,70 +43,76 @@ impl EncryptHelper {
     }
 
     /// Determines if we can and should encrypt.
-    ///
-    /// `e2ee_guaranteed` should be set to true for replies to encrypted messages (as required by
-    /// Autocrypt Level 1, version 1.1) and for messages sent in protected groups.
-    ///
-    /// Returns an error if `e2ee_guaranteed` is true, but one or more keys are missing.
     pub(crate) async fn should_encrypt(
         &self,
         context: &Context,
-        e2ee_guaranteed: bool,
         peerstates: &[(Option<Peerstate>, String)],
     ) -> Result<bool> {
         let is_chatmail = context.is_chatmail().await?;
-        let missing_peerstate_addr = peerstates.iter().find_map(|(peerstate, addr)| {
+        for (peerstate, _addr) in peerstates {
             if let Some(peerstate) = peerstate {
-                if is_chatmail
-                    || e2ee_guaranteed
-                    || peerstate.prefer_encrypt != EncryptPreference::Reset
-                {
-                    return None;
+                // For chatmail we ignore the encryption preference,
+                // because we can either send encrypted or not at all.
+                if is_chatmail || peerstate.prefer_encrypt != EncryptPreference::Reset {
+                    continue;
                 }
             }
-            Some(addr)
-        });
-        if let Some(addr) = missing_peerstate_addr {
-            if e2ee_guaranteed {
-                return Err(format_err!(
-                    "Peerstate for {addr:?} missing, cannot encrypt"
-                ));
-            }
+            return Ok(false);
         }
-        Ok(missing_peerstate_addr.is_none())
+        Ok(true)
     }
 
-    /// Tries to encrypt the passed in `mail`.
-    pub async fn encrypt(
-        self,
+    /// Constructs a vector of public keys for given peerstates.
+    ///
+    /// In addition returns the set of recipient addresses
+    /// for which there is no key available.
+    ///
+    /// Returns an error if there are recipients
+    /// other than self, but no recipient keys are available.
+    pub(crate) fn encryption_keyring(
+        &self,
         context: &Context,
         verified: bool,
-        mail_to_encrypt: MimePart<'static>,
-        peerstates: Vec<(Option<Peerstate>, String)>,
-        compress: bool,
-    ) -> Result<String> {
-        let mut keyring: Vec<SignedPublicKey> = Vec::new();
+        peerstates: &[(Option<Peerstate>, String)],
+    ) -> Result<(Vec<SignedPublicKey>, BTreeSet<String>)> {
+        // Encrypt to self unconditionally,
+        // even for a single-device setup.
+        let mut keyring = vec![self.public_key.clone()];
+        let mut missing_key_addresses = BTreeSet::new();
+
+        if peerstates.is_empty() {
+            return Ok((keyring, missing_key_addresses));
+        }
 
         let mut verifier_addresses: Vec<&str> = Vec::new();
 
-        for (peerstate, addr) in peerstates
-            .iter()
-            .filter_map(|(state, addr)| state.clone().map(|s| (s, addr)))
-        {
-            let key = peerstate
-                .take_key(verified)
-                .with_context(|| format!("proper enc-key for {addr} missing, cannot encrypt"))?;
-            keyring.push(key);
-            verifier_addresses.push(addr);
+        for (peerstate, addr) in peerstates {
+            if let Some(peerstate) = peerstate {
+                if let Some(key) = peerstate.clone().take_key(verified) {
+                    keyring.push(key);
+                    verifier_addresses.push(addr);
+                } else {
+                    warn!(context, "Encryption key for {addr} is missing.");
+                    missing_key_addresses.insert(addr.clone());
+                }
+            } else {
+                warn!(context, "Peerstate for {addr} is missing.");
+                missing_key_addresses.insert(addr.clone());
+            }
         }
 
-        // Encrypt to self.
-        keyring.push(self.public_key.clone());
+        debug_assert!(
+            !keyring.is_empty(),
+            "At least our own key is in the keyring"
+        );
+        if keyring.len() <= 1 {
+            bail!("No recipient keys are available, cannot encrypt");
+        }
 
         // Encrypt to secondary verified keys
         // if we also encrypt to the introducer ("verifier") of the key.
         if verified {
-            for (peerstate, _addr) in &peerstates {
+            for (peerstate, _addr) in peerstates {
                 if let Some(peerstate) = peerstate {
                     if let (Some(key), Some(verifier)) = (
                         peerstate.secondary_verified_key.as_ref(),
@@ -119,6 +126,17 @@ impl EncryptHelper {
             }
         }
 
+        Ok((keyring, missing_key_addresses))
+    }
+
+    /// Tries to encrypt the passed in `mail`.
+    pub async fn encrypt(
+        self,
+        context: &Context,
+        keyring: Vec<SignedPublicKey>,
+        mail_to_encrypt: MimePart<'static>,
+        compress: bool,
+    ) -> Result<String> {
         let sign_key = load_self_secret_key(context).await?;
 
         let mut raw_message = Vec::new();
@@ -315,22 +333,17 @@ Sent with my Delta Chat Messenger: https://delta.chat";
         let encrypt_helper = EncryptHelper::new(&t).await.unwrap();
 
         let ps = new_peerstates(EncryptPreference::NoPreference);
-        assert!(encrypt_helper.should_encrypt(&t, true, &ps).await?);
-        // Own preference is `Mutual` and we have the peer's key.
-        assert!(encrypt_helper.should_encrypt(&t, false, &ps).await?);
+        assert!(encrypt_helper.should_encrypt(&t, &ps).await?);
 
         let ps = new_peerstates(EncryptPreference::Reset);
-        assert!(encrypt_helper.should_encrypt(&t, true, &ps).await?);
-        assert!(!encrypt_helper.should_encrypt(&t, false, &ps).await?);
+        assert!(!encrypt_helper.should_encrypt(&t, &ps).await?);
 
         let ps = new_peerstates(EncryptPreference::Mutual);
-        assert!(encrypt_helper.should_encrypt(&t, true, &ps).await?);
-        assert!(encrypt_helper.should_encrypt(&t, false, &ps).await?);
+        assert!(encrypt_helper.should_encrypt(&t, &ps).await?);
 
         // test with missing peerstate
         let ps = vec![(None, "bob@foo.bar".to_string())];
-        assert!(encrypt_helper.should_encrypt(&t, true, &ps).await.is_err());
-        assert!(!encrypt_helper.should_encrypt(&t, false, &ps).await?);
+        assert!(!encrypt_helper.should_encrypt(&t, &ps).await?);
         Ok(())
     }
 

src/message.rs

@@ -1732,6 +1732,10 @@ pub async fn delete_msgs_ex(
             !delete_for_all || msg.from_id == ContactId::SELF,
             "Can delete only own messages for others"
         );
+        ensure!(
+            !delete_for_all || msg.get_showpadlock(),
+            "Cannot request deletion of unencrypted message for others"
+        );
 
         modified_chat_ids.insert(msg.chat_id);
         deleted_rfc724_mid.push(msg.rfc724_mid.clone());

src/mimefactory.rs

@@ -86,7 +86,9 @@ pub struct MimeFactory {
     /// Vector of pairs of recipient name and address that goes into the `To` field.
     ///
     /// The list of actual message recipient addresses may be different,
-    /// e.g. if members are hidden for broadcast lists.
+    /// e.g. if members are hidden for broadcast lists
+    /// or if the keys for some recipients are missing
+    /// and encrypted message cannot be sent to them.
     to: Vec<(String, String)>,
 
     /// Vector of pairs of past group member names and addresses.
@@ -784,9 +786,7 @@ impl MimeFactory {
 
         let peerstates = self.peerstates_for_recipients(context).await?;
         let is_encrypted = !self.should_force_plaintext()
-            && encrypt_helper
-                .should_encrypt(context, e2ee_guaranteed, &peerstates)
-                .await?;
+            && (e2ee_guaranteed || encrypt_helper.should_encrypt(context, &peerstates).await?);
         let is_securejoin_message = if let Loaded::Message { msg, .. } = &self.loaded {
             msg.param.get_cmd() == SystemMessage::SecurejoinMessage
         } else {
@@ -982,14 +982,23 @@ impl MimeFactory {
                 Loaded::Mdn { .. } => true,
             };
 
+            let (encryption_keyring, missing_key_addresses) =
+                encrypt_helper.encryption_keyring(context, verified, &peerstates)?;
+
             // XXX: additional newline is needed
             // to pass filtermail at
             // <https://github.com/deltachat/chatmail/blob/4d915f9800435bf13057d41af8d708abd34dbfa8/chatmaild/src/chatmaild/filtermail.py#L84-L86>
             let encrypted = encrypt_helper
-                .encrypt(context, verified, message, peerstates, compress)
+                .encrypt(context, encryption_keyring, message, compress)
                 .await?
                 + "\n";
 
+            // Remove recipients for which the key is missing.
+            if !missing_key_addresses.is_empty() {
+                self.recipients
+                    .retain(|addr| !missing_key_addresses.contains(addr));
+            }
+
             // Set the appropriate Content-Type for the outer message
             MimePart::new(
                 "multipart/encrypted; protocol=\"application/pgp-encrypted\"",

src/receive_imf/receive_imf_tests.rs

@@ -4924,11 +4924,10 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> {
     let fiona_addr = fiona.get_config(Config::Addr).await?.unwrap();
     mark_as_verified(alice, fiona).await;
     let alice_fiona_id = alice.add_or_lookup_contact(fiona).await.id;
-    assert!(add_contact_to_chat(alice, group_id, alice_fiona_id)
-        .await
-        .is_err());
-    // Sending the message failed,
-    // but member is added to the chat locally already.
+    add_contact_to_chat(alice, group_id, alice_fiona_id).await?;
+
+    // The message is not sent to Bob,
+    // but member is added to the chat locally anyway.
     assert!(is_contact_in_chat(alice, group_id, alice_fiona_id).await?);
     let msg = alice.get_last_msg_in(group_id).await;
     assert!(msg.is_info());
@@ -4937,10 +4936,6 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> {
         stock_str::msg_add_member_local(alice, &fiona_addr, ContactId::SELF).await
     );
 
-    // Now the chat has a message "You added member fiona@example.net. [INFO] !!" (with error) that
-    // may be confusing, but if the error is displayed in UIs, it's more or less ok. This is not a
-    // normal scenario anyway.
-
     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;
@@ -4949,7 +4944,6 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> {
         msg.get_text(),
         stock_str::msg_del_member_local(alice, &bob_addr, ContactId::SELF,).await
     );
-    assert!(msg.error().is_some());
     Ok(())
 }
 

src/tests/verified_chats.rs

@@ -1,7 +1,9 @@
 use anyhow::Result;
 use pretty_assertions::assert_eq;
 
-use crate::chat::{self, add_contact_to_chat, Chat, ProtectionStatus};
+use crate::chat::{
+    self, add_contact_to_chat, remove_contact_from_chat, send_msg, Chat, ProtectionStatus,
+};
 use crate::chatlist::Chatlist;
 use crate::config::Config;
 use crate::constants::{Chattype, DC_GCL_FOR_FORWARDING};
@@ -953,6 +955,70 @@ async fn test_no_unencrypted_name_if_encrypted() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_verified_lost_member_added() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = &tcm.alice().await;
+    let bob = &tcm.bob().await;
+    let fiona = &tcm.fiona().await;
+
+    tcm.execute_securejoin(bob, alice).await;
+    tcm.execute_securejoin(fiona, alice).await;
+
+    let alice_chat_id = alice
+        .create_group_with_members(ProtectionStatus::Protected, "Group", &[bob])
+        .await;
+    let alice_sent = alice.send_text(alice_chat_id, "Hi!").await;
+    let bob_chat_id = bob.recv_msg(&alice_sent).await.chat_id;
+    assert_eq!(chat::get_chat_contacts(bob, bob_chat_id).await?.len(), 2);
+
+    // Attempt to add member, but message is lost.
+    let fiona_id = alice.add_or_lookup_contact(fiona).await.id;
+    add_contact_to_chat(alice, alice_chat_id, fiona_id).await?;
+    alice.pop_sent_msg().await;
+
+    let alice_sent = alice.send_text(alice_chat_id, "Hi again!").await;
+    bob.recv_msg(&alice_sent).await;
+    assert_eq!(chat::get_chat_contacts(bob, bob_chat_id).await?.len(), 3);
+
+    bob_chat_id.accept(bob).await?;
+    let sent = bob.send_text(bob_chat_id, "Hello!").await;
+    let sent_msg = Message::load_from_db(bob, sent.sender_msg_id).await?;
+    assert_eq!(sent_msg.get_showpadlock(), true);
+
+    // The message will not be sent to Fiona.
+    // Test that Fiona will not be able to decrypt it.
+    let fiona_rcvd = fiona.recv_msg(&sent).await;
+    assert_eq!(fiona_rcvd.get_showpadlock(), false);
+    assert_eq!(
+        fiona_rcvd.get_text(),
+        "[...] – [This message was encrypted for another setup.]"
+    );
+
+    // Advance the time so Alice does not leave at the same second
+    // as the group was created.
+    SystemTime::shift(std::time::Duration::from_secs(100));
+
+    // Alice leaves the chat.
+    remove_contact_from_chat(alice, alice_chat_id, ContactId::SELF).await?;
+    assert_eq!(
+        chat::get_chat_contacts(alice, alice_chat_id).await?.len(),
+        2
+    );
+    bob.recv_msg(&alice.pop_sent_msg().await).await;
+
+    // Now only Bob and Fiona are in the chat.
+    assert_eq!(chat::get_chat_contacts(bob, bob_chat_id).await?.len(), 2);
+
+    // Bob cannot send messages anymore because there are no recipients
+    // other than self for which Bob has the key.
+    let mut msg = Message::new_text("No key for Fiona".to_string());
+    let result = send_msg(bob, bob_chat_id, &mut msg).await;
+    assert!(result.is_err());
+
+    Ok(())
+}
+
 // ============== Helper Functions ==============
 
 async fn assert_verified(this: &TestContext, other: &TestContext, protected: ProtectionStatus) {