feat: better errors: 'ImplementationError' was way too often used as …

…a fallback when the developer was too lazy to create a new error. This tries to cure that, especially with e2ei errors. It also tries to distinguish client errors from internal errors

crypto-ffi/src/generic.rs

@@ -132,7 +132,7 @@ impl UniffiCustomTypeConverter for Ciphersuite {
     fn into_custom(val: Self::Builtin) -> uniffi::Result<Self> {
         core_crypto::prelude::CiphersuiteName::try_from(val)
             .map(Into::into)
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError.into())
+            .map_err(|_| CryptoError::ImplementationError.into())
     }
 
     fn from_custom(obj: Self) -> Self::Builtin {
@@ -173,7 +173,7 @@ impl UniffiCustomTypeConverter for Ciphersuites {
         val.iter().try_fold(Self(vec![]), |mut acc, c| -> uniffi::Result<Self> {
             let cs = core_crypto::prelude::CiphersuiteName::try_from(*c)
                 .map(Into::into)
-                .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError)?;
+                .map_err(|_| CryptoError::ImplementationError)?;
             acc.0.push(cs);
             Ok(acc)
         })
@@ -814,7 +814,7 @@ impl CoreCrypto {
             central.take().close().await?;
             Ok(())
         } else {
-            Err(core_crypto::prelude::CryptoError::LockPoisonError.into())
+            Err(CryptoError::LockPoisonError.into())
         }
     }
 
@@ -825,7 +825,7 @@ impl CoreCrypto {
             central.take().wipe().await?;
             Ok(())
         } else {
-            Err(core_crypto::prelude::CryptoError::LockPoisonError.into())
+            Err(CryptoError::LockPoisonError.into())
         }
     }
 
@@ -862,7 +862,7 @@ impl CoreCrypto {
                 Ok(kp
                     .tls_serialize_detached()
                     .map_err(MlsError::from)
-                    .map_err(core_crypto::prelude::CryptoError::from)?)
+                    .map_err(CryptoError::from)?)
             })
             .collect::<CoreCryptoResult<Vec<Vec<u8>>>>()
     }
@@ -1281,7 +1281,8 @@ impl CoreCrypto {
         expiry_days: u32,
         ciphersuite: Ciphersuite,
     ) -> CoreCryptoResult<std::sync::Arc<E2eiEnrollment>> {
-        self.central
+        Ok(self
+            .central
             .lock()
             .await
             .e2ei_new_enrollment(
@@ -1295,8 +1296,7 @@ impl CoreCrypto {
             .map(async_lock::Mutex::new)
             .map(std::sync::Arc::new)
             .map(E2eiEnrollment)
-            .map(std::sync::Arc::new)
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError.into())
+            .map(std::sync::Arc::new)?)
     }
 
     /// See [core_crypto::mls::MlsCentral::e2ei_new_activation_enrollment]
@@ -1309,7 +1309,8 @@ impl CoreCrypto {
         expiry_days: u32,
         ciphersuite: Ciphersuite,
     ) -> CoreCryptoResult<std::sync::Arc<E2eiEnrollment>> {
-        self.central
+        Ok(self
+            .central
             .lock()
             .await
             .e2ei_new_activation_enrollment(
@@ -1323,8 +1324,7 @@ impl CoreCrypto {
             .map(async_lock::Mutex::new)
             .map(std::sync::Arc::new)
             .map(E2eiEnrollment)
-            .map(std::sync::Arc::new)
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError.into())
+            .map(std::sync::Arc::new)?)
     }
 
     /// See [core_crypto::mls::MlsCentral::e2ei_new_rotate_enrollment]
@@ -1337,7 +1337,8 @@ impl CoreCrypto {
         expiry_days: u32,
         ciphersuite: Ciphersuite,
     ) -> CoreCryptoResult<std::sync::Arc<E2eiEnrollment>> {
-        self.central
+        Ok(self
+            .central
             .lock()
             .await
             .e2ei_new_rotate_enrollment(
@@ -1351,8 +1352,7 @@ impl CoreCrypto {
             .map(async_lock::Mutex::new)
             .map(std::sync::Arc::new)
             .map(E2eiEnrollment)
-            .map(std::sync::Arc::new)
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError.into())
+            .map(std::sync::Arc::new)?)
     }
 
     /// See [core_crypto::mls::MlsCentral::e2ei_mls_init_only]
@@ -1370,23 +1370,22 @@ impl CoreCrypto {
                 std::sync::Arc::decrement_strong_count(std::sync::Arc::as_ptr(&enrollment));
             }
         }
-        let e2ei =
-            std::sync::Arc::into_inner(enrollment).ok_or_else(|| core_crypto::prelude::CryptoError::LockPoisonError)?;
+        let e2ei = std::sync::Arc::into_inner(enrollment).ok_or_else(|| CryptoError::LockPoisonError)?;
         let e2ei = std::sync::Arc::into_inner(e2ei.0)
-            .ok_or_else(|| core_crypto::prelude::CryptoError::LockPoisonError)?
+            .ok_or_else(|| CryptoError::LockPoisonError)?
             .into_inner();
 
         let nb_key_package = nb_key_package
             .map(usize::try_from)
             .transpose()
             .map_err(CryptoError::from)?;
 
-        self.central
+        Ok(self
+            .central
             .lock()
             .await
             .e2ei_mls_init_only(e2ei, certificate_chain, nb_key_package)
-            .await
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError.into())
+            .await?)
     }
 
     /// See [core_crypto::mls::MlsCentral::e2ei_rotate_all]
