From 4eba6b0387d8ce3c487b0c80ff099a5778cae088 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 5 Feb 2024 18:57:12 +0100 Subject: [PATCH] Fix the StorageManger detecting a false positive consistency check when manually migrating to rust from labs (#12225) * Fix StorageManager checks for rust migration manual opt-in * fix restricted imports * Moved utility to check db internals into js-sdk * fix typos and test names * more timeout for migration test (cherry picked from commit cdfcd37b9426def8ef85269e5af6d47afbb3d1fe) --- package.json | 1 + playwright/e2e/crypto/staged-rollout.spec.ts | 47 +++++ src/utils/StorageManager.ts | 99 +++++++---- test/utils/StorageManager-test.ts | 175 +++++++++++++++++++ yarn.lock | 5 + 5 files changed, 291 insertions(+), 36 deletions(-) create mode 100644 test/utils/StorageManager-test.ts diff --git a/package.json b/package.json index 4cf1662372..066f475c6a 100644 --- a/package.json +++ b/package.json @@ -151,6 +151,7 @@ "@babel/preset-typescript": "^7.12.7", "@babel/register": "^7.12.10", "@casualbot/jest-sonar-reporter": "2.2.7", + "fake-indexeddb": "^5.0.2", "@peculiar/webcrypto": "^1.4.3", "@playwright/test": "^1.40.1", "@testing-library/dom": "^9.0.0", diff --git a/playwright/e2e/crypto/staged-rollout.spec.ts b/playwright/e2e/crypto/staged-rollout.spec.ts index 4395b9c6b0..6e85e7e970 100644 --- a/playwright/e2e/crypto/staged-rollout.spec.ts +++ b/playwright/e2e/crypto/staged-rollout.spec.ts @@ -16,6 +16,7 @@ limitations under the License. import { test, expect } from "../../element-web-test"; import { logIntoElement } from "./utils"; +import { SettingLevel } from "../../../src/settings/SettingLevel"; test.describe("Adoption of rust stack", () => { test("Test migration of existing logins when rollout is 100%", async ({ @@ -30,6 +31,7 @@ test.describe("Adoption of rust stack", () => { "No need to test this on Rust Crypto as we override the config manually", ); await page.goto("/#/login"); + test.slow(); let featureRustCrypto = false; let stagedRolloutPercent = 0; @@ -86,6 +88,7 @@ test.describe("Adoption of rust stack", () => { workerInfo.project.name === "Rust Crypto", "No need to test this on Rust Crypto as we override the config manually", ); + test.slow(); await page.goto("/#/login"); await context.route(`http://localhost:8080/config.json*`, async (route) => { @@ -123,6 +126,7 @@ test.describe("Adoption of rust stack", () => { workerInfo.project.name === "Rust Crypto", "No need to test this on Rust Crypto as we override the config manually", ); + test.slow(); await page.goto("/#/login"); @@ -150,4 +154,47 @@ test.describe("Adoption of rust stack", () => { await app.settings.openUserSettings("Help & About"); await expect(page.getByText("Crypto version: Olm")).toBeVisible(); }); + + test("Migrate using labflag should work", async ({ page, context, app, credentials, homeserver }, workerInfo) => { + test.skip( + workerInfo.project.name === "Rust Crypto", + "No need to test this on Rust Crypto as we override the config manually", + ); + test.slow(); + + await page.goto("/#/login"); + + // In the project.name = "Legacy crypto" it will be olm crypto + await logIntoElement(page, homeserver, credentials); + + await app.settings.openUserSettings("Help & About"); + await expect(page.getByText("Crypto version: Olm")).toBeVisible(); + + // We need to enable devtools for this test + await app.settings.setValue("developerMode", null, SettingLevel.ACCOUNT, true); + + // Now simulate a refresh with `feature_rust_crypto` enabled but ensure no automatic migration + await context.route(`http://localhost:8080/config.json*`, async (route) => { + const json = {}; + json["features"] = { + feature_rust_crypto: true, + }; + json["setting_defaults"] = { + "RustCrypto.staged_rollout_percent": 0, + }; + await route.fulfill({ json }); + }); + + await page.reload(); + + // Go to the labs flag and enable the migration + await app.settings.openUserSettings("Labs"); + await page.getByRole("switch", { name: "Rust cryptography implementation" }).click(); + + // Fixes a bug where a missing session data was shown + // https://github.com/element-hq/element-web/issues/26970 + + await app.settings.openUserSettings("Help & About"); + await expect(page.getByText("Crypto version: Rust SDK")).toBeVisible(); + }); }); diff --git a/src/utils/StorageManager.ts b/src/utils/StorageManager.ts index bec3729b40..faf5f7d27a 100644 --- a/src/utils/StorageManager.ts +++ b/src/utils/StorageManager.ts @@ -22,26 +22,20 @@ import { Features } from "../settings/Settings"; const localStorage = window.localStorage; -// just *accessing* indexedDB throws an exception in firefox with -// indexeddb disabled. -let indexedDB: IDBFactory; -try { - indexedDB = window.indexedDB; -} catch (e) {} +// make this lazy in order to make testing easier +function getIndexedDb(): IDBFactory | undefined { + // just *accessing* _indexedDB throws an exception in firefox with + // indexeddb disabled. + try { + return window.indexedDB; + } catch (e) {} +} // The JS SDK will add a prefix of "matrix-js-sdk:" to the sync store name. const SYNC_STORE_NAME = "riot-web-sync"; const LEGACY_CRYPTO_STORE_NAME = "matrix-js-sdk:crypto"; const RUST_CRYPTO_STORE_NAME = "matrix-js-sdk::matrix-sdk-crypto"; -function cryptoStoreName(): string { - if (SettingsStore.getValue(Features.RustCrypto)) { - return RUST_CRYPTO_STORE_NAME; - } else { - return LEGACY_CRYPTO_STORE_NAME; - } -} - function log(msg: string): void { logger.log(`StorageManager: ${msg}`); } @@ -74,7 +68,7 @@ export async function checkConsistency(): Promise<{ }> { log("Checking storage consistency"); log(`Local storage supported? ${!!localStorage}`); - log(`IndexedDB supported? ${!!indexedDB}`); + log(`IndexedDB supported? ${!!getIndexedDb()}`); let dataInLocalStorage = false; let dataInCryptoStore = false; @@ -92,7 +86,7 @@ export async function checkConsistency(): Promise<{ error("Local storage cannot be used on this browser"); } - if (indexedDB && localStorage) { + if (getIndexedDb() && localStorage) { const results = await checkSyncStore(); if (!results.healthy) { healthy = false; @@ -102,7 +96,7 @@ export async function checkConsistency(): Promise<{ error("Sync store cannot be used on this browser"); } - if (indexedDB) { + if (getIndexedDb()) { const results = await checkCryptoStore(); dataInCryptoStore = results.exists; if (!results.healthy) { @@ -144,7 +138,7 @@ interface StoreCheck { async function checkSyncStore(): Promise { let exists = false; try { - exists = await IndexedDBStore.exists(indexedDB, SYNC_STORE_NAME); + exists = await IndexedDBStore.exists(getIndexedDb()!, SYNC_STORE_NAME); log(`Sync store using IndexedDB contains data? ${exists}`); return { exists, healthy: true }; } catch (e) { @@ -155,23 +149,56 @@ async function checkSyncStore(): Promise { } async function checkCryptoStore(): Promise { - let exists = false; - try { - exists = await IndexedDBCryptoStore.exists(indexedDB, cryptoStoreName()); - log(`Crypto store using IndexedDB contains data? ${exists}`); - return { exists, healthy: true }; - } catch (e) { - error("Crypto store using IndexedDB inaccessible", e); + if (await SettingsStore.getValue(Features.RustCrypto)) { + // check first if there is a rust crypto store + try { + const rustDbExists = await IndexedDBCryptoStore.exists(getIndexedDb()!, RUST_CRYPTO_STORE_NAME); + log(`Rust Crypto store using IndexedDB contains data? ${rustDbExists}`); + + if (rustDbExists) { + // There was an existing rust database, so consider it healthy. + return { exists: true, healthy: true }; + } else { + // No rust store, so let's check if there is a legacy store not yet migrated. + try { + const legacyIdbExists = await IndexedDBCryptoStore.existsAndIsNotMigrated( + getIndexedDb()!, + LEGACY_CRYPTO_STORE_NAME, + ); + log(`Legacy Crypto store using IndexedDB contains non migrated data? ${legacyIdbExists}`); + return { exists: legacyIdbExists, healthy: true }; + } catch (e) { + error("Legacy crypto store using IndexedDB inaccessible", e); + } + + // No need to check local storage or memory as rust stack doesn't support them. + // Given that rust stack requires indexeddb, set healthy to false. + return { exists: false, healthy: false }; + } + } catch (e) { + error("Rust crypto store using IndexedDB inaccessible", e); + return { exists: false, healthy: false }; + } + } else { + let exists = false; + // legacy checks + try { + exists = await IndexedDBCryptoStore.exists(getIndexedDb()!, LEGACY_CRYPTO_STORE_NAME); + log(`Crypto store using IndexedDB contains data? ${exists}`); + return { exists, healthy: true }; + } catch (e) { + error("Crypto store using IndexedDB inaccessible", e); + } + try { + exists = LocalStorageCryptoStore.exists(localStorage); + log(`Crypto store using local storage contains data? ${exists}`); + return { exists, healthy: true }; + } catch (e) { + error("Crypto store using local storage inaccessible", e); + } + log("Crypto store using memory only"); + return { exists, healthy: false }; } - try { - exists = LocalStorageCryptoStore.exists(localStorage); - log(`Crypto store using local storage contains data? ${exists}`); - return { exists, healthy: true }; - } catch (e) { - error("Crypto store using local storage inaccessible", e); - } - log("Crypto store using memory only"); - return { exists, healthy: false }; } /** @@ -194,11 +221,11 @@ export function setCryptoInitialised(cryptoInited: boolean): void { let idb: IDBDatabase | null = null; async function idbInit(): Promise { - if (!indexedDB) { + if (!getIndexedDb()) { throw new Error("IndexedDB not available"); } idb = await new Promise((resolve, reject) => { - const request = indexedDB.open("matrix-react-sdk", 1); + const request = getIndexedDb()!.open("matrix-react-sdk", 1); request.onerror = reject; request.onsuccess = (): void => { resolve(request.result); diff --git a/test/utils/StorageManager-test.ts b/test/utils/StorageManager-test.ts new file mode 100644 index 0000000000..786e20caea --- /dev/null +++ b/test/utils/StorageManager-test.ts @@ -0,0 +1,175 @@ +/* +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. +*/ + +import "fake-indexeddb/auto"; + +import { IDBFactory } from "fake-indexeddb"; +import { IndexedDBCryptoStore } from "matrix-js-sdk/src/matrix"; + +import * as StorageManager from "../../src/utils/StorageManager"; +import SettingsStore from "../../src/settings/SettingsStore"; + +const LEGACY_CRYPTO_STORE_NAME = "matrix-js-sdk:crypto"; +const RUST_CRYPTO_STORE_NAME = "matrix-js-sdk::matrix-sdk-crypto"; + +describe("StorageManager", () => { + async function createDB(name: string, withStores: string[] | undefined = undefined): Promise { + const request = indexedDB.open(name); + return new Promise((resolve, reject) => { + request.onupgradeneeded = function (event) { + const db = request.result; + if (withStores) { + withStores.forEach((storeName) => { + db.createObjectStore(storeName); + }); + } + }; + request.onsuccess = function (event) { + const db = request.result; + resolve(db); + }; + request.onerror = function (event) { + reject(event); + }; + }); + } + + async function populateLegacyStore(migrationState: number | undefined) { + const db = await createDB(LEGACY_CRYPTO_STORE_NAME, [IndexedDBCryptoStore.STORE_ACCOUNT]); + + if (migrationState) { + const transaction = db.transaction([IndexedDBCryptoStore.STORE_ACCOUNT], "readwrite"); + const store = transaction.objectStore(IndexedDBCryptoStore.STORE_ACCOUNT); + store.put(migrationState, "migrationState"); + await new Promise((resolve, reject) => { + transaction.oncomplete = resolve; + transaction.onerror = reject; + }); + } + } + + beforeEach(() => { + global.structuredClone = (v) => JSON.parse(JSON.stringify(v)); + }); + + describe("Crypto store checks", () => { + async function populateHealthySession() { + // Storage manager only check for the existence of the `riot-web-sync` store, so just create one. + await createDB("riot-web-sync"); + } + + beforeEach(async () => { + await populateHealthySession(); + // eslint-disable-next-line no-global-assign + indexedDB = new IDBFactory(); + }); + + describe("with `feature_rust_crypto` enabled", () => { + beforeEach(() => { + jest.spyOn(SettingsStore, "getValue").mockImplementation(async (key) => { + if (key === "feature_rust_crypto") { + return true; + } + throw new Error(`Unknown key ${key}`); + }); + }); + + it("should not be ok if sync store but no crypto store", async () => { + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(false); + }); + + it("should be ok if sync store and a rust crypto store", async () => { + await createDB(RUST_CRYPTO_STORE_NAME); + + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(true); + }); + + describe("without rust store", () => { + it("should be ok if there is non migrated legacy crypto store", async () => { + await populateLegacyStore(undefined); + + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(true); + }); + + it("should be ok if legacy store in MigrationState `NOT_STARTED`", async () => { + await populateLegacyStore(0 /* MigrationState.NOT_STARTED*/); + + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(true); + }); + + it("should not be ok if MigrationState greater than `NOT_STARTED`", async () => { + await populateLegacyStore(1 /*INITIAL_DATA_MIGRATED*/); + + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(false); + }); + + it("should not be healthy if no indexeddb", async () => { + // eslint-disable-next-line no-global-assign + indexedDB = {} as IDBFactory; + + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(false); + + // eslint-disable-next-line no-global-assign + indexedDB = new IDBFactory(); + }); + }); + }); + + describe("with `feature_rust_crypto` disabled", () => { + beforeEach(() => { + jest.spyOn(SettingsStore, "getValue").mockImplementation(async (key) => { + if (key === "feature_rust_crypto") { + return false; + } + throw new Error(`Unknown key ${key}`); + }); + }); + + it("should not be ok if sync store but no crypto store", async () => { + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(false); + }); + + it("should not be ok if sync store but no crypto store and a rust store", async () => { + await createDB(RUST_CRYPTO_STORE_NAME); + + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(false); + }); + + it("should be healthy if sync store and a legacy crypto store", async () => { + await createDB(LEGACY_CRYPTO_STORE_NAME); + + const result = await StorageManager.checkConsistency(); + expect(result.healthy).toBe(true); + expect(result.dataInCryptoStore).toBe(true); + }); + }); + }); +}); diff --git a/yarn.lock b/yarn.lock index a62e4bfd6a..94bfd1e586 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5110,6 +5110,11 @@ extend@^3.0.0: resolved "https://registry.yarnpkg.com/extend/-/extend-3.0.2.tgz#f8b1136b4071fbd8eb140aff858b1019ec2915fa" integrity sha512-fjquC59cD7CyW6urNXK0FBufkZcoiGG80wTuPujX590cB5Ttln20E2UB4S/WARVqhXffZl2LNgS+gQdPIIim/g== +fake-indexeddb@^5.0.2: + version "5.0.2" + resolved "https://registry.yarnpkg.com/fake-indexeddb/-/fake-indexeddb-5.0.2.tgz#8e0b6c75c6dc6639cbb50c1aa948772147d7c93e" + integrity sha512-cB507r5T3D55DfclY01GLkninZLfU7HXV/mhVRTnTRm5k2u+fY7Fof2dBkr80p5t7G7dlA/G5dI87QiMdPpMCQ== + fast-deep-equal@^3.1.1, fast-deep-equal@^3.1.3: version "3.1.3" resolved "https://registry.yarnpkg.com/fast-deep-equal/-/fast-deep-equal-3.1.3.tgz#3a7d56b559d6cbc3eb512325244e619a65c6c525"