From 83d6e8ea827d8efd9babd85e553c38446fac4fb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Wed, 4 May 2022 13:19:07 +0200 Subject: [PATCH] refactor(crypto): Simplify the verification object constructors This patch moves all the important identities that we need for verification into the already existing IdentitiesBeingVerified struct. It adds a method to get those identities to the store, and streamlines our to-device and in-room verification constructors. --- .../src/identities/device.rs | 1 + .../matrix-sdk-crypto/src/identities/user.rs | 1 + .../src/verification/machine.rs | 68 ++--- .../matrix-sdk-crypto/src/verification/mod.rs | 23 +- .../src/verification/qrcode.rs | 101 ++++--- .../src/verification/requests.rs | 252 ++++++------------ .../src/verification/sas/inner_sas.rs | 41 +-- .../src/verification/sas/mod.rs | 181 +++++-------- .../src/verification/sas/sas_state.rs | 41 +-- 9 files changed, 251 insertions(+), 458 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/device.rs b/crates/matrix-sdk-crypto/src/identities/device.rs index cce606e84..58d15be65 100644 --- a/crates/matrix-sdk-crypto/src/identities/device.rs +++ b/crates/matrix-sdk-crypto/src/identities/device.rs @@ -218,6 +218,7 @@ impl Device { if self.user_id() == self.verification_machine.own_user_id() { Ok(self .verification_machine + .store .private_identity .lock() .await diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index ee6522c69..b64e79a67 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -230,6 +230,7 @@ impl UserIdentity { if self.user_id() != self.verification_machine.own_user_id() { Ok(self .verification_machine + .store .private_identity .lock() .await diff --git a/crates/matrix-sdk-crypto/src/verification/machine.rs b/crates/matrix-sdk-crypto/src/verification/machine.rs index a530caffe..c7bf1bc1a 100644 --- a/crates/matrix-sdk-crypto/src/verification/machine.rs +++ b/crates/matrix-sdk-crypto/src/verification/machine.rs @@ -47,7 +47,6 @@ use crate::{ #[derive(Clone, Debug)] pub struct VerificationMachine { - pub(crate) private_identity: Arc>, pub(crate) store: VerificationStore, verifications: VerificationCache, requests: Arc>>, @@ -60,8 +59,7 @@ impl VerificationMachine { store: Arc, ) -> Self { Self { - private_identity: identity, - store: VerificationStore { account, inner: store }, + store: VerificationStore { account, private_identity: identity, inner: store }, verifications: VerificationCache::new(), requests: Default::default(), } @@ -85,7 +83,6 @@ impl VerificationMachine { let verification = VerificationRequest::new( self.verifications.clone(), - self.private_identity.lock().await.clone(), self.store.clone(), flow_id, user_id, @@ -111,7 +108,6 @@ impl VerificationMachine { let request = VerificationRequest::new( self.verifications.clone(), - self.private_identity.lock().await.clone(), self.store.clone(), flow_id, identity.user_id(), @@ -128,21 +124,8 @@ impl VerificationMachine { &self, device: ReadOnlyDevice, ) -> Result<(Sas, OutgoingVerificationRequest), CryptoStoreError> { - let identity = self.store.get_user_identity(device.user_id()).await?; - let own_identity = - self.store.get_user_identity(self.own_user_id()).await?.and_then(|i| i.into_own()); - let private_identity = self.private_identity.lock().await.clone(); - - let (sas, content) = Sas::start( - private_identity, - device.clone(), - self.store.clone(), - own_identity, - identity, - None, - true, - None, - ); + let identities = self.store.get_identities(device.clone()).await?; + let (sas, content) = Sas::start(identities, None, true, None); let request = match content { OutgoingContent::Room(r, c) => { @@ -337,7 +320,6 @@ impl VerificationMachine { if !event_sent_from_us(&event, r.from_device()) { let request = VerificationRequest::from_request( self.verifications.clone(), - self.private_identity.lock().await.clone(), self.store.clone(), event.sender(), flow_id, @@ -408,25 +390,9 @@ impl VerificationMachine { if let Some(device) = self.store.get_device(event.sender(), c.from_device()).await? { - let private_identity = self.private_identity.lock().await.clone(); - let own_identity = self - .store - .get_user_identity(self.own_user_id()) - .await? - .and_then(|i| i.into_own()); - let identity = self.store.get_user_identity(event.sender()).await?; + let identities = self.store.get_identities(device).await?; - match Sas::from_start_event( - flow_id, - c, - self.store.clone(), - private_identity, - device, - own_identity, - identity, - None, - false, - ) { + match Sas::from_start_event(flow_id, c, identities, None, false) { Ok(sas) => { self.verifications.insert_sas(sas); } @@ -563,20 +529,20 @@ mod tests { store.save_devices(vec![bob_device]).await; bob_store.save_devices(vec![alice_device.clone()]).await; - let bob_store = VerificationStore { account: bob, inner: Arc::new(bob_store) }; + let identity = Arc::new(Mutex::new(PrivateCrossSigningIdentity::empty(bob_id()))); + let bob_store = VerificationStore { + account: bob, + inner: Arc::new(bob_store), + private_identity: identity, + }; - let identity = Arc::new(Mutex::new(PrivateCrossSigningIdentity::empty(alice_id()))); - let machine = VerificationMachine::new(alice, identity, Arc::new(store)); - let (bob_sas, start_content) = Sas::start( - PrivateCrossSigningIdentity::empty(bob_id()), - alice_device, - bob_store, - None, - None, - None, - true, - None, + let identities = bob_store.get_identities(alice_device).await.unwrap(); + let machine = VerificationMachine::new( + alice, + Mutex::new(PrivateCrossSigningIdentity::empty(alice_id())).into(), + Arc::new(store), ); + let (bob_sas, start_content) = Sas::start(identities, None, true, None); machine .receive_any_event(&wrap_any_to_device_content(bob_sas.user_id(), start_content)) diff --git a/crates/matrix-sdk-crypto/src/verification/mod.rs b/crates/matrix-sdk-crypto/src/verification/mod.rs index 6045f0577..eb24d0178 100644 --- a/crates/matrix-sdk-crypto/src/verification/mod.rs +++ b/crates/matrix-sdk-crypto/src/verification/mod.rs @@ -58,12 +58,13 @@ use crate::{ gossiping::{GossipMachine, GossipRequest}, olm::{PrivateCrossSigningIdentity, ReadOnlyAccount, Session}, store::{Changes, CryptoStore}, - CryptoStoreError, LocalTrust, ReadOnlyDevice, ReadOnlyUserIdentities, + CryptoStoreError, LocalTrust, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, }; #[derive(Clone, Debug)] pub(crate) struct VerificationStore { pub account: ReadOnlyAccount, + pub private_identity: Arc>, inner: Arc, } @@ -101,6 +102,25 @@ impl VerificationStore { self.inner.get_user_identity(user_id).await } + pub async fn get_identities( + &self, + device_being_verified: ReadOnlyDevice, + ) -> Result { + let identity_being_verified = + self.get_user_identity(device_being_verified.user_id()).await?; + + Ok(IdentitiesBeingVerified { + private_identity: self.private_identity.lock().await.clone(), + store: self.clone(), + device_being_verified, + own_identity: self + .get_user_identity(self.account.user_id()) + .await? + .and_then(|i| i.into_own()), + identity_being_verified, + }) + } + pub async fn save_changes(&self, changes: Changes) -> Result<(), CryptoStoreError> { self.inner.save_changes(changes).await } @@ -413,6 +433,7 @@ pub struct IdentitiesBeingVerified { private_identity: PrivateCrossSigningIdentity, store: VerificationStore, device_being_verified: ReadOnlyDevice, + own_identity: Option, identity_being_verified: Option, } diff --git a/crates/matrix-sdk-crypto/src/verification/qrcode.rs b/crates/matrix-sdk-crypto/src/verification/qrcode.rs index f463c02d2..554f0c5f0 100644 --- a/crates/matrix-sdk-crypto/src/verification/qrcode.rs +++ b/crates/matrix-sdk-crypto/src/verification/qrcode.rs @@ -47,8 +47,8 @@ use super::{ VerificationStore, }; use crate::{ - olm::PrivateCrossSigningIdentity, CryptoStoreError, OutgoingVerificationRequest, - ReadOnlyDevice, ReadOnlyUserIdentities, RoomMessageRequest, ToDeviceRequest, + CryptoStoreError, OutgoingVerificationRequest, ReadOnlyDevice, ReadOnlyUserIdentities, + RoomMessageRequest, ToDeviceRequest, }; const SECRET_SIZE: usize = 16; @@ -504,10 +504,8 @@ impl QrVerification { Self::new_helper(flow_id, inner, identities, we_started, request_handle) } - #[allow(clippy::too_many_arguments)] pub(crate) async fn from_scan( store: VerificationStore, - private_identity: PrivateCrossSigningIdentity, other_user_id: OwnedUserId, other_device_id: OwnedDeviceId, flow_id: FlowId, @@ -522,19 +520,22 @@ impl QrVerification { }); } - let own_identity = - store.get_user_identity(store.account.user_id()).await?.ok_or_else(|| { - ScanError::MissingCrossSigningIdentity(store.account.user_id().to_owned()) - })?; - let other_identity = store - .get_user_identity(&other_user_id) - .await? - .ok_or_else(|| ScanError::MissingCrossSigningIdentity(other_user_id.clone()))?; let other_device = store.get_device(&other_user_id, &other_device_id).await?.ok_or_else(|| { ScanError::MissingDeviceKeys(other_user_id.clone(), other_device_id.clone()) })?; + let identities = store.get_identities(other_device).await?; + + let own_identity = identities.own_identity.as_ref().ok_or_else(|| { + ScanError::MissingCrossSigningIdentity(store.account.user_id().to_owned()) + })?; + + let other_identity = identities + .identity_being_verified + .as_ref() + .ok_or_else(|| ScanError::MissingCrossSigningIdentity(other_user_id.clone()))?; + let check_master_key = |key, identity: &ReadOnlyUserIdentities| { let master_key = identity.master_key().get_first_key().ok_or_else(|| { ScanError::MissingCrossSigningIdentity(identity.user_id().to_owned()) @@ -550,28 +551,25 @@ impl QrVerification { } }; - let (device_being_verified, identity_being_verified) = match qr_code { + match qr_code { QrVerificationData::Verification(_) => { - check_master_key(qr_code.first_key(), &other_identity)?; - check_master_key(qr_code.second_key(), &own_identity)?; - - (other_device, Some(other_identity)) + check_master_key(qr_code.first_key(), other_identity)?; + check_master_key(qr_code.second_key(), &own_identity.to_owned().into())?; } QrVerificationData::SelfVerification(_) => { - check_master_key(qr_code.first_key(), &other_identity)?; + check_master_key(qr_code.first_key(), other_identity)?; if qr_code.second_key() != store.account.identity_keys().ed25519 { return Err(ScanError::KeyMismatch { expected: store.account.identity_keys().ed25519.to_base64(), found: qr_code.second_key().to_base64(), }); } - - (other_device, Some(other_identity)) } QrVerificationData::SelfVerificationNoMasterKey(_) => { - let device_key = other_device.ed25519_key().ok_or_else(|| { - ScanError::MissingDeviceKeys(other_user_id.clone(), other_device_id.clone()) - })?; + let device_key = + identities.device_being_verified.ed25519_key().ok_or_else(|| { + ScanError::MissingDeviceKeys(other_user_id.clone(), other_device_id.clone()) + })?; if qr_code.first_key() != device_key { return Err(ScanError::KeyMismatch { @@ -579,18 +577,10 @@ impl QrVerification { found: qr_code.first_key().to_base64(), }); } - check_master_key(qr_code.second_key(), &other_identity)?; - (other_device, Some(other_identity)) + check_master_key(qr_code.second_key(), other_identity)?; } }; - let identities = IdentitiesBeingVerified { - private_identity, - store: store.clone(), - device_being_verified, - identity_being_verified, - }; - let secret = qr_code.secret().to_owned(); let own_device_id = store.account.device_id().to_owned(); @@ -800,6 +790,7 @@ mod tests { use std::{convert::TryFrom, sync::Arc}; use matrix_qrcode::QrVerificationData; + use matrix_sdk_common::locks::Mutex; use matrix_sdk_test::async_test; use ruma::{device_id, event_id, room_id, user_id, DeviceId, UserId}; @@ -808,7 +799,7 @@ mod tests { store::{Changes, CryptoStore, MemoryStore}, verification::{ event_enums::{DoneContent, OutgoingContent, StartContent}, - FlowId, IdentitiesBeingVerified, VerificationStore, + FlowId, VerificationStore, }, QrVerification, ReadOnlyDevice, }; @@ -830,23 +821,22 @@ mod tests { let store = memory_store(); let account = ReadOnlyAccount::new(user_id(), device_id()); - let store = VerificationStore { account: account.clone(), inner: store }; - let private_identity = PrivateCrossSigningIdentity::new(user_id().to_owned()).await; - let flow_id = FlowId::ToDevice("test_transaction".into()); - - let device_key = account.identity_keys.ed25519; let master_key = private_identity.master_public_key().await.unwrap(); let master_key = master_key.get_first_key().unwrap().to_owned(); + let store = VerificationStore { + account: account.clone(), + inner: store, + private_identity: Mutex::new(private_identity).into(), + }; + + let flow_id = FlowId::ToDevice("test_transaction".into()); + + let device_key = account.identity_keys.ed25519; let alice_device = ReadOnlyDevice::from_account(&account).await; - let identities = IdentitiesBeingVerified { - private_identity, - store: store.clone(), - device_being_verified: alice_device, - identity_being_verified: None, - }; + let identities = store.get_identities(alice_device).await.unwrap(); let verification = QrVerification::new_self_no_master( store.clone(), @@ -893,7 +883,13 @@ mod tests { let alice_account = ReadOnlyAccount::new(user_id(), device_id()); let store = memory_store(); - let store = VerificationStore { account: alice_account.clone(), inner: store }; + let private_identity = PrivateCrossSigningIdentity::new(user_id().to_owned()).await; + + let store = VerificationStore { + account: alice_account.clone(), + inner: store, + private_identity: Mutex::new(private_identity).into(), + }; let bob_account = ReadOnlyAccount::new(alice_account.user_id(), device_id!("BOBDEVICE")); @@ -912,12 +908,7 @@ mod tests { changes.devices.new.push(bob_device.clone()); store.save_changes(changes).await.unwrap(); - let identities = IdentitiesBeingVerified { - private_identity: PrivateCrossSigningIdentity::empty(alice_account.user_id()), - store: store.clone(), - device_being_verified: alice_device.clone(), - identity_being_verified: Some(identity.clone().into()), - }; + let identities = store.get_identities(alice_device.clone()).await.unwrap(); let alice_verification = QrVerification::new_self_no_master( store, @@ -930,7 +921,12 @@ mod tests { let bob_store = memory_store(); - let bob_store = VerificationStore { account: bob_account.clone(), inner: bob_store }; + let private_identity = PrivateCrossSigningIdentity::new(user_id().to_owned()).await; + let bob_store = VerificationStore { + account: bob_account.clone(), + inner: bob_store, + private_identity: Mutex::new(private_identity).into(), + }; let mut changes = Changes::default(); changes.identities.new.push(identity.into()); @@ -942,7 +938,6 @@ mod tests { let bob_verification = QrVerification::from_scan( bob_store, - private_identity, alice_account.user_id().to_owned(), alice_account.device_id().to_owned(), flow_id, diff --git a/crates/matrix-sdk-crypto/src/verification/requests.rs b/crates/matrix-sdk-crypto/src/verification/requests.rs index 38e42751f..9248657cc 100644 --- a/crates/matrix-sdk-crypto/src/verification/requests.rs +++ b/crates/matrix-sdk-crypto/src/verification/requests.rs @@ -38,22 +38,18 @@ use ruma::{ }; use tracing::{info, trace, warn}; +#[cfg(feature = "qrcode")] +use super::qrcode::{QrVerification, ScanError}; use super::{ cache::VerificationCache, event_enums::{ CancelContent, DoneContent, OutgoingContent, ReadyContent, RequestContent, StartContent, }, - CancelInfo, Cancelled, FlowId, Verification, VerificationStore, -}; -#[cfg(feature = "qrcode")] -use super::{ - qrcode::{QrVerification, ScanError}, - IdentitiesBeingVerified, + CancelInfo, Cancelled, FlowId, IdentitiesBeingVerified, Verification, VerificationStore, }; use crate::{ - olm::{PrivateCrossSigningIdentity, ReadOnlyAccount}, - CryptoStoreError, OutgoingVerificationRequest, ReadOnlyDevice, ReadOnlyOwnUserIdentity, - ReadOnlyUserIdentities, RoomMessageRequest, Sas, ToDeviceRequest, + olm::ReadOnlyAccount, CryptoStoreError, OutgoingVerificationRequest, RoomMessageRequest, Sas, + ToDeviceRequest, }; const SUPPORTED_METHODS: &[VerificationMethod] = &[ @@ -110,10 +106,8 @@ impl From>> for RequestHandle { } impl VerificationRequest { - #[allow(clippy::too_many_arguments)] pub(crate) fn new( cache: VerificationCache, - private_cross_signing_identity: PrivateCrossSigningIdentity, store: VerificationStore, flow_id: FlowId, other_user: &UserId, @@ -122,7 +116,6 @@ impl VerificationRequest { ) -> Self { let account = store.account.clone(); let inner = Mutex::new(InnerRequest::Created(RequestState::new( - private_cross_signing_identity, cache.clone(), store, other_user, @@ -330,7 +323,6 @@ impl VerificationRequest { let fut = if let InnerRequest::Ready(r) = &*self.inner.lock().unwrap() { Some(QrVerification::from_scan( r.store.clone(), - r.private_cross_signing_identity.clone(), r.other_user_id.clone(), r.state.other_device_id.clone(), r.flow_id.as_ref().to_owned(), @@ -354,7 +346,6 @@ impl VerificationRequest { pub(crate) fn from_request( cache: VerificationCache, - private_cross_signing_identity: PrivateCrossSigningIdentity, store: VerificationStore, sender: &UserId, flow_id: FlowId, @@ -365,12 +356,7 @@ impl VerificationRequest { Self { verification_cache: cache.clone(), inner: Arc::new(Mutex::new(InnerRequest::Requested(RequestState::from_request_event( - private_cross_signing_identity, - cache, - store, - sender, - &flow_id, - content, + cache, store, sender, &flow_id, content, )))), account, other_user_id: sender.into(), @@ -635,15 +621,8 @@ impl VerificationRequest { Ok(match &inner { InnerRequest::Ready(s) => { - if let Some((sas, content)) = s - .clone() - .start_sas( - s.store.clone(), - s.private_cross_signing_identity.clone(), - self.we_started, - self.inner.clone().into(), - ) - .await? + if let Some((sas, content)) = + s.clone().start_sas(self.we_started, self.inner.clone().into()).await? { self.verification_cache.insert_sas(sas.clone()); @@ -761,7 +740,6 @@ impl InnerRequest { #[derive(Clone, Debug)] struct RequestState { - private_cross_signing_identity: PrivateCrossSigningIdentity, verification_cache: VerificationCache, store: VerificationStore, flow_id: Arc, @@ -776,7 +754,6 @@ struct RequestState { impl RequestState { fn into_done(self, _: &DoneContent<'_>) -> RequestState { RequestState:: { - private_cross_signing_identity: self.private_cross_signing_identity, verification_cache: self.verification_cache, store: self.store, flow_id: self.flow_id, @@ -791,7 +768,6 @@ impl RequestState { cancel_code: &CancelCode, ) -> RequestState { RequestState:: { - private_cross_signing_identity: self.private_cross_signing_identity, verification_cache: self.verification_cache, store: self.store, flow_id: self.flow_id, @@ -803,7 +779,6 @@ impl RequestState { impl RequestState { fn new( - private_identity: PrivateCrossSigningIdentity, cache: VerificationCache, store: VerificationStore, other_user_id: &UserId, @@ -814,7 +789,6 @@ impl RequestState { Self { other_user_id: other_user_id.to_owned(), - private_cross_signing_identity: private_identity, state: Created { our_methods }, verification_cache: cache, store, @@ -827,7 +801,6 @@ impl RequestState { RequestState { flow_id: self.flow_id, verification_cache: self.verification_cache, - private_cross_signing_identity: self.private_cross_signing_identity, store: self.store, other_user_id: self.other_user_id, state: Ready { @@ -856,7 +829,6 @@ struct Requested { impl RequestState { fn from_request_event( - private_identity: PrivateCrossSigningIdentity, cache: VerificationCache, store: VerificationStore, sender: &UserId, @@ -865,7 +837,6 @@ impl RequestState { ) -> RequestState { // TODO only create this if we support the methods RequestState { - private_cross_signing_identity: private_identity, store, verification_cache: cache, flow_id: flow_id.to_owned().into(), @@ -881,7 +852,6 @@ impl RequestState { RequestState { flow_id: self.flow_id, verification_cache: self.verification_cache, - private_cross_signing_identity: self.private_cross_signing_identity, store: self.store, other_user_id: self.other_user_id, state: Passive { other_device_id: content.from_device().to_owned() }, @@ -892,7 +862,6 @@ impl RequestState { let state = RequestState { store: self.store, verification_cache: self.verification_cache, - private_cross_signing_identity: self.private_cross_signing_identity, flow_id: self.flow_id.clone(), other_user_id: self.other_user_id, state: Ready { @@ -944,20 +913,14 @@ impl RequestState { fn to_started_sas<'a>( &self, content: &StartContent<'a>, - other_device: ReadOnlyDevice, - own_identity: Option, - other_identity: Option, + identities: IdentitiesBeingVerified, we_started: bool, request_handle: RequestHandle, ) -> Result { Sas::from_start_event( (*self.flow_id).to_owned(), content, - self.store.clone(), - self.private_cross_signing_identity.clone(), - other_device, - own_identity, - other_identity, + identities, Some(request_handle), we_started, ) @@ -969,6 +932,8 @@ impl RequestState { we_started: bool, request_handle: RequestHandle, ) -> Result, CryptoStoreError> { + use crate::ReadOnlyUserIdentities; + // If we didn't state that we support showing QR codes or if the other // side doesn't support scanning QR codes bail early. if !self.state.our_methods.contains(&VerificationMethod::QrCodeShowV1) @@ -991,24 +956,19 @@ impl RequestState { return Ok(None); }; - let identites = IdentitiesBeingVerified { - private_identity: self.private_cross_signing_identity.clone(), - store: self.store.clone(), - device_being_verified: device, - identity_being_verified: self.store.get_user_identity(&self.other_user_id).await?, - }; + let identities = self.store.get_identities(device).await?; - let verification = if let Some(identity) = &identites.identity_being_verified { + let verification = if let Some(identity) = &identities.identity_being_verified { match &identity { ReadOnlyUserIdentities::Own(i) => { if let Some(master_key) = i.master_key().get_first_key() { - if identites.can_sign_devices().await { - if let Some(device_key) = identites.other_device().ed25519_key() { + if identities.can_sign_devices().await { + if let Some(device_key) = identities.other_device().ed25519_key() { Some(QrVerification::new_self( self.flow_id.as_ref().to_owned(), master_key.to_owned(), device_key.to_owned(), - identites, + identities, we_started, Some(request_handle), )) @@ -1026,7 +986,7 @@ impl RequestState { self.store.clone(), self.flow_id.as_ref().to_owned(), master_key.to_owned(), - identites, + identities, we_started, Some(request_handle), )) @@ -1046,8 +1006,8 @@ impl RequestState { // TODO we can get the master key from the public // identity if we don't have the private one and we // trust the public one. - if let Some(own_master) = self - .private_cross_signing_identity + if let Some(own_master) = identities + .private_identity .master_public_key() .await .and_then(|m| m.get_first_key().map(|m| m.to_owned())) @@ -1056,7 +1016,7 @@ impl RequestState { self.flow_id.as_ref().to_owned(), own_master, other_master.to_owned(), - identites, + identities, we_started, Some(request_handle), )) @@ -1123,22 +1083,13 @@ impl RequestState { return Ok(()); }; - let identity = self.store.get_user_identity(sender).await?; + let identities = self.store.get_identities(device.clone()).await?; let own_user_id = self.store.account.user_id(); let own_device_id = self.store.account.device_id(); - let own_identity = - self.store.get_user_identity(own_user_id).await?.and_then(|i| i.into_own()); match content.method() { StartMethod::SasV1(_) => { - match self.to_started_sas( - content, - device.clone(), - own_identity, - identity, - we_started, - request_handle, - ) { + match self.to_started_sas(content, identities, we_started, request_handle) { Ok(s) => { let start_new = if let Some(Verification::SasV1(_sas)) = self.verification_cache.get(sender, self.flow_id.as_str()) @@ -1163,8 +1114,8 @@ impl RequestState { } Err(c) => { warn!( - user_id = device.user_id().as_str(), - device_id = device.device_id().as_str(), + user_id = %device.user_id(), + device_id = %device.device_id(), content = ?c, "Can't start key verification, canceling.", ); @@ -1185,8 +1136,8 @@ impl RequestState { self.verification_cache.add_request(request.into()) } trace!( - sender = device.user_id().as_str(), - device_id = device.device_id().as_str(), + sender = %identities.device_being_verified.user_id(), + device_id = %identities.device_being_verified.device_id(), verification = ?qr_verification, "Received a QR code reciprocation" ) @@ -1202,8 +1153,6 @@ impl RequestState { async fn start_sas( self, - store: VerificationStore, - private_identity: PrivateCrossSigningIdentity, we_started: bool, request_handle: RequestHandle, ) -> Result, CryptoStoreError> { @@ -1212,13 +1161,6 @@ impl RequestState { } // TODO signal why starting the sas flow doesn't work? - let other_identity = store.get_user_identity(&self.other_user_id).await?; - let own_identity = self - .store - .get_user_identity(self.store.account.user_id()) - .await? - .and_then(|i| i.into_own()); - let device = if let Some(device) = self.store.get_device(&self.other_user_id, &self.state.other_device_id).await? { @@ -1233,29 +1175,19 @@ impl RequestState { return Ok(None); }; + let identities = self.store.get_identities(device).await?; + Ok(Some(match self.flow_id.as_ref() { FlowId::ToDevice(t) => { - let (sas, content) = Sas::start( - private_identity, - device, - store, - own_identity, - other_identity, - Some(t.to_owned()), - we_started, - Some(request_handle), - ); + let (sas, content) = + Sas::start(identities, Some(t.to_owned()), we_started, Some(request_handle)); (sas, content) } FlowId::InRoom(r, e) => { let (sas, content) = Sas::start_in_room( e.to_owned(), r.to_owned(), - private_identity, - device, - store, - own_identity, - other_identity, + identities, we_started, request_handle, ); @@ -1280,6 +1212,7 @@ mod tests { use std::convert::{TryFrom, TryInto}; + use matrix_sdk_common::locks::Mutex; use matrix_sdk_test::async_test; use ruma::{device_id, event_id, room_id, user_id, DeviceId, UserId}; @@ -1311,31 +1244,59 @@ mod tests { device_id!("BOBDEVCIE") } + async fn setup_stores() -> (VerificationStore, VerificationStore) { + let alice = ReadOnlyAccount::new(alice_id(), alice_device_id()); + let alice_store: Box = Box::new(MemoryStore::new()); + let alice_identity = Mutex::new(PrivateCrossSigningIdentity::empty(alice_id())); + + let alice_store = VerificationStore { + account: alice, + inner: alice_store.into(), + private_identity: alice_identity.into(), + }; + + let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); + let bob_store: Box = Box::new(MemoryStore::new()); + let bob_identity = Mutex::new(PrivateCrossSigningIdentity::empty(bob_id())); + + let bob_store = VerificationStore { + account: bob.clone(), + inner: bob_store.into(), + private_identity: bob_identity.into(), + }; + + let alice_device = ReadOnlyDevice::from_account(&alice_store.account).await; + let bob_device = ReadOnlyDevice::from_account(&bob_store.account).await; + + let mut changes = Changes::default(); + changes.devices.new.push(bob_device.clone()); + alice_store.save_changes(changes).await.unwrap(); + + let mut changes = Changes::default(); + changes.devices.new.push(alice_device.clone()); + bob_store.save_changes(changes).await.unwrap(); + + (alice_store, bob_store) + } + #[async_test] async fn test_request_accepting() { let event_id = event_id!("$1234localhost").to_owned(); let room_id = room_id!("!test:localhost").to_owned(); - let alice = ReadOnlyAccount::new(alice_id(), alice_device_id()); - let alice_store: Box = Box::new(MemoryStore::new()); - let alice_identity = PrivateCrossSigningIdentity::empty(alice_id()); + let (alice_store, bob_store) = setup_stores().await; - let alice_store = VerificationStore { account: alice, inner: alice_store.into() }; - - let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); - let bob_store: Box = Box::new(MemoryStore::new()); - let bob_identity = PrivateCrossSigningIdentity::empty(alice_id()); - - let bob_store = VerificationStore { account: bob.clone(), inner: bob_store.into() }; - - let content = - VerificationRequest::request(bob.user_id(), bob.device_id(), alice_id(), None); + let content = VerificationRequest::request( + bob_store.account.user_id(), + bob_store.account.device_id(), + alice_id(), + None, + ); let flow_id = FlowId::InRoom(room_id, event_id); let bob_request = VerificationRequest::new( VerificationCache::new(), - bob_identity, bob_store, flow_id.clone(), alice_id(), @@ -1346,7 +1307,6 @@ mod tests { #[allow(clippy::needless_borrow)] let alice_request = VerificationRequest::from_request( VerificationCache::new(), - alice_identity, alice_store, bob_id(), flow_id, @@ -1367,36 +1327,19 @@ mod tests { let event_id = event_id!("$1234localhost"); let room_id = room_id!("!test:localhost"); - let alice = ReadOnlyAccount::new(alice_id(), alice_device_id()); - let alice_device = ReadOnlyDevice::from_account(&alice).await; + let (alice_store, bob_store) = setup_stores().await; + let bob_device = ReadOnlyDevice::from_account(&bob_store.account).await; - let alice_store: Box = Box::new(MemoryStore::new()); - let alice_identity = PrivateCrossSigningIdentity::empty(alice_id()); - - let alice_store = VerificationStore { account: alice.clone(), inner: alice_store.into() }; - - let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); - let bob_device = ReadOnlyDevice::from_account(&bob).await; - let bob_store: Box = Box::new(MemoryStore::new()); - let bob_identity = PrivateCrossSigningIdentity::empty(alice_id()); - - let bob_store = VerificationStore { account: bob.clone(), inner: bob_store.into() }; - - let mut changes = Changes::default(); - changes.devices.new.push(bob_device.clone()); - alice_store.save_changes(changes).await.unwrap(); - - let mut changes = Changes::default(); - changes.devices.new.push(alice_device.clone()); - bob_store.save_changes(changes).await.unwrap(); - - let content = - VerificationRequest::request(bob.user_id(), bob.device_id(), alice_id(), None); + let content = VerificationRequest::request( + bob_store.account.user_id(), + bob_store.account.device_id(), + alice_id(), + None, + ); let flow_id = FlowId::from((room_id, event_id)); let bob_request = VerificationRequest::new( VerificationCache::new(), - bob_identity, bob_store, flow_id.clone(), alice_id(), @@ -1407,7 +1350,6 @@ mod tests { #[allow(clippy::needless_borrow)] let alice_request = VerificationRequest::from_request( VerificationCache::new(), - alice_identity, alice_store, bob_id(), flow_id, @@ -1437,34 +1379,13 @@ mod tests { #[async_test] async fn test_requesting_until_sas_to_device() { - let alice = ReadOnlyAccount::new(alice_id(), alice_device_id()); - let alice_device = ReadOnlyDevice::from_account(&alice).await; - - let alice_store: Box = Box::new(MemoryStore::new()); - let alice_identity = PrivateCrossSigningIdentity::empty(alice_id()); - - let alice_store = VerificationStore { account: alice.clone(), inner: alice_store.into() }; - - let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); - let bob_device = ReadOnlyDevice::from_account(&bob).await; - let bob_store: Box = Box::new(MemoryStore::new()); - let bob_identity = PrivateCrossSigningIdentity::empty(alice_id()); - - let mut changes = Changes::default(); - changes.devices.new.push(bob_device.clone()); - alice_store.save_changes(changes).await.unwrap(); - - let mut changes = Changes::default(); - changes.devices.new.push(alice_device.clone()); - bob_store.save_changes(changes).await.unwrap(); - - let bob_store = VerificationStore { account: bob.clone(), inner: bob_store.into() }; + let (alice_store, bob_store) = setup_stores().await; + let bob_device = ReadOnlyDevice::from_account(&bob_store.account).await; let flow_id = FlowId::ToDevice("TEST_FLOW_ID".into()); let bob_request = VerificationRequest::new( VerificationCache::new(), - bob_identity, bob_store, flow_id, alice_id(), @@ -1479,7 +1400,6 @@ mod tests { let alice_request = VerificationRequest::from_request( VerificationCache::new(), - alice_identity, alice_store, bob_id(), flow_id, @@ -1505,5 +1425,7 @@ mod tests { assert!(!bob_sas.is_cancelled()); assert!(!alice_sas.is_cancelled()); + assert!(alice_sas.started_from_request()); + assert!(bob_sas.started_from_request()); } } diff --git a/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs b/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs index 34b29c7ea..ef6927bb6 100644 --- a/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs +++ b/crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs @@ -18,7 +18,7 @@ use std::sync::Arc; use matrix_sdk_common::instant::Instant; use ruma::{ events::key::verification::{cancel::CancelCode, ShortAuthenticationString}, - OwnedEventId, OwnedRoomId, OwnedTransactionId, UserId, + UserId, }; use super::{ @@ -57,7 +57,8 @@ impl InnerSas { other_device: ReadOnlyDevice, own_identity: Option, other_identity: Option, - transaction_id: Option, + transaction_id: FlowId, + started_from_request: bool, ) -> (InnerSas, OutgoingContent) { let sas = SasState::::new( account, @@ -65,6 +66,7 @@ impl InnerSas { own_identity, other_identity, transaction_id, + started_from_request, ); let content = sas.as_content(); (InnerSas::Created(sas), content.into()) @@ -133,26 +135,6 @@ impl InnerSas { } } - pub fn start_in_room( - event_id: OwnedEventId, - room_id: OwnedRoomId, - account: ReadOnlyAccount, - other_device: ReadOnlyDevice, - own_identity: Option, - other_identity: Option, - ) -> (InnerSas, OutgoingContent) { - let sas = SasState::::new_in_room( - room_id, - event_id, - account, - other_device, - own_identity, - other_identity, - ); - let content = sas.as_content(); - (InnerSas::Created(sas), content.into()) - } - pub fn from_start_event( account: ReadOnlyAccount, other_device: ReadOnlyDevice, @@ -380,21 +362,6 @@ impl InnerSas { } } - pub fn verification_flow_id(&self) -> Arc { - match self { - InnerSas::Created(s) => s.verification_flow_id.clone(), - InnerSas::Started(s) => s.verification_flow_id.clone(), - InnerSas::Cancelled(s) => s.verification_flow_id.clone(), - InnerSas::Accepted(s) => s.verification_flow_id.clone(), - InnerSas::KeyReceived(s) => s.verification_flow_id.clone(), - InnerSas::Confirmed(s) => s.verification_flow_id.clone(), - InnerSas::MacReceived(s) => s.verification_flow_id.clone(), - InnerSas::WaitingForDone(s) => s.verification_flow_id.clone(), - InnerSas::Done(s) => s.verification_flow_id.clone(), - InnerSas::WeAccepted(s) => s.verification_flow_id.clone(), - } - } - pub fn emoji(&self) -> Option<[Emoji; 7]> { match self { InnerSas::KeyReceived(s) => Some(s.get_emoji()), diff --git a/crates/matrix-sdk-crypto/src/verification/sas/mod.rs b/crates/matrix-sdk-crypto/src/verification/sas/mod.rs index 28c6e6991..e6c1ca608 100644 --- a/crates/matrix-sdk-crypto/src/verification/sas/mod.rs +++ b/crates/matrix-sdk-crypto/src/verification/sas/mod.rs @@ -34,14 +34,13 @@ use tracing::trace; use super::{ event_enums::{AnyVerificationContent, OutgoingContent, OwnedAcceptContent, StartContent}, requests::RequestHandle, - CancelInfo, FlowId, IdentitiesBeingVerified, VerificationResult, VerificationStore, + CancelInfo, FlowId, IdentitiesBeingVerified, VerificationResult, }; use crate::{ identities::{ReadOnlyDevice, ReadOnlyUserIdentities}, - olm::PrivateCrossSigningIdentity, requests::{OutgoingVerificationRequest, RoomMessageRequest}, store::CryptoStoreError, - Emoji, ReadOnlyAccount, ReadOnlyOwnUserIdentity, ToDeviceRequest, + Emoji, ReadOnlyAccount, ToDeviceRequest, }; /// Short authentication string object. @@ -143,33 +142,33 @@ impl Sas { } fn start_helper( - inner_sas: InnerSas, - private_identity: PrivateCrossSigningIdentity, - other_device: ReadOnlyDevice, - store: VerificationStore, - other_identity: Option, + flow_id: FlowId, + identities: IdentitiesBeingVerified, we_started: bool, request_handle: Option, - ) -> Sas { - let flow_id = inner_sas.verification_flow_id(); + ) -> (Sas, OutgoingContent) { + let (inner, content) = InnerSas::start( + identities.store.account.clone(), + identities.device_being_verified.clone(), + identities.own_identity.clone(), + identities.identity_being_verified.clone(), + flow_id.clone(), + request_handle.is_some(), + ); - let account = store.account.clone(); + let account = identities.store.account.clone(); - let identities = IdentitiesBeingVerified { - private_identity, - store, - device_being_verified: other_device, - identity_being_verified: other_identity, - }; - - Sas { - inner: Arc::new(Mutex::new(inner_sas)), - account, - identities_being_verified: identities, - flow_id, - we_started, - request_handle, - } + ( + Sas { + inner: Arc::new(Mutex::new(inner)), + account, + identities_being_verified: identities, + flow_id: flow_id.into(), + we_started, + request_handle, + }, + content, + ) } /// Start a new SAS auth flow with the given device. @@ -182,37 +181,15 @@ impl Sas { /// /// Returns the new `Sas` object and a `StartEventContent` that needs to be /// sent out through the server to the other device. - #[allow(clippy::too_many_arguments)] pub(crate) fn start( - private_identity: PrivateCrossSigningIdentity, - other_device: ReadOnlyDevice, - store: VerificationStore, - own_identity: Option, - other_identity: Option, + identities: IdentitiesBeingVerified, transaction_id: Option, we_started: bool, request_handle: Option, ) -> (Sas, OutgoingContent) { - let (inner, content) = InnerSas::start( - store.account.clone(), - other_device.clone(), - own_identity, - other_identity.clone(), - transaction_id, - ); + let flow_id = FlowId::ToDevice(transaction_id.unwrap_or_else(TransactionId::new)); - ( - Self::start_helper( - inner, - private_identity, - other_device, - store, - other_identity, - we_started, - request_handle, - ), - content, - ) + Self::start_helper(flow_id, identities, we_started, request_handle) } /// Start a new SAS auth flow with the given device inside the given room. @@ -229,35 +206,12 @@ impl Sas { pub(crate) fn start_in_room( flow_id: OwnedEventId, room_id: OwnedRoomId, - private_identity: PrivateCrossSigningIdentity, - other_device: ReadOnlyDevice, - store: VerificationStore, - own_identity: Option, - other_identity: Option, + identities: IdentitiesBeingVerified, we_started: bool, request_handle: RequestHandle, ) -> (Sas, OutgoingContent) { - let (inner, content) = InnerSas::start_in_room( - flow_id, - room_id, - store.account.clone(), - other_device.clone(), - own_identity, - other_identity.clone(), - ); - - ( - Self::start_helper( - inner, - private_identity, - other_device, - store, - other_identity, - we_started, - Some(request_handle), - ), - content, - ) + let flow_id = FlowId::InRoom(room_id, flow_id); + Self::start_helper(flow_id, identities, we_started, Some(request_handle)) } /// Create a new Sas object from a m.key.verification.start request. @@ -270,37 +224,33 @@ impl Sas { /// /// * `event` - The m.key.verification.start event that was sent to us by /// the other side. - #[allow(clippy::too_many_arguments)] pub(crate) fn from_start_event( flow_id: FlowId, content: &StartContent<'_>, - store: VerificationStore, - private_identity: PrivateCrossSigningIdentity, - other_device: ReadOnlyDevice, - own_identity: Option, - other_identity: Option, + identities: IdentitiesBeingVerified, request_handle: Option, we_started: bool, ) -> Result { let inner = InnerSas::from_start_event( - store.account.clone(), - other_device.clone(), - flow_id, + identities.store.account.clone(), + identities.device_being_verified.clone(), + flow_id.clone(), content, - own_identity, - other_identity.clone(), + identities.own_identity.clone(), + identities.identity_being_verified.clone(), request_handle.is_some(), )?; - Ok(Self::start_helper( - inner, - private_identity, - other_device, - store, - other_identity, + let account = identities.store.account.clone(); + + Ok(Sas { + inner: Arc::new(Mutex::new(inner)), + account, + identities_being_verified: identities, + flow_id: flow_id.into(), we_started, request_handle, - )) + }) } /// Accept the SAS verification. @@ -560,6 +510,7 @@ impl AcceptSettings { mod tests { use std::{convert::TryFrom, sync::Arc}; + use matrix_sdk_common::locks::Mutex; use matrix_sdk_test::async_test; use ruma::{device_id, user_id, DeviceId, UserId}; @@ -598,40 +549,30 @@ mod tests { let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); let bob_device = ReadOnlyDevice::from_account(&bob).await; - let alice_store = - VerificationStore { account: alice.clone(), inner: Arc::new(MemoryStore::new()) }; + let alice_store = VerificationStore { + account: alice.clone(), + inner: Arc::new(MemoryStore::new()), + private_identity: Mutex::new(PrivateCrossSigningIdentity::empty(alice_id())).into(), + }; let bob_store = MemoryStore::new(); bob_store.save_devices(vec![alice_device.clone()]).await; - let bob_store = VerificationStore { account: bob.clone(), inner: Arc::new(bob_store) }; + let bob_store = VerificationStore { + account: bob.clone(), + inner: Arc::new(bob_store), + private_identity: Mutex::new(PrivateCrossSigningIdentity::empty(bob_id())).into(), + }; - let (alice, content) = Sas::start( - PrivateCrossSigningIdentity::empty(alice_id()), - bob_device, - alice_store, - None, - None, - None, - true, - None, - ); + let identities = alice_store.get_identities(bob_device).await.unwrap(); + + let (alice, content) = Sas::start(identities, None, true, None); let flow_id = alice.flow_id().to_owned(); let content = StartContent::try_from(&content).unwrap(); - let bob = Sas::from_start_event( - flow_id, - &content, - bob_store, - PrivateCrossSigningIdentity::empty(bob_id()), - alice_device, - None, - None, - None, - false, - ) - .unwrap(); + let identities = bob_store.get_identities(alice_device).await.unwrap(); + let bob = Sas::from_start_event(flow_id, &content, identities, None, false).unwrap(); let request = bob.accept().unwrap(); let content = OutgoingContent::try_from(request).unwrap(); diff --git a/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs b/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs index d3cb9e509..f71b2bfac 100644 --- a/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs +++ b/crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs @@ -40,7 +40,7 @@ use ruma::{ AnyMessageLikeEventContent, AnyToDeviceEventContent, }, serde::Base64, - DeviceId, OwnedEventId, OwnedRoomId, OwnedTransactionId, TransactionId, UserId, + DeviceId, UserId, }; use tracing::info; use vodozemac::{ @@ -397,10 +397,9 @@ impl SasState { other_device: ReadOnlyDevice, own_identity: Option, other_identity: Option, - transaction_id: Option, + flow_id: FlowId, + started_from_request: bool, ) -> SasState { - let started_from_request = transaction_id.is_some(); - let flow_id = FlowId::ToDevice(transaction_id.unwrap_or_else(TransactionId::new)); Self::new_helper( flow_id, account, @@ -411,30 +410,6 @@ impl SasState { ) } - /// Create a new SAS in-room verification flow. - /// - /// # Arguments - /// - /// * `event_id` - The event id of the `m.key.verification.request` event - /// that started the verification flow. - /// - /// * `account` - Our own account. - /// - /// * `other_device` - The other device which we are going to verify. - /// - /// * `other_identity` - The identity of the other user if one exists. - pub fn new_in_room( - room_id: OwnedRoomId, - event_id: OwnedEventId, - account: ReadOnlyAccount, - other_device: ReadOnlyDevice, - own_identity: Option, - other_identity: Option, - ) -> SasState { - let flow_id = FlowId::InRoom(room_id, event_id); - Self::new_helper(flow_id, account, other_device, own_identity, other_identity, false) - } - fn new_helper( flow_id: FlowId, account: ReadOnlyAccount, @@ -1260,7 +1235,7 @@ mod tests { ShortAuthenticationString, }, serde::Base64, - user_id, DeviceId, UserId, + user_id, DeviceId, TransactionId, UserId, }; use serde_json::json; @@ -1296,7 +1271,9 @@ mod tests { let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); let bob_device = ReadOnlyDevice::from_account(&bob).await; - let alice_sas = SasState::::new(alice.clone(), bob_device, None, None, None); + let flow_id = TransactionId::new().into(); + let alice_sas = + SasState::::new(alice.clone(), bob_device, None, None, flow_id, false); let start_content = alice_sas.as_content(); let flow_id = start_content.flow_id(); @@ -1479,7 +1456,9 @@ mod tests { let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); let bob_device = ReadOnlyDevice::from_account(&bob).await; - let alice_sas = SasState::::new(alice.clone(), bob_device, None, None, None); + let flow_id = TransactionId::new().into(); + let alice_sas = + SasState::::new(alice.clone(), bob_device, None, None, flow_id, false); let mut start_content = alice_sas.as_content(); let method = start_content.method_mut();