@@ -1404,36 +1403,27 @@ impl CoreCrypto {
                 std::sync::Arc::decrement_strong_count(std::sync::Arc::as_ptr(&enrollment));
             }
         }
-        let e2ei =
-            std::sync::Arc::into_inner(enrollment).ok_or_else(|| core_crypto::prelude::CryptoError::LockPoisonError)?;
+        let e2ei = std::sync::Arc::into_inner(enrollment).ok_or_else(|| CryptoError::LockPoisonError)?;
         let e2ei = std::sync::Arc::into_inner(e2ei.0)
-            .ok_or_else(|| core_crypto::prelude::CryptoError::LockPoisonError)?
+            .ok_or_else(|| CryptoError::LockPoisonError)?
             .into_inner();
 
         self.central
             .lock()
             .await
             .e2ei_rotate_all(e2ei, certificate_chain, new_key_packages_count as usize)
-            .await
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError)?
+            .await?
             .try_into()
     }
 
     /// See [core_crypto::mls::MlsCentral::e2ei_enrollment_stash]
     pub async fn e2ei_enrollment_stash(&self, enrollment: std::sync::Arc<E2eiEnrollment>) -> CoreCryptoResult<Vec<u8>> {
-        let enrollment =
-            std::sync::Arc::into_inner(enrollment).ok_or_else(|| core_crypto::prelude::CryptoError::LockPoisonError)?;
+        let enrollment = std::sync::Arc::into_inner(enrollment).ok_or_else(|| CryptoError::LockPoisonError)?;
         let enrollment = std::sync::Arc::into_inner(enrollment.0)
-            .ok_or_else(|| core_crypto::prelude::CryptoError::LockPoisonError)?
+            .ok_or_else(|| CryptoError::LockPoisonError)?
             .into_inner();
 
-        Ok(self
-            .central
-            .lock()
-            .await
-            .e2ei_enrollment_stash(enrollment)
-            .await
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError)?)
+        Ok(self.central.lock().await.e2ei_enrollment_stash(enrollment).await?)
     }
 
     /// See [core_crypto::mls::MlsCentral::e2ei_enrollment_stash_pop]
@@ -1447,8 +1437,7 @@ impl CoreCrypto {
             .map(async_lock::Mutex::new)
             .map(std::sync::Arc::new)
             .map(E2eiEnrollment)
-            .map(std::sync::Arc::new)
-            .map_err(|_| core_crypto::prelude::CryptoError::ImplementationError)?)
+            .map(std::sync::Arc::new)?)
     }
 
     /// See [core_crypto::mls::MlsCentral::e2ei_conversation_state]

crypto-ffi/src/wasm.rs

@@ -2406,7 +2406,6 @@ impl CoreCrypto {
                         ciphersuite.into(),
                     )
                     .map(E2eiEnrollment)
