crypto: Move the logic for marking identities as verified out of the Sas struct

matrix_sdk/src/sas.rs

@@ -138,7 +138,7 @@ impl Sas {
     }
 
     /// Get the other users device that we're veryfying.
-    pub fn other_device(&self) -> ReadOnlyDevice {
+    pub fn other_device(&self) -> &ReadOnlyDevice {
         self.inner.other_device()
     }
 }

matrix_sdk_crypto/src/verification/machine.rs

@@ -28,7 +28,8 @@ use tracing::{info, trace, warn};
 
 use super::{
     requests::VerificationRequest,
-    sas::{content_to_request, OutgoingContent, Sas, VerificationResult},
+    sas::{content_to_request, OutgoingContent, Sas},
+    VerificationResult,
 };
 use crate::{
     olm::PrivateCrossSigningIdentity,
@@ -341,8 +342,10 @@ impl VerificationMachine {
                                         );
                                     }
                                 }
-                                VerificationResult::Cancel(r) => {
+                                VerificationResult::Cancel(c) => {
-                                    self.verifications.add_request(r.into());
+                                    if let Some(r) = s.cancel_with_code(c) {
+                                        self.verifications.add_request(r.into());
+                                    }
                                 }
                                 VerificationResult::SignatureUpload(r) => {
                                     self.verifications.add_request(r.into());
@@ -453,8 +456,10 @@ impl VerificationMachine {
                     if s.is_done() {
                         match s.mark_as_done().await? {
                             VerificationResult::Ok => (),
-                            VerificationResult::Cancel(r) => {
+                            VerificationResult::Cancel(c) => {
-                                self.verifications.add_request(r.into());
+                                if let Some(r) = s.cancel_with_code(c) {
+                                    self.verifications.add_request(r.into());
+                                }
                             }
                             VerificationResult::SignatureUpload(r) => {
                                 self.verifications.add_request(r.into());

matrix_sdk_crypto/src/verification/mod.rs

@@ -16,16 +16,28 @@ mod machine;
 mod requests;
 mod sas;
 
-pub use machine::{VerificationCache, VerificationMachine};
+use std::sync::Arc;
+
 use matrix_sdk_common::{
+    api::r0::keys::upload_signatures::Request as SignatureUploadRequest,
     events::key::verification::{
         cancel::{CancelCode, CancelEventContent, CancelToDeviceEventContent},
         Relation,
     },
-    identifiers::{EventId, RoomId},
+    identifiers::{DeviceId, EventId, RoomId, UserId},
 };
+
+pub use machine::{VerificationCache, VerificationMachine};
 pub use requests::VerificationRequest;
-pub use sas::{AcceptSettings, Sas, VerificationResult};
+pub use sas::{AcceptSettings, Sas};
+use tracing::{error, info, trace, warn};
+
+use crate::{
+    error::SignatureError,
+    olm::PrivateCrossSigningIdentity,
+    store::{Changes, CryptoStore, DeviceChanges},
+    CryptoStoreError, LocalTrust, ReadOnlyDevice, UserIdentities,
+};
 
 use self::sas::CancelContent;
 
@@ -116,6 +128,253 @@ impl From<(RoomId, EventId)> for FlowId {
     }
 }
 
+/// A result of a verification flow.
+#[derive(Clone, Debug)]
+pub enum VerificationResult {
+    /// The verification succeeded, nothing needs to be done.
+    Ok,
+    /// The verification was canceled.
+    Cancel(CancelCode),
+    /// The verification is done and has signatures that need to be uploaded.
+    SignatureUpload(SignatureUploadRequest),
+}
+
+#[derive(Clone, Debug)]
+pub struct IdentitiesBeingVerified {
+    private_identity: PrivateCrossSigningIdentity,
+    store: Arc<Box<dyn CryptoStore>>,
+    device_being_verified: ReadOnlyDevice,
+    identity_being_verified: Option<UserIdentities>,
+}
+
+#[allow(dead_code)]
+impl IdentitiesBeingVerified {
+    fn user_id(&self) -> &UserId {
+        self.private_identity.user_id()
+    }
+
+    fn other_user_id(&self) -> &UserId {
+        self.device_being_verified.user_id()
+    }
+
+    fn other_device_id(&self) -> &DeviceId {
+        self.device_being_verified.device_id()
+    }
+
+    fn other_device(&self) -> &ReadOnlyDevice {
+        &self.device_being_verified
+    }
+
+    pub async fn mark_as_done(
+        &self,
+        verified_devices: Option<&[ReadOnlyDevice]>,
+        verified_identities: Option<&[UserIdentities]>,
+    ) -> Result<VerificationResult, CryptoStoreError> {
+        if let Some(device) = self.mark_device_as_verified(verified_devices).await? {
+            let identity = self.mark_identity_as_verified(verified_identities).await?;
+
+            // We only sign devices of our own user here.
+            let signature_request = if device.user_id() == self.user_id() {
+                match self.private_identity.sign_device(&device).await {
+                    Ok(r) => Some(r),
+                    Err(SignatureError::MissingSigningKey) => {
+                        warn!(
+                            "Can't sign the device keys for {} {}, \
+                                  no private user signing key found",
+                            device.user_id(),
+                            device.device_id(),
+                        );
+
+                        None
+                    }
+                    Err(e) => {
+                        error!(
+                            "Error signing device keys for {} {} {:?}",
+                            device.user_id(),
+                            device.device_id(),
+                            e
+                        );
+                        None
+                    }
+                }
+            } else {
+                None
+            };
+
+            let mut changes = Changes {
+                devices: DeviceChanges { changed: vec![device], ..Default::default() },
+                ..Default::default()
+            };
+
+            let identity_signature_request = if let Some(i) = identity {
+                // We only sign other users here.
+                let request = if let Some(i) = i.other() {
+                    // Signing can fail if the user signing key is missing.
+                    match self.private_identity.sign_user(&i).await {
+                        Ok(r) => Some(r),
+                        Err(SignatureError::MissingSigningKey) => {
+                            warn!(
+                                "Can't sign the public cross signing keys for {}, \
+                                  no private user signing key found",
+                                i.user_id()
+                            );
+                            None
+                        }
+                        Err(e) => {
+                            error!(
+                                "Error signing the public cross signing keys for {} {:?}",
+                                i.user_id(),
+                                e
+                            );
+                            None
+                        }
+                    }
+                } else {
+                    None
+                };
+
+                changes.identities.changed.push(i);
+
+                request
+            } else {
+                None
+            };
+
+            // If there are two signature upload requests, merge them. Otherwise
+            // use the one we have or None.
+            //
+            // Realistically at most one request will be used but let's make
+            // this future proof.
+            let merged_request = if let Some(mut r) = signature_request {
+                if let Some(user_request) = identity_signature_request {
+                    r.signed_keys.extend(user_request.signed_keys);
+                    Some(r)
+                } else {
+                    Some(r)
+                }
+            } else {
+                identity_signature_request
+            };
+
+            // TODO store the signature upload request as well.
+            self.store.save_changes(changes).await?;
+            Ok(merged_request
+                .map(VerificationResult::SignatureUpload)
+                .unwrap_or(VerificationResult::Ok))
+        } else {
+            Ok(VerificationResult::Cancel(CancelCode::UserMismatch))
+        }
+    }
+
+    async fn mark_identity_as_verified(
+        &self,
+        verified_identities: Option<&[UserIdentities]>,
+    ) -> Result<Option<UserIdentities>, CryptoStoreError> {
+        // If there wasn't an identity available during the verification flow
+        // return early as there's nothing to do.
+        if self.identity_being_verified.is_none() {
+            return Ok(None);
+        }
+
+        // TODO signal an error, e.g. when the identity got deleted so we don't
+        // verify/save the device either.
+        let identity = self.store.get_user_identity(self.other_user_id()).await?;
+
+        if let Some(identity) = identity {
+            if self
+                .identity_being_verified
+                .as_ref()
+                .map_or(false, |i| i.master_key() == identity.master_key())
+            {
+                if verified_identities.map_or(false, |i| i.contains(&identity)) {
+                    trace!("Marking user identity of {} as verified.", identity.user_id(),);
+
+                    if let UserIdentities::Own(i) = &identity {
+                        i.mark_as_verified();
+                    }
+
+                    Ok(Some(identity))
+                } else {
+                    info!(
+                        "The interactive verification process didn't verify \
+                         the user identity of {} {:?}",
+                        identity.user_id(),
+                        verified_identities,
+                    );
+
+                    Ok(None)
+                }
+            } else {
+                warn!(
+                    "The master keys of {} have changed while an interactive \
+                      verification was going on, not marking the identity as verified.",
+                    identity.user_id(),
+                );
+
+                Ok(None)
+            }
+        } else {
+            info!(
+                "The identity for {} was deleted while an interactive \
+                 verification was going on.",
+                self.other_user_id(),
+            );
+            Ok(None)
+        }
+    }
+
+    async fn mark_device_as_verified(
+        &self,
+        verified_devices: Option<&[ReadOnlyDevice]>,
+    ) -> Result<Option<ReadOnlyDevice>, CryptoStoreError> {
+        let device = self.store.get_device(self.other_user_id(), self.other_device_id()).await?;
+
+        if let Some(device) = device {
+            if device.keys() == self.device_being_verified.keys() {
+                if verified_devices.map_or(false, |v| v.contains(&device)) {
+                    trace!(
+                        user_id = device.user_id().as_str(),
+                        device_id = device.device_id().as_str(),
+                        "Marking device as verified.",
+                    );
+
+                    device.set_trust_state(LocalTrust::Verified);
+
+                    Ok(Some(device))
+                } else {
+                    info!(
+                        user_id = device.user_id().as_str(),
+                        device_id = device.device_id().as_str(),
+                        "The interactive verification process didn't verify \
+                        the device",
+                    );
+
+                    Ok(None)
+                }
+            } else {
+                warn!(
+                    user_id = device.user_id().as_str(),
+                    device_id = device.device_id().as_str(),
+                    "The device keys have changed while an interactive \
+                     verification was going on, not marking the device as verified.",
+                );
+                Ok(None)
+            }
+        } else {
+            let device = &self.device_being_verified;
+
+            info!(
+                user_id = device.user_id().as_str(),
+                device_id = device.device_id().as_str(),
+                "The device was deleted while an interactive verification was \
+                 going on.",
+            );
+
+            Ok(None)
+        }
+    }
+}
+
 #[cfg(test)]
 pub(crate) mod test {
     use matrix_sdk_common::{

matrix_sdk_crypto/src/verification/requests.rs

@@ -23,6 +23,7 @@ use matrix_sdk_common::{
     api::r0::to_device::DeviceIdOrAllDevices,
     events::{
         key::verification::{
+            done::{DoneEventContent, DoneToDeviceEventContent},
             ready::{ReadyEventContent, ReadyToDeviceEventContent},
             request::RequestToDeviceEventContent,
             start::{StartEventContent, StartMethod, StartToDeviceEventContent},
@@ -35,7 +36,7 @@ use matrix_sdk_common::{
     uuid::Uuid,
     MilliSecondsSinceUnixEpoch,
 };
-use tracing::{info, warn};
+use tracing::{info, trace, warn};
 
 use super::{
     sas::{content_to_request, OutgoingContent, StartContent as OwnedStartContent},
@@ -202,6 +203,32 @@ impl<'a> TryFrom<&'a OutgoingContent> for StartContent<'a> {
     }
 }
 
+pub enum DoneContent<'a> {
+    ToDevice(&'a DoneToDeviceEventContent),
+    Room(&'a DoneEventContent),
+}
+
+impl<'a> From<&'a DoneEventContent> for DoneContent<'a> {
+    fn from(c: &'a DoneEventContent) -> Self {
+        Self::Room(c)
+    }
+}
+
+impl<'a> From<&'a DoneToDeviceEventContent> for DoneContent<'a> {
+    fn from(c: &'a DoneToDeviceEventContent) -> Self {
+        Self::ToDevice(c)
+    }
+}
+
+impl<'a> DoneContent<'a> {
+    pub fn flow_id(&self) -> &str {
+        match self {
+            Self::ToDevice(c) => &c.transaction_id,
+            Self::Room(c) => &c.relation.event_id.as_str(),
+        }
+    }
+}
+
 #[derive(Clone, Debug)]
 /// TODO
 pub struct VerificationRequest {

matrix_sdk_crypto/src/verification/sas/event_enums.rs

@@ -109,6 +109,7 @@ impl From<(RoomId, AcceptEventContent)> for AcceptContent {
     }
 }
 
+#[derive(Clone, Debug)]
 pub enum KeyContent {
     ToDevice(KeyToDeviceEventContent),
     Room(RoomId, KeyEventContent),
@@ -142,6 +143,7 @@ impl From<(RoomId, KeyEventContent)> for KeyContent {
     }
 }
 
+#[derive(Clone, Debug)]
 pub enum MacContent {
     ToDevice(MacToDeviceEventContent),
     Room(RoomId, MacEventContent),
@@ -182,6 +184,7 @@ impl From<(RoomId, MacEventContent)> for MacContent {
     }
 }
 
+#[derive(Clone, Debug)]
 pub enum CancelContent {
     ToDevice(CancelToDeviceEventContent),
     Room(RoomId, CancelEventContent),
@@ -199,6 +202,7 @@ impl From<CancelToDeviceEventContent> for CancelContent {
     }
 }
 
+#[derive(Clone, Debug)]
 pub enum DoneContent {
     Room(RoomId, DoneEventContent),
 }

matrix_sdk_crypto/src/verification/sas/mod.rs

@@ -38,39 +38,22 @@ use matrix_sdk_common::{
     identifiers::{DeviceId, EventId, RoomId, UserId},
     uuid::Uuid,
 };
-use tracing::{error, info, trace, warn};
 
-use super::FlowId;
+use super::{FlowId, IdentitiesBeingVerified, VerificationResult};
 use crate::{
-    error::SignatureError,
+    identities::{ReadOnlyDevice, UserIdentities},
-    identities::{LocalTrust, ReadOnlyDevice, UserIdentities},
     olm::PrivateCrossSigningIdentity,
     requests::{OutgoingVerificationRequest, RoomMessageRequest},
-    store::{Changes, CryptoStore, CryptoStoreError, DeviceChanges},
+    store::{CryptoStore, CryptoStoreError},
     ReadOnlyAccount, ToDeviceRequest,
 };
 
-#[derive(Debug)]
-/// A result of a verification flow.
-#[allow(clippy::large_enum_variant)]
-pub enum VerificationResult {
-    /// The verification succeeded, nothing needs to be done.
-    Ok,
-    /// The verification was canceled.
-    Cancel(OutgoingVerificationRequest),
-    /// The verification is done and has signatures that need to be uploaded.
-    SignatureUpload(SignatureUploadRequest),
-}
-
 #[derive(Clone, Debug)]
 /// Short authentication string object.
 pub struct Sas {
     inner: Arc<Mutex<InnerSas>>,
-    store: Arc<Box<dyn CryptoStore>>,
     account: ReadOnlyAccount,
-    private_identity: PrivateCrossSigningIdentity,
+    identities_being_verified: IdentitiesBeingVerified,
-    other_device: ReadOnlyDevice,
-    other_identity: Option<UserIdentities>,
     flow_id: Arc<FlowId>,
 }
 
@@ -87,17 +70,17 @@ impl Sas {
 
     /// Get the user id of the other side.
     pub fn other_user_id(&self) -> &UserId {
-        self.other_device.user_id()
+        self.identities_being_verified.other_user_id()
     }
 
     /// Get the device id of the other side.
     pub fn other_device_id(&self) -> &DeviceId {
-        self.other_device.device_id()
+        self.identities_being_verified.other_device_id()
     }
 
     /// Get the device of the other user.
-    pub fn other_device(&self) -> ReadOnlyDevice {
+    pub fn other_device(&self) -> &ReadOnlyDevice {
-        self.other_device.clone()
+        self.identities_being_verified.other_device()
     }
 
     /// Get the unique ID that identifies this SAS verification flow.
@@ -127,14 +110,18 @@ impl Sas {
     ) -> Sas {
         let flow_id = inner_sas.verification_flow_id();
 
+        let identities = IdentitiesBeingVerified {
+            private_identity: private_identity.clone(),
+            store: store.clone(),
+            device_being_verified: other_device.clone(),
+            identity_being_verified: other_identity.clone(),
+        };
+
         Sas {
             inner: Arc::new(Mutex::new(inner_sas)),
             account,
-            private_identity,
+            identities_being_verified: identities,
-            store,
-            other_device,
             flow_id,
-            other_identity,
         }
     }
 
@@ -241,17 +228,14 @@ impl Sas {
             other_identity.clone(),
         )?;
 
-        let flow_id = inner.verification_flow_id();
+        Ok(Self::start_helper(
-
+            inner,
-        Ok(Sas {
-            inner: Arc::new(Mutex::new(inner)),
             account,
             private_identity,
             other_device,
-            other_identity,
             store,
-            flow_id,
+            other_identity,
-        })
+        ))
     }
 
     /// Accept the SAS verification.
@@ -322,7 +306,7 @@ impl Sas {
 
         if done {
             match self.mark_as_done().await? {
-                VerificationResult::Cancel(r) => Ok((Some(r), None)),
+                VerificationResult::Cancel(c) => Ok((self.cancel_with_code(c), None)),
                 VerificationResult::Ok => Ok((mac_request, None)),
                 VerificationResult::SignatureUpload(r) => Ok((mac_request, Some(r))),
             }
@@ -332,205 +316,9 @@ impl Sas {
     }
 
     pub(crate) async fn mark_as_done(&self) -> Result<VerificationResult, CryptoStoreError> {
-        if let Some(device) = self.mark_device_as_verified().await? {
+        self.identities_being_verified
-            let identity = self.mark_identity_as_verified().await?;
+            .mark_as_done(self.verified_devices().as_deref(), self.verified_identities().as_deref())
-
+            .await
-            // We only sign devices of our own user here.
-            let signature_request = if device.user_id() == self.user_id() {
-                match self.private_identity.sign_device(&device).await {
-                    Ok(r) => Some(r),
-                    Err(SignatureError::MissingSigningKey) => {
-                        warn!(
-                            "Can't sign the device keys for {} {}, \
-                                  no private user signing key found",
-                            device.user_id(),
-                            device.device_id(),
-                        );
-
-                        None
-                    }
-                    Err(e) => {
-                        error!(
-                            "Error signing device keys for {} {} {:?}",
-                            device.user_id(),
-                            device.device_id(),
-                            e
-                        );
-                        None
-                    }
-                }
-            } else {
-                None
-            };
-
-            let mut changes = Changes {
-                devices: DeviceChanges { changed: vec![device], ..Default::default() },
-                ..Default::default()
-            };
-
-            let identity_signature_request = if let Some(i) = identity {
-                // We only sign other users here.
-                let request = if let Some(i) = i.other() {
-                    // Signing can fail if the user signing key is missing.
-                    match self.private_identity.sign_user(&i).await {
-                        Ok(r) => Some(r),
-                        Err(SignatureError::MissingSigningKey) => {
-                            warn!(
-                                "Can't sign the public cross signing keys for {}, \
-                                  no private user signing key found",
-                                i.user_id()
-                            );
-                            None
-                        }
-                        Err(e) => {
-                            error!(
-                                "Error signing the public cross signing keys for {} {:?}",
-                                i.user_id(),
-                                e
-                            );
-                            None
-                        }
-                    }
-                } else {
-                    None
-                };
-
-                changes.identities.changed.push(i);
-
-                request
-            } else {
-                None
-            };
-
-            // If there are two signature upload requests, merge them. Otherwise
-            // use the one we have or None.
-            //
-            // Realistically at most one reuqest will be used but let's make
-            // this future proof.
-            let merged_request = if let Some(mut r) = signature_request {
-                if let Some(user_request) = identity_signature_request {
-                    r.signed_keys.extend(user_request.signed_keys);
-                    Some(r)
-                } else {
-                    Some(r)
-                }
-            } else {
-                identity_signature_request
-            };
-
-            // TODO store the request as well.
-            self.store.save_changes(changes).await?;
-            Ok(merged_request
-                .map(VerificationResult::SignatureUpload)
-                .unwrap_or(VerificationResult::Ok))
-        } else {
-            Ok(self.cancel().map(VerificationResult::Cancel).unwrap_or(VerificationResult::Ok))
-        }
-    }
-
-    pub(crate) async fn mark_identity_as_verified(
-        &self,
-    ) -> Result<Option<UserIdentities>, CryptoStoreError> {
-        // If there wasn't an identity available during the verification flow
-        // return early as there's nothing to do.
-        if self.other_identity.is_none() {
-            return Ok(None);
-        }
-
-        // TODO signal an error, e.g. when the identity got deleted so we don't
-        // verify/save the device either.
-        let identity = self.store.get_user_identity(self.other_user_id()).await?;
-
-        if let Some(identity) = identity {
-            if self
-                .other_identity
-                .as_ref()
-                .map_or(false, |i| i.master_key() == identity.master_key())
-            {
-                if self.verified_identities().map_or(false, |i| i.contains(&identity)) {
-                    trace!("Marking user identity of {} as verified.", identity.user_id(),);
-
-                    if let UserIdentities::Own(i) = &identity {
-                        i.mark_as_verified();
-                    }
-
-                    Ok(Some(identity))
-                } else {
-                    info!(
-                        "The interactive verification process didn't contain a \
-                        MAC for the user identity of {} {:?}",
-                        identity.user_id(),
-                        self.verified_identities(),
-                    );
-
-                    Ok(None)
-                }
-            } else {
-                warn!(
-                    "The master keys of {} have changed while an interactive \
-                      verification was going on, not marking the identity as verified.",
-                    identity.user_id(),
-                );
-
-                Ok(None)
-            }
-        } else {
-            info!(
-                "The identity for {} was deleted while an interactive \
-                  verification was going on.",
-                self.other_user_id(),
-            );
-            Ok(None)
-        }
-    }
-
-    pub(crate) async fn mark_device_as_verified(
-        &self,
-    ) -> Result<Option<ReadOnlyDevice>, CryptoStoreError> {
-        let device = self.store.get_device(self.other_user_id(), self.other_device_id()).await?;
-
-        if let Some(device) = device {
-            if device.keys() == self.other_device.keys() {
-                if self.verified_devices().map_or(false, |v| v.contains(&device)) {
-                    trace!(
-                        "Marking device {} {} as verified.",
-                        device.user_id(),
-                        device.device_id()
-                    );
-
-                    device.set_trust_state(LocalTrust::Verified);
-
-                    Ok(Some(device))
-                } else {
-                    info!(
-                        "The interactive verification process didn't contain a \
-                        MAC for the device {} {}",
-                        device.user_id(),
-                        device.device_id()
-                    );
-
-                    Ok(None)
-                }
-            } else {
-                warn!(
-                    "The device keys of {} {} have changed while an interactive \
-                      verification was going on, not marking the device as verified.",
-                    device.user_id(),
-                    device.device_id()
-                );
-                Ok(None)
-            }
-        } else {
-            let device = self.other_device();
-
-            info!(
-                "The device {} {} was deleted while an interactive \
-                  verification was going on.",
-                device.user_id(),
-                device.device_id()
-            );
-            Ok(None)
-        }
     }
 
     /// Cancel the verification.
@@ -540,11 +328,14 @@ impl Sas {
     /// Returns None if the `Sas` object is already in a canceled state,
     /// otherwise it returns a request that needs to be sent out.
     pub fn cancel(&self) -> Option<OutgoingVerificationRequest> {
+        self.cancel_with_code(CancelCode::User)
+    }
+
+    pub(crate) fn cancel_with_code(&self, code: CancelCode) -> Option<OutgoingVerificationRequest> {
         let mut guard = self.inner.lock().unwrap();
         let sas: InnerSas = (*guard).clone();
-        let (sas, content) = sas.cancel(CancelCode::User);
+        let (sas, content) = sas.cancel(code);
         *guard = sas;
-
         content.map(|c| match c {
             CancelContent::Room(room_id, content) => RoomMessageRequest {
                 room_id,
@@ -562,21 +353,7 @@ impl Sas {
         if self.is_cancelled() || self.is_done() {
             None
         } else if self.timed_out() {
-            let mut guard = self.inner.lock().unwrap();
+            self.cancel_with_code(CancelCode::Timeout)
-            let sas: InnerSas = (*guard).clone();
-            let (sas, content) = sas.cancel(CancelCode::Timeout);
-            *guard = sas;
-            content.map(|c| match c {
-                CancelContent::Room(room_id, content) => RoomMessageRequest {
-                    room_id,
-                    txn_id: Uuid::new_v4(),
-                    content: AnyMessageEventContent::KeyVerificationCancel(content),
-                }
-                .into(),
-                CancelContent::ToDevice(c) => self
-                    .content_to_request(AnyToDeviceEventContent::KeyVerificationCancel(c))
-                    .into(),
-            })
         } else {
             None
         }