From f94d9d8e3ec1303994670b7b7afced6eac3ed37b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 7 Aug 2024 13:47:10 +0100 Subject: [PATCH 1/5] indexeddb: Add missing `do_schema_upgrade` call from v11 migration We weren't updating the database schema version immediately after the v10 -> v11 migration. This was fine in practice, because (a) for now, there is no v12 migration so we ended up setting the schema version immediately anyway; (b) the migration is idempotent. However, it's inconsistent with the other migrations and confusing, and is about to make my test fail, so let's clean it up. --- .../src/crypto_store/migrations/mod.rs | 1 + .../src/crypto_store/migrations/v10_to_v11.rs | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs index e87e5db07e5..23ccda98202 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs @@ -109,6 +109,7 @@ pub async fn open_and_upgrade_db( if old_version < 11 { v10_to_v11::data_migrate(name, serializer).await?; + v10_to_v11::schema_bump(name).await?; } // Open and return the DB (we know it's at the latest version) diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs index 7bb94ffd349..042b221384f 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs @@ -17,12 +17,12 @@ use indexed_db_futures::IdbQuerySource; use wasm_bindgen::JsValue; -use web_sys::IdbTransactionMode; +use web_sys::{DomException, IdbTransactionMode}; use crate::crypto_store::{ indexeddb_serializer::IndexeddbSerializer, keys, - migrations::{old_keys, MigrationDb}, + migrations::{do_schema_upgrade, old_keys, MigrationDb}, }; /// Migrate data from `backup_keys.backup_key_v1` to @@ -52,3 +52,10 @@ pub(crate) async fn data_migrate( store.delete(&JsValue::from_str(old_keys::BACKUP_KEY_V1))?.await?; Ok(()) } + +/// Perform the schema upgrade v10 to v11, just bumping the schema version. +pub(crate) async fn schema_bump(name: &str) -> crate::crypto_store::Result<(), DomException> { + // Just bump the version number to 11 to demonstrate that we have run the data + // changes from data_migrate. + do_schema_upgrade(name, 11, |_, _| Ok(())).await +} From e8a1561a9df446cce4b6edf0c7aa93b750d9da56 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 7 Aug 2024 13:23:59 +0100 Subject: [PATCH 2/5] indexeddb: Future-proofing: accept any db schema version up to 19 ... so that next time we make a non-breaking change to the schema, it doesn't break rollback --- .../src/crypto_store/migrations/mod.rs | 133 +++++++++++++++++- .../src/crypto_store/mod.rs | 2 + 2 files changed, 129 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs index 23ccda98202..aeaa53903b6 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs @@ -62,6 +62,40 @@ impl Drop for MigrationDb { } } +/// The latest version of the schema we can support. If we encounter a database +/// version with a higher schema version, we will return an error. +/// +/// A note on how this works. +/// +/// Normally, when you open an indexeddb database, you tell it the "schema +/// version" that you know about. If the existing database is older than +/// that, it lets you run a migration. If the existing database is newer, then +/// it assumes that there have been incompatible schema changes and complains +/// with an error ("The requested version (10) is less than the existing version +/// (11)"). +/// +/// The problem with this is that, if someone upgrades their installed +/// application, then realises it was a terrible mistake and tries to roll +/// back, then suddenly every user's session is completely hosed. (They see +/// an "unable to restore session" dialog.) Often, schema updates aren't +/// actually backwards-incompatible — for example, existing code will work just +/// fine if someone adds a new store or a new index — so this approach is too +/// heavy-handed. +/// +/// The solution we take here is to say "any schema changes up to +/// [`MAX_SUPPORTED_SCHEMA_VERSION`] will be backwards-compatible". If, at some +/// point, we do make a breaking change, we will give that schema version a +/// higher number. Then, rather than using the implicit version check that comes +/// with `indexedDB.open(name, version)`, we explicitly check the version +/// ourselves. +/// +/// It is expected that we will use version numbers that are multiples of 10 to +/// represent breaking changes — for example, version 20 is a breaking change, +/// as is version 30, but versions 21-29 are all backwards compatible with +/// version 20. In other words, if you divide by 10, you get something +/// approaching semver: version 20 is major version 2, minor version 0. +const MAX_SUPPORTED_SCHEMA_VERSION: u32 = 19; + /// Open the indexeddb with the given name, upgrading it to the latest version /// of the schema if necessary. pub async fn open_and_upgrade_db( @@ -82,6 +116,16 @@ pub async fn open_and_upgrade_db( let old_version = db_version(name).await?; + // If the database version is too new, bail out. We assume that schema updates + // all the way up to `MAX_SUPPORTED_SCHEMA_VERSION` will be + // backwards-compatible. + if old_version > MAX_SUPPORTED_SCHEMA_VERSION { + return Err(IndexeddbCryptoStoreError::SchemaTooNewError { + max_supported_version: MAX_SUPPORTED_SCHEMA_VERSION, + current_version: old_version, + }); + } + if old_version < 5 { v0_to_v5::schema_add(name).await?; } @@ -112,8 +156,15 @@ pub async fn open_and_upgrade_db( v10_to_v11::schema_bump(name).await?; } + // If you add more migrations here, you'll need to update + // `tests::EXPECTED_SCHEMA_VERSION`. + + // NOTE: IF YOU MAKE A BREAKING CHANGE TO THE SCHEMA, BUMP THE SCHEMA VERSION TO + // SOMETHING HIGHER THAN `MAX_SUPPORTED_SCHEMA_VERSION`! (And then bump + // `MAX_SUPPORTED_SCHEMA_VERSION` itself to the next multiple of 10). + // Open and return the DB (we know it's at the latest version) - Ok(IdbDatabase::open_u32(name, 11)?.await?) + Ok(IdbDatabase::open(name)?.await?) } async fn db_version(name: &str) -> Result { @@ -170,8 +221,9 @@ fn add_unique_index<'a>( #[cfg(all(test, target_arch = "wasm32"))] mod tests { - use std::{future::Future, sync::Arc}; + use std::{cell::Cell, future::Future, rc::Rc, sync::Arc}; + use assert_matches::assert_matches; use gloo_utils::format::JsValueSerdeExt; use indexed_db_futures::prelude::*; use matrix_sdk_common::js_tracing::make_tracing_subscriber; @@ -190,15 +242,15 @@ mod tests { use super::{v0_to_v5, v7::InboundGroupSessionIndexedDbObject2}; use crate::{ - crypto_store::{ - indexeddb_serializer::MaybeEncrypted, keys, migrations::*, - InboundGroupSessionIndexedDbObject, - }, + crypto_store::{keys, migrations::*, InboundGroupSessionIndexedDbObject}, IndexeddbCryptoStore, }; wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); + /// The schema version we expect after we open the store. + const EXPECTED_SCHEMA_VERSION: u32 = 11; + /// Adjust this to test do a more comprehensive perf test const NUM_RECORDS_FOR_PERF: usize = 2_000; @@ -638,6 +690,75 @@ mod tests { IdbDatabase::open_u32(name, 5)?.await } + /// Opening a db that has been upgraded to MAX_SUPPORTED_SCHEMA_VERSION + /// should be ok + #[async_test] + async fn can_open_max_supported_schema_version() { + let _ = make_tracing_subscriber(None).try_init(); + + let db_prefix = "test_can_open_max_supported_schema_version"; + // Create a database at MAX_SUPPORTED_SCHEMA_VERSION + create_future_schema_db(db_prefix, MAX_SUPPORTED_SCHEMA_VERSION).await; + + // Now, try opening it again + IndexeddbCryptoStore::open_with_store_cipher(&db_prefix, None).await.unwrap(); + } + + /// Opening a db that has been upgraded beyond MAX_SUPPORTED_SCHEMA_VERSION + /// should throw an error + #[async_test] + async fn can_not_open_too_new_db() { + let _ = make_tracing_subscriber(None).try_init(); + + let db_prefix = "test_can_not_open_too_new_db"; + // Create a database at MAX_SUPPORTED_SCHEMA_VERSION+1 + create_future_schema_db(db_prefix, MAX_SUPPORTED_SCHEMA_VERSION + 1).await; + + // Now, try opening it again + let result = IndexeddbCryptoStore::open_with_store_cipher(&db_prefix, None).await; + assert_matches!( + result, + Err(IndexeddbCryptoStoreError::SchemaTooNewError { + max_supported_version, + current_version + }) => { + assert_eq!(max_supported_version, MAX_SUPPORTED_SCHEMA_VERSION); + assert_eq!(current_version, MAX_SUPPORTED_SCHEMA_VERSION + 1); + } + ); + } + + // Create a database, and increase its schema version to the given version + // number. + async fn create_future_schema_db(db_prefix: &str, version: u32) { + let db_name = format!("{db_prefix}::matrix-sdk-crypto"); + + // delete the db in case it was used in a previous run + let _ = IdbDatabase::delete_by_name(&db_name); + + // Open, and close, the store at the regular version. + IndexeddbCryptoStore::open_with_store_cipher(&db_prefix, None).await.unwrap(); + + // Now upgrade to the given version, keeping a record of the previous version so + // that we can double-check it. + let mut db_req: OpenDbRequest = IdbDatabase::open_u32(&db_name, version).unwrap(); + + let old_version: Rc>> = Rc::new(Cell::new(None)); + let old_version2 = old_version.clone(); + db_req.set_on_upgrade_needed(Some(move |evt: &IdbVersionChangeEvent| { + old_version2.set(Some(evt.old_version() as u32)); + Ok(()) + })); + + let db = db_req.await.unwrap(); + assert_eq!( + old_version.get(), + Some(EXPECTED_SCHEMA_VERSION), + "Existing store had unexpected version number" + ); + db.close(); + } + /// Emulate the old behaviour of [`IndexeddbSerializer::serialize_value`]. /// /// We used to use an inefficient format for serializing objects in the diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs index c0cd6b28be6..c041c63a0fd 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs @@ -138,6 +138,8 @@ pub enum IndexeddbCryptoStoreError { }, #[error(transparent)] CryptoStoreError(#[from] CryptoStoreError), + #[error("The schema version of the crypto store is too new. Existing version: {current_version}; max supported version: {max_supported_version}")] + SchemaTooNewError { max_supported_version: u32, current_version: u32 }, } impl From for IndexeddbCryptoStoreError { From 147354d3128a8279dd4cdcba6419c767466513a4 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 2 Aug 2024 11:44:05 +0100 Subject: [PATCH 3/5] crypto: Pass the db upgrade transaction into do_schema_upgrade --- .../matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs | 4 ++-- .../src/crypto_store/migrations/v0_to_v5.rs | 2 +- .../src/crypto_store/migrations/v10_to_v11.rs | 2 +- .../src/crypto_store/migrations/v5_to_v7.rs | 4 ++-- .../src/crypto_store/migrations/v7_to_v8.rs | 2 +- .../src/crypto_store/migrations/v8_to_v10.rs | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs index aeaa53903b6..79be30b5152 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs @@ -178,7 +178,7 @@ type OldVersion = u32; async fn do_schema_upgrade(name: &str, version: u32, f: F) -> Result<(), DomException> where - F: Fn(&IdbDatabase, OldVersion) -> Result<(), JsValue> + 'static, + F: Fn(&IdbDatabase, IdbTransaction<'_>, OldVersion) -> Result<(), JsValue> + 'static, { info!("IndexeddbCryptoStore upgrade schema -> v{version} starting"); let mut db_req: OpenDbRequest = IdbDatabase::open_u32(name, version)?; @@ -190,7 +190,7 @@ where let old_version = evt.old_version() as u32; // Run the upgrade code we were supplied - f(evt.db(), old_version) + f(evt.db(), evt.transaction(), old_version) })); let db = db_req.await?; diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v0_to_v5.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v0_to_v5.rs index 0800b7e14ea..75b1dc69865 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v0_to_v5.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v0_to_v5.rs @@ -26,7 +26,7 @@ use crate::crypto_store::{ /// Perform schema migrations as needed, up to schema version 5. pub(crate) async fn schema_add(name: &str) -> Result<(), DomException> { - do_schema_upgrade(name, 5, |db, old_version| { + do_schema_upgrade(name, 5, |db, _, old_version| { // An old_version of 1 could either mean actually the first version of the // schema, or a completely empty schema that has been created with a // call to `IdbDatabase::open` with no explicit "version". So, to determine diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs index 042b221384f..11de8b32a90 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v10_to_v11.rs @@ -57,5 +57,5 @@ pub(crate) async fn data_migrate( pub(crate) async fn schema_bump(name: &str) -> crate::crypto_store::Result<(), DomException> { // Just bump the version number to 11 to demonstrate that we have run the data // changes from data_migrate. - do_schema_upgrade(name, 11, |_, _| Ok(())).await + do_schema_upgrade(name, 11, |_, _, _| Ok(())).await } diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v5_to_v7.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v5_to_v7.rs index ea2ffc7e7ec..473960b3ff4 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v5_to_v7.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v5_to_v7.rs @@ -36,7 +36,7 @@ use crate::{ /// Perform the schema upgrade v5 to v6, creating `inbound_group_sessions2`. pub(crate) async fn schema_add(name: &str) -> Result<(), DomException> { - do_schema_upgrade(name, 6, |db, _| { + do_schema_upgrade(name, 6, |db, _, _| { let object_store = db.create_object_store(old_keys::INBOUND_GROUP_SESSIONS_V2)?; add_nonunique_index( @@ -109,7 +109,7 @@ pub(crate) async fn data_migrate(name: &str, serializer: &IndexeddbSerializer) - /// Perform the schema upgrade v6 to v7, deleting `inbound_group_sessions`. pub(crate) async fn schema_delete(name: &str) -> Result<(), DomException> { - do_schema_upgrade(name, 7, |db, _| { + do_schema_upgrade(name, 7, |db, _, _| { db.delete_object_store(old_keys::INBOUND_GROUP_SESSIONS_V1)?; Ok(()) }) diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v7_to_v8.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v7_to_v8.rs index efa43c347aa..9cba57a7b1f 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v7_to_v8.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v7_to_v8.rs @@ -113,7 +113,7 @@ pub(crate) async fn data_migrate(name: &str, serializer: &IndexeddbSerializer) - /// Perform the schema upgrade v7 to v8, Just bumping the schema version. pub(crate) async fn schema_bump(name: &str) -> Result<(), DomException> { - do_schema_upgrade(name, 8, |_, _| { + do_schema_upgrade(name, 8, |_, _, _| { // Just bump the version number to 8 to demonstrate that we have run the data // changes from prepare_data_for_v8. Ok(()) diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs index 1b5c06f96ce..c6e805c9b78 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs @@ -35,7 +35,7 @@ use crate::{ /// Perform the schema upgrade v8 to v9, creating `inbound_group_sessions3`. pub(crate) async fn schema_add(name: &str) -> Result<(), DomException> { - do_schema_upgrade(name, 9, |db, _| { + do_schema_upgrade(name, 9, |db, _, _| { let object_store = db.create_object_store(keys::INBOUND_GROUP_SESSIONS_V3)?; add_nonunique_index( @@ -131,7 +131,7 @@ pub(crate) async fn data_migrate(name: &str, serializer: &IndexeddbSerializer) - /// Perform the schema upgrade v8 to v10, deleting `inbound_group_sessions2`. pub(crate) async fn schema_delete(name: &str) -> Result<(), DomException> { - do_schema_upgrade(name, 10, |db, _| { + do_schema_upgrade(name, 10, |db, _, _| { db.delete_object_store(old_keys::INBOUND_GROUP_SESSIONS_V2)?; Ok(()) }) From 3d9502c16afd69c64d6d124b0243d385583bec73 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 2 Aug 2024 13:11:37 +0100 Subject: [PATCH 4/5] crypto: Allow returning the type of the SenderData of an InboundGroupSession --- .../src/olm/group_sessions/inbound.rs | 11 +++++++++-- .../src/olm/group_sessions/sender_data.rs | 19 +++++++++---------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index c46747a2c31..51b774fe28c 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -36,8 +36,8 @@ use vodozemac::{ }; use super::{ - BackedUpRoomKey, ExportedRoomKey, OutboundGroupSession, SenderData, SessionCreationError, - SessionKey, + BackedUpRoomKey, ExportedRoomKey, OutboundGroupSession, SenderData, SenderDataType, + SessionCreationError, SessionKey, }; use crate::{ error::{EventError, MegolmResult}, @@ -477,6 +477,13 @@ impl InboundGroupSession { pub(crate) fn mark_as_imported(&mut self) { self.imported = true; } + + /// Return the [`SenderDataType`] of our [`SenderData`]. This is used during + /// serialization, to allow us to store the type in a separate queryable + /// column/property. + pub fn sender_data_type(&self) -> SenderDataType { + self.sender_data.to_type() + } } #[cfg(not(tarpaulin_include))] diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs index 14d8b95fb6c..f86495d185d 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data.rs @@ -97,6 +97,15 @@ impl SenderData { pub fn legacy() -> Self { Self::UnknownDevice { legacy_session: true, owner_check_failed: false } } + + /// Return our type: `UnknownDevice`, `DeviceInfo`, or `SenderKnown`. + pub fn to_type(&self) -> SenderDataType { + match self { + Self::UnknownDevice { .. } => SenderDataType::UnknownDevice, + Self::DeviceInfo { .. } => SenderDataType::DeviceInfo, + Self::SenderKnown { .. } => SenderDataType::SenderKnown, + } + } } /// Used when deserialising and the sender_data property is missing. @@ -124,16 +133,6 @@ pub enum SenderDataType { SenderKnown = 3, } -impl From for SenderDataType { - fn from(value: SenderData) -> Self { - match value { - SenderData::UnknownDevice { .. } => Self::UnknownDevice, - SenderData::DeviceInfo { .. } => Self::DeviceInfo, - SenderData::SenderKnown { .. } => Self::SenderKnown, - } - } -} - #[cfg(test)] mod tests { use assert_matches2::assert_let; From b08403fb0531107e581b5f3ce469c3ab0fca52d5 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 2 Aug 2024 13:34:54 +0100 Subject: [PATCH 5/5] crypto: Add curve_key and sender_data_type columns to inboundgroupsessions table/store --- .../src/crypto_store/migrations/mod.rs | 66 ++++++++++--- .../src/crypto_store/migrations/v11_to_v12.rs | 37 ++++++++ .../src/crypto_store/migrations/v8_to_v10.rs | 2 + .../src/crypto_store/mod.rs | 95 ++++++++++++++++++- ...oup_session_curve_key_sender_data_type.sql | 8 ++ crates/matrix-sdk-sqlite/src/crypto_store.rs | 23 ++++- 6 files changed, 210 insertions(+), 21 deletions(-) create mode 100644 crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v11_to_v12.rs create mode 100644 crates/matrix-sdk-sqlite/migrations/crypto_store/009_inbound_group_session_curve_key_sender_data_type.sql diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs index 79be30b5152..e8ef42c26f8 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs @@ -26,6 +26,7 @@ use crate::{ mod old_keys; mod v0_to_v5; mod v10_to_v11; +mod v11_to_v12; mod v5_to_v7; mod v7; mod v7_to_v8; @@ -156,6 +157,10 @@ pub async fn open_and_upgrade_db( v10_to_v11::schema_bump(name).await?; } + if old_version < 12 { + v11_to_v12::schema_add(name).await?; + } + // If you add more migrations here, you'll need to update // `tests::EXPECTED_SCHEMA_VERSION`. @@ -249,7 +254,7 @@ mod tests { wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); /// The schema version we expect after we open the store. - const EXPECTED_SCHEMA_VERSION: u32 = 11; + const EXPECTED_SCHEMA_VERSION: u32 = 12; /// Adjust this to test do a more comprehensive perf test const NUM_RECORDS_FOR_PERF: usize = 2_000; @@ -401,6 +406,8 @@ mod tests { pickled_session: serializer.maybe_encrypt_value(pickled_session).unwrap(), needs_backup: false, backed_up_to: -1, + curve_key: None, + sender_data_type: None, }; let session_js: JsValue = serde_wasm_bindgen::to_value(&session_dbo).unwrap(); @@ -466,22 +473,25 @@ mod tests { /// Test migrating `inbound_group_sessions` data from store v5 to latest, /// on a store with encryption disabled. #[async_test] - async fn test_v8_v10_migration_unencrypted() { - test_v8_v10_migration_with_cipher("test_v8_migration_unencrypted", None).await + async fn test_v8_v10_v12_migration_unencrypted() { + test_v8_v10_v12_migration_with_cipher("test_v8_migration_unencrypted", None).await } /// Test migrating `inbound_group_sessions` data from store v5 to store v8, /// on a store with encryption enabled. #[async_test] - async fn test_v8_v10_migration_encrypted() { + async fn test_v8_v10_v12_migration_encrypted() { let cipher = StoreCipher::new().unwrap(); - test_v8_v10_migration_with_cipher("test_v8_migration_encrypted", Some(Arc::new(cipher))) - .await; + test_v8_v10_v12_migration_with_cipher( + "test_v8_migration_encrypted", + Some(Arc::new(cipher)), + ) + .await; } - /// Helper function for `test_v8_v10_migration_{un,}encrypted`: test - /// migrating `inbound_group_sessions` data from store v5 to store v10. - async fn test_v8_v10_migration_with_cipher( + /// Helper function for `test_v8_v10_v12_migration_{un,}encrypted`: test + /// migrating `inbound_group_sessions` data from store v5 to store v12. + async fn test_v8_v10_v12_migration_with_cipher( db_prefix: &str, store_cipher: Option>, ) { @@ -525,13 +535,17 @@ mod tests { assert!(!fetched_not_backed_up_session.backed_up()); // For v10: they have the backed_up_to property and it is indexed - assert_matches_v10_schema(db_name, store, fetched_backed_up_session).await; + assert_matches_v10_schema(&db_name, &store, &fetched_backed_up_session).await; + + // For v12: they have the curve_key and sender_data_type properties and they are + // indexed + assert_matches_v12_schema(&db_name, &store, &fetched_backed_up_session).await; } async fn assert_matches_v10_schema( - db_name: String, - store: IndexeddbCryptoStore, - fetched_backed_up_session: InboundGroupSession, + db_name: &str, + store: &IndexeddbCryptoStore, + fetched_backed_up_session: &InboundGroupSession, ) { let db = IdbDatabase::open(&db_name).unwrap().await.unwrap(); assert!(db.version() >= 10.0); @@ -551,6 +565,32 @@ mod tests { db.close(); } + async fn assert_matches_v12_schema( + db_name: &str, + store: &IndexeddbCryptoStore, + session: &InboundGroupSession, + ) { + let db = IdbDatabase::open(&db_name).unwrap().await.unwrap(); + assert!(db.version() >= 10.0); + let transaction = db.transaction_on_one("inbound_group_sessions3").unwrap(); + let raw_store = transaction.object_store("inbound_group_sessions3").unwrap(); + let key = store + .serializer + .encode_key(keys::INBOUND_GROUP_SESSIONS_V3, (session.room_id(), session.session_id())); + let idb_object: InboundGroupSessionIndexedDbObject = + serde_wasm_bindgen::from_value(raw_store.get(&key).unwrap().await.unwrap().unwrap()) + .unwrap(); + + assert_eq!(idb_object.curve_key, None); + assert_eq!(idb_object.sender_data_type, None); + assert!(raw_store + .index_names() + .find(|idx| idx == "inbound_group_session_curve_key_sender_data_type_idx") + .is_some()); + + db.close(); + } + fn create_sessions(room_id: &RoomId) -> (InboundGroupSession, InboundGroupSession) { let curve_key = Curve25519PublicKey::from(&Curve25519SecretKey::new()); let ed_key = Ed25519SecretKey::new().public_key(); diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v11_to_v12.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v11_to_v12.rs new file mode 100644 index 00000000000..f3f4e0a3ee5 --- /dev/null +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v11_to_v12.rs @@ -0,0 +1,37 @@ +// Copyright 2024 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Migration code that moves from inbound_group_sessions2 to +//! inbound_group_sessions3, shrinking the values stored in each record. + +use indexed_db_futures::IdbKeyPath; +use web_sys::DomException; + +use crate::crypto_store::{keys, migrations::do_schema_upgrade, Result}; + +/// Perform the schema upgrade v11 to v12, adding an index on +/// `(curve_key, sender_data_type)` to `inbound_group_sessions3`. +pub(crate) async fn schema_add(name: &str) -> Result<(), DomException> { + do_schema_upgrade(name, 12, |_, transaction, _| { + let object_store = transaction.object_store(keys::INBOUND_GROUP_SESSIONS_V3)?; + + object_store.create_index( + keys::INBOUND_GROUP_SESSIONS_CURVE_KEY_INDEX, + &IdbKeyPath::str_sequence(&["curve_key", "sender_data_type"]), + )?; + + Ok(()) + }) + .await +} diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs index c6e805c9b78..04ffb0b0762 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v8_to_v10.rs @@ -103,6 +103,8 @@ pub(crate) async fn data_migrate(name: &str, serializer: &IndexeddbSerializer) - let new_value = InboundGroupSessionIndexedDbObject::new( serializer.maybe_encrypt_value(session.pickle().await)?, !session.backed_up(), + None, + None, ); // Write it to the new store diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs index c041c63a0fd..6156aa6a03b 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs @@ -24,7 +24,7 @@ use indexed_db_futures::prelude::*; use matrix_sdk_crypto::{ olm::{ InboundGroupSession, OlmMessageHash, OutboundGroupSession, PickledInboundGroupSession, - PrivateCrossSigningIdentity, Session, StaticAccountData, + PrivateCrossSigningIdentity, SenderDataType, Session, StaticAccountData, }, store::{ caches::SessionStore, BackupKeys, Changes, CryptoStore, CryptoStoreError, PendingChanges, @@ -62,6 +62,8 @@ mod keys { pub const INBOUND_GROUP_SESSIONS_V3: &str = "inbound_group_sessions3"; pub const INBOUND_GROUP_SESSIONS_BACKUP_INDEX: &str = "backup"; pub const INBOUND_GROUP_SESSIONS_BACKED_UP_TO_INDEX: &str = "backed_up_to"; + pub const INBOUND_GROUP_SESSIONS_CURVE_KEY_INDEX: &str = + "inbound_group_session_curve_key_sender_data_type_idx"; pub const OUTBOUND_GROUP_SESSIONS: &str = "outbound_group_sessions"; @@ -403,6 +405,8 @@ impl IndexeddbCryptoStore { let obj = InboundGroupSessionIndexedDbObject::new( self.serializer.maybe_encrypt_value(session.pickle().await)?, !session.backed_up(), + Some(session.sender_key().to_base64()), + Some(session.sender_data_type()), ); Ok(serde_wasm_bindgen::to_value(&obj)?) } @@ -1618,7 +1622,7 @@ struct GossipRequestIndexedDbObject { unsent: bool, } -/// The objects we store in the inbound_group_sessions2 indexeddb object store +/// The objects we store in the inbound_group_sessions3 indexeddb object store #[derive(serde::Serialize, serde::Deserialize)] struct InboundGroupSessionIndexedDbObject { /// Possibly encrypted @@ -1651,16 +1655,43 @@ struct InboundGroupSessionIndexedDbObject { /// "refer to the `needs_backup` property". See: /// https://github.com/element-hq/element-web/issues/26892#issuecomment-1906336076 backed_up_to: i32, + + /// The curve key of the device that sent us this room key, base64-encoded. + /// + /// Added in database schema v12, and lazily populated, so it is only + /// present for sessions received or modified since DB schema v12. + #[serde(default, skip_serializing_if = "Option::is_none")] + curve_key: Option, + + /// The type of the [`SenderData`] within this session, converted to a u8 + /// from [`SenderDataType`]. + /// + /// Added in database schema v12, and lazily populated, so it is only + /// present for sessions received or modified since DB schema v12. + #[serde(default, skip_serializing_if = "Option::is_none")] + sender_data_type: Option, } impl InboundGroupSessionIndexedDbObject { - pub fn new(pickled_session: MaybeEncrypted, needs_backup: bool) -> Self { - Self { pickled_session, needs_backup, backed_up_to: -1 } + pub fn new( + pickled_session: MaybeEncrypted, + needs_backup: bool, + curve_key: Option, + sender_data_type: Option, + ) -> Self { + Self { + pickled_session, + needs_backup, + backed_up_to: -1, + curve_key, + sender_data_type: sender_data_type.map(|t| t as u8), + } } } #[cfg(test)] mod unit_tests { + use matrix_sdk_crypto::olm::SenderDataType; use matrix_sdk_store_encryption::EncryptedValueBase64; use super::InboundGroupSessionIndexedDbObject; @@ -1671,6 +1702,8 @@ mod unit_tests { let session_needs_backup = InboundGroupSessionIndexedDbObject::new( MaybeEncrypted::Encrypted(EncryptedValueBase64::new(1, "", "")), true, + None, + None, ); // Testing the exact JSON here is theoretically flaky in the face of @@ -1686,6 +1719,8 @@ mod unit_tests { let session_backed_up = InboundGroupSessionIndexedDbObject::new( MaybeEncrypted::Encrypted(EncryptedValueBase64::new(1, "", "")), false, + None, + None, ); assert!( @@ -1693,10 +1728,24 @@ mod unit_tests { "The needs_backup field should be missing!" ); } + + #[test] + fn curve_key_and_sender_data_type_are_serialized_in_json() { + let db_object = InboundGroupSessionIndexedDbObject::new( + MaybeEncrypted::Encrypted(EncryptedValueBase64::new(1, "", "")), + true, + Some("KEY".to_owned()), + Some(SenderDataType::SenderKnown), + ); + + assert!(serde_json::to_string(&db_object).unwrap().contains(r#""curve_key":"KEY""#),); + assert!(serde_json::to_string(&db_object).unwrap().contains(r#""sender_data_type":3"#),); + } } #[cfg(all(test, target_arch = "wasm32"))] mod wasm_unit_tests { + use matrix_sdk_crypto::olm::SenderDataType; use matrix_sdk_store_encryption::EncryptedValueBase64; use matrix_sdk_test::async_test; use wasm_bindgen::JsValue; @@ -1715,6 +1764,8 @@ mod wasm_unit_tests { let session_needs_backup = InboundGroupSessionIndexedDbObject::new( MaybeEncrypted::Encrypted(EncryptedValueBase64::new(3, "", "")), true, + None, + None, ); let js_value = serde_wasm_bindgen::to_value(&session_needs_backup).unwrap(); @@ -1728,12 +1779,48 @@ mod wasm_unit_tests { let session_backed_up = InboundGroupSessionIndexedDbObject::new( MaybeEncrypted::Encrypted(EncryptedValueBase64::new(3, "", "")), false, + None, + None, ); let js_value = serde_wasm_bindgen::to_value(&session_backed_up).unwrap(); assert!(!js_sys::Reflect::has(&js_value, &"needs_backup".into()).unwrap()); } + + #[async_test] + fn curve_key_and_device_type_are_serialized_in_js() { + let db_object = InboundGroupSessionIndexedDbObject::new( + MaybeEncrypted::Encrypted(EncryptedValueBase64::new(1, "", "")), + true, + Some("KEY".to_owned()), + Some(SenderDataType::DeviceInfo), + ); + + let js_value = serde_wasm_bindgen::to_value(&db_object).unwrap(); + + assert!(js_value.is_object()); + assert_field_equals(&js_value, "sender_data_type", 2); + assert_eq!( + js_sys::Reflect::get(&js_value, &"curve_key".into()).unwrap(), + JsValue::from_str("KEY"), + ); + } + + #[async_test] + fn none_values_are_serialized_with_missing_fields_in_js() { + let db_object = InboundGroupSessionIndexedDbObject::new( + MaybeEncrypted::Encrypted(EncryptedValueBase64::new(1, "", "")), + false, + None, + None, + ); + + let js_value = serde_wasm_bindgen::to_value(&db_object).unwrap(); + + assert!(!js_sys::Reflect::has(&js_value, &"curve_key".into()).unwrap()); + assert!(!js_sys::Reflect::has(&js_value, &"sender_data_type".into()).unwrap()); + } } #[cfg(all(test, target_arch = "wasm32"))] diff --git a/crates/matrix-sdk-sqlite/migrations/crypto_store/009_inbound_group_session_curve_key_sender_data_type.sql b/crates/matrix-sdk-sqlite/migrations/crypto_store/009_inbound_group_session_curve_key_sender_data_type.sql new file mode 100644 index 00000000000..a54042b307d --- /dev/null +++ b/crates/matrix-sdk-sqlite/migrations/crypto_store/009_inbound_group_session_curve_key_sender_data_type.sql @@ -0,0 +1,8 @@ +ALTER TABLE "inbound_group_session" + ADD COLUMN "curve_key" BLOB; + +ALTER TABLE "inbound_group_session" + ADD COLUMN "sender_data_type" INTEGER; + +CREATE INDEX "inbound_group_session_curve_key_sender_data_type_idx" + ON "inbound_group_session" ("curve_key", "sender_data_type"); diff --git a/crates/matrix-sdk-sqlite/src/crypto_store.rs b/crates/matrix-sdk-sqlite/src/crypto_store.rs index c8c32157df2..474a7dddc1b 100644 --- a/crates/matrix-sdk-sqlite/src/crypto_store.rs +++ b/crates/matrix-sdk-sqlite/src/crypto_store.rs @@ -194,7 +194,7 @@ impl SqliteCryptoStore { } } -const DATABASE_VERSION: u8 = 8; +const DATABASE_VERSION: u8 = 9; /// Run migrations for the given version of the database. async fn run_migrations(conn: &SqliteConn, version: u8) -> Result<()> { @@ -269,6 +269,15 @@ async fn run_migrations(conn: &SqliteConn, version: u8) -> Result<()> { .await?; } + if version < 9 { + conn.with_transaction(|txn| { + txn.execute_batch(include_str!( + "../migrations/crypto_store/009_inbound_group_session_curve_key_sender_data_type.sql" + )) + }) + .await?; + } + conn.set_kv("version", vec![DATABASE_VERSION]).await?; Ok(()) @@ -288,6 +297,8 @@ trait SqliteConnectionExt { session_id: &[u8], data: &[u8], backed_up: bool, + curve_key: Option<&str>, + sender_data_type: Option, ) -> rusqlite::Result<()>; fn set_outbound_group_session(&self, room_id: &[u8], data: &[u8]) -> rusqlite::Result<()>; @@ -340,12 +351,14 @@ impl SqliteConnectionExt for rusqlite::Connection { session_id: &[u8], data: &[u8], backed_up: bool, + curve_key: Option<&str>, + sender_data_type: Option, ) -> rusqlite::Result<()> { self.execute( - "INSERT INTO inbound_group_session (session_id, room_id, data, backed_up) \ - VALUES (?1, ?2, ?3, ?4) + "INSERT INTO inbound_group_session (session_id, room_id, data, backed_up, curve_key, sender_data_type) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6) ON CONFLICT (session_id) DO UPDATE SET data = ?3, backed_up = ?4", - (session_id, room_id, data, backed_up), + (session_id, room_id, data, backed_up, curve_key, sender_data_type), )?; Ok(()) } @@ -845,6 +858,8 @@ impl CryptoStore for SqliteCryptoStore { session_id, &serialized_session, pickle.backed_up, + Some(&pickle.sender_key.to_base64()), + Some(pickle.sender_data.to_type() as u8), )?; }