From 369ca7024f00f9f744f7ff1d0d12bc24fb6c0137 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Fri, 24 Nov 2023 17:14:33 +0100 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Denis Kasak --- .../src/encryption/backups/futures.rs | 11 +-- .../matrix-sdk/src/encryption/backups/mod.rs | 89 ++++++++++--------- .../src/encryption/backups/types.rs | 45 +++++----- crates/matrix-sdk/src/encryption/mod.rs | 2 +- crates/matrix-sdk/src/utils.rs | 9 +- examples/backups/src/main.rs | 4 +- 6 files changed, 82 insertions(+), 78 deletions(-) diff --git a/crates/matrix-sdk/src/encryption/backups/futures.rs b/crates/matrix-sdk/src/encryption/backups/futures.rs index 101555312..730468206 100644 --- a/crates/matrix-sdk/src/encryption/backups/futures.rs +++ b/crates/matrix-sdk/src/encryption/backups/futures.rs @@ -26,7 +26,8 @@ use tracing::trace; use super::{Backups, UploadState}; use crate::utils::ChannelObservable; -/// Error describin how waiting for the backup upload to settle down can fail. +/// Error describing the ways that waiting for the backup upload to settle down +/// can fail. #[derive(Clone, Copy, Debug, Error)] pub enum SteadyStateError { /// The currently active backup got either deleted or a new one was created. @@ -35,11 +36,11 @@ pub enum SteadyStateError { /// backup. #[error("The backup got disabled while waiting for the room keys to be uploaded.")] BackupDisabled, - /// Uploading the room keys to the homeserver failed. + /// Uploading the room keys to the homeserver failed due to a network error. /// /// Uploading will be retried again at a later point in time, or /// immediately if you wait for the steady state again. - #[error("There was a connection error.")] + #[error("There was a network connection error.")] Connection, /// We missed some updates to the [`UploadState`] from the upload task. /// @@ -94,7 +95,7 @@ impl<'a> IntoFuture for WaitForSteadyState<'a> { trace!("Creating a stream to wait for the steady state"); - let mut stream = progress.subscribe(); + let mut progress_stream = progress.subscribe(); let old_delay = if let Some(delay) = timeout { let mut lock = backups.client.inner.backup_state.upload_delay.write().unwrap(); @@ -117,7 +118,7 @@ impl<'a> IntoFuture for WaitForSteadyState<'a> { // TODO: Do we want to be smart here and remember the count when we started // waiting and prevent the total from increasing, in case new room // keys arrive after we started waiting. - while let Some(state) = stream.next().await { + while let Some(state) = progress_stream.next().await { trace!(?state, "Update state while waiting for the backup steady state"); match state { diff --git a/crates/matrix-sdk/src/encryption/backups/mod.rs b/crates/matrix-sdk/src/encryption/backups/mod.rs index 549239503..7fd8e4415 100644 --- a/crates/matrix-sdk/src/encryption/backups/mod.rs +++ b/crates/matrix-sdk/src/encryption/backups/mod.rs @@ -58,10 +58,10 @@ pub struct Backups { } impl Backups { - /// Create a new backup recovery key and backup version. + /// Create a new backup version, encrypted with a new backup recovery key. /// /// The backup recovery key will be persisted locally and shared with - /// trusted devices as `m.secret.send` events. + /// trusted devices as `m.secret.send` to-device messages. /// /// After the backup has been created, all room keys will be uploaded to the /// homeserver. @@ -87,7 +87,8 @@ impl Backups { self.set_state(BackupState::Creating); - // Create a future so we can catch errors and go back to the `Unknown` state. + // Create a future so we can catch errors and go back to the `Unknown` + // state. This is a hack to get around the lack of `try` blocks in Rust. let future = async { let olm_machine = self.client.olm_machine().await; let olm_machine = olm_machine.as_ref().ok_or(Error::NoOlmMachine)?; @@ -106,9 +107,9 @@ impl Backups { // `BackupDecryptionKey` can do so, as per [spec]: // // Clients must only store keys in backups after they have ensured that the - // auth_data is trusted. This can be done either by: + // `auth_data` has not been tampered with. This can be done either by: // - // * checking that it is signed by the user’s master cross-signing key or by a + // * checking that it is signed by the user's master cross-signing key or by a // verified device belonging to the same user, or // * by deriving the public key from a private key that it obtained from a // trusted source. Trusted sources for the private key include the user @@ -116,9 +117,6 @@ impl Backups { // obtaining the key via secret sharing from a verified device belonging to // the same user. // - // Do note, I do think that the signature checking should be completely removed, - // and once we have proper authentication for backups from MSC4048, activating a - // backup without access to the backup recovery key won't be possible. // // [1]: https://spec.matrix.org/v1.8/client-server-api/#post_matrixclientv3room_keysversion // [spec]: https://spec.matrix.org/v1.8/client-server-api/#server-side-key-backups @@ -221,11 +219,11 @@ impl Backups { result } - /// Wait for room keys to be uploaded. + /// Returns a future to wait for room keys to be uploaded. /// - /// This method will wake up a task to upload room keys which have not yet - /// been uploaded to the homeserver. It will then wait for the task to - /// finish uploading. + /// Awaiting the future will wake up a task to upload room keys which have + /// not yet been uploaded to the homeserver. It will then wait for the task + /// to finish uploading. /// /// # Examples /// @@ -335,7 +333,8 @@ impl Backups { Ok(self.get_current_version().await?.is_some()) } - /// Subscribe to a stream of room keys we download and import from backups. + /// Subscribe to a stream that notifies when a room key for the specified + /// room is downloaded from the key backup. pub fn room_keys_for_room_stream( &self, room_id: &RoomId, @@ -343,10 +342,13 @@ impl Backups { { let room_id = room_id.to_owned(); - // TODO: This is a bit crap to say the least, the type is non descriptive and - // doesn't even contain all the important data, it should be a stream of - // `RoomKeyInfo` like the OlmMachine has... But on the other hand we - // should just used the OlmMachine stream and remove this. + // TODO: This is a bit crap to say the least. The type is + // non-descriptive and doesn't even contain all the important data. It + // should be a stream of `RoomKeyInfo` like the OlmMachine has... But on + // the other hand we should just be able to use the corresponding + // OlmMachine stream and remove this. Currently we can't do this because + // the OlmMachine gets destroyed and recreated all the time to be able + // to support the notifications-related multiprocessing on iOS. self.room_keys_stream().filter_map(move |import_result| { let room_id = room_id.to_owned(); @@ -359,8 +361,8 @@ impl Backups { }) } - /// Download all room keys for a certain room from the backup on the - /// homeserver. + /// Download all room keys for a certain room from the server-side key + /// backup. pub async fn download_room_keys_for_room(&self, room_id: &RoomId) -> Result<(), Error> { let olm_machine = self.client.olm_machine().await; let olm_machine = olm_machine.as_ref().ok_or(Error::NoOlmMachine)?; @@ -372,6 +374,8 @@ impl Backups { let request = get_backup_keys_for_room::v3::Request::new(version, room_id.to_owned()); let response = self.client.send(request, Default::default()).await?; + + // Transform response to standard format (map of room ID -> room key). let response = get_backup_keys::v3::Response::new(BTreeMap::from([( room_id.to_owned(), RoomKeyBackup::new(response.sessions), @@ -384,7 +388,7 @@ impl Backups { Ok(()) } - /// Download a single room key from the backup on the homeserver. + /// Download a single room key from the server-side key backup. pub async fn download_room_key(&self, room_id: &RoomId, session_id: &str) -> Result<(), Error> { let olm_machine = self.client.olm_machine().await; let olm_machine = olm_machine.as_ref().ok_or(Error::NoOlmMachine)?; @@ -399,6 +403,8 @@ impl Backups { session_id.to_owned(), ); let response = self.client.send(request, Default::default()).await?; + + // Transform response to standard format (map of room ID -> room key). let response = get_backup_keys::v3::Response::new(BTreeMap::from([( room_id.to_owned(), RoomKeyBackup::new(BTreeMap::from([( @@ -449,7 +455,7 @@ impl Backups { for (session_id, room_key) in room_keys.sessions { // TODO: Log that we're skipping some keys here. let Ok(room_key) = room_key.deserialize() else { - warn!("Couldn't deseriazlie a room key we downloaded from backups, session ID: {session_id}"); + warn!("Couldn't deserialize a room key we downloaded from backups, session ID: {session_id}"); continue; }; @@ -532,9 +538,9 @@ impl Backups { ) -> Result<(), Error> { trace!("Uploading some room keys"); - let request = add_backup_keys::v3::Request::new(request.version, request.rooms); + let add_backup_keys = add_backup_keys::v3::Request::new(request.version, request.rooms); - match self.client.send(request, Default::default()).await { + match self.client.send(add_backup_keys, Default::default()).await { Ok(response) => { olm_machine.mark_request_as_sent(request_id, &response).await?; @@ -559,14 +565,14 @@ impl Backups { if let Some(kind) = error.client_api_error_kind() { match kind { ErrorKind::NotFound => { - warn!("No backup found on the server, the backup got likely deleted, disabling backups."); + warn!("No backup found on the server, the backup likely got deleted, disabling backups."); self.handle_deleted_backup_version(olm_machine).await?; } ErrorKind::WrongRoomKeysVersion { current_version } => { warn!( new_version = current_version, - "A new backup version was found on the server, disabling backups" + "A new backup version was found on the server, disabling backups." ); // TODO: If we're verified and there are other devices besides us, @@ -628,16 +634,17 @@ impl Backups { /// /// This should be called if we receive a backup recovery, either: /// - /// * As a `m.secret.send` to-device event from a trusted device - /// * From the `m.megolm_backup.v1` global account data event. + /// * As an `m.secret.send` to-device message from a trusted device. + /// * From 4S (i.e. from the `m.megolm_backup.v1` event global account + /// data). /// /// In both cases the method will compare the currently active backup - /// version to the backup recovery key and if we have received a - /// machting key activate backups on this device and start uploading - /// room keys to the backup. + /// version to the backup recovery key's version and, if there is a match, + /// activate backups on this device and start uploading room keys to the + /// backup. /// - /// Returns true if backups are already or have been enabled, otherwise - /// false. + /// Returns true if backups were just enabled or were already enabled, + /// otherwise false. #[instrument(skip_all)] pub(crate) async fn maybe_enable_backups( &self, @@ -683,8 +690,8 @@ impl Backups { Ok(true) } else if decryption_key.backup_key_matches(&backup_info) { info!( - "We have found the correct backup recovery key, storing the backup recovery \ - key and enabling backups" + "We have found the correct backup recovery key. Storing the backup recovery \ + key and enabling backups." ); // We're enabling a new backup, reset the `backed_up` flags on the room keys and @@ -732,7 +739,7 @@ impl Backups { warn!( ?derived_key, ?downloaded_key, - "Found an active backup but the recovery key we received isn't the one used in \ + "Found an active backup but the recovery key we received isn't the one used for \ this backup version" ); @@ -783,8 +790,8 @@ impl Backups { } } - /// Try to resume backups by iterating through the `m.secret.send` events - /// the [`OlmMachine`] has received and stored in the secret inbox. + /// Try to resume backups by iterating through the `m.secret.send` to-device + /// messages the [`OlmMachine`] has received and stored in the secret inbox. async fn maybe_resume_from_secret_inbox(&self, olm_machine: &OlmMachine) -> Result<(), Error> { let secrets = olm_machine.store().get_secrets_from_inbox(&SecretName::RecoveryKey).await?; @@ -815,7 +822,7 @@ impl Backups { Ok(()) } - /// Listen for `m.secret.send` to-device events and check the secret inbox + /// Listen for `m.secret.send` to-device messages and check the secret inbox /// if we do receive one. pub(crate) async fn secret_send_event_handler(_: ToDeviceSecretSendEvent, client: Client) { let olm_machine = client.olm_machine().await; @@ -837,7 +844,7 @@ impl Backups { /// Try to enable backups using the secret found in the [`GossippedSecret`]. /// - /// The [`GossippedSecret`] represents a `m.secret.send` event we received + /// The [`GossippedSecret`] represents an `m.secret.send` event we received /// from a trusted device and have pulled out of the secret inbox. /// /// Returns true if we enabled backups, false otherwise. @@ -860,8 +867,8 @@ impl Backups { } } - /// Send a notification to the task which is responsible to upload room keys - /// to the backup that it might have new room keys to back up. + /// Send a notification to the task which is responsible for uploading room + /// keys to the backup that it might have new room keys to back up. pub(crate) fn maybe_trigger_backup(&self) { let tasks = self.client.inner.tasks.lock().unwrap(); diff --git a/crates/matrix-sdk/src/encryption/backups/types.rs b/crates/matrix-sdk/src/encryption/backups/types.rs index eea34a1a0..bfe03d38a 100644 --- a/crates/matrix-sdk/src/encryption/backups/types.rs +++ b/crates/matrix-sdk/src/encryption/backups/types.rs @@ -35,7 +35,7 @@ use crate::{ /// [`Backups::wait_for_steady_state()`]: crate::encryption::backups::Backups::wait_for_steady_state #[derive(Clone, Debug)] pub enum UploadState { - /// The task is iddle, waiting for new room keys to arrive to try to upload + /// The task is idle, waiting for new room keys to arrive to try to upload /// them. Idle, /// The task has awoken and is checking if new room keys need to be @@ -71,20 +71,19 @@ impl Default for BackupClientState { } } -/// The states the backup support for the current [`Client`] can be in. +/// The possible states of the [`Client`]'s room key backup mechanism. /// -/// Backups can be either enabled automatically if we receive a valid backup -/// recovery key[[1]] or the [`Client`] can create a new backup themselves. +/// A local backup instance can be created either by receiving a valid backup +/// recovery key [[1]] or by having the [`Client`] create a new backup itself. /// /// The [`Client`] can also delete and disable a currently active backup. /// /// Backups will be enabled automatically if we receive the backup recovery key /// either from: /// -/// * Another device using `m.secret.send`[[2]], this usually happens after -/// interactive -/// verification has been done. -/// * Secret storage[[3]], this can be done with the +/// * Another device using `m.secret.send`[[2]], which usually happens after +/// completing interactive verification. +/// * Secret storage[[3]], which is done by calling the /// [`SecretStore::import_secrets()`] method. /// /// [1]: https://spec.matrix.org/v1.8/client-server-api/#recovery-key @@ -92,36 +91,34 @@ impl Default for BackupClientState { /// [3]: https://spec.matrix.org/v1.8/client-server-api/#secret-storage #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub enum BackupState { - /// Backups are not active locally and we don't know if they exist on the - /// server. + /// There is no locally active backup and we don't know whether there backup + /// exists on the server. /// - /// The reason why we don't know if a backup might exist is that we don't - /// get notified by the server about the creation or deletion of - /// backups. If we want to know the current state of things we need to poll - /// the server, this can be done with the - /// [`Backups::exists_on_server()`] method. + /// The reason we don't know whether a server-side backup exists is that we + /// don't get notified by the server about the creation and deletion of + /// backups. If we want to know the current state, we need to poll the + /// server, which is done using the [`Backups::exists_on_server()`] method. #[default] Unknown, - /// A new backup is being created by this [`Client`], this state will be + /// A new backup is being created by this [`Client`]. This state will be /// entered if you call the [`Backups::create()`] method. Creating, - /// An existing backup is being enabled to be used by this [`Client`]. We + /// An existing backup is being enabled for use by this [`Client`]. We /// will enter this state if we have received a backup recovery key. Enabling, /// An existing backup will be enabled to be used by this [`Client`] after /// the client has been restored. This state happens every time a - /// [`Client`] is restored and we previously have enabled a backup. + /// [`Client`] is restored after we'd previously enabled a backup. Resuming, - /// Backups are enabled and room keys are actively being backed up. + /// The backup is enabled and room keys are actively being backed up. Enabled, /// Room keys are currently being downloaded. This state will only happen - /// after a `Enabling` state. The [`Client`] will attempt to download + /// after an `Enabling` state. The [`Client`] will attempt to download /// all room keys from the backup before transitioning into the /// `Enabled` state. Downloading, - /// The backups are being disabled and deleted from the server. This state - /// will happen when you call the [`Backups::disable()`] method, after - /// backups have been disabled we're going to transition into the - /// `Unknown` state. + /// The backup is being disabled and deleted from the server. This state + /// will happen when you call the [`Backups::disable()`] method. After it + /// has been disabled, we're going to transition into the `Unknown` state. Disabling, } diff --git a/crates/matrix-sdk/src/encryption/mod.rs b/crates/matrix-sdk/src/encryption/mod.rs index 37ad3f687..377990054 100644 --- a/crates/matrix-sdk/src/encryption/mod.rs +++ b/crates/matrix-sdk/src/encryption/mod.rs @@ -109,7 +109,7 @@ pub struct EncryptionSettings { /// /// 1. Received as a `m.secret.send` to-device event, after a successful /// interactive verification. - /// 2. Imported from secret storage using the + /// 2. Imported from secret storage (4S) using the /// [`SecretStore::import_secrets()`] method. /// /// [`SecretStore::import_secrets()`]: crate::encryption::secret_storage::SecretStore::import_secrets diff --git a/crates/matrix-sdk/src/utils.rs b/crates/matrix-sdk/src/utils.rs index 269f79d39..5c02e7b67 100644 --- a/crates/matrix-sdk/src/utils.rs +++ b/crates/matrix-sdk/src/utils.rs @@ -36,9 +36,9 @@ use crate::Room; /// An observable with channel semantics. /// -/// This type allows you to have a shared mutable value where updates to the -/// value will be send out to subscribers. Each update will be send to all the -/// subscribers and intermediate updates to the value will not be skipped. +/// Channel semantics means that each update to the shared mutable value will be +/// sent out to subscribers. That is, intermediate updates to the value will not +/// be skipped like they would be in an observable without channel semantics. #[cfg(feature = "e2e-encryption")] #[derive(Clone, Debug)] pub(crate) struct ChannelObservable { @@ -63,8 +63,7 @@ impl ChannelObservable { Self { value: RwLock::new(value).into(), channel } } - /// Subscribe to updates to the underlying data of the - /// [`ChannelObservable`]. + /// Subscribe to updates to the observable value. /// /// The current value will always be emitted as the first item in the /// stream. diff --git a/examples/backups/src/main.rs b/examples/backups/src/main.rs index 031b87504..d7e1ae62b 100644 --- a/examples/backups/src/main.rs +++ b/examples/backups/src/main.rs @@ -106,8 +106,8 @@ async fn main() -> Result<()> { client.encryption().secret_storage().open_secret_store(&cli.secret_store_key).await?; let _task = tokio::spawn({ - let client = client.to_owned(); - async move { listen_for_backup_state_changes(client.to_owned()).await } + let client = client.clone(); + async move { listen_for_backup_state_changes(client).await } }); import_known_secrets(&client, secret_store).await?;