crypto: Add a backup_version argument to group session backup methods

Author: andybalaam

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -17,6 +17,10 @@ Breaking changes:
 - Add new `dehydrated` property to `olm::account::PickledAccount`.
   ([#3164](https://github.com/matrix-org/matrix-rust-sdk/pull/3164))
 
+- Add a `backup_version` argument to `CryptoStore`'s
+  `inbound_group_sessions_for_backup` and
+  `mark_inbound_group_sessions_as_backed_up` methods.
+
 Additions:
 
 - Log more details about the Olm session after encryption and decryption.

crates/matrix-sdk-crypto/src/backups/mod.rs

@@ -476,7 +476,12 @@ impl BackupMachine {
 
                 trace!(request_id = ?r.request_id, keys = ?r.sessions, "Marking room keys as backed up");
 
-                self.store.mark_inbound_group_sessions_as_backed_up(&room_and_session_ids).await?;
+                self.store
+                    .mark_inbound_group_sessions_as_backed_up(
+                        &r.request.version,
+                        &room_and_session_ids,
+                    )
+                    .await?;
 
                 trace!(
                     request_id = ?r.request_id,
@@ -514,7 +519,7 @@ impl BackupMachine {
         };
 
         let sessions =
-            self.store.inbound_group_sessions_for_backup(Self::BACKUP_BATCH_SIZE).await?;
+            self.store.inbound_group_sessions_for_backup(&version, Self::BACKUP_BATCH_SIZE).await?;
 
         if sessions.is_empty() {
             trace!(?backup_key, "No room keys need to be backed up");

crates/matrix-sdk-crypto/src/store/integration_tests.rs

@@ -326,7 +326,7 @@ macro_rules! cryptostore_integration_tests {
                 assert_eq!(store.inbound_group_session_counts().await.unwrap().total, 1);
                 assert_eq!(store.inbound_group_session_counts().await.unwrap().backed_up, 0);
 
-                let to_back_up = store.inbound_group_sessions_for_backup(1).await.unwrap();
+                let to_back_up = store.inbound_group_sessions_for_backup("bkpver", 1).await.unwrap();
                 assert_eq!(to_back_up, vec![session])
             }
 
@@ -342,14 +342,10 @@ macro_rules! cryptostore_integration_tests {
                 }
                 let changes = Changes { inbound_group_sessions: sessions.clone(), ..Default::default() };
                 store.save_changes(changes).await.expect("Can't save group session");
-                assert_eq!(store.inbound_group_sessions_for_backup(100).await.unwrap().len(), 10);
-
-                fn session_info(session: &InboundGroupSession) -> (&RoomId, &str) {
-                    (&session.room_id(), &session.session_id())
-                }
+                assert_eq!(store.inbound_group_sessions_for_backup("bkpver", 100).await.unwrap().len(), 10);
 
                 // When I mark some as backed up
-                store.mark_inbound_group_sessions_as_backed_up(&[
+                store.mark_inbound_group_sessions_as_backed_up("bkpver", &[
                     session_info(&sessions[1]),
                     session_info(&sessions[3]),
                     session_info(&sessions[5]),
@@ -358,7 +354,7 @@ macro_rules! cryptostore_integration_tests {
                 ]).await.expect("Failed to mark sessions as backed up");
 
                 // And ask which still need backing up
-                let to_back_up = store.inbound_group_sessions_for_backup(10).await.unwrap();
+                let to_back_up = store.inbound_group_sessions_for_backup("bkpver", 10).await.unwrap();
                 let needs_backing_up = |i: usize| to_back_up.iter().any(|s| s.session_id() == sessions[i].session_id());
 
                 // Then the sessions we said were backed up no longer need backing up
@@ -386,22 +382,30 @@ macro_rules! cryptostore_integration_tests {
                 let room_id = &room_id!("!test:localhost");
                 let (_, session) = account.create_group_session_pair_with_defaults(room_id).await;
 
-                session.mark_as_backed_up();
-
                 let changes =
                     Changes { inbound_group_sessions: vec![session.clone()], ..Default::default() };
 
                 store.save_changes(changes).await.expect("Can't save group session");
 
+                // Given we have backed up our session
+                store
+                    .mark_inbound_group_sessions_as_backed_up("bkpver1", &[session_info(&session)])
+                    .await
+                    .expect("Failed to mark_inbound_group_sessions_as_backed_up.");
+
                 assert_eq!(store.inbound_group_session_counts().await.unwrap().total, 1);
                 assert_eq!(store.inbound_group_session_counts().await.unwrap().backed_up, 1);
 
-                let to_back_up = store.inbound_group_sessions_for_backup(1).await.unwrap();
+                // Sanity: before resetting, we have nothing to back up
+                let to_back_up = store.inbound_group_sessions_for_backup("bkpver1", 1).await.unwrap();
                 assert_eq!(to_back_up, vec![]);
 
+                // When we reset the backup
                 store.reset_backup_state().await.unwrap();
 
-                let to_back_up = store.inbound_group_sessions_for_backup(1).await.unwrap();
+                // Then after resetting, even if we supply the same backup version number, we need
+                // to back up the session
+                let to_back_up = store.inbound_group_sessions_for_backup("bkpver1", 1).await.unwrap();
                 assert_eq!(to_back_up, vec![session]);
             }
 
@@ -960,6 +964,10 @@ macro_rules! cryptostore_integration_tests {
                 let loaded_2 = store.get_custom_value("B").await.unwrap();
                 assert_eq!(None, loaded_2);
             }
+
+            fn session_info(session: &InboundGroupSession) -> (&RoomId, &str) {
+                (&session.room_id(), &session.session_id())
+            }
         }
     };
 }

crates/matrix-sdk-crypto/src/store/memorystore.rs

@@ -279,6 +279,7 @@ impl CryptoStore for MemoryStore {
 
     async fn inbound_group_sessions_for_backup(
         &self,
+        _backup_version: &str,
         limit: usize,
     ) -> Result<Vec<InboundGroupSession>> {
         Ok(self
@@ -292,6 +293,7 @@ impl CryptoStore for MemoryStore {
 
     async fn mark_inbound_group_sessions_as_backed_up(
         &self,
+        _backup_version: &str,
         room_and_session_ids: &[(&RoomId, &str)],
     ) -> Result<()> {
         for (room_id, session_id) in room_and_session_ids {
@@ -757,16 +759,20 @@ mod integration_tests {
 
         async fn inbound_group_sessions_for_backup(
             &self,
+            backup_version: &str,
             limit: usize,
         ) -> Result<Vec<InboundGroupSession>, Self::Error> {
-            self.0.inbound_group_sessions_for_backup(limit).await
+            self.0.inbound_group_sessions_for_backup(backup_version, limit).await
         }
 
         async fn mark_inbound_group_sessions_as_backed_up(
             &self,
+            backup_version: &str,
             room_and_session_ids: &[(&RoomId, &str)],
         ) -> Result<(), Self::Error> {
-            self.0.mark_inbound_group_sessions_as_backed_up(room_and_session_ids).await
+            self.0
+                .mark_inbound_group_sessions_as_backed_up(backup_version, room_and_session_ids)
+                .await
         }
 
         async fn reset_backup_state(&self) -> Result<(), Self::Error> {

crates/matrix-sdk-crypto/src/store/traits.rs

@@ -107,20 +107,38 @@ pub trait CryptoStore: AsyncTraitDeps {
     /// backed up.
     async fn inbound_group_session_counts(&self) -> Result<RoomKeyCounts, Self::Error>;
 
-    /// Get all the inbound group sessions we have not backed up yet.
+    /// Return a batch of ['InboundGroupSession'] ("room keys") that have not
+    /// yet been backed up in the supplied backup version.
+    ///
+    /// The size of the returned `Vec` is <= `limit`.
+    ///
+    /// Note: some implementations ignore `backup_version` and assume the
+    /// current backup version, which is normally the same.
     async fn inbound_group_sessions_for_backup(
         &self,
+        backup_version: &str,
         limit: usize,
     ) -> Result<Vec<InboundGroupSession>, Self::Error>;
 
-    /// Mark the inbound group sessions with the supplied room and session IDs
-    /// as backed up
+    /// Store the fact that the supplied sessions were backed up into the backup
+    /// with version `backup_version`.
+    ///
+    /// Note: some implementations ignore `backup_version` and assume the
+    /// current backup version, which is normally the same.
     async fn mark_inbound_group_sessions_as_backed_up(
         &self,
+        backup_version: &str,
         room_and_session_ids: &[(&RoomId, &str)],
     ) -> Result<(), Self::Error>;
 
     /// Reset the backup state of all the stored inbound group sessions.
+    ///
+    /// Note: this is mostly implemented by stores that ignore the
+    /// `backup_version` argument on `inbound_group_sessions_for_backup` and
+    /// `mark_inbound_group_sessions_as_backed_up`. Implementations that
+    /// pay attention to the supplied backup version probably don't need to
+    /// update their storage when the current backup version changes, so have
+    /// empty implementations of this method.
     async fn reset_backup_state(&self) -> Result<(), Self::Error>;
 
     /// Get the backup keys we have stored.
@@ -331,20 +349,21 @@ impl<T: CryptoStore> CryptoStore for EraseCryptoStoreError<T> {
     async fn inbound_group_session_counts(&self) -> Result<RoomKeyCounts> {
         self.0.inbound_group_session_counts().await.map_err(Into::into)
     }
-
     async fn inbound_group_sessions_for_backup(
         &self,
+        backup_version: &str,
         limit: usize,
     ) -> Result<Vec<InboundGroupSession>> {
-        self.0.inbound_group_sessions_for_backup(limit).await.map_err(Into::into)
+        self.0.inbound_group_sessions_for_backup(backup_version, limit).await.map_err(Into::into)
     }
 
     async fn mark_inbound_group_sessions_as_backed_up(
         &self,
+        backup_version: &str,
         room_and_session_ids: &[(&RoomId, &str)],
     ) -> Result<()> {
         self.0
-            .mark_inbound_group_sessions_as_backed_up(room_and_session_ids)
+            .mark_inbound_group_sessions_as_backed_up(backup_version, room_and_session_ids)
             .await
             .map_err(Into::into)
     }

crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs

@@ -889,6 +889,7 @@ impl_crypto_store! {
 
     async fn inbound_group_sessions_for_backup(
         &self,
+        _backup_version: &str,
         limit: usize,
     ) -> Result<Vec<InboundGroupSession>> {
         let tx = self
@@ -933,7 +934,10 @@ impl_crypto_store! {
         Ok(result)
     }
 
-    async fn mark_inbound_group_sessions_as_backed_up(&self, room_and_session_ids: &[(&RoomId, &str)]) -> Result<()> {
+    async fn mark_inbound_group_sessions_as_backed_up(&self,
+        _backup_version: &str,
+        room_and_session_ids: &[(&RoomId, &str)]
+    ) -> Result<()> {
         let tx = self
             .inner
             .transaction_on_one_with_mode(

crates/matrix-sdk-sqlite/src/crypto_store.rs

@@ -946,6 +946,7 @@ impl CryptoStore for SqliteCryptoStore {
 
     async fn inbound_group_sessions_for_backup(
         &self,
+        _backup_version: &str,
         limit: usize,
     ) -> Result<Vec<InboundGroupSession>> {
         self.acquire()
@@ -962,6 +963,7 @@ impl CryptoStore for SqliteCryptoStore {
 
     async fn mark_inbound_group_sessions_as_backed_up(
         &self,
+        _backup_version: &str,
         session_ids: &[(&RoomId, &str)],
     ) -> Result<()> {
         Ok(self