From ce2e68858d5c59bdf001cc95eca75b425d6a89c4 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 25 Apr 2025 18:30:25 -0400 Subject: [PATCH 1/5] initial application of recovery key input redesign --- .../security/AccessSecretStorageDialog.tsx | 433 +++++------------- src/i18n/strings/en_EN.json | 16 +- 2 files changed, 112 insertions(+), 337 deletions(-) diff --git a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx b/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx index 14c144a538c..9bac4b7c348 100644 --- a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx +++ b/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx @@ -9,24 +9,12 @@ Please see LICENSE files in the repository root for full details. import { debounce } from "lodash"; import classNames from "classnames"; import React, { type ChangeEvent, type FormEvent } from "react"; -import { logger } from "matrix-js-sdk/src/logger"; -import { decodeRecoveryKey } from "matrix-js-sdk/src/crypto-api"; import { type SecretStorage } from "matrix-js-sdk/src/matrix"; -import { MatrixClientPeg } from "../../../../MatrixClientPeg"; import Field from "../../elements/Field"; -import AccessibleButton, { type ButtonEvent } from "../../elements/AccessibleButton"; import { _t } from "../../../../languageHandler"; -import { accessSecretStorage } from "../../../../SecurityManager"; -import Modal from "../../../../Modal"; import DialogButtons from "../../elements/DialogButtons"; import BaseDialog from "../BaseDialog"; -import { chromeFileInputFix } from "../../../../utils/BrowserWorkarounds"; - -// Maximum acceptable size of a key file. It's 59 characters including the spaces we encode, -// so this should be plenty and allow for people putting extra whitespace in the file because -// maybe that's a thing people would do? -const KEY_FILE_MAX_SIZE = 128; // 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; @@ -34,27 +22,33 @@ const VALIDATION_THROTTLE_MS = 200; export type KeyParams = { passphrase?: string; recoveryKey?: string }; interface IProps { + /** + * Information about the Secret Storage key that we want to get. + */ keyInfo: SecretStorage.SecretStorageKeyDescription; + /** + * Callback to check whether the given key is correct. + */ checkPrivateKey: (k: KeyParams) => Promise; + /** + * Callback for when the user is done with this dialog. `result` will + * contain information about the key that was entered, or will be `false` if + * the user cancelled. + */ onFinished(result?: false | KeyParams): void; } interface IState { + //! The recovery key/phrase that the user entered recoveryKey: string; - recoveryKeyValid: boolean | null; + //! Is the recovery key/phrase correct? `null` means no key/phrase has been entered recoveryKeyCorrect: boolean | null; - recoveryKeyFileError: boolean | null; - forceRecoveryKey: boolean; - passPhrase: string; - keyMatches: boolean | null; - resetting: boolean; } /* * Access Secure Secret Storage by requesting the user's passphrase. */ export default class AccessSecretStorageDialog extends React.PureComponent { - private fileUpload = React.createRef(); private inputRef = React.createRef(); public constructor(props: IProps) { @@ -62,362 +56,151 @@ export default class AccessSecretStorageDialog extends React.PureComponent { - if (this.state.resetting) { - this.setState({ resetting: false }); - } this.props.onFinished(false); }; - private onUseRecoveryKeyClick = (): void => { - this.setState({ - forceRecoveryKey: true, - }); - }; - private validateRecoveryKeyOnChange = debounce(async (): Promise => { - await this.validateRecoveryKey(); + // FIXME: don't call this if we're already validating + await this.validateRecoveryKey(this.state.recoveryKey); }, VALIDATION_THROTTLE_MS); - private async validateRecoveryKey(): Promise { - if (this.state.recoveryKey === "") { + /** + * Checks whether the security key/phrase is correct. + * + * Sets `state.recoveryKeyCorrect` accordingly, and if the key/phrase is + * correct, returns a `KeyParams` structure. + */ + private async validateRecoveryKey(recoveryKey: string): Promise { + if (recoveryKey === "") { this.setState({ - recoveryKeyValid: null, recoveryKeyCorrect: null, }); - return; } + const hasPassphrase = this.props.keyInfo?.passphrase?.salt && this.props.keyInfo?.passphrase?.iterations; + + // If the user has a passphrase, we want to try validating it both as a + // key and as a passphrase. We first try to validate it as a key, since + // that check is faster. + try { - const cli = MatrixClientPeg.safeGet(); - const decodedKey = decodeRecoveryKey(this.state.recoveryKey); - const correct = await cli.secretStorage.checkKey(decodedKey, this.props.keyInfo); - this.setState({ - recoveryKeyValid: true, - recoveryKeyCorrect: correct, - }); - } catch { - this.setState({ - recoveryKeyValid: false, - recoveryKeyCorrect: false, - }); + const input = { recoveryKey: this.state.recoveryKey }; + const recoveryKeyCorrect = await this.props.checkPrivateKey(input); + if (recoveryKeyCorrect) { + this.setState({ recoveryKeyCorrect }); + return input; + } + } catch {} + + if (hasPassphrase) { + try { + const input = { passphrase: this.state.recoveryKey }; + const recoveryKeyCorrect = await this.props.checkPrivateKey(input); + if (recoveryKeyCorrect) { + this.setState({ recoveryKeyCorrect }); + return input; + } + } catch {} } + + this.setState({ + recoveryKeyCorrect: false, + }); } private onRecoveryKeyChange = (ev: ChangeEvent): void => { this.setState({ recoveryKey: ev.target.value, - recoveryKeyFileError: null, }); - // also clear the file upload control so that the user can upload the same file - // the did before (otherwise the onchange wouldn't fire) - if (this.fileUpload.current) this.fileUpload.current.value = ""; - - // We don't use Field's validation here because a) we want it in a separate place rather - // than in a tooltip and b) we want it to display feedback based on the uploaded file - // as well as the text box. Ideally we would refactor Field's validation logic so we could + // We don't use Field's validation here because we want it in a separate place rather + // than in a tooltip. Ideally we would refactor Field's validation logic so we could // re-use some of it. this.validateRecoveryKeyOnChange(); }; - private onRecoveryKeyFileChange = async (ev: ChangeEvent): Promise => { - if (!ev.target.files?.length) return; - - const f = ev.target.files[0]; - - if (f.size > KEY_FILE_MAX_SIZE) { - this.setState({ - recoveryKeyFileError: true, - recoveryKeyCorrect: false, - recoveryKeyValid: false, - }); - } else { - const contents = await f.text(); - // test it's within the base58 alphabet. We could be more strict here, eg. require the - // right number of characters, but it's really just to make sure that what we're reading is - // text because we'll put it in the text field. - if (/^[123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz\s]+$/.test(contents)) { - this.setState({ - recoveryKeyFileError: null, - recoveryKey: contents.trim(), - }); - await this.validateRecoveryKey(); - } else { - this.setState({ - recoveryKeyFileError: true, - recoveryKeyCorrect: false, - recoveryKeyValid: false, - recoveryKey: "", - }); - } - } - }; - - private onRecoveryKeyFileUploadClick = (): void => { - this.fileUpload.current?.click(); - }; - - private onPassPhraseNext = async (ev: FormEvent | React.MouseEvent): Promise => { - ev.preventDefault(); - - if (this.state.passPhrase.length <= 0) { - this.inputRef.current?.focus(); - return; - } - - this.setState({ keyMatches: null }); - const input = { passphrase: this.state.passPhrase }; - const keyMatches = await this.props.checkPrivateKey(input); - if (keyMatches) { - this.props.onFinished(input); - } else { - this.setState({ keyMatches }); - this.inputRef.current?.focus(); - } - }; - private onRecoveryKeyNext = async (ev: FormEvent | React.MouseEvent): Promise => { ev.preventDefault(); - if (!this.state.recoveryKeyValid) return; + const keyParams = await this.validateRecoveryKey(this.state.recoveryKey); - this.setState({ keyMatches: null }); - const input = { recoveryKey: this.state.recoveryKey }; - const keyMatches = await this.props.checkPrivateKey(input); - if (keyMatches) { - this.props.onFinished(input); + if (this.state.recoveryKeyCorrect) { + this.props.onFinished(keyParams); } else { - this.setState({ keyMatches }); + this.inputRef.current?.focus(); } }; - private onPassPhraseChange = (ev: ChangeEvent): void => { - this.setState({ - passPhrase: ev.target.value, - keyMatches: null, - }); - }; - - private onResetAllClick = (ev: ButtonEvent): void => { - ev.preventDefault(); - this.setState({ resetting: true }); - }; - - private onConfirmResetAllClick = async (): Promise => { - // Hide ourselves so the user can interact with the reset dialogs. - // We don't conclude the promise chain (onFinished) yet to avoid confusing - // any upstream code flows. - // - // Note: this will unmount us, so don't call `setState` or anything in the - // rest of this function. - Modal.toggleCurrentDialogVisibility(); - - try { - // Force reset secret storage (which resets the key backup) - await accessSecretStorage( - async (): Promise => { - // Now we can indicate that the user is done pressing buttons, finally. - // Upstream flows will detect the new secret storage, key backup, etc and use it. - this.props.onFinished({}); - }, - { forceReset: true, resetCrossSigning: true }, - ); - } catch (e) { - logger.error(e); - this.props.onFinished(false); - } - }; + private getKeyValidationClasses(): string { + return classNames({ + "mx_AccessSecretStorageDialog_recoveryKeyFeedback": true, + "mx_AccessSecretStorageDialog_recoveryKeyFeedback--valid": this.state.recoveryKeyCorrect === true, + "mx_AccessSecretStorageDialog_recoveryKeyFeedback--invalid": this.state.recoveryKeyCorrect === false, + }); + } private getKeyValidationText(): string { - if (this.state.recoveryKeyFileError) { - return _t("encryption|access_secret_storage_dialog|key_validation_text|wrong_file_type"); - } else if (this.state.recoveryKeyCorrect) { + if (this.state.recoveryKeyCorrect) { return _t("encryption|access_secret_storage_dialog|key_validation_text|recovery_key_is_correct"); - } else if (this.state.recoveryKeyValid) { - return _t("encryption|access_secret_storage_dialog|key_validation_text|wrong_security_key"); - } else if (this.state.recoveryKeyValid === null) { - return ""; + } else if (this.state.recoveryKeyCorrect === null) { + return _t("encryption|access_secret_storage_dialog|alternatives"); } else { - return _t("encryption|access_secret_storage_dialog|key_validation_text|invalid_security_key"); + return _t("encryption|access_secret_storage_dialog|key_validation_text|wrong_security_key"); } } - public render(): React.ReactNode { - const hasPassphrase = this.props.keyInfo?.passphrase?.salt && this.props.keyInfo?.passphrase?.iterations; - - const resetLine = ( - - {_t("encryption|reset_all_button", undefined, { - a: (sub) => ( - - {sub} - - ), - })} - - ); + private getRecoveryKeyFeedback(): React.ReactNode { + return
{this.getKeyValidationText()}
+ } - let content; - let title; - let titleClass; - if (this.state.resetting) { - title = _t("encryption|access_secret_storage_dialog|reset_title"); - titleClass = ["mx_AccessSecretStorageDialog_titleWithIcon mx_AccessSecretStorageDialog_resetBadge"]; - content = ( -
-

{_t("encryption|access_secret_storage_dialog|reset_warning_1")}

-

{_t("encryption|access_secret_storage_dialog|reset_warning_2")}

+ public render(): React.ReactNode { + const title = _t("encryption|access_secret_storage_dialog|security_key_title"); + const titleClass = ["mx_AccessSecretStorageDialog_titleWithIcon mx_AccessSecretStorageDialog_secureBackupTitle"]; + + const recoveryKeyFeedback = this.getRecoveryKeyFeedback(); + const content = ( +
+

{_t("encryption|access_secret_storage_dialog|privacy_warning")}

+ +
+
+
+ +
+
+ {recoveryKeyFeedback} -
- ); - } else if (hasPassphrase && !this.state.forceRecoveryKey) { - title = _t("encryption|access_secret_storage_dialog|security_phrase_title"); - titleClass = ["mx_AccessSecretStorageDialog_titleWithIcon mx_AccessSecretStorageDialog_securePhraseTitle"]; - - let keyStatus; - if (this.state.keyMatches === false) { - keyStatus = ( -
- {"\uD83D\uDC4E "} - {_t("encryption|access_secret_storage_dialog|security_phrase_incorrect_error")} -
- ); - } else { - keyStatus =
; - } - - content = ( -
-

- {_t( - "encryption|access_secret_storage_dialog|enter_phrase_or_key_prompt", - {}, - { - button: (s) => ( - - {s} - - ), - }, - )} -

- - - - {keyStatus} - - -
- ); - } else { - title = _t("encryption|access_secret_storage_dialog|security_key_title"); - titleClass = ["mx_AccessSecretStorageDialog_titleWithIcon mx_AccessSecretStorageDialog_secureBackupTitle"]; - - const feedbackClasses = classNames({ - "mx_AccessSecretStorageDialog_recoveryKeyFeedback": true, - "mx_AccessSecretStorageDialog_recoveryKeyFeedback--valid": this.state.recoveryKeyCorrect === true, - "mx_AccessSecretStorageDialog_recoveryKeyFeedback--invalid": this.state.recoveryKeyCorrect === false, - }); - const recoveryKeyFeedback =
{this.getKeyValidationText()}
; - - content = ( -
-

{_t("encryption|access_secret_storage_dialog|use_security_key_prompt")}

- -
-
-
- -
- - {_t("encryption|access_secret_storage_dialog|separator", { - recoveryFile: "", - securityKey: "", - })} - -
- - - {_t("action|upload")} - -
-
- {recoveryKeyFeedback} - - -
- ); - } + +
+ ); return ( use your Recovery Key to continue.", + "alternatives": "If you have a security key or security phrase, this will work too.", "key_validation_text": { - "invalid_security_key": "Invalid Recovery Key", "recovery_key_is_correct": "Looks good!", - "wrong_file_type": "Wrong file type", - "wrong_security_key": "Wrong Recovery Key" + "wrong_security_key": "The recovery key you entered is not correct." }, - "reset_title": "Reset everything", - "reset_warning_1": "Only do this if you have no other device to complete verification with.", - "reset_warning_2": "If you reset everything, you will restart with no trusted sessions, no trusted users, and might not be able to see past messages.", + "privacy_warning": "Make sure nobody can see this screen!", "restoring": "Restoring keys from backup", - "security_key_title": "Recovery Key", - "security_phrase_incorrect_error": "Unable to access secret storage. Please verify that you entered the correct Security Phrase.", - "security_phrase_title": "Security Phrase", - "separator": "%(securityKey)s or %(recoveryFile)s", - "use_security_key_prompt": "Use your Recovery Key to continue." + "security_key_title": "Enter your recovery key" }, "bootstrap_title": "Setting up keys", "cancel_entering_passphrase_description": "Are you sure you want to cancel entering passphrase?", From d9d86eca93f4f01241a504194c13164431addea9 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 16 May 2025 18:19:47 -0400 Subject: [PATCH 2/5] update styling to agree more with design, and fix jest tests --- res/css/_common.pcss | 1 + .../security/_AccessSecretStorageDialog.pcss | 110 ++---------------- .../security/AccessSecretStorageDialog.tsx | 92 ++++++++------- src/i18n/strings/en_EN.json | 3 +- .../AccessSecretStorageDialog-test.tsx | 58 ++------- 5 files changed, 68 insertions(+), 196 deletions(-) diff --git a/res/css/_common.pcss b/res/css/_common.pcss index 75180013f62..671654db3eb 100644 --- a/res/css/_common.pcss +++ b/res/css/_common.pcss @@ -600,6 +600,7 @@ legend { .mx_Dialog_nonDialogButton, .mx_AccessibleButton, .mx_IdentityServerPicker button, + .mx_AccessSecretStorageDialog button, [class|="maplibregl"] ), .mx_Dialog_buttons button:not(.mx_Dialog_nonDialogButton, .mx_AccessibleButton), diff --git a/res/css/views/dialogs/security/_AccessSecretStorageDialog.pcss b/res/css/views/dialogs/security/_AccessSecretStorageDialog.pcss index 83b9fe96b45..943ec3a41fe 100644 --- a/res/css/views/dialogs/security/_AccessSecretStorageDialog.pcss +++ b/res/css/views/dialogs/security/_AccessSecretStorageDialog.pcss @@ -7,62 +7,14 @@ Please see LICENSE files in the repository root for full details. */ .mx_AccessSecretStorageDialog { - .mx_AccessSecretStorageDialog_titleWithIcon { - &::before { - content: ""; - display: inline-block; - width: 24px; - height: 24px; - margin-inline-end: $spacing-8; - position: relative; - top: 5px; - background-color: $primary-content; - } - - &.mx_AccessSecretStorageDialog_resetBadge::before { - /* The image isn't capable of masking, so we use a background instead. */ - background-image: url("@vector-im/compound-design-tokens/icons/error-solid.svg"); - background-size: 24px; - background-color: transparent; - } - - &.mx_AccessSecretStorageDialog_secureBackupTitle::before { - mask-image: url("$(res)/img/feather-customised/secure-backup.svg"); - } - - &.mx_AccessSecretStorageDialog_securePhraseTitle::before { - mask-image: url("$(res)/img/feather-customised/secure-phrase.svg"); - } + &.mx_EncryptionCard { + /* override some styles that we don't need */ + border: 0px none; + box-shadow: none; + padding: 0px; } .mx_AccessSecretStorageDialog_primaryContainer { - .mx_AccessSecretStorageDialog_passPhraseInput { - width: 300px; - border: 1px solid $accent; - border-radius: 5px; - } - - .mx_AccessSecretStorageDialog_keyStatus { - height: 30px; - } - - .mx_AccessSecretStorageDialog_recoveryKeyEntry { - display: flex; - align-items: center; - - .mx_AccessSecretStorageDialog_recoveryKeyEntry_textInput { - flex-grow: 1; - } - - .mx_AccessSecretStorageDialog_recoveryKeyEntry_entryControlSeparatorText { - margin: $spacing-16; - } - - .mx_AccessSecretStorageDialog_recoveryKeyEntry_fileInput { - display: none; - } - } - .mx_AccessSecretStorageDialog_recoveryKeyFeedback { &::before { content: ""; @@ -76,15 +28,6 @@ Please see LICENSE files in the repository root for full details. margin-inline-end: 5px; } - &.mx_AccessSecretStorageDialog_recoveryKeyFeedback--valid { - color: $accent; - - &::before { - mask-image: url("@vector-im/compound-design-tokens/icons/check.svg"); - background-color: $accent; - } - } - &.mx_AccessSecretStorageDialog_recoveryKeyFeedback--invalid { color: $alert; @@ -94,46 +37,9 @@ Please see LICENSE files in the repository root for full details. } } } + } - .mx_Dialog_buttons { - $spacingStart: $spacing-24; /* 16px icon + 8px padding */ - - text-align: initial; - display: flex; - flex-flow: column; - gap: 14px; - - .mx_Dialog_buttons_additive { - float: none; - - .mx_AccessSecretStorageDialog_reset { - position: relative; - padding-inline-start: $spacingStart; - /* To avoid bold styling inherent with elements */ - font-weight: inherit; - - &::before { - content: ""; - display: inline-block; - position: absolute; - height: 16px; - width: 16px; - left: 0; - top: 2px; /* alignment */ - background-image: url("@vector-im/compound-design-tokens/icons/error-solid.svg"); - background-size: contain; - } - - .mx_AccessSecretStorageDialog_reset_link { - color: $alert; - } - } - } - - .mx_Dialog_buttons_row { - gap: $spacing-16; /* TODO: needs normalization */ - padding-inline-start: $spacingStart; - } - } + .mx_EncryptionCard_buttons { + margin-top: var(--cpd-space-20x); } } diff --git a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx b/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx index 9bac4b7c348..1290ded66ee 100644 --- a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx +++ b/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx @@ -6,6 +6,8 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com Please see LICENSE files in the repository root for full details. */ +import { Button } from "@vector-im/compound-web"; +import LockSolidIcon from "@vector-im/compound-design-tokens/assets/web/icons/lock-solid"; import { debounce } from "lodash"; import classNames from "classnames"; import React, { type ChangeEvent, type FormEvent } from "react"; @@ -13,8 +15,8 @@ import { type SecretStorage } from "matrix-js-sdk/src/matrix"; import Field from "../../elements/Field"; import { _t } from "../../../../languageHandler"; -import DialogButtons from "../../elements/DialogButtons"; -import BaseDialog from "../BaseDialog"; +import { EncryptionCard } from "../../settings/encryption/EncryptionCard"; +import { EncryptionCardButtons } from "../../settings/encryption/EncryptionCardButtons"; // 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; @@ -49,7 +51,7 @@ interface IState { * Access Secure Secret Storage by requesting the user's passphrase. */ export default class AccessSecretStorageDialog extends React.PureComponent { - private inputRef = React.createRef(); + private inputRef = React.createRef(); public constructor(props: IProps) { super(props); @@ -89,7 +91,7 @@ export default class AccessSecretStorageDialog extends React.PureComponent): void => { + private onRecoveryKeyChange = (ev: ChangeEvent): void => { this.setState({ recoveryKey: ev.target.value, }); @@ -137,16 +139,15 @@ export default class AccessSecretStorageDialog extends React.PureComponent{this.getKeyValidationText()}
+ private getRecoveryKeyFeedback(): React.ReactNode | null { + const validationText = this.getKeyValidationText(); + if (validationText === null) { + return null; + } else { + return
{validationText}
; + } } public render(): React.ReactNode { const title = _t("encryption|access_secret_storage_dialog|security_key_title"); - const titleClass = ["mx_AccessSecretStorageDialog_titleWithIcon mx_AccessSecretStorageDialog_secureBackupTitle"]; const recoveryKeyFeedback = this.getRecoveryKeyFeedback(); const content = (
-

{_t("encryption|access_secret_storage_dialog|privacy_warning")}

-
-
- -
+
{recoveryKeyFeedback} - + + + +
); return ( - -
{content}
- + {content} + ); } } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 7570e7d9527..f52d1d988b1 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -903,12 +903,11 @@ "access_secret_storage_dialog": { "alternatives": "If you have a security key or security phrase, this will work too.", "key_validation_text": { - "recovery_key_is_correct": "Looks good!", "wrong_security_key": "The recovery key you entered is not correct." }, "privacy_warning": "Make sure nobody can see this screen!", "restoring": "Restoring keys from backup", - "security_key_title": "Enter your recovery key" + "security_key_title": "Recovery key" }, "bootstrap_title": "Setting up keys", "cancel_entering_passphrase_description": "Are you sure you want to cancel entering passphrase?", diff --git a/test/unit-tests/components/views/dialogs/AccessSecretStorageDialog-test.tsx b/test/unit-tests/components/views/dialogs/AccessSecretStorageDialog-test.tsx index e0320b29086..2d597f8e18f 100644 --- a/test/unit-tests/components/views/dialogs/AccessSecretStorageDialog-test.tsx +++ b/test/unit-tests/components/views/dialogs/AccessSecretStorageDialog-test.tsx @@ -29,9 +29,9 @@ describe("AccessSecretStorageDialog", () => { render(); }; - const enterRecoveryKey = (placeholder = "Recovery Key"): void => { + const enterRecoveryKey = (): void => { act(() => { - fireEvent.change(screen.getByPlaceholderText(placeholder), { + fireEvent.change(screen.getByRole("textbox"), { target: { value: recoveryKey, }, @@ -67,19 +67,19 @@ describe("AccessSecretStorageDialog", () => { renderComponent({ onFinished, checkPrivateKey }); // check that the input field is focused - expect(screen.getByPlaceholderText("Recovery Key")).toHaveFocus(); + expect(screen.getByRole("textbox")).toHaveFocus(); await enterRecoveryKey(); await submitDialog(); - expect(screen.getByText("Looks good!")).toBeInTheDocument(); + expect(screen.getByText("Continue")).not.toHaveAttribute("aria-disabled", "true"); expect(checkPrivateKey).toHaveBeenCalledWith({ recoveryKey }); expect(onFinished).toHaveBeenCalledWith({ recoveryKey }); }); it("Notifies the user if they input an invalid Recovery Key", async () => { const onFinished = jest.fn(); - const checkPrivateKey = jest.fn().mockResolvedValue(true); + const checkPrivateKey = jest.fn().mockResolvedValue(false); renderComponent({ onFinished, checkPrivateKey }); jest.spyOn(mockClient.secretStorage, "checkKey").mockImplementation(() => { @@ -89,8 +89,8 @@ describe("AccessSecretStorageDialog", () => { await enterRecoveryKey(); await submitDialog(); - expect(screen.getByText("Continue")).toBeDisabled(); - expect(screen.getByText("Invalid Recovery Key")).toBeInTheDocument(); + expect(screen.getByText("The recovery key you entered is not correct.")).toBeInTheDocument(); + expect(screen.getByText("Continue")).toHaveAttribute("aria-disabled", "true"); }); it("Notifies the user if they input an invalid passphrase", async function () { @@ -110,46 +110,10 @@ describe("AccessSecretStorageDialog", () => { const checkPrivateKey = jest.fn().mockResolvedValue(false); renderComponent({ checkPrivateKey, keyInfo }); - await enterRecoveryKey("Security Phrase"); - expect(screen.getByPlaceholderText("Security Phrase")).toHaveValue(recoveryKey); - await submitDialog(); - - await expect( - screen.findByText( - "👎 Unable to access secret storage. Please verify that you entered the correct Security Phrase.", - ), - ).resolves.toBeInTheDocument(); - - expect(screen.getByPlaceholderText("Security Phrase")).toHaveFocus(); - }); - - it("Can reset secret storage", async () => { - jest.spyOn(mockClient.secretStorage, "checkKey").mockResolvedValue(true); - - const onFinished = jest.fn(); - const checkPrivateKey = jest.fn().mockResolvedValue(true); - renderComponent({ onFinished, checkPrivateKey }); - - await userEvent.click(screen.getByText("Reset all"), { delay: null }); - - // It will prompt the user to confirm resetting - expect(screen.getByText("Reset everything")).toBeInTheDocument(); - await userEvent.click(screen.getByText("Reset"), { delay: null }); - - // Then it will prompt the user to create a key/passphrase - await screen.findByText("Set up Secure Backup"); - document.execCommand = jest.fn().mockReturnValue(true); - jest.spyOn(mockClient.getCrypto()!, "createRecoveryKeyFromPassphrase").mockResolvedValue({ - privateKey: new Uint8Array(), - encodedPrivateKey: recoveryKey, - }); - screen.getByRole("button", { name: "Continue" }).click(); - - await screen.findByText(/Save your Recovery Key/); - screen.getByRole("button", { name: "Copy" }).click(); - await screen.findByText("Copied!"); - screen.getByRole("button", { name: "Continue" }).click(); + await enterRecoveryKey(); + expect(screen.getByRole("textbox")).toHaveValue(recoveryKey); - await screen.findByText("Secure Backup successful"); + await expect(screen.findByText("The recovery key you entered is not correct.")).resolves.toBeInTheDocument(); + expect(screen.getByText("Continue")).toHaveAttribute("aria-disabled", "true"); }); }); From 40160a5392daa3a424717e4b228181a658753915 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 22 May 2025 12:18:52 -0400 Subject: [PATCH 3/5] look for the right element for entering the key --- playwright/e2e/crypto/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/playwright/e2e/crypto/utils.ts b/playwright/e2e/crypto/utils.ts index 77f1b747a18..1ec8c101888 100644 --- a/playwright/e2e/crypto/utils.ts +++ b/playwright/e2e/crypto/utils.ts @@ -263,7 +263,7 @@ export async function verifySession(app: ElementAppPage, securityKey: string) { const settings = await app.settings.openUserSettings("Encryption"); await settings.getByRole("button", { name: "Verify this device" }).click(); await app.page.getByRole("button", { name: "Verify with Recovery Key" }).click(); - await app.page.locator(".mx_Dialog").locator('input[type="password"]').fill(securityKey); + await app.page.locator(".mx_Dialog").locator("textarea").fill(securityKey); await app.page.getByRole("button", { name: "Continue", disabled: false }).click(); await app.page.getByRole("button", { name: "Done" }).click(); await app.settings.closeDialog(); From 6d028735c4ef1a4fea7734f0cf27b88d6597a5c5 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 22 May 2025 15:39:16 -0400 Subject: [PATCH 4/5] fix more playwright tests --- playwright/e2e/crypto/device-verification.spec.ts | 9 ++++----- playwright/e2e/crypto/utils.ts | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/playwright/e2e/crypto/device-verification.spec.ts b/playwright/e2e/crypto/device-verification.spec.ts index 9be79452c45..64846ac86dc 100644 --- a/playwright/e2e/crypto/device-verification.spec.ts +++ b/playwright/e2e/crypto/device-verification.spec.ts @@ -169,8 +169,8 @@ test.describe("Device verification", { tag: "@no-webkit" }, () => { // Fill the passphrase const dialog = page.locator(".mx_Dialog"); - await dialog.locator("input").fill("new passphrase"); - await dialog.locator(".mx_Dialog_primary:not([disabled])", { hasText: "Continue" }).click(); + await dialog.locator("textarea").fill("new passphrase"); + await dialog.getByRole("button", { name: "Continue", disabled: false }).click(); await page.locator(".mx_AuthPage").getByRole("button", { name: "Done" }).click(); @@ -190,10 +190,9 @@ test.describe("Device verification", { tag: "@no-webkit" }, () => { // Fill the recovery key const dialog = page.locator(".mx_Dialog"); - await dialog.getByRole("button", { name: "use your Recovery Key" }).click(); const aliceRecoveryKey = await aliceBotClient.getRecoveryKey(); - await dialog.locator("#mx_securityKey").fill(aliceRecoveryKey.encodedPrivateKey); - await dialog.locator(".mx_Dialog_primary:not([disabled])", { hasText: "Continue" }).click(); + await dialog.locator("textarea").fill(aliceRecoveryKey.encodedPrivateKey); + await dialog.getByRole("button", { name: "Continue", disabled: false }).click(); await page.locator(".mx_AuthPage").getByRole("button", { name: "Done" }).click(); diff --git a/playwright/e2e/crypto/utils.ts b/playwright/e2e/crypto/utils.ts index 7b3310b17ee..289b123e86e 100644 --- a/playwright/e2e/crypto/utils.ts +++ b/playwright/e2e/crypto/utils.ts @@ -228,8 +228,8 @@ export async function logIntoElement(page: Page, credentials: Credentials, secur await useSecurityKey.click(); } // Fill in the recovery key - await page.locator(".mx_Dialog").locator('input[type="password"]').fill(securityKey); - await page.locator(".mx_Dialog_primary:not([disabled])", { hasText: "Continue" }).click(); + await page.locator(".mx_Dialog").locator("textarea").fill(securityKey); + await page.getByRole("button", { name: "Continue", disabled: false }).click(); await page.getByRole("button", { name: "Done" }).click(); } } From 0d2bd3a9f1e691764e7e6a4f0200a4458d21a0a8 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 22 May 2025 16:43:38 -0400 Subject: [PATCH 5/5] use return value of validation function instead of state --- .../views/dialogs/security/AccessSecretStorageDialog.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx b/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx index b0523404038..5b1fb3e142d 100644 --- a/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx +++ b/src/components/views/dialogs/security/AccessSecretStorageDialog.tsx @@ -67,7 +67,6 @@ export default class AccessSecretStorageDialog extends React.PureComponent => { - // FIXME: don't call this if we're already validating await this.validateRecoveryKey(this.state.recoveryKey); }, VALIDATION_THROTTLE_MS); @@ -133,7 +132,7 @@ export default class AccessSecretStorageDialog extends React.PureComponent