From 7eb16b3361c7539ed68868423353176fafff8c6f Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Fri, 6 Jun 2025 12:21:29 +0100
Subject: [PATCH] AccessSecretStorageDialog: fix inability to enter recovery
key (#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
---
.../e2e/crypto/device-verification.spec.ts | 51 +++++++++++--------
src/components/views/dialogs/BaseDialog.tsx | 27 +++++++---
.../security/AccessSecretStorageDialog.tsx | 21 +++++---
.../views/dialogs/BaseDialog-test.tsx | 21 ++++++++
4 files changed, 82 insertions(+), 38 deletions(-)
create mode 100644 test/unit-tests/components/views/dialogs/BaseDialog-test.tsx
diff --git a/playwright/e2e/crypto/device-verification.spec.ts b/playwright/e2e/crypto/device-verification.spec.ts
index 64846ac86d..5ae53f649e 100644
--- a/playwright/e2e/crypto/device-verification.spec.ts
+++ b/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,38 +164,44 @@ 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);
@@ -202,7 +209,7 @@ test.describe("Device verification", { tag: "@no-webkit" }, () => {
// 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);
diff --git a/src/components/views/dialogs/BaseDialog.tsx b/src/components/views/dialogs/BaseDialog.tsx
index bf23919771..56f6d0c528 100644
--- a/src/components/views/dialogs/BaseDialog.tsx
+++ b/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 {
diff --git a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx b/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx
index 5b1fb3e142..b44e456404 100644
--- a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx
+++ b/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>
);
}
}
diff --git a/test/unit-tests/components/views/dialogs/BaseDialog-test.tsx b/test/unit-tests/components/views/dialogs/BaseDialog-test.tsx
new file mode 100644
index 0000000000..e8737fe88d
--- /dev/null
+++ b/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();
+ });
+});
--
GitLab