From 81bfec38042c9f668d259c4536fdb8b7f28b98e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 17 Jun 2024 14:24:20 +0200 Subject: [PATCH 1/3] utils: Return the old value in the set method of ChannelObservable --- crates/matrix-sdk/src/utils.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk/src/utils.rs b/crates/matrix-sdk/src/utils.rs index 5c02e7b6751..bef0b63b042 100644 --- a/crates/matrix-sdk/src/utils.rs +++ b/crates/matrix-sdk/src/utils.rs @@ -76,10 +76,16 @@ impl ChannelObservable { } /// Set the underlying data to the new value. - pub(crate) fn set(&self, new_value: T) { - *self.value.write().unwrap() = new_value.to_owned(); + pub(crate) fn set(&self, new_value: T) -> T { + let old_value = { + let mut guard = self.value.write().unwrap(); + std::mem::replace(&mut (*guard), new_value.clone()) + }; + // We're ignoring the error case where no receivers exist. let _ = self.channel.send(new_value); + + old_value } /// Get the current value of the underlying data. @@ -183,3 +189,17 @@ impl IntoRawStateEventContent for &Box { self.clone().into_raw_state_event_content() } } + +#[cfg(test)] +mod test { + #[cfg(feature = "e2e-encryption")] + #[test] + fn test_channel_observable_get_set() { + let observable = super::ChannelObservable::new(0); + + assert_eq!(observable.get(), 0); + assert_eq!(observable.set(1), 0); + assert_eq!(observable.set(10), 1); + assert_eq!(observable.get(), 10); + } +} From 4f6a1737844cafc35d21c2163fb6e92ba78ec75f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Fri, 28 Jun 2024 14:57:57 +0200 Subject: [PATCH 2/3] encryption: Log when the backup and recovery state changes --- crates/matrix-sdk/src/encryption/backups/mod.rs | 9 +++++++-- crates/matrix-sdk/src/encryption/recovery/mod.rs | 6 +++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk/src/encryption/backups/mod.rs b/crates/matrix-sdk/src/encryption/backups/mod.rs index 6881b5cc847..4272eac1237 100644 --- a/crates/matrix-sdk/src/encryption/backups/mod.rs +++ b/crates/matrix-sdk/src/encryption/backups/mod.rs @@ -423,8 +423,12 @@ impl Backups { } /// Set the state of the backup. - fn set_state(&self, state: BackupState) { - self.client.inner.e2ee.backup_state.global_state.set(state); + fn set_state(&self, new_state: BackupState) { + let old_state = self.client.inner.e2ee.backup_state.global_state.set(new_state); + + if old_state != new_state { + info!("Backup state changed from {old_state:?} to {new_state:?}"); + } } /// Set the backup state to the `Enabled` variant and insert the backup key @@ -860,6 +864,7 @@ impl Backups { /// Listen for `m.secret.send` to-device messages and check the secret inbox /// if we do receive one. + #[instrument(skip_all)] pub(crate) async fn secret_send_event_handler(_: ToDeviceSecretSendEvent, client: Client) { let olm_machine = client.olm_machine().await; diff --git a/crates/matrix-sdk/src/encryption/recovery/mod.rs b/crates/matrix-sdk/src/encryption/recovery/mod.rs index bea746f77d4..152f4badeb6 100644 --- a/crates/matrix-sdk/src/encryption/recovery/mod.rs +++ b/crates/matrix-sdk/src/encryption/recovery/mod.rs @@ -488,7 +488,11 @@ impl Recovery { async fn update_recovery_state(&self) -> Result<()> { let new_state = self.check_recovery_state().await?; - self.client.inner.e2ee.recovery_state.set(new_state); + let old_state = self.client.inner.e2ee.recovery_state.set(new_state); + + if new_state != old_state { + info!("Recovery state changed from {old_state:?} to {new_state:?}"); + } Ok(()) } From 44a4505bb2ce5a2bfb0b929c65e6bd0ac1060be9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Fri, 28 Jun 2024 16:05:01 +0200 Subject: [PATCH 3/3] recovery: Ensure that we don't miss updates to the backup state This patch switches the way we update the recovery state upon changes in the backup state. Previously two places updated the recovery state after the backup state changed: 1. A method living in the recovery subsystem that the backup subsystem itself calls. 2. An event handler which is called when we receive a m.secret.send event. The first method is a hack because it introduces a circular dependency between the recovery and backup subsystems. More importantly, the second method can miss updates, because the backup subsystem has a similar event handler which then processes the secret we received and if the secret was a backup recovery key, enables backups. Depending on the order these event handlers are called, the recovery subsystem might update the recovery state before the secret has been handled. The backup subsystem provides an async stream which broadcasts updates to the backup state, letting the recovery subsystem listen to this stream and update its state if we notice such updates fixes both problems we listed above. The method in the first bullet point was completely removed, the event handler is kept for other secret types but we don't rely on it for the backup state anymore. --- .../matrix-sdk/src/encryption/backups/mod.rs | 1 - crates/matrix-sdk/src/encryption/mod.rs | 14 ++++++ .../matrix-sdk/src/encryption/recovery/mod.rs | 50 ++++++++++++++++--- crates/matrix-sdk/src/encryption/tasks.rs | 2 + .../tests/integration/encryption/recovery.rs | 20 +++++--- 5 files changed, 71 insertions(+), 16 deletions(-) diff --git a/crates/matrix-sdk/src/encryption/backups/mod.rs b/crates/matrix-sdk/src/encryption/backups/mod.rs index 4272eac1237..9bd6b68335c 100644 --- a/crates/matrix-sdk/src/encryption/backups/mod.rs +++ b/crates/matrix-sdk/src/encryption/backups/mod.rs @@ -926,7 +926,6 @@ impl Backups { /// removed on the homeserver. async fn handle_deleted_backup_version(&self, olm_machine: &OlmMachine) -> Result<(), Error> { olm_machine.backup_machine().disable_backup().await?; - self.client.encryption().recovery().update_state_after_backup_disabling().await; self.set_state(BackupState::Unknown); Ok(()) diff --git a/crates/matrix-sdk/src/encryption/mod.rs b/crates/matrix-sdk/src/encryption/mod.rs index 318c455bafa..34d121077a0 100644 --- a/crates/matrix-sdk/src/encryption/mod.rs +++ b/crates/matrix-sdk/src/encryption/mod.rs @@ -135,6 +135,20 @@ impl EncryptionData { tasks.download_room_keys = Some(BackupDownloadTask::new(weak_client)); } } + + /// Initialize the background task which listens for changes in the + /// [`backups::BackupState`] and updataes the [`recovery::RecoveryState`]. + /// + /// This should happen after the usual tasks have been set up and after the + /// E2EE initialization tasks have been set up. + pub fn initialize_recovery_state_update_task(&self, client: &Client) { + let mut guard = self.tasks.lock().unwrap(); + + let future = Recovery::update_state_after_backup_state_change(client); + let join_handle = spawn(future); + + guard.update_recovery_state_after_backup = Some(join_handle); + } } /// Settings for end-to-end encryption features. diff --git a/crates/matrix-sdk/src/encryption/recovery/mod.rs b/crates/matrix-sdk/src/encryption/recovery/mod.rs index 152f4badeb6..3c37c495e41 100644 --- a/crates/matrix-sdk/src/encryption/recovery/mod.rs +++ b/crates/matrix-sdk/src/encryption/recovery/mod.rs @@ -90,7 +90,8 @@ //! //! [`Recovery key`]: https://spec.matrix.org/v1.8/client-server-api/#recovery-key -use futures_core::Stream; +use futures_core::{Future, Stream}; +use futures_util::StreamExt as _; use ruma::{ api::client::keys::get_keys, events::{ @@ -105,7 +106,7 @@ use crate::encryption::{ backups::Backups, secret_storage::{SecretStorage, SecretStore}, }; -use crate::Client; +use crate::{client::WeakClient, encryption::backups::BackupState, Client}; pub mod futures; mod types; @@ -437,6 +438,7 @@ impl Recovery { self.client.add_event_handler(Self::default_key_event_handler); self.client.add_event_handler(Self::secret_send_event_handler); + self.client.inner.e2ee.initialize_recovery_state_update_task(&self.client); self.update_recovery_state().await?; @@ -513,12 +515,44 @@ impl Recovery { client.encryption().recovery().update_recovery_state_no_fail().await; } - #[instrument] - pub(crate) async fn update_state_after_backup_disabling(&self) { - // TODO: This is quite ugly, this method is called by the backups subsystem. - // Backups shouldn't depend on recovery, recovery should listen to the - // backup state change. - self.update_recovery_state_no_fail().await; + /// Listen for changes in the [`BackupState`] and, if necessary, update the + /// [`RecoveryState`] accordingly. + /// + /// This should not be called directly, this method is put into a background + /// task which is always listening for updates in the [`BackupState`]. + pub(crate) fn update_state_after_backup_state_change( + client: &Client, + ) -> impl Future { + let mut stream = client.encryption().backups().state_stream(); + let weak = WeakClient::from_client(client); + + async move { + while let Some(update) = stream.next().await { + if let Some(client) = weak.get() { + match update { + Ok(update) => { + // The recovery state only cares about these two states, the + // intermediate states that tell us that + // we're creating a backup are not interesting. + if matches!(update, BackupState::Unknown | BackupState::Enabled) { + client + .encryption() + .recovery() + .update_recovery_state_no_fail() + .await; + } + } + Err(_) => { + // We missed some updates, let's update our state in case something + // changed. + client.encryption().recovery().update_recovery_state_no_fail().await; + } + } + } else { + break; + } + } + } } #[instrument] diff --git a/crates/matrix-sdk/src/encryption/tasks.rs b/crates/matrix-sdk/src/encryption/tasks.rs index f164aded730..d4cf23d0feb 100644 --- a/crates/matrix-sdk/src/encryption/tasks.rs +++ b/crates/matrix-sdk/src/encryption/tasks.rs @@ -43,6 +43,8 @@ pub(crate) struct ClientTasks { pub(crate) upload_room_keys: Option, #[cfg(feature = "e2e-encryption")] pub(crate) download_room_keys: Option, + #[cfg(feature = "e2e-encryption")] + pub(crate) update_recovery_state_after_backup: Option>, pub(crate) setup_e2ee: Option>, } diff --git a/crates/matrix-sdk/tests/integration/encryption/recovery.rs b/crates/matrix-sdk/tests/integration/encryption/recovery.rs index 9be27e6bf34..3d713865749 100644 --- a/crates/matrix-sdk/tests/integration/encryption/recovery.rs +++ b/crates/matrix-sdk/tests/integration/encryption/recovery.rs @@ -66,7 +66,7 @@ async fn test_client(user_id: &UserId) -> (Client, wiremock::MockServer) { "errcode": "M_NOT_FOUND", "error": "Account data not found" }))) - .expect(1) + .expect(2) .named("m.secret_storage.default_key account data GET") .mount_as_scoped(&server) .await; @@ -319,10 +319,13 @@ async fn enable( ))) .and(header("authorization", "Bearer 1234")) .respond_with(move |_: &wiremock::Request| { - let content = default_key_content.lock().unwrap().take().unwrap(); - ResponseTemplate::new(200).set_body_json(content) + if let Some(content) = default_key_content.lock().unwrap().as_ref() { + ResponseTemplate::new(200).set_body_json(content) + } else { + ResponseTemplate::new(404) + } }) - .expect(1) + .expect(2) .mount_as_scoped(server) .await; @@ -633,10 +636,13 @@ async fn recovery_disabling() { ))) .and(header("authorization", "Bearer 1234")) .respond_with(move |_: &wiremock::Request| { - let content = default_key_content.lock().unwrap().take().unwrap(); - ResponseTemplate::new(200).set_body_json(content) + if let Some(content) = default_key_content.lock().unwrap().as_ref() { + ResponseTemplate::new(200).set_body_json(content) + } else { + ResponseTemplate::new(404) + } }) - .expect(1) + .expect(2) .named("m.secret_storage.default_key account data GET") .mount(&server) .await;