Merge pull request #12230 from matrix-org/backport-12225-to-staging

[Backport staging] Fix the StorageManger detecting a false positive consistency check when manually migrating to rust from labs

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",

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();
+    });
 });

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<StoreCheck> {
     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<StoreCheck> {
 }
 
 async function checkCryptoStore(): Promise<StoreCheck> {
-    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<void> {
-    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);

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<IDBDatabase> {
+        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);
+            });
+        });
+    });
+});

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"