AccessSecretStorageDialog: fix inability to enter recovery key (element-hq#30090)

* BaseDialog: fix documentation, and make `onFinished` optional

Since `onFinished` isn't used if `hasCancel` is false, it's a bit silly to make
it mandatory.

* AccessSecretStorageDialog: fix inability to enter recovery key

Wrap AccessSecretStorageDialog in a `BaseDialog`. The main thing this achieves
is a `FocusLock`.

* playwright: factor out helper for verification

We have two copies of the same code, and we're about to add a third...

* playwright: test for verifying from Settings

* Add a unit test for BaseDialog

Author: richvdh

playwright/e2e/crypto/device-verification.spec.ts

@@ -22,6 +22,7 @@ import {
 } from "./utils";
 import { type Bot } from "../../pages/bot";
 import { Toasts } from "../../pages/toasts.ts";
+import type { ElementAppPage } from "../../pages/ElementAppPage.ts";
 
 test.describe("Device verification", { tag: "@no-webkit" }, () => {
     let aliceBotClient: Bot;
@@ -163,46 +164,52 @@ test.describe("Device verification", { tag: "@no-webkit" }, () => {
 
     test("Verify device with Security Phrase during login", async ({ page, app, credentials, homeserver }) => {
         await logIntoElement(page, credentials);
+        await enterRecoveryKeyAndCheckVerified(page, app, "new passphrase");
+    });
 
-        // Select the security phrase
-        await page.locator(".mx_AuthPage").getByRole("button", { name: "Verify with Recovery Key or Phrase" }).click();
+    test("Verify device with Recovery Key during login", async ({ page, app, credentials, homeserver }) => {
+        const recoveryKey = (await aliceBotClient.getRecoveryKey()).encodedPrivateKey;
 
-        // Fill the passphrase
-        const dialog = page.locator(".mx_Dialog");
-        await dialog.locator("textarea").fill("new passphrase");
-        await dialog.getByRole("button", { name: "Continue", disabled: false }).click();
+        await logIntoElement(page, credentials);
+        await enterRecoveryKeyAndCheckVerified(page, app, recoveryKey);
+    });
 
-        await page.locator(".mx_AuthPage").getByRole("button", { name: "Done" }).click();
+    test("Verify device with Recovery Key from settings", async ({ page, app, credentials }) => {
+        const recoveryKey = (await aliceBotClient.getRecoveryKey()).encodedPrivateKey;
 
-        // Check that our device is now cross-signed
-        await checkDeviceIsCrossSigned(app);
+        await logIntoElement(page, credentials);
 
-        // Check that the current device is connected to key backup
-        // The backup decryption key should be in cache also, as we got it directly from the 4S
-        await checkDeviceIsConnectedKeyBackup(app, expectedBackupVersion, true);
-    });
+        /* Dismiss "Verify this device" */
+        const authPage = page.locator(".mx_AuthPage");
+        await authPage.getByRole("button", { name: "Skip verification for now" }).click();
+        await authPage.getByRole("button", { name: "I'll verify later" }).click();
+        await page.waitForSelector(".mx_MatrixChat");
 
-    test("Verify device with Recovery Key during login", async ({ page, app, credentials, homeserver }) => {
-        await logIntoElement(page, credentials);
+        const settings = await app.settings.openUserSettings("Encryption");
+        await settings.getByRole("button", { name: "Verify this device" }).click();
+        await enterRecoveryKeyAndCheckVerified(page, app, recoveryKey);
+    });
 
-        // Select the security phrase
-        await page.locator(".mx_AuthPage").getByRole("button", { name: "Verify with Recovery Key or Phrase" }).click();
+    /** Helper for the three tests above which verify by recovery key */
+    async function enterRecoveryKeyAndCheckVerified(page: Page, app: ElementAppPage, recoveryKey: string) {
+        await page.getByRole("button", { name: "Verify with Recovery Key or Phrase" }).click();
 
-        // Fill the recovery key
+        // Enter the recovery key
         const dialog = page.locator(".mx_Dialog");
-        const aliceRecoveryKey = await aliceBotClient.getRecoveryKey();
-        await dialog.locator("textarea").fill(aliceRecoveryKey.encodedPrivateKey);
+        // We use `pressSequentially` here to make sure that the FocusLock isn't causing us any problems
+        // (cf https://github.com/element-hq/element-web/issues/30089)
+        await dialog.locator("textarea").pressSequentially(recoveryKey);
         await dialog.getByRole("button", { name: "Continue", disabled: false }).click();
 
-        await page.locator(".mx_AuthPage").getByRole("button", { name: "Done" }).click();
+        await page.getByRole("button", { name: "Done" }).click();
 
         // Check that our device is now cross-signed
         await checkDeviceIsCrossSigned(app);
 
         // Check that the current device is connected to key backup
         // The backup decryption key should be in cache also, as we got it directly from the 4S
         await checkDeviceIsConnectedKeyBackup(app, expectedBackupVersion, true);
-    });
+    }
 
     test("Handle incoming verification request with SAS", async ({ page, credentials, homeserver, toasts }) => {
         await logIntoElement(page, credentials);

src/components/views/dialogs/BaseDialog.tsx

@@ -23,13 +23,25 @@ import { getKeyBindingsManager } from "../../../KeyBindingsManager";
 import { KeyBindingAction } from "../../../accessibility/KeyboardShortcuts";
 
 interface IProps {
-    // Whether the dialog should have a 'close' button that will
-    // cause the dialog to be cancelled. This should only be set
-    // to false if there is nothing the app can sensibly do if the
-    // dialog is cancelled, eg. "We can't restore your session and
-    // the app cannot work". Default: true.
+    /**
+     * Whether the dialog should have a 'close' button and a keyDown handler which
+     * will intercept 'Escape'.
+     *
+     * This should only be set to `false` if there is nothing the app can sensibly do if the
+     * dialog is cancelled, eg. "We can't restore your session and
+     * the app cannot work".
+     *
+     * Default: `true`.
+     */
     "hasCancel"?: boolean;
 
+    /**
+     * Callback that will be called when the 'close' button is clicked or 'Escape' is pressed.
+     *
+     * Not used if `hasCancel` is false.
+     */
+    "onFinished"?: () => void;
+
     // called when a key is pressed
     "onKeyDown"?: (e: KeyboardEvent | React.KeyboardEvent) => void;
 
@@ -66,7 +78,6 @@ interface IProps {
 
     // optional Posthog ScreenName to supply during the lifetime of this dialog
     "screenName"?: ScreenName;
-    onFinished(): void;
 }
 
 /*
@@ -103,13 +114,13 @@ export default class BaseDialog extends React.Component<IProps> {
 
                 e.stopPropagation();
                 e.preventDefault();
-                this.props.onFinished();
+                this.props.onFinished?.();
                 break;
         }
     };
 
     private onCancelClick = (): void => {
-        this.props.onFinished();
+        this.props.onFinished?.();
     };
 
     public render(): React.ReactNode {

src/components/views/dialogs/security/AccessSecretStorageDialog.tsx

@@ -17,6 +17,7 @@ import Field from "../../elements/Field";
 import { _t } from "../../../../languageHandler";
 import { EncryptionCard } from "../../settings/encryption/EncryptionCard";
 import { EncryptionCardButtons } from "../../settings/encryption/EncryptionCardButtons";
+import BaseDialog from "../BaseDialog";
 
 // Don't shout at the user that their key is invalid every time they type a key: wait a short time
 const VALIDATION_THROTTLE_MS = 200;
@@ -205,15 +206,19 @@ export default class AccessSecretStorageDialog extends React.PureComponent<IProp
             </div>
         );
 
+        // We wrap the content in `BaseDialog` mostly so that we get a `FocusLock` container; otherwise, if the
+        // SettingsDialog is open, then the `FocusLock` in *that* stops us getting the focus.
         return (
-            <EncryptionCard
-                Icon={LockSolidIcon}
-                className="mx_AccessSecretStorageDialog"
-                title={title}
-                description={_t("encryption|access_secret_storage_dialog|privacy_warning")}
-            >
-                {content}
-            </EncryptionCard>
+            <BaseDialog fixedWidth={false} hasCancel={false}>
+                <EncryptionCard
+                    Icon={LockSolidIcon}
+                    className="mx_AccessSecretStorageDialog"
+                    title={title}
+                    description={_t("encryption|access_secret_storage_dialog|privacy_warning")}
+                >
+                    {content}
+                </EncryptionCard>
+            </BaseDialog>
         );
     }
 }

test/unit-tests/components/views/dialogs/BaseDialog-test.tsx

@@ -0,0 +1,21 @@
+/*
+Copyright 2025 New Vector Ltd.
+
+SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
+Please see LICENSE files in the repository root for full details.
+*/
+
+import React from "react";
+import { render } from "jest-matrix-react";
+import userEvent from "@testing-library/user-event";
+
+import BaseDialog from "../../../../../src/components/views/dialogs/BaseDialog.tsx";
+
+describe("BaseDialog", () => {
+    it("calls onFinished when Escape is pressed", async () => {
+        const onFinished = jest.fn();
+        render(<BaseDialog onFinished={onFinished} />);
+        await userEvent.keyboard("{Escape}");
+        expect(onFinished).toHaveBeenCalled();
+    });
+});