Crypto: Return an error when sharing a room key to a verified user, who has an unverified device (optional encryption setting)

bindings/matrix-sdk-crypto-ffi/src/lib.rs

@@ -668,6 +668,8 @@ pub struct EncryptionSettings {
     /// Should untrusted devices receive the room key, or should they be
     /// excluded from the conversation.
     pub only_allow_trusted_devices: bool,
+    /// Should fail to send when a verified user has unverified devices.
+    pub error_on_verified_user_problem: bool,
 }
 
 impl From<EncryptionSettings> for RustEncryptionSettings {
@@ -677,7 +679,10 @@ impl From<EncryptionSettings> for RustEncryptionSettings {
             rotation_period: Duration::from_secs(v.rotation_period),
             rotation_period_msgs: v.rotation_period_msgs,
             history_visibility: v.history_visibility.into(),
-            sharing_strategy: CollectStrategy::new_device_based(v.only_allow_trusted_devices),
+            sharing_strategy: CollectStrategy::DeviceBasedStrategy {
+                only_allow_trusted_devices: v.only_allow_trusted_devices,
+                error_on_verified_user_problem: v.error_on_verified_user_problem,
+            },
         }
     }
 }

crates/matrix-sdk-base/src/client.rs

@@ -28,8 +28,8 @@ use futures_util::Stream;
 use matrix_sdk_common::instant::Instant;
 #[cfg(feature = "e2e-encryption")]
 use matrix_sdk_crypto::{
-    store::DynCryptoStore, EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine,
-    ToDeviceRequest,
+    store::DynCryptoStore, CollectStrategy, EncryptionSettings, EncryptionSyncChanges, OlmError,
+    OlmMachine, ToDeviceRequest,
 };
 #[cfg(feature = "e2e-encryption")]
 use ruma::events::{
@@ -1391,7 +1391,14 @@ impl BaseClient {
                 let members = self.store.get_user_ids(room_id, filter).await?;
 
                 let settings = settings.ok_or(Error::EncryptionNotEnabled)?;
-                let settings = EncryptionSettings::new(settings, history_visibility, false);
+                let settings = EncryptionSettings::new(
+                    settings,
+                    history_visibility,
+                    CollectStrategy::DeviceBasedStrategy {
+                        only_allow_trusted_devices: false,
+                        error_on_verified_user_problem: false,
+                    },
+                );
 
                 Ok(o.share_room_key(room_id, members.iter().map(Deref::deref), settings).await?)
             }

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -32,6 +32,18 @@ Changes:
 
 Breaking changes:
 
+- Add a new `error_on_verified_user_problem` property to
+  `CollectStrategy::DeviceBasedStrategy`, which, when set, causes
+  `OlmMachine::share_room_key` to fail with an error if any verified users on
+  the recipient list have unsigned devices.
+
+  Also remove `CollectStrategy::new_device_based`: callers should construct a
+  `CollectStrategy::DeviceBasedStrategy` directly.
+
+  `EncryptionSettings::new` now takes a `CollectStrategy` argument, instead of
+  a list of booleans.
+  ([#3810](https://github.com/matrix-org/matrix-rust-sdk/pull/3810))
+
 - Remove the method `OlmMachine::clear_crypto_cache()`, crypto stores are not
   supposed to have any caches anymore.
 

crates/matrix-sdk-crypto/src/error.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::collections::BTreeMap;
+
 use ruma::{CanonicalJsonError, IdParseError, OwnedDeviceId, OwnedRoomId, OwnedUserId};
 use serde::{ser::SerializeMap, Serializer};
 use serde_json::Error as SerdeError;
@@ -23,6 +25,8 @@ use crate::{
     olm::SessionExportError,
     types::{events::room_key_withheld::WithheldCode, SignedKey},
 };
+#[cfg(doc)]
+use crate::{CollectStrategy, Device, LocalTrust};
 
 pub type OlmResult<T> = Result<T, OlmError>;
 pub type MegolmResult<T> = Result<T, MegolmError>;
@@ -70,6 +74,10 @@ pub enum OlmError {
             have a valid Olm session with us"
     )]
     MissingSession,
+
+    /// Encryption failed due to an error collecting the recipient devices.
+    #[error("encryption failed due to an error collecting the recipient devices: {0}")]
+    SessionRecipientCollectionError(SessionRecipientCollectionError),
 }
 
 /// Error representing a failure during a group encryption operation.
@@ -360,3 +368,21 @@ pub enum SetRoomSettingsError {
     #[error(transparent)]
     Store(#[from] CryptoStoreError),
 }
+
+/// Error representing a problem when collecting the recipient devices for the
+/// room key, during an encryption operation.
+#[derive(Error, Debug)]
+pub enum SessionRecipientCollectionError {
+    /// One or more verified users has one or more unsigned devices.
+    ///
+    /// Happens only with [`CollectStrategy::DeviceBasedStrategy`] when
+    /// [`error_on_verified_user_problem`](`CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem`)
+    /// is true.
+    ///
+    /// In order to resolve this, the caller can set the trust level of the
+    /// affected devices to [`LocalTrust::Ignored`] or
+    /// [`LocalTrust::BlackListed`] (see [`Device::set_local_trust`]), and
+    /// then retry the encryption operation.
+    #[error("one or more verified users have unsigned devices")]
+    VerifiedUserHasUnsignedDevice(BTreeMap<OwnedUserId, Vec<OwnedDeviceId>>),
+}

crates/matrix-sdk-crypto/src/identities/device.rs

@@ -297,9 +297,7 @@ impl Device {
         self.device_owner_identity.as_ref().is_some_and(|id| match id {
             UserIdentityData::Own(own_identity) => own_identity.is_verified(),
             UserIdentityData::Other(other_identity) => {
-                self.own_identity.as_ref().is_some_and(|oi| {
-                    oi.is_verified() && oi.is_identity_signed(other_identity).is_ok()
-                })
+                self.own_identity.as_ref().is_some_and(|oi| oi.is_identity_verified(other_identity))
             }
         })
     }
@@ -744,21 +742,19 @@ impl DeviceData {
     ) -> bool {
         own_identity.as_ref().zip(device_owner.as_ref()).is_some_and(
             |(own_identity, device_identity)| {
-                // Our own identity needs to be marked as verified.
-                own_identity.is_verified()
-                    && match device_identity {
-                        // If it's one of our own devices, just check that
-                        // we signed the device.
-                        UserIdentityData::Own(_) => own_identity.is_device_signed(self).is_ok(),
-
-                        // If it's a device from someone else, first check
-                        // that our user has signed the other user and then
-                        // check if the other user has signed this device.
-                        UserIdentityData::Other(device_identity) => {
-                            own_identity.is_identity_signed(device_identity).is_ok()
-                                && device_identity.is_device_signed(self).is_ok()
-                        }
+                match device_identity {
+                    UserIdentityData::Own(_) => {
+                        own_identity.is_verified() && own_identity.is_device_signed(self).is_ok()
                     }
+
+                    // If it's a device from someone else, first check
+                    // that our user has verified the other user and then
+                    // check if the other user has signed this device.
+                    UserIdentityData::Other(device_identity) => {
+                        own_identity.is_identity_verified(device_identity)
+                            && device_identity.is_device_signed(self).is_ok()
+                    }
+                }
             },
         )
     }

crates/matrix-sdk-crypto/src/identities/user.rs

@@ -228,11 +228,9 @@ impl Deref for UserIdentity {
 impl UserIdentity {
     /// Is this user identity verified.
     pub fn is_verified(&self) -> bool {
-        self.own_identity.as_ref().is_some_and(|own_identity| {
-            // The identity of another user is verified iff our own identity is verified and
-            // if our own identity has signed the other user's identity.
-            own_identity.is_verified() && own_identity.is_identity_signed(&self.inner).is_ok()
-        })
+        self.own_identity
+            .as_ref()
+            .is_some_and(|own_identity| own_identity.is_identity_verified(&self.inner))
     }
 
     /// Manually verify this user.
@@ -886,8 +884,26 @@ impl OwnUserIdentityData {
         &self.user_signing_key
     }
 
+    /// Check if the given user identity has been verified.
+    ///
+    /// The identity of another user is verified iff our own identity is
+    /// verified and if our own identity has signed the other user's
+    /// identity.
+    ///
+    /// # Arguments
+    ///
+    /// * `identity` - The identity of another user which we want to check has
+    ///   been verified.
+    pub fn is_identity_verified(&self, identity: &OtherUserIdentityData) -> bool {
+        self.is_verified() && self.is_identity_signed(identity).is_ok()
+    }
+
     /// Check if the given identity has been signed by this identity.
     ///
+    /// Note that, normally, you'll also want to check that the
+    /// `OwnUserIdentityData` has been verified; for that,
+    /// [`Self::is_identity_verified`] is more appropriate.
+    ///
     /// # Arguments
     ///
     /// * `identity` - The identity of another user that we want to check if it

crates/matrix-sdk-crypto/src/lib.rs

@@ -71,7 +71,8 @@ impl RoomKeyImportResult {
 }
 
 pub use error::{
-    EventError, MegolmError, OlmError, SessionCreationError, SetRoomSettingsError, SignatureError,
+    EventError, MegolmError, OlmError, SessionCreationError, SessionRecipientCollectionError,
+    SetRoomSettingsError, SignatureError,
 };
 pub use file_encryption::{
     decrypt_room_key_export, encrypt_room_key_export, AttachmentDecryptor, AttachmentEncryptor,

crates/matrix-sdk-crypto/src/machine/tests/mod.rs

@@ -587,7 +587,10 @@ async fn test_withheld_unverified() {
 
     let encryption_settings = EncryptionSettings::default();
     let encryption_settings = EncryptionSettings {
-        sharing_strategy: CollectStrategy::new_device_based(true),
+        sharing_strategy: CollectStrategy::DeviceBasedStrategy {
+            only_allow_trusted_devices: true,
+            error_on_verified_user_problem: false,
+        },
         ..encryption_settings
     };
 

crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs

@@ -116,12 +116,11 @@ impl Default for EncryptionSettings {
 
 impl EncryptionSettings {
     /// Create new encryption settings using an `RoomEncryptionEventContent`,
-    /// a history visibility, and setting if only trusted devices should receive
-    /// a room key.
+    /// a history visibility, and key sharing strategy.
     pub fn new(
         content: RoomEncryptionEventContent,
         history_visibility: HistoryVisibility,
-        only_allow_trusted_devices: bool,
+        sharing_strategy: CollectStrategy,
     ) -> Self {
         let rotation_period: Duration =
             content.rotation_period_ms.map_or(ROTATION_PERIOD, |r| Duration::from_millis(r.into()));
@@ -133,7 +132,7 @@ impl EncryptionSettings {
             rotation_period,
             rotation_period_msgs,
             history_visibility,
-            sharing_strategy: CollectStrategy::new_device_based(only_allow_trusted_devices),
+            sharing_strategy,
         }
     }
 }
@@ -782,20 +781,35 @@ mod tests {
     };
 
     use super::{EncryptionSettings, ROTATION_MESSAGES, ROTATION_PERIOD};
+    use crate::CollectStrategy;
 
     #[test]
     fn test_encryption_settings_conversion() {
         let mut content =
             RoomEncryptionEventContent::new(EventEncryptionAlgorithm::MegolmV1AesSha2);
-        let settings = EncryptionSettings::new(content.clone(), HistoryVisibility::Joined, false);
+        let settings = EncryptionSettings::new(
+            content.clone(),
+            HistoryVisibility::Joined,
+            CollectStrategy::DeviceBasedStrategy {
+                only_allow_trusted_devices: false,
+                error_on_verified_user_problem: false,
+            },
+        );
 
         assert_eq!(settings.rotation_period, ROTATION_PERIOD);
         assert_eq!(settings.rotation_period_msgs, ROTATION_MESSAGES);
 
         content.rotation_period_ms = Some(uint!(3600));
         content.rotation_period_msgs = Some(uint!(500));
 
-        let settings = EncryptionSettings::new(content, HistoryVisibility::Shared, false);
+        let settings = EncryptionSettings::new(
+            content,
+            HistoryVisibility::Shared,
+            CollectStrategy::DeviceBasedStrategy {
+                only_allow_trusted_devices: false,
+                error_on_verified_user_problem: false,
+            },
+        );
 
         assert_eq!(settings.rotation_period, Duration::from_millis(3600));
         assert_eq!(settings.rotation_period_msgs, 500);

crates/matrix-sdk-crypto/src/session_manager/group_sessions/mod.rs

@@ -1162,7 +1162,10 @@ mod tests {
             .any(|d| d.user_id() == user_id && d.device_id() == device_id));
 
         let settings = EncryptionSettings {
-            sharing_strategy: CollectStrategy::new_device_based(true),
+            sharing_strategy: CollectStrategy::DeviceBasedStrategy {
+                only_allow_trusted_devices: true,
+                error_on_verified_user_problem: false,
+            },
             ..Default::default()
         };
         let users = [user_id].into_iter();
@@ -1222,7 +1225,10 @@ mod tests {
 
         let users = keys_claim.one_time_keys.keys().map(Deref::deref);
         let settings = EncryptionSettings {
-            sharing_strategy: CollectStrategy::new_device_based(true),
+            sharing_strategy: CollectStrategy::DeviceBasedStrategy {
+                only_allow_trusted_devices: true,
+                error_on_verified_user_problem: false,
+            },
             ..Default::default()
         };