Stop showing a dialog prompting the user to enter an old recovery key (#29143)

* SecurityManager: improve logging * Only prompt user for default 4S key We don't really support the concept of having multiple 4S keys active, so prompting the user to enter a non-default 4S key without even telling them which one we want is rather silly. * playwright: factor out helper for setting up 4S We seem to already have about 5 copies of this code, so before I add another, let's factor it out. * Playwright test for dehydrated device in reset flow This should be fixed by the previous commit, so let's check it stays that way.
2025-01-30 16:27:45 +00:00
parent 12932e2dc6
commit 099c3073b6
6 changed files with 232 additions and 78 deletions
--- a/playwright/e2e/crypto/backups.spec.ts
+++ b/playwright/e2e/crypto/backups.spec.ts
@@ -10,6 +10,7 @@ import { type Page } from "@playwright/test";

 import { test, expect } from "../../element-web-test";
 import { isDendrite } from "../../plugins/homeserver/dendrite";
+import { completeCreateSecretStorageDialog } from "./utils.ts";

 async function expectBackupVersionToBe(page: Page, version: string) {
    await expect(page.locator(".mx_SecureBackupPanel_statusList tr:nth-child(5) td")).toHaveText(
@@ -35,19 +36,7 @@ test.describe("Backups", () => {
            await expect(securityTab.getByRole("heading", { name: "Secure Backup" })).toBeVisible();
            await securityTab.getByRole("button", { name: "Set up", exact: true }).click();

-            const currentDialogLocator = page.locator(".mx_Dialog");
-
-            // It's the first time and secure storage is not set up, so it will create one
-            await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
-            await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
-            await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
-            await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
-            // copy the recovery key to use it later
-            const securityKey = await app.getClipboard();
-            await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
-
-            await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
-            await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
+            const securityKey = await completeCreateSecretStorageDialog(page);

            // Open the settings again
            await app.settings.openUserSettings("Security & Privacy");
@@ -62,6 +51,7 @@ test.describe("Backups", () => {
            await expectBackupVersionToBe(page, "1");

            await securityTab.getByRole("button", { name: "Delete Backup", exact: true }).click();
+            const currentDialogLocator = page.locator(".mx_Dialog");
            await expect(currentDialogLocator.getByRole("heading", { name: "Delete Backup" })).toBeVisible();
            // Delete it
            await currentDialogLocator.getByTestId("dialog-primary-button").click(); // Click "Delete Backup"
--- a/playwright/e2e/crypto/crypto.spec.ts
+++ b/playwright/e2e/crypto/crypto.spec.ts
@@ -8,7 +8,14 @@ Please see LICENSE files in the repository root for full details.

 import type { Page } from "@playwright/test";
 import { expect, test } from "../../element-web-test";
-import { autoJoin, copyAndContinue, createSharedRoomWithUser, enableKeyBackup, verify } from "./utils";
+import {
+    autoJoin,
+    completeCreateSecretStorageDialog,
+    copyAndContinue,
+    createSharedRoomWithUser,
+    enableKeyBackup,
+    verify,
+} from "./utils";
 import { Bot } from "../../pages/bot";
 import { ElementAppPage } from "../../pages/ElementAppPage";
 import { isDendrite } from "../../plugins/homeserver/dendrite";
@@ -111,18 +118,7 @@ test.describe("Cryptography", function () {
                await app.settings.openUserSettings("Security & Privacy");
                await page.getByRole("button", { name: "Set up Secure Backup" }).click();

-                const dialog = page.locator(".mx_Dialog");
-                // Recovery key is selected by default
-                await dialog.getByRole("button", { name: "Continue" }).click();
-                await copyAndContinue(page);
-
-                // If the device is unverified, there should be a "Setting up keys" step; however, it
-                // can be quite quick, and playwright can miss it, so we can't test for it.
-
-                // Either way, we end up at a success dialog:
-                await expect(dialog.getByText("Secure Backup successful")).toBeVisible();
-                await dialog.getByRole("button", { name: "Done" }).click();
-                await expect(dialog.getByText("Secure Backup successful")).not.toBeVisible();
+                await completeCreateSecretStorageDialog(page);

                // Verify that the SSSS keys are in the account data stored in the server
                await verifyKey(app, "master");
--- a/playwright/e2e/crypto/dehydration.spec.ts
+++ b/playwright/e2e/crypto/dehydration.spec.ts
@@ -11,6 +11,8 @@ import { Locator, type Page } from "@playwright/test";
 import { test, expect } from "../../element-web-test";
 import { viewRoomSummaryByName } from "../right-panel/utils";
 import { isDendrite } from "../../plugins/homeserver/dendrite";
+import { completeCreateSecretStorageDialog, createBot, logIntoElement } from "./utils.ts";
+import { Client } from "../../pages/client.ts";

 const ROOM_NAME = "Test room";
 const NAME = "Alice";
@@ -44,7 +46,7 @@ test.use({
 test.describe("Dehydration", () => {
    test.skip(isDendrite, "does not yet support dehydration v2");

-    test("Create dehydrated device", async ({ page, user, app }, workerInfo) => {
+    test("'Set up secure backup' creates dehydrated device", async ({ page, user, app }, workerInfo) => {
        // Create a backup (which will create SSSS, and dehydrated device)

        const securityTab = await app.settings.openUserSettings("Security & Privacy");
@@ -53,17 +55,7 @@ test.describe("Dehydration", () => {
        await expect(securityTab.getByText("Offline device enabled")).not.toBeVisible();
        await securityTab.getByRole("button", { name: "Set up", exact: true }).click();

-        const currentDialogLocator = page.locator(".mx_Dialog");
-
-        // It's the first time and secure storage is not set up, so it will create one
-        await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
-        await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
-        await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
-        await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
-        await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
-
-        await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
-        await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
+        await completeCreateSecretStorageDialog(page);

        // Open the settings again
        await app.settings.openUserSettings("Security & Privacy");
@@ -96,4 +88,49 @@ test.describe("Dehydration", () => {
        await expect(page.locator(".mx_UserInfo_devices").getByText("Offline device enabled")).toBeVisible();
        await expect(page.locator(".mx_UserInfo_devices").getByText("Dehydrated device")).not.toBeVisible();
    });
+
+    test("Reset recovery key during login re-creates dehydrated device", async ({
+        page,
+        homeserver,
+        app,
+        credentials,
+    }) => {
+        // Set up cross-signing and recovery
+        const { botClient } = await createBot(page, homeserver, credentials);
+        // ... and dehydration
+        await botClient.evaluate(async (client) => await client.getCrypto().startDehydration());
+
+        const initialDehydratedDeviceIds = await getDehydratedDeviceIds(botClient);
+        expect(initialDehydratedDeviceIds.length).toBe(1);
+
+        await botClient.evaluate(async (client) => client.stopClient());
+
+        // Log in our client
+        await logIntoElement(page, credentials);
+
+        // Oh no, we forgot our recovery key
+        await page.locator(".mx_AuthPage").getByRole("button", { name: "Reset all" }).click();
+        await page.locator(".mx_AuthPage").getByRole("button", { name: "Proceed with reset" }).click();
+
+        await completeCreateSecretStorageDialog(page, { accountPassword: credentials.password });
+
+        // There should be a brand new dehydrated device
+        const dehydratedDeviceIds = await getDehydratedDeviceIds(app.client);
+        expect(dehydratedDeviceIds.length).toBe(1);
+        expect(dehydratedDeviceIds[0]).not.toEqual(initialDehydratedDeviceIds[0]);
+    });
 });
+
+async function getDehydratedDeviceIds(client: Client): Promise<string[]> {
+    return await client.evaluate(async (client) => {
+        const userId = client.getUserId();
+        const devices = await client.getCrypto().getUserDeviceInfo([userId]);
+        return Array.from(
+            devices
+                .get(userId)
+                .values()
+                .filter((d) => d.dehydrated)
+                .map((d) => d.deviceId),
+        );
+    });
+}
--- a/playwright/e2e/crypto/utils.ts
+++ b/playwright/e2e/crypto/utils.ts
@@ -288,19 +288,52 @@ export async function doTwoWaySasVerification(page: Page, verifier: JSHandle<Ver
 export async function enableKeyBackup(app: ElementAppPage): Promise<string> {
    await app.settings.openUserSettings("Security & Privacy");
    await app.page.getByRole("button", { name: "Set up Secure Backup" }).click();
-    const dialog = app.page.locator(".mx_Dialog");
-    // Recovery key is selected by default
-    await dialog.getByRole("button", { name: "Continue" }).click({ timeout: 60000 });

-    // copy the text ourselves
-    const securityKey = await dialog.locator(".mx_CreateSecretStorageDialog_recoveryKey code").textContent();
-    await copyAndContinue(app.page);
+    return await completeCreateSecretStorageDialog(app.page);
+}

-    await expect(dialog.getByText("Secure Backup successful")).toBeVisible();
-    await dialog.getByRole("button", { name: "Done" }).click();
-    await expect(dialog.getByText("Secure Backup successful")).not.toBeVisible();
+/**
+ * Go through the "Set up Secure Backup" dialog (aka the `CreateSecretStorageDialog`).
+ *
+ * Assumes the dialog is already open for some reason (see also {@link enableKeyBackup}).
+ *
+ * @param page - The playwright `Page` fixture.
+ * @param opts - Options object
+ * @param opts.accountPassword - The user's account password. If we are also resetting cross-signing, then we will need
+ *   to upload the public cross-signing keys, which will cause the app to prompt for the password.
+ *
+ * @returns the new recovery key.
+ */
+export async function completeCreateSecretStorageDialog(
+    page: Page,
+    opts?: { accountPassword?: string },
+): Promise<string> {
+    const currentDialogLocator = page.locator(".mx_Dialog");

-    return securityKey;
+    await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
+    // "Generate a Security Key" is selected by default
+    await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
+    await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
+    await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
+    // copy the recovery key to use it later
+    const recoveryKey = await page.evaluate(() => navigator.clipboard.readText());
+    await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
+
+    // If the device is unverified, there should be a "Setting up keys" step.
+    // If this is not the first time we are setting up cross-signing, the app will prompt for our password; otherwise
+    // the step is quite quick, and playwright can miss it, so we can't test for it.
+    if (opts && Object.hasOwn(opts, "accountPassword")) {
+        await expect(currentDialogLocator.getByRole("heading", { name: "Setting up keys" })).toBeVisible();
+        await page.getByPlaceholder("Password").fill(opts!.accountPassword);
+        await currentDialogLocator.getByRole("button", { name: "Continue" }).click();
+    }
+
+    // Either way, we end up at a success dialog:
+    await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
+    await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
+    await expect(currentDialogLocator.getByText("Secure Backup successful")).not.toBeVisible();
+
+    return recoveryKey;
 }

 /**
--- a/src/SecurityManager.ts
+++ b/src/SecurityManager.ts
@@ -9,7 +9,7 @@ Please see LICENSE files in the repository root for full details.
 import { lazy } from "react";
 import { SecretStorage } from "matrix-js-sdk/src/matrix";
 import { deriveRecoveryKeyFromPassphrase, decodeRecoveryKey, CryptoCallbacks } from "matrix-js-sdk/src/crypto-api";
-import { logger } from "matrix-js-sdk/src/logger";
+import { logger as rootLogger } from "matrix-js-sdk/src/logger";

 import Modal from "./Modal";
 import { MatrixClientPeg } from "./MatrixClientPeg";
@@ -29,6 +29,8 @@ let secretStorageKeys: Record<string, Uint8Array> = {};
 let secretStorageKeyInfo: Record<string, SecretStorage.SecretStorageKeyDescription> = {};
 let secretStorageBeingAccessed = false;

+const logger = rootLogger.getChild("SecurityManager:");
+
 /**
 * This can be used by other components to check if secret storage access is in
 * progress, so that we can e.g. avoid intermittently showing toasts during
@@ -70,33 +72,34 @@ function makeInputToKey(
    };
 }

-async function getSecretStorageKey({
+async function getSecretStorageKey(
+    {
        keys: keyInfos,
-}: {
+    }: {
        keys: Record<string, SecretStorage.SecretStorageKeyDescription>;
-}): Promise<[string, Uint8Array]> {
+    },
+    secretName: string,
+): Promise<[string, Uint8Array]> {
    const cli = MatrixClientPeg.safeGet();
-    let keyId = await cli.secretStorage.getDefaultKeyId();
-    let keyInfo!: SecretStorage.SecretStorageKeyDescription;
-    if (keyId) {
-        // use the default SSSS key if set
-        keyInfo = keyInfos[keyId];
-        if (!keyInfo) {
-            // if the default key is not available, pretend the default key
-            // isn't set
-            keyId = null;
-        }
-    }
-    if (!keyId) {
-        // if no default SSSS key is set, fall back to a heuristic of using the
+    const defaultKeyId = await cli.secretStorage.getDefaultKeyId();
+
+    let keyId: string;
+    // If the defaultKey is useful, use that
+    if (defaultKeyId && keyInfos[defaultKeyId]) {
+        keyId = defaultKeyId;
+    } else {
+        // Fall back to a heuristic of using the
        // only available key, if only one key is set
-        const keyInfoEntries = Object.entries(keyInfos);
-        if (keyInfoEntries.length > 1) {
+        const usefulKeys = Object.keys(keyInfos);
+        if (usefulKeys.length > 1) {
            throw new Error("Multiple storage key requests not implemented");
        }
-        [keyId, keyInfo] = keyInfoEntries[0];
+        keyId = usefulKeys[0];
    }
-    logger.debug(`getSecretStorageKey: request for 4S keys [${Object.keys(keyInfos)}]: looking for key ${keyId}`);
+    const keyInfo = keyInfos[keyId];
+    logger.debug(
+        `getSecretStorageKey: request for 4S keys [${Object.keys(keyInfos)}] for secret \`${secretName}\`: looking for key ${keyId}`,
+    );

    // Check the in-memory cache
    if (secretStorageBeingAccessed && secretStorageKeys[keyId]) {
@@ -106,12 +109,18 @@ async function getSecretStorageKey({

    const keyFromCustomisations = ModuleRunner.instance.extensions.cryptoSetup.getSecretStorageKey();
    if (keyFromCustomisations) {
-        logger.log("getSecretStorageKey: Using secret storage key from CryptoSetupExtension");
+        logger.debug("getSecretStorageKey: Using secret storage key from CryptoSetupExtension");
        cacheSecretStorageKey(keyId, keyInfo, keyFromCustomisations);
        return [keyId, keyFromCustomisations];
    }

-    logger.debug("getSecretStorageKey: prompting user for key");
+    // We only prompt the user for the default key
+    if (keyId !== defaultKeyId) {
+        logger.debug(`getSecretStorageKey: request for non-default key ${keyId}: not prompting user`);
+        throw new Error("Request for non-default 4S key");
+    }
+
+    logger.debug(`getSecretStorageKey: prompting user for key ${keyId}`);
    const inputToKey = makeInputToKey(keyInfo);
    const { finished } = Modal.createDialog(
        AccessSecretStorageDialog,
@@ -139,7 +148,7 @@ async function getSecretStorageKey({
    if (!keyParams) {
        throw new AccessCancelledError();
    }
-    logger.debug("getSecretStorageKey: got key from user");
+    logger.debug(`getSecretStorageKey: got key ${keyId} from user`);
    const key = await inputToKey(keyParams);

    // Save to cache to avoid future prompts in the current session
@@ -154,6 +163,7 @@ function cacheSecretStorageKey(
    key: Uint8Array,
 ): void {
    if (secretStorageBeingAccessed) {
+        logger.debug(`Caching 4S key ${keyId}`);
        secretStorageKeys[keyId] = key;
        secretStorageKeyInfo[keyId] = keyInfo;
    }
@@ -173,13 +183,13 @@ export const crossSigningCallbacks: CryptoCallbacks = {
 * @param func - The operation to be wrapped.
 */
 export async function withSecretStorageKeyCache<T>(func: () => Promise<T>): Promise<T> {
-    logger.debug("SecurityManager: enabling 4S key cache");
+    logger.debug("enabling 4S key cache");
    secretStorageBeingAccessed = true;
    try {
        return await func();
    } finally {
        // Clear secret storage key cache now that work is complete
-        logger.debug("SecurityManager: disabling 4S key cache");
+        logger.debug("disabling 4S key cache");
        secretStorageBeingAccessed = false;
        secretStorageKeys = {};
        secretStorageKeyInfo = {};
--- a/test/unit-tests/SecurityManager-test.ts
+++ b/test/unit-tests/SecurityManager-test.ts
@@ -7,11 +7,18 @@ Please see LICENSE files in the repository root for full details.
 */

 import { mocked } from "jest-mock";
-import { CryptoApi } from "matrix-js-sdk/src/crypto-api";
+import { act } from "react";
+import { Crypto } from "@peculiar/webcrypto";
+import { CryptoApi, deriveRecoveryKeyFromPassphrase } from "matrix-js-sdk/src/crypto-api";
+import { SecretStorage } from "matrix-js-sdk/src/matrix";

-import { accessSecretStorage } from "../../src/SecurityManager";
+import { accessSecretStorage, crossSigningCallbacks } from "../../src/SecurityManager";
 import { filterConsole, stubClient } from "../test-utils";
 import Modal from "../../src/Modal.tsx";
+import {
+    default as AccessSecretStorageDialog,
+    KeyParams,
+} from "../../src/components/views/dialogs/security/AccessSecretStorageDialog.tsx";

 jest.mock("react", () => {
    const React = jest.requireActual("react");
@@ -19,6 +26,10 @@ jest.mock("react", () => {
    return React;
 });

+afterEach(() => {
+    jest.restoreAllMocks();
+});
+
 describe("SecurityManager", () => {
    describe("accessSecretStorage", () => {
        filterConsole("Not setting dehydration key: no SSSS key found");
@@ -74,4 +85,81 @@ describe("SecurityManager", () => {
            await expect(spy.mock.lastCall![0]).resolves.toEqual(expect.objectContaining({ __test: true }));
        });
    });
+
+    describe("getSecretStorageKey", () => {
+        const { getSecretStorageKey } = crossSigningCallbacks;
+
+        /** Polyfill crypto.subtle, which is unavailable in jsdom */
+        function polyFillSubtleCrypto() {
+            Object.defineProperty(globalThis.crypto, "subtle", { value: new Crypto().subtle });
+        }
+
+        it("should prompt the user if the key is uncached", async () => {
+            polyFillSubtleCrypto();
+
+            const client = stubClient();
+            mocked(client.secretStorage.getDefaultKeyId).mockResolvedValue("my_default_key");
+
+            const passphrase = "s3cret";
+            const { recoveryKey, keyInfo } = await deriveKeyFromPassphrase(passphrase);
+
+            jest.spyOn(Modal, "createDialog").mockImplementation((component) => {
+                expect(component).toBe(AccessSecretStorageDialog);
+
+                const modalFunc = async () => [{ passphrase }] as [KeyParams];
+                return {
+                    finished: modalFunc(),
+                    close: () => {},
+                };
+            });
+
+            const [keyId, key] = (await act(() =>
+                getSecretStorageKey!({ keys: { my_default_key: keyInfo } }, "my_secret"),
+            ))!;
+            expect(keyId).toEqual("my_default_key");
+            expect(key).toEqual(recoveryKey);
+        });
+
+        it("should not prompt the user if the requested key is not the default", async () => {
+            const client = stubClient();
+            mocked(client.secretStorage.getDefaultKeyId).mockResolvedValue("my_default_key");
+            const createDialogSpy = jest.spyOn(Modal, "createDialog");
+
+            await expect(
+                act(() =>
+                    getSecretStorageKey!(
+                        { keys: { other_key: {} as SecretStorage.SecretStorageKeyDescription } },
+                        "my_secret",
+                    ),
+                ),
+            ).rejects.toThrow("Request for non-default 4S key");
+            expect(createDialogSpy).not.toHaveBeenCalled();
+        });
+    });
 });
+
+/** Derive a key from a passphrase, also returning the KeyInfo */
+async function deriveKeyFromPassphrase(
+    passphrase: string,
+): Promise<{ recoveryKey: Uint8Array; keyInfo: SecretStorage.SecretStorageKeyDescription }> {
+    const salt = "SALTYGOODNESS";
+    const iterations = 1000;
+
+    const recoveryKey = await deriveRecoveryKeyFromPassphrase(passphrase, salt, iterations);
+
+    const check = await SecretStorage.calculateKeyCheck(recoveryKey);
+    return {
+        recoveryKey,
+        keyInfo: {
+            iv: check.iv,
+            mac: check.mac,
+            algorithm: SecretStorage.SECRET_STORAGE_ALGORITHM_V1_AES,
+            name: "",
+            passphrase: {
+                algorithm: "m.pbkdf2",
+                iterations,
+                salt,
+            },
+        },
+    };
+}