Create a separate error variant to indicate a failure importing a secret. (#5647)

Part of the fix to
element-hq/element-x-android#5099

Allows applications to distinguish between errors that occur when
unlocking Secret Storage, or errors that occur when importing a secret,
so that they can display appropriate feedback (or not) to the user.

- [ ] Public API changes documented in changelogs (optional)

<!-- Sign-off, if not part of the commits -->
<!-- See CONTRIBUTING.md if you don't know what this is -->
Signed-off-by:

---------

Signed-off-by: Hubert Chathi <[email protected]>

Author: uhoreg

bindings/matrix-sdk-ffi/src/encryption.rs

@@ -79,16 +79,24 @@ pub enum RecoveryError {
     #[error(transparent)]
     Client { source: crate::ClientError },
 
-    /// Error in the secret storage subsystem.
+    /// Error in the secret storage subsystem, except for when importing a
+    /// secret.
     #[error("Error in the secret-storage subsystem: {error_message}")]
     SecretStorage { error_message: String },
+
+    /// Error when importing a secret from secret storage.
+    #[error("Error importing a secret: {error_message}")]
+    Import { error_message: String },
 }
 
 impl From<matrix_sdk::encryption::recovery::RecoveryError> for RecoveryError {
     fn from(value: matrix_sdk::encryption::recovery::RecoveryError) -> Self {
         match value {
             recovery::RecoveryError::BackupExistsOnServer => Self::BackupExistsOnServer,
             recovery::RecoveryError::Sdk(e) => Self::Client { source: ClientError::from(e) },
+            recovery::RecoveryError::SecretStorage(
+                matrix_sdk::encryption::secret_storage::SecretStorageError::ImportError { .. },
+            ) => Self::Import { error_message: value.to_string() },
             recovery::RecoveryError::SecretStorage(e) => {
                 Self::SecretStorage { error_message: e.to_string() }
             }

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -15,6 +15,10 @@ All notable changes to this project will be documented in this file.
 
 ### Features
 
+- [**breaking**] Add `name` fields to some of the variants of
+  `store::SecretImportError` to indicate what secret was being imported when the
+  error occurred.
+  ([#5647](https://github.com/matrix-org/matrix-rust-sdk/pull/5647))
 - Log message index for Megolm sessions received over encrypted to-device messages. ([#5599](https://github.com/matrix-org/matrix-rust-sdk/pull/5599))
 - Add `RoomSettings::encrypt_state_events` flag. ([#5511](https://github.com/matrix-org/matrix-rust-sdk/pull/5511))
 - Make sure to accept historic room key bundles only if the sender is trusted

crates/matrix-sdk-crypto/src/olm/signing/mod.rs

@@ -235,36 +235,53 @@ impl PrivateCrossSigningIdentity {
         user_signing_key: Option<&str>,
     ) -> Result<(), SecretImportError> {
         let master = if let Some(master_key) = master_key {
-            let master = MasterSigning::from_base64(self.user_id().to_owned(), master_key)?;
+            let master =
+                MasterSigning::from_base64(self.user_id().to_owned(), master_key).map_err(|e| {
+                    SecretImportError::Key { name: SecretName::CrossSigningMasterKey, error: e }
+                })?;
 
             if public_identity.master_key() == master.public_key() {
                 Some(master)
             } else {
-                return Err(SecretImportError::MismatchedPublicKeys);
+                return Err(SecretImportError::MismatchedPublicKeys {
+                    name: SecretName::CrossSigningMasterKey,
+                });
             }
         } else {
             None
         };
 
         let user_signing = if let Some(user_signing_key) = user_signing_key {
-            let subkey = UserSigning::from_base64(self.user_id().to_owned(), user_signing_key)?;
+            let subkey = UserSigning::from_base64(self.user_id().to_owned(), user_signing_key)
+                .map_err(|e| SecretImportError::Key {
+                    name: SecretName::CrossSigningUserSigningKey,
+                    error: e,
+                })?;
 
             if public_identity.user_signing_key() == subkey.public_key() {
                 Ok(Some(subkey))
             } else {
-                Err(SecretImportError::MismatchedPublicKeys)
+                Err(SecretImportError::MismatchedPublicKeys {
+                    name: SecretName::CrossSigningUserSigningKey,
+                })
             }
         } else {
             Ok(None)
         }?;
 
         let self_signing = if let Some(self_signing_key) = self_signing_key {
-            let subkey = SelfSigning::from_base64(self.user_id().to_owned(), self_signing_key)?;
+            let subkey = SelfSigning::from_base64(self.user_id().to_owned(), self_signing_key)
+                .map_err(|e| SecretImportError::Key {
+                    name: SecretName::CrossSigningSelfSigningKey,
+                    error: e,
+                })?;
 
             if public_identity.self_signing_key() == subkey.public_key() {
                 Ok(Some(subkey))
             } else {
-                Err(SecretImportError::MismatchedPublicKeys)
+                Err(SecretImportError::MismatchedPublicKeys {
+                    name: SecretName::CrossSigningSelfSigningKey,
+                })
             }
         } else {
             Ok(None)
@@ -299,17 +316,28 @@ impl PrivateCrossSigningIdentity {
         user_signing_key: Option<&str>,
     ) -> Result<(), SecretImportError> {
         if let Some(master_key) = master_key {
-            let master = MasterSigning::from_base64(self.user_id().to_owned(), master_key)?;
+            let master =
+                MasterSigning::from_base64(self.user_id().to_owned(), master_key).map_err(|e| {
+                    SecretImportError::Key { name: SecretName::CrossSigningMasterKey, error: e }
+                })?;
             *self.master_key.lock().await = Some(master);
         }
 
         if let Some(user_signing_key) = user_signing_key {
-            let subkey = UserSigning::from_base64(self.user_id().to_owned(), user_signing_key)?;
+            let subkey = UserSigning::from_base64(self.user_id().to_owned(), user_signing_key)
+                .map_err(|e| SecretImportError::Key {
+                    name: SecretName::CrossSigningUserSigningKey,
+                    error: e,
+                })?;
             *self.user_signing_key.lock().await = Some(subkey);
         }
 
         if let Some(self_signing_key) = self_signing_key {
-            let subkey = SelfSigning::from_base64(self.user_id().to_owned(), self_signing_key)?;
+            let subkey = SelfSigning::from_base64(self.user_id().to_owned(), self_signing_key)
+                .map_err(|e| SecretImportError::Key {
+                    name: SecretName::CrossSigningSelfSigningKey,
+                    error: e,
+                })?;
             *self.self_signing_key.lock().await = Some(subkey);
         }
 

crates/matrix-sdk-crypto/src/store/mod.rs

@@ -492,15 +492,23 @@ struct StoreInner {
 #[derive(Debug, Error)]
 pub enum SecretImportError {
     /// The key that we tried to import was invalid.
-    #[error(transparent)]
-    Key(#[from] vodozemac::KeyError),
-    /// The public key of the imported private key doesn't match to the public
+    #[error("Error while importing {name}: {error}")]
+    Key {
+        /// The name of the secret that was being imported.
+        name: SecretName,
+        /// The key error that occurred.
+        error: vodozemac::KeyError,
+    },
+    /// The public key of the imported private key doesn't match the public
     /// key that was uploaded to the server.
     #[error(
-        "The public key of the imported private key doesn't match to the \
-            public key that was uploaded to the server"
+        "Error while importing {name}: The public key of the imported private \
+            key doesn't match the public key that was uploaded to the server"
     )]
-    MismatchedPublicKeys,
+    MismatchedPublicKeys {
+        /// The name of the secret that was being imported.
+        name: SecretName,
+    },
     /// The new version of the identity couldn't be stored.
     #[error(transparent)]
     Store(#[from] CryptoStoreError),

crates/matrix-sdk/CHANGELOG.md

@@ -8,6 +8,10 @@ All notable changes to this project will be documented in this file.
 
 ### Features
 
+- [**breaking**] Add `encryption::secret_storage::SecretStorageError::ImportError` to indicate
+  an error that occurred when importing a secret from secret storage.
+  ([#5647](https://github.com/matrix-org/matrix-rust-sdk/pull/5647))
+
 ### Refactor
 - The Matrix SDK crate now uses the 2024 edition of Rust.
   ([#5677](https://github.com/matrix-org/matrix-rust-sdk/pull/5677))

crates/matrix-sdk/src/encryption/recovery/mod.rs

@@ -728,3 +728,173 @@ impl IdentityResetHandle {
         self.cross_signing_reset_handle.cancel().await;
     }
 }
+
+// The http mocking library is not supported for wasm32
+#[cfg(all(test, not(target_family = "wasm")))]
+pub(crate) mod tests {
+    use assert_matches::assert_matches;
+    use matrix_sdk_test::async_test;
+    use ruma::{
+        events::{secret::request::SecretName, secret_storage::key},
+        serde::Base64,
+    };
+    use serde_json::json;
+
+    use super::Recovery;
+    use crate::{
+        encryption::{recovery::types::RecoveryError, secret_storage::SecretStorageError},
+        test_utils::mocks::MatrixMockServer,
+    };
+
+    // If recovery fails due when importing a secret from secret storage, we
+    // should get the `ImportError` variant of `SecretStorageError`.  The
+    // following tests test different import failures.
+    #[async_test]
+    async fn test_recover_with_no_cross_signing_key() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        server
+            .mock_get_secret_storage_key()
+            .ok(
+                client.user_id().unwrap(),
+                &key::SecretStorageKeyEventContent::new(
+                    "abc".into(),
+                    key::SecretStorageEncryptionAlgorithm::V1AesHmacSha2(
+                        key::SecretStorageV1AesHmacSha2Properties::new(
+                            Some(Base64::parse("xv5b6/p3ExEw++wTyfSHEg==").unwrap()),
+                            Some(
+                                Base64::parse("ujBBbXahnTAMkmPUX2/0+VTfUh63pGyVRuBcDMgmJC8=")
+                                    .unwrap(),
+                            ),
+                        ),
+                    ),
+                ),
+            )
+            .mount()
+            .await;
+        server
+            .mock_get_default_secret_storage_key()
+            .ok(client.user_id().unwrap(), "abc")
+            .mount()
+            .await;
+
+        let recovery = Recovery { client };
+
+        let ret =
+            recovery.recover("EsTj 3yST y93F SLpB jJsz eAXc 2XzA ygD3 w69H fGaN TKBj jXEd").await;
+
+        assert_matches!(
+            ret,
+            Err(RecoveryError::SecretStorage(SecretStorageError::ImportError {
+                name: SecretName::CrossSigningMasterKey,
+                error: _
+            }))
+        );
+    }
+
+    #[async_test]
+    async fn test_recover_with_invalid_cross_signing_key() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        server
+            .mock_get_secret_storage_key()
+            .ok(
+                client.user_id().unwrap(),
+                &key::SecretStorageKeyEventContent::new(
+                    "abc".into(),
+                    key::SecretStorageEncryptionAlgorithm::V1AesHmacSha2(
+                        key::SecretStorageV1AesHmacSha2Properties::new(
+                            Some(Base64::parse("xv5b6/p3ExEw++wTyfSHEg==").unwrap()),
+                            Some(
+                                Base64::parse("ujBBbXahnTAMkmPUX2/0+VTfUh63pGyVRuBcDMgmJC8=")
+                                    .unwrap(),
+                            ),
+                        ),
+                    ),
+                ),
+            )
+            .mount()
+            .await;
+        server
+            .mock_get_default_secret_storage_key()
+            .ok(client.user_id().unwrap(), "abc")
+            .mount()
+            .await;
+        server.mock_get_master_signing_key().ok(client.user_id().unwrap(), json!({})).mount().await;
+
+        let recovery = Recovery { client };
+
+        let ret =
+            recovery.recover("EsTj 3yST y93F SLpB jJsz eAXc 2XzA ygD3 w69H fGaN TKBj jXEd").await;
+
+        assert_matches!(
+            ret,
+            Err(RecoveryError::SecretStorage(SecretStorageError::ImportError {
+                name: SecretName::CrossSigningMasterKey,
+                error: _
+            }))
+        );
+    }
+
+    #[async_test]
+    async fn test_recover_with_undecryptable_cross_signing_key() {
+        let server = MatrixMockServer::new().await;
+        let client = server.client_builder().build().await;
+
+        server
+            .mock_get_secret_storage_key()
+            .ok(
+                client.user_id().unwrap(),
+                &key::SecretStorageKeyEventContent::new(
+                    "abc".into(),
+                    key::SecretStorageEncryptionAlgorithm::V1AesHmacSha2(
+                        key::SecretStorageV1AesHmacSha2Properties::new(
+                            Some(Base64::parse("xv5b6/p3ExEw++wTyfSHEg==").unwrap()),
+                            Some(
+                                Base64::parse("ujBBbXahnTAMkmPUX2/0+VTfUh63pGyVRuBcDMgmJC8=")
+                                    .unwrap(),
+                            ),
+                        ),
+                    ),
+                ),
+            )
+            .mount()
+            .await;
+        server
+            .mock_get_default_secret_storage_key()
+            .ok(client.user_id().unwrap(), "abc")
+            .mount()
+            .await;
+        server
+            .mock_get_master_signing_key()
+            .ok(
+                client.user_id().unwrap(),
+                json!({
+                    "encrypted": {
+                        "abc": {
+                            "iv": "xv5b6/p3ExEw++wTyfSHEg==",
+                            "mac": "ujBBbXahnTAMkmPUX2/0+VTfUh63pGyVRuBcDMgmJC8=",
+                            "ciphertext": "abcd"
+                        }
+                    }
+                }),
+            )
+            .mount()
+            .await;
+
+        let recovery = Recovery { client };
+
+        let ret =
+            recovery.recover("EsTj 3yST y93F SLpB jJsz eAXc 2XzA ygD3 w69H fGaN TKBj jXEd").await;
+
+        assert_matches!(
+            ret,
+            Err(RecoveryError::SecretStorage(SecretStorageError::ImportError {
+                name: SecretName::CrossSigningMasterKey,
+                error: _
+            }))
+        );
+    }
+}