From 8aae16ffd7c7a04b316c725f38ea54bb1009a287 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 29 Nov 2024 14:46:05 +0000 Subject: [PATCH] feat(crypto) Provide a method to check whether server backup exists without hitting the server every time --- crates/matrix-sdk/CHANGELOG.md | 6 + .../matrix-sdk/src/encryption/backups/mod.rs | 207 ++++++++++++++++-- .../src/encryption/backups/types.rs | 32 +++ crates/matrix-sdk/src/test_utils/mocks.rs | 50 ++++- 4 files changed, 276 insertions(+), 19 deletions(-) diff --git a/crates/matrix-sdk/CHANGELOG.md b/crates/matrix-sdk/CHANGELOG.md index d2922bc3d..87862d867 100644 --- a/crates/matrix-sdk/CHANGELOG.md +++ b/crates/matrix-sdk/CHANGELOG.md @@ -17,6 +17,7 @@ All notable changes to this project will be documented in this file. - Do not use the encrypted original file's content type as the encrypted thumbnail's content type. ([#ecf4434](https://github.com/matrix-org/matrix-rust-sdk/commit/ecf44348cf6a872b843fb7d7af1a88f724c58c3e)) + ### Features - Enable persistent storage for the `EventCache`. This allows events received @@ -28,6 +29,11 @@ All notable changes to this project will be documented in this file. - [**breaking**] Make all fields of Thumbnail required ([#4324](https://github.com/matrix-org/matrix-rust-sdk/pull/4324)) +- `Backups::exists_on_server`, which always fetches up-to-date information from the + server about whether a key storage backup exists, was renamed to + `fetch_exists_on_the_server`, and a new implementation of `exists_on_server` + which caches the most recent answer is now provided. + ## [0.8.0] - 2024-11-19 ### Bug Fixes diff --git a/crates/matrix-sdk/src/encryption/backups/mod.rs b/crates/matrix-sdk/src/encryption/backups/mod.rs index d0d0ad43e..23fa8cb5b 100644 --- a/crates/matrix-sdk/src/encryption/backups/mod.rs +++ b/crates/matrix-sdk/src/encryption/backups/mod.rs @@ -90,6 +90,7 @@ impl Backups { /// # anyhow::Ok(()) }; /// ``` pub async fn create(&self) -> Result<(), Error> { + self.client.inner.e2ee.backup_state.clear_backup_exists_on_server(); let _guard = self.client.locks().backup_modify_lock.lock().await; self.set_state(BackupState::Creating); @@ -387,7 +388,30 @@ impl Backups { /// This method will request info about the current backup from the /// homeserver and if a backup exits return `true`, otherwise `false`. pub async fn exists_on_server(&self) -> Result { - Ok(self.get_current_version().await?.is_some()) + let exists_on_server = self.get_current_version().await?.is_some(); + self.client.inner.e2ee.backup_state.set_backup_exists_on_server(exists_on_server); + Ok(exists_on_server) + } + + /// Does a backup exist on the server? + /// + /// This method is identical to [`Self::exists_on_server`] except that we + /// cache the latest answer in memory and only empty the cache if the local + /// device adds or deletes a backup itself. + /// + /// Do not use this method if you need an accurate answer about whether a + /// backup exists - instead use [`Self::exists_on_server`]. This method is + /// useful when performance is more important than guaranteed accuracy, + /// such as when classifying UTDs. + pub async fn fast_exists_on_server(&self) -> Result { + // If we have an answer cached, return it immediately + if let Some(cached_value) = self.client.inner.e2ee.backup_state.backup_exists_on_server() { + return Ok(cached_value); + } + + // Otherwise, delegate to exists_on_server. (It will update the cached value for + // us.) + self.exists_on_server().await } /// Subscribe to a stream that notifies when a room key for the specified @@ -621,7 +645,7 @@ impl Backups { async fn delete_backup_from_server(&self, version: String) -> Result<(), Error> { let request = ruma::api::client::backup::delete_backup_version::v3::Request::new(version); - match self.client.send(request, Default::default()).await { + let ret = match self.client.send(request, Default::default()).await { Ok(_) => Ok(()), Err(e) => { if let Some(kind) = e.client_api_error_kind() { @@ -634,7 +658,11 @@ impl Backups { Err(e.into()) } } - } + }; + + self.client.inner.e2ee.backup_state.clear_backup_exists_on_server(); + + ret } #[instrument(skip(self, olm_machine, request))] @@ -1135,6 +1163,23 @@ mod test { assert!(exists, "We should deduce that a backup exists on the server"); } + #[async_test] + async fn test_repeated_calls_to_exists_on_server_makes_repeated_requests() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + + // Expect 2 requests to the server + server.mock_room_keys_version().exists().expect(2).mount().await; + + let backups = client.encryption().backups(); + + // Call exists_on_server twice + backups.exists_on_server().await.unwrap(); + let exists = backups.exists_on_server().await.unwrap(); + + assert!(exists, "We should deduce that a backup exists on the server"); + } + #[async_test] async fn test_when_no_backup_exists_then_exists_on_server_returns_false() { let server = MatrixMockServer::new().await; @@ -1177,20 +1222,148 @@ mod test { } #[async_test] - async fn test_waiting_for_steady_state_resets_the_delay() { - let server = MockServer::start().await; - let client = logged_in_client(Some(server.uri())).await; + async fn test_when_a_backup_exists_then_fast_exists_on_server_returns_true() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; - Mock::given(method("POST")) - .and(path("_matrix/client/unstable/room_keys/version")) - .and(header("authorization", "Bearer 1234")) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "version": "1" - }))) - .expect(1) - .named("POST for the backup creation") - .mount(&server) - .await; + server.mock_room_keys_version().exists().expect(1).mount().await; + + let exists = client + .encryption() + .backups() + .fast_exists_on_server() + .await + .expect("We should be able to check if backups exist on the server"); + + assert!(exists, "We should deduce that a backup exists on the server"); + } + + #[async_test] + async fn test_when_no_backup_exists_then_fast_exists_on_server_returns_false() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + + server.mock_room_keys_version().none().expect(1).mount().await; + + let exists = client + .encryption() + .backups() + .fast_exists_on_server() + .await + .expect("We should be able to check if backups exist on the server"); + + assert!(!exists, "We should deduce that no backup exists on the server"); + } + + #[async_test] + async fn test_when_server_returns_an_error_then_fast_exists_on_server_returns_an_error() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + + { + let _scope = + server.mock_room_keys_version().error429().expect(1).mount_as_scoped().await; + + client.encryption().backups().fast_exists_on_server().await.expect_err( + "If the /version endpoint returns a non 404 error we should throw an error", + ); + } + + { + let _scope = + server.mock_room_keys_version().error404().expect(1).mount_as_scoped().await; + + client.encryption().backups().fast_exists_on_server().await.expect_err( + "If the /version endpoint returns a non-Matrix 404 error we should throw an error", + ); + } + } + + #[async_test] + async fn test_repeated_calls_to_fast_exists_on_server_do_not_make_additional_requests() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + + // Create a mock stating that the request should only be made once + server.mock_room_keys_version().exists().expect(1).mount().await; + + let backups = client.encryption().backups(); + + // Call fast_exists_on_server several times + backups.fast_exists_on_server().await.unwrap(); + backups.fast_exists_on_server().await.unwrap(); + backups.fast_exists_on_server().await.unwrap(); + + let exists = backups + .fast_exists_on_server() + .await + .expect("We should be able to check if backups exist on the server"); + + assert!(exists, "We should deduce that a backup exists on the server"); + + // We check expectations here, confirming that only one call was made + } + + #[async_test] + async fn test_adding_a_backup_invalidates_fast_exists_on_server_cache() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + let backups = client.encryption().backups(); + + { + let _scope = server.mock_room_keys_version().none().expect(1).mount_as_scoped().await; + + // Call fast_exists_on_server to fill the cache + let exists = backups.fast_exists_on_server().await.unwrap(); + assert!(!exists, "No backup exists at this point"); + } + + // Create a new backup. Should invalidate the cache + server.mock_add_room_keys_version().ok().expect(1).mount().await; + backups.create().await.expect("Failed to create a backup"); + + server.mock_room_keys_version().exists().expect(1).mount().await; + let exists = backups + .fast_exists_on_server() + .await + .expect("We should be able to check if backups exist on the server"); + + assert!(exists, "But now a backup does exist"); + } + + #[async_test] + async fn test_removing_a_backup_invalidates_fast_exists_on_server_cache() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + let backups = client.encryption().backups(); + + { + let _scope = server.mock_room_keys_version().exists().expect(1).mount_as_scoped().await; + + // Call fast_exists_on_server to fill the cache + let exists = backups.fast_exists_on_server().await.unwrap(); + assert!(exists, "A backup exists at this point"); + } + + // Delete the backup. Should invalidate the cache + server.mock_delete_room_keys_version().ok().expect(1).mount().await; + backups.delete_backup_from_server("1".to_owned()).await.expect("Failed to delete a backup"); + + server.mock_room_keys_version().none().expect(1).mount().await; + let exists = backups + .fast_exists_on_server() + .await + .expect("We should be able to check if backups exist on the server"); + + assert!(!exists, "But now there is no backup"); + } + + #[async_test] + async fn test_waiting_for_steady_state_resets_the_delay() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + + server.mock_add_room_keys_version().ok().expect(1).mount().await; client .encryption() @@ -1247,7 +1420,5 @@ mod test { { client.inner.e2ee.backup_state.upload_delay.read().unwrap().to_owned() }; assert_eq!(old_duration, current_duration); - - server.verify().await; } } diff --git a/crates/matrix-sdk/src/encryption/backups/types.rs b/crates/matrix-sdk/src/encryption/backups/types.rs index 8585078ad..9d6c089f5 100644 --- a/crates/matrix-sdk/src/encryption/backups/types.rs +++ b/crates/matrix-sdk/src/encryption/backups/types.rs @@ -53,6 +53,37 @@ pub(crate) struct BackupClientState { pub(crate) upload_progress: ChannelObservable, pub(super) global_state: ChannelObservable, pub(super) room_keys_broadcaster: broadcast::Sender, + + /// Whether a key storage backup exists on the server, as far as we know. + /// + /// This is `None` if we have not asked the server yet, and `Some` + /// otherwise. This value is not always up-to-date: if the backup status + /// on the server was changed by some other client, we will have a old + /// value. + pub(super) backup_exists_on_server: RwLock>, +} + +impl BackupClientState { + /// Update the cached value indicating whether a key storage backup exists + /// on the server + pub(crate) fn set_backup_exists_on_server(&self, exists_on_server: bool) { + *self.backup_exists_on_server.write().unwrap() = Some(exists_on_server); + } + + /// Ask whether the key storage backup exists on the server. Returns `None` + /// if we haven't checked. Note that this value will be out-of-date if + /// some other client changed the state since the last time we checked. + pub(crate) fn backup_exists_on_server(&self) -> Option { + *self.backup_exists_on_server.read().unwrap() + } + + /// Clear out the cached value indicating whether a key storage backup + /// exists on the server, meaning that the code in + /// [`super::Backups`] will repopulate it when needed + /// with an up-to-date value. + pub(crate) fn clear_backup_exists_on_server(&self) { + *self.backup_exists_on_server.write().unwrap() = None; + } } const DEFAULT_BACKUP_UPLOAD_DELAY: Duration = Duration::from_millis(100); @@ -64,6 +95,7 @@ impl Default for BackupClientState { upload_progress: ChannelObservable::new(UploadState::Idle), global_state: Default::default(), room_keys_broadcaster: broadcast::Sender::new(100), + backup_exists_on_server: RwLock::new(None), } } } diff --git a/crates/matrix-sdk/src/test_utils/mocks.rs b/crates/matrix-sdk/src/test_utils/mocks.rs index 9c583b95b..da51b8388 100644 --- a/crates/matrix-sdk/src/test_utils/mocks.rs +++ b/crates/matrix-sdk/src/test_utils/mocks.rs @@ -591,6 +591,22 @@ impl MatrixMockServer { .and(header("authorization", "Bearer 1234")); MockEndpoint { mock, server: &self.server, endpoint: RoomKeysVersionEndpoint } } + + /// Create a prebuilt mock for adding key storage backups via POST + pub fn mock_add_room_keys_version(&self) -> MockEndpoint<'_, AddRoomKeysVersionEndpoint> { + let mock = Mock::given(method("POST")) + .and(path_regex(r"_matrix/client/v3/room_keys/version")) + .and(header("authorization", "Bearer 1234")); + MockEndpoint { mock, server: &self.server, endpoint: AddRoomKeysVersionEndpoint } + } + + /// Create a prebuilt mock for adding key storage backups via POST + pub fn mock_delete_room_keys_version(&self) -> MockEndpoint<'_, DeleteRoomKeysVersionEndpoint> { + let mock = Mock::given(method("DELETE")) + .and(path_regex(r"_matrix/client/v3/room_keys/version/[^/]*")) + .and(header("authorization", "Bearer 1234")); + MockEndpoint { mock, server: &self.server, endpoint: DeleteRoomKeysVersionEndpoint } + } } /// Parameter to [`MatrixMockServer::sync_room`]. @@ -1669,7 +1685,8 @@ impl<'a> MockEndpoint<'a, PublicRoomsEndpoint> { } } -/// A prebuilt mock for `room_keys/version`: storage ("backup") of room keys. +/// A prebuilt mock for `GET room_keys/version`: storage ("backup") of room +/// keys. pub struct RoomKeysVersionEndpoint; impl<'a> MockEndpoint<'a, RoomKeysVersionEndpoint> { @@ -1713,3 +1730,34 @@ impl<'a> MockEndpoint<'a, RoomKeysVersionEndpoint> { MatrixMock { server: self.server, mock } } } + +/// A prebuilt mock for `POST room_keys/version`: adding room key backups. +pub struct AddRoomKeysVersionEndpoint; + +impl<'a> MockEndpoint<'a, AddRoomKeysVersionEndpoint> { + /// Returns an endpoint that may be used to add room key backups + pub fn ok(self) -> MatrixMock<'a> { + let mock = self + .mock + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "version": "1" + }))) + .named("POST for the backup creation"); + MatrixMock { server: self.server, mock } + } +} + +/// A prebuilt mock for `DELETE room_keys/version/xxx`: deleting room key +/// backups. +pub struct DeleteRoomKeysVersionEndpoint; + +impl<'a> MockEndpoint<'a, DeleteRoomKeysVersionEndpoint> { + /// Returns an endpoint that allows deleting room key backups + pub fn ok(self) -> MatrixMock<'a> { + let mock = self + .mock + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .named("DELETE for the backup deletion"); + MatrixMock { server: self.server, mock } + } +}