-                    .map_err(|_| CryptoError::ImplementationError)
                     .map_err(CoreCryptoError::from)?;
 
                 WasmCryptoResult::Ok(enrollment.into())
@@ -2442,7 +2441,6 @@ impl CoreCrypto {
                         ciphersuite.into(),
                     )
                     .map(E2eiEnrollment)
-                    .map_err(|_| CryptoError::ImplementationError)
                     .map_err(CoreCryptoError::from)?;
 
                 WasmCryptoResult::Ok(enrollment.into())
@@ -2478,7 +2476,6 @@ impl CoreCrypto {
                         ciphersuite.into(),
                     )
                     .map(E2eiEnrollment)
-                    .map_err(|_| CryptoError::ImplementationError)
                     .map_err(CoreCryptoError::from)?;
 
                 WasmCryptoResult::Ok(enrollment.into())
@@ -2554,7 +2551,6 @@ impl CoreCrypto {
                     .e2ei_enrollment_stash_pop(handle.to_vec())
                     .await
                     .map(E2eiEnrollment)
-                    .map_err(|_| CryptoError::ImplementationError)
                     .map_err(CoreCryptoError::from)?;
 
                 WasmCryptoResult::Ok(enrollment.into())
@@ -2633,7 +2629,7 @@ impl CoreCrypto {
         let this = self.inner.clone();
         future_to_promise(
             async move {
-                let identities: HashMap<String, Vec<WireIdentity>> = this
+                let identities = this
                     .write()
                     .await
                     .get_user_identities(&conversation_id, user_ids.deref())

crypto/src/e2e_identity/crypto.rs

@@ -1,6 +1,6 @@
 use super::error::*;
 use crate::prelude::MlsCiphersuite;
-use crate::CryptoError;
+use crate::{CryptoError, CryptoResult, MlsError};
 use mls_crypto_provider::MlsCryptoProvider;
 use openmls_basic_credential::SignatureKeyPair;
 use openmls_traits::{crypto::OpenMlsCrypto, types::Ciphersuite, OpenMlsCryptoProvider};
@@ -14,18 +14,20 @@ impl super::E2eiEnrollment {
     pub(super) fn new_sign_key(
         ciphersuite: MlsCiphersuite,
         backend: &MlsCryptoProvider,
-    ) -> E2eIdentityResult<E2eiSignatureKeypair> {
+    ) -> CryptoResult<E2eiSignatureKeypair> {
         let crypto = backend.crypto();
         let cs = openmls_traits::types::Ciphersuite::from(ciphersuite);
-        let (sk, pk) = crypto.signature_key_gen(cs.signature_algorithm())?;
+        let (sk, pk) = crypto
+            .signature_key_gen(cs.signature_algorithm())
+            .map_err(MlsError::from)?;
         Ok((sk, pk).into())
     }
 
-    pub(super) fn get_sign_key_for_mls(&self) -> E2eIdentityResult<Vec<u8>> {
+    pub(super) fn get_sign_key_for_mls(&self) -> CryptoResult<Vec<u8>> {
         let sk = match self.sign_sk.len() {
             SIGN_KEYPAIR_LENGTH => &self.sign_sk[..SIGN_KEY_LENGTH],
             SIGN_KEY_LENGTH => &self.sign_sk,
-            _ => return Err(E2eIdentityError::ImplementationError),
+            _ => return Err(E2eIdentityError::InvalidSignatureKey.into()),
         };
         Ok(sk.to_vec())
     }
@@ -67,7 +69,7 @@ impl TryFrom<SignatureKeyPair> for E2eiSignatureKeypair {
         let sk = match sk.len() {
             SIGN_KEY_LENGTH => sk,
             SIGN_KEYPAIR_LENGTH => &sk[..SIGN_KEY_LENGTH],
-            _ => return Err(CryptoError::ImplementationError),
+            _ => return Err(E2eIdentityError::InvalidSignatureKey.into()),
         };
         Ok((sk.to_vec(), kp.to_public_vec()).into())
     }

crypto/src/e2e_identity/enabled.rs

@@ -12,7 +12,7 @@ impl MlsCentral {
             None => {
                 client
                     .find_most_recent_credential_bundle(signature_scheme, MlsCredentialType::Basic)
-                    .ok_or(CryptoError::CredentialNotFound)?;
+                    .ok_or(CryptoError::CredentialNotFound(MlsCredentialType::Basic))?;
                 Ok(false)
             }
             Some(_) => Ok(true),
@@ -69,7 +69,7 @@ pub mod tests {
                 };
                 assert!(matches!(
                     cc.e2ei_is_enabled(other_sc).unwrap_err(),
-                    CryptoError::CredentialNotFound
+                    CryptoError::CredentialNotFound(_)
                 ));
             })
         })

crypto/src/e2e_identity/error.rs

@@ -1,6 +1,6 @@
 //! End to end identity errors
 
-use crate::CryptoError;
+use crate::prelude::MlsCredentialType;
 
 /// Wrapper over a [Result] of an end to end identity error
 pub type E2eIdentityResult<T> = Result<T, E2eIdentityError>;
@@ -16,6 +16,18 @@ pub enum E2eIdentityError {
     /// Incoming support
     #[error("Not yet supported")]
     NotYetSupported,
+    /// The required local MLS client was not initialized. It's likely a consumer error
+    #[error("Expected a MLS client with credential type {0:?} but none found")]
+    MissingExistingClient(MlsCredentialType),
+    /// Cannot read the identity in the EE certificate
+    #[error("Could not the identity information in the Credential's certificate")]
+    InvalidIdentity,
+    /// Failed converting the MLS signature key for the e2ei enrollment
+    #[error("Failed converting the MLS signature key for the e2ei enrollment")]
+    InvalidSignatureKey,
+    /// Enrollment methods are called out of order
+    #[error("Enrollment methods are called out of order: {0}")]
+    OutOfOrderEnrollment(&'static str),
     /// Error when an end-to-end-identity domain is not well-formed utf-16, which means it's out of spec
     #[error("The E2EI provided domain is invalid utf-16")]
     E2eiInvalidDomain,
@@ -34,9 +46,6 @@ pub enum E2eIdentityError {
     /// Utf8 error
     #[error(transparent)]
     Utf8Error(#[from] ::core::str::Utf8Error),
-    /// MLS error
-    #[error(transparent)]
-    MlsError(#[from] CryptoError),
     /// !!!! Something went very wrong and one of our locks has been poisoned by an in-thread panic !!!!
     #[error("One of the locks has been poisoned")]
     LockPoisonError,

crypto/src/e2e_identity/identity.rs

@@ -86,7 +86,7 @@ impl MlsCentral {
 impl MlsConversation {
     fn get_device_identities(&self, device_ids: &[ClientId]) -> CryptoResult<Vec<WireIdentity>> {
         if device_ids.is_empty() {
-            return Err(CryptoError::ImplementationError);
+            return Err(CryptoError::ConsumerError);
         }
         self.members()
             .into_iter()
@@ -97,7 +97,7 @@ impl MlsConversation {
 
     fn get_user_identities(&self, user_ids: &[String]) -> CryptoResult<HashMap<String, Vec<WireIdentity>>> {
         if user_ids.is_empty() {
-            return Err(CryptoError::ImplementationError);
+            return Err(CryptoError::ConsumerError);
         }
         let user_ids = user_ids.iter().map(|uid| uid.as_bytes()).collect::<Vec<_>>();
         self.members()
@@ -191,7 +191,7 @@ pub mod tests {
                     );
 
                     let invalid = alice_android_central.get_device_identities(&id, &[]).await;
-                    assert!(matches!(invalid.unwrap_err(), CryptoError::ImplementationError));
+                    assert!(matches!(invalid.unwrap_err(), CryptoError::ConsumerError));
                 })
             },
         )
@@ -311,7 +311,7 @@ pub mod tests {
 
                     // Invalid usage
                     let invalid = alice_android_central.get_user_identities(&id, &[]).await;
-                    assert!(matches!(invalid.unwrap_err(), CryptoError::ImplementationError));
+                    assert!(matches!(invalid.unwrap_err(), CryptoError::ConsumerError));
                 })
             },
         )