From 980b9223481cecfbdfa34e48625ee82670ac4bb4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 17 Dec 2024 14:50:48 +0000 Subject: [PATCH] Enable key backup by default (#28691) * Factor out crypto setup process into a store To make components pure and avoid react 18 dev mode problems due to components making requests when mounted. * fix test * test for the store * Add comment * Enable key backup by default When we set up cross signing, so the key backup key will be stored locally along with the cross signing keys until the user sets up recovery (4s). This will mean that a user can restore their backup if they log in on a new device as long as they verify with the one they registered on. Replaces https://github.com/element-hq/element-web/pull/28267 * Fix test * Prompt user to set up 4S on logout * Fix test * Add playwright test for key backup by default * Fix imports * This isn't unexpected anymore * Update doc * Fix docs and function name on renderSetupBackupDialog() * Use checkKeyBackupAndEnable * Docs for setup encryption toast * Also test the toast appears * Update mock for the method we use now * Okay fine I guess we need both * Swap here too * Fix comment & doc comments --- playwright/e2e/crypto/backups.spec.ts | 28 ++++++++++++ src/CreateCrossSigning.ts | 15 ++++--- src/DeviceListener.ts | 28 +++++++----- src/components/views/dialogs/LogoutDialog.tsx | 26 ++++++++--- src/i18n/strings/en_EN.json | 3 ++ src/stores/InitialCryptoSetupStore.ts | 7 +++ src/toasts/SetupEncryptionToast.ts | 44 +++++++++++++++++-- test/test-utils/test-utils.ts | 1 + test/unit-tests/DeviceListener-test.ts | 4 +- .../components/structures/MatrixChat-test.tsx | 2 + .../views/dialogs/LogoutDialog-test.tsx | 10 ++++- .../toasts/SetupEncryptionToast-test.tsx | 24 ++++++++++ 12 files changed, 162 insertions(+), 30 deletions(-) create mode 100644 test/unit-tests/toasts/SetupEncryptionToast-test.tsx diff --git a/playwright/e2e/crypto/backups.spec.ts b/playwright/e2e/crypto/backups.spec.ts index da162474fa7..d174cc89e5a 100644 --- a/playwright/e2e/crypto/backups.spec.ts +++ b/playwright/e2e/crypto/backups.spec.ts @@ -9,6 +9,8 @@ Please see LICENSE files in the repository root for full details. import { type Page } from "@playwright/test"; import { test, expect } from "../../element-web-test"; +import { test as masTest, registerAccountMas } from "../oidc"; +import { isDendrite } from "../../plugins/homeserver/dendrite"; async function expectBackupVersionToBe(page: Page, version: string) { await expect(page.locator(".mx_SecureBackupPanel_statusList tr:nth-child(5) td")).toHaveText( @@ -18,6 +20,32 @@ async function expectBackupVersionToBe(page: Page, version: string) { await expect(page.locator(".mx_SecureBackupPanel_statusList tr:nth-child(6) td")).toHaveText(version); } +masTest.describe("Encryption state after registration", () => { + masTest.skip(isDendrite, "does not yet support MAS"); + + masTest("Key backup is enabled by default", async ({ page, mailhog, app }) => { + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + await registerAccountMas(page, mailhog.api, "alice", "alice@email.com", "Pa$sW0rD!"); + + await app.settings.openUserSettings("Security & Privacy"); + expect(page.getByText("This session is backing up your keys.")).toBeVisible(); + }); + + masTest("user is prompted to set up recovery", async ({ page, mailhog, app }) => { + await page.goto("/#/login"); + await page.getByRole("button", { name: "Continue" }).click(); + await registerAccountMas(page, mailhog.api, "alice", "alice@email.com", "Pa$sW0rD!"); + + await page.getByRole("button", { name: "Add room" }).click(); + await page.getByRole("menuitem", { name: "New room" }).click(); + await page.getByRole("textbox", { name: "Name" }).fill("test room"); + await page.getByRole("button", { name: "Create room" }).click(); + + await expect(page.getByRole("heading", { name: "Set up recovery" })).toBeVisible(); + }); +}); + test.describe("Backups", () => { test.use({ displayName: "Hanako", diff --git a/src/CreateCrossSigning.ts b/src/CreateCrossSigning.ts index e67e030f60e..c38f1a3dd57 100644 --- a/src/CreateCrossSigning.ts +++ b/src/CreateCrossSigning.ts @@ -16,18 +16,19 @@ import { _t } from "./languageHandler"; import InteractiveAuthDialog from "./components/views/dialogs/InteractiveAuthDialog"; /** - * Determine if the homeserver allows uploading device keys with only password auth. + * Determine if the homeserver allows uploading device keys with only password auth, or with no auth at + * all (ie. if the homeserver supports MSC3967). * @param cli The Matrix Client to use - * @returns True if the homeserver allows uploading device keys with only password auth, otherwise false + * @returns True if the homeserver allows uploading device keys with only password auth or with no auth + * at all, otherwise false */ async function canUploadKeysWithPasswordOnly(cli: MatrixClient): Promise { try { await cli.uploadDeviceSigningKeys(undefined, {} as CrossSigningKeys); - // We should never get here: the server should always require - // UI auth to upload device signing keys. If we do, we upload - // no keys which would be a no-op. - logger.log("uploadDeviceSigningKeys unexpectedly succeeded without UI auth!"); - return false; + // If we get here, it's because the server is allowing us to upload keys without + // auth the first time due to MSC3967. Therefore, yes, we can upload keys + // (with or without password, technically, but that's fine). + return true; } catch (error) { if (!(error instanceof MatrixError) || !error.data || !error.data.flows) { logger.log("uploadDeviceSigningKeys advertised no flows!"); diff --git a/src/DeviceListener.ts b/src/DeviceListener.ts index 84d83827da5..e34af95962c 100644 --- a/src/DeviceListener.ts +++ b/src/DeviceListener.ts @@ -295,21 +295,29 @@ export default class DeviceListener { await crypto.getUserDeviceInfo([cli.getSafeUserId()]); // cross signing isn't enabled - nag to enable it - // There are 2 different toasts for: + // There are 3 different toasts for: if (!(await crypto.getCrossSigningKeyId()) && (await crypto.userHasCrossSigningKeys())) { - // Cross-signing on account but this device doesn't trust the master key (verify this session) + // Toast 1. Cross-signing on account but this device doesn't trust the master key (verify this session) showSetupEncryptionToast(SetupKind.VERIFY_THIS_SESSION); this.checkKeyBackupStatus(); } else { - // No cross-signing or key backup on account (set up encryption) - await cli.waitForClientWellKnown(); - if (isSecureBackupRequired(cli) && isLoggedIn()) { - // If we're meant to set up, and Secure Backup is required, - // trigger the flow directly without a toast once logged in. - hideSetupEncryptionToast(); - accessSecretStorage(); + const backupInfo = await this.getKeyBackupInfo(); + if (backupInfo) { + // Toast 2: Key backup is enabled but recovery (4S) is not set up: prompt user to set up recovery. + // Since we now enable key backup at registration time, this will be the common case for + // new users. + showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY); } else { - showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION); + // Toast 3: No cross-signing or key backup on account (set up encryption) + await cli.waitForClientWellKnown(); + if (isSecureBackupRequired(cli) && isLoggedIn()) { + // If we're meant to set up, and Secure Backup is required, + // trigger the flow directly without a toast once logged in. + hideSetupEncryptionToast(); + accessSecretStorage(); + } else { + showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION); + } } } } diff --git a/src/components/views/dialogs/LogoutDialog.tsx b/src/components/views/dialogs/LogoutDialog.tsx index 4731c593bc3..af8f91533e7 100644 --- a/src/components/views/dialogs/LogoutDialog.tsx +++ b/src/components/views/dialogs/LogoutDialog.tsx @@ -38,6 +38,9 @@ enum BackupStatus { /** there is a backup on the server but we are not backing up to it */ SERVER_BACKUP_BUT_DISABLED, + /** Key backup is set up but recovery (4s) is not */ + BACKUP_NO_RECOVERY, + /** backup is not set up locally and there is no backup on the server */ NO_BACKUP, @@ -104,7 +107,11 @@ export default class LogoutDialog extends React.Component { } if ((await crypto.getActiveSessionBackupVersion()) !== null) { - this.setState({ backupStatus: BackupStatus.BACKUP_ACTIVE }); + if (await crypto.isSecretStorageReady()) { + this.setState({ backupStatus: BackupStatus.BACKUP_ACTIVE }); + } else { + this.setState({ backupStatus: BackupStatus.BACKUP_NO_RECOVERY }); + } return; } @@ -164,13 +171,17 @@ export default class LogoutDialog extends React.Component { }; /** - * Show a dialog prompting the user to set up key backup. + * Show a dialog prompting the user to set up their recovery method. + * + * Either: + * * There is no backup at all ({@link BackupStatus.NO_BACKUP}) + * * There is a backup set up but recovery (4s) is not ({@link BackupStatus.BACKUP_NO_RECOVERY}) + * * There is a backup on the server but we are not connected to it ({@link BackupStatus.SERVER_BACKUP_BUT_DISABLED}) + * * We were unable to pull the backup data ({@link BackupStatus.ERROR}). * - * Either there is no backup at all ({@link BackupStatus.NO_BACKUP}), there is a backup on the server but - * we are not connected to it ({@link BackupStatus.SERVER_BACKUP_BUT_DISABLED}), or we were unable to pull the - * backup data ({@link BackupStatus.ERROR}). In all three cases, we should prompt the user to set up key backup. + * In all four cases, we should prompt the user to set up a method of recovery. */ - private renderSetupBackupDialog(): React.ReactNode { + private renderSetupRecoveryMethod(): React.ReactNode { const description = (

{_t("auth|logout_dialog|setup_secure_backup_description_1")}

@@ -254,7 +265,8 @@ export default class LogoutDialog extends React.Component { case BackupStatus.NO_BACKUP: case BackupStatus.SERVER_BACKUP_BUT_DISABLED: case BackupStatus.ERROR: - return this.renderSetupBackupDialog(); + case BackupStatus.BACKUP_NO_RECOVERY: + return this.renderSetupRecoveryMethod(); } } } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index e9ac73b48b4..f3c514fca38 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -914,6 +914,9 @@ "warning": "If you didn't remove the recovery method, an attacker may be trying to access your account. Change your account password and set a new recovery method immediately in Settings." }, "reset_all_button": "Forgotten or lost all recovery methods? Reset all", + "set_up_recovery": "Set up recovery", + "set_up_recovery_later": "Not now", + "set_up_recovery_toast_description": "Generate a recovery key that can be used to restore your encrypted message history in case you lose access to your devices.", "set_up_toast_description": "Safeguard against losing access to encrypted messages & data", "set_up_toast_title": "Set up Secure Backup", "setup_secure_backup": { diff --git a/src/stores/InitialCryptoSetupStore.ts b/src/stores/InitialCryptoSetupStore.ts index 0c2e49f5caf..5554a15d260 100644 --- a/src/stores/InitialCryptoSetupStore.ts +++ b/src/stores/InitialCryptoSetupStore.ts @@ -114,8 +114,15 @@ export class InitialCryptoSetupStore extends EventEmitter { this.emit("update"); try { + // Create the user's cross-signing keys await createCrossSigning(this.client, this.isTokenLogin, this.stores.accountPasswordStore.getPassword()); + // Check for any existing backup and enable key backup if there isn't one + const currentKeyBackup = await cryptoApi.checkKeyBackupAndEnable(); + if (currentKeyBackup === null) { + await cryptoApi.resetKeyBackup(); + } + this.reset(); this.status = "complete"; diff --git a/src/toasts/SetupEncryptionToast.ts b/src/toasts/SetupEncryptionToast.ts index 0dd54bb18fd..406b51cf167 100644 --- a/src/toasts/SetupEncryptionToast.ts +++ b/src/toasts/SetupEncryptionToast.ts @@ -23,15 +23,19 @@ const getTitle = (kind: Kind): string => { switch (kind) { case Kind.SET_UP_ENCRYPTION: return _t("encryption|set_up_toast_title"); + case Kind.SET_UP_RECOVERY: + return _t("encryption|set_up_recovery"); case Kind.VERIFY_THIS_SESSION: return _t("encryption|verify_toast_title"); } }; -const getIcon = (kind: Kind): string => { +const getIcon = (kind: Kind): string | undefined => { switch (kind) { case Kind.SET_UP_ENCRYPTION: return "secure_backup"; + case Kind.SET_UP_RECOVERY: + return undefined; case Kind.VERIFY_THIS_SESSION: return "verification_warning"; } @@ -41,22 +45,49 @@ const getSetupCaption = (kind: Kind): string => { switch (kind) { case Kind.SET_UP_ENCRYPTION: return _t("action|continue"); + case Kind.SET_UP_RECOVERY: + return _t("action|continue"); case Kind.VERIFY_THIS_SESSION: return _t("action|verify"); } }; +const getSecondaryButtonLabel = (kind: Kind): string => { + switch (kind) { + case Kind.SET_UP_RECOVERY: + return _t("encryption|set_up_recovery_later"); + case Kind.SET_UP_ENCRYPTION: + case Kind.VERIFY_THIS_SESSION: + return _t("encryption|verification|unverified_sessions_toast_reject"); + } +}; + const getDescription = (kind: Kind): string => { switch (kind) { case Kind.SET_UP_ENCRYPTION: return _t("encryption|set_up_toast_description"); + case Kind.SET_UP_RECOVERY: + return _t("encryption|set_up_recovery_toast_description"); case Kind.VERIFY_THIS_SESSION: return _t("encryption|verify_toast_description"); } }; +/** + * The kind of toast to show. + */ export enum Kind { + /** + * Prompt the user to set up encryption + */ SET_UP_ENCRYPTION = "set_up_encryption", + /** + * Prompt the user to set up a recovery key + */ + SET_UP_RECOVERY = "set_up_recovery", + /** + * Prompt the user to verify this session + */ VERIFY_THIS_SESSION = "verify_this_session", } @@ -64,6 +95,11 @@ const onReject = (): void => { DeviceListener.sharedInstance().dismissEncryptionSetup(); }; +/** + * Show a toast prompting the user for some action related to setting up their encryption. + * + * @param kind The kind of toast to show + */ export const showToast = (kind: Kind): void => { if ( ModuleRunner.instance.extensions.cryptoSetup.setupEncryptionNeeded({ @@ -101,15 +137,17 @@ export const showToast = (kind: Kind): void => { description: getDescription(kind), primaryLabel: getSetupCaption(kind), onPrimaryClick: onAccept, - secondaryLabel: _t("encryption|verification|unverified_sessions_toast_reject"), + secondaryLabel: getSecondaryButtonLabel(kind), onSecondaryClick: onReject, - destructive: "secondary", }, component: GenericToast, priority: kind === Kind.VERIFY_THIS_SESSION ? 95 : 40, }); }; +/** + * Hide the encryption setup toast if it is currently being shown. + */ export const hideToast = (): void => { ToastStore.sharedInstance().dismissToast(TOAST_KEY); }; diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index f9aee512a30..39852f049ee 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -134,6 +134,7 @@ export function createTestClient(): MatrixClient { restoreKeyBackupWithPassphrase: jest.fn(), loadSessionBackupPrivateKeyFromSecretStorage: jest.fn(), storeSessionBackupPrivateKey: jest.fn(), + checkKeyBackupAndEnable: jest.fn().mockResolvedValue(null), getKeyBackupInfo: jest.fn().mockResolvedValue(null), getEncryptionInfoForEvent: jest.fn().mockResolvedValue(null), }), diff --git a/test/unit-tests/DeviceListener-test.ts b/test/unit-tests/DeviceListener-test.ts index ad7f14e1190..1c8fe1a1c73 100644 --- a/test/unit-tests/DeviceListener-test.ts +++ b/test/unit-tests/DeviceListener-test.ts @@ -352,13 +352,13 @@ describe("DeviceListener", () => { mockCrypto!.getCrossSigningKeyId.mockResolvedValue("abc"); }); - it("shows set up encryption toast when user has a key backup available", async () => { + it("shows set up recovery toast when user has a key backup available", async () => { // non falsy response mockCrypto.getKeyBackupInfo.mockResolvedValue({} as unknown as KeyBackupInfo); await createAndStart(); expect(SetupEncryptionToast.showToast).toHaveBeenCalledWith( - SetupEncryptionToast.Kind.SET_UP_ENCRYPTION, + SetupEncryptionToast.Kind.SET_UP_RECOVERY, ); }); }); diff --git a/test/unit-tests/components/structures/MatrixChat-test.tsx b/test/unit-tests/components/structures/MatrixChat-test.tsx index fd17ccf5838..d5db4f190a2 100644 --- a/test/unit-tests/components/structures/MatrixChat-test.tsx +++ b/test/unit-tests/components/structures/MatrixChat-test.tsx @@ -1003,7 +1003,9 @@ describe("", () => { userHasCrossSigningKeys: jest.fn().mockResolvedValue(false), // This needs to not finish immediately because we need to test the screen appears bootstrapCrossSigning: jest.fn().mockImplementation(() => bootstrapDeferred.promise), + resetKeyBackup: jest.fn(), isEncryptionEnabledInRoom: jest.fn().mockResolvedValue(false), + checkKeyBackupAndEnable: jest.fn().mockResolvedValue(null), }; loginClient.getCrypto.mockReturnValue(mockCrypto as any); }); diff --git a/test/unit-tests/components/views/dialogs/LogoutDialog-test.tsx b/test/unit-tests/components/views/dialogs/LogoutDialog-test.tsx index 0557e538d0e..98f758ebbd6 100644 --- a/test/unit-tests/components/views/dialogs/LogoutDialog-test.tsx +++ b/test/unit-tests/components/views/dialogs/LogoutDialog-test.tsx @@ -42,12 +42,20 @@ describe("LogoutDialog", () => { expect(rendered.container).toMatchSnapshot(); }); - it("shows a regular dialog if backups are working", async () => { + it("shows a regular dialog if backups and recovery are working", async () => { mockCrypto.getActiveSessionBackupVersion.mockResolvedValue("1"); + mockCrypto.isSecretStorageReady.mockResolvedValue(true); const rendered = renderComponent(); await rendered.findByText("Are you sure you want to sign out?"); }); + it("prompts user to set up recovery if backups are enabled but recovery isn't", async () => { + mockCrypto.getActiveSessionBackupVersion.mockResolvedValue("1"); + mockCrypto.isSecretStorageReady.mockResolvedValue(false); + const rendered = renderComponent(); + await rendered.findByText("You'll lose access to your encrypted messages"); + }); + it("Prompts user to connect backup if there is a backup on the server", async () => { mockCrypto.getKeyBackupInfo.mockResolvedValue({} as KeyBackupInfo); const rendered = renderComponent(); diff --git a/test/unit-tests/toasts/SetupEncryptionToast-test.tsx b/test/unit-tests/toasts/SetupEncryptionToast-test.tsx new file mode 100644 index 00000000000..5ce3fab9aeb --- /dev/null +++ b/test/unit-tests/toasts/SetupEncryptionToast-test.tsx @@ -0,0 +1,24 @@ +/* +Copyright 2024 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only +Please see LICENSE files in the repository root for full details. +*/ + +import React from "react"; +import { render, screen } from "jest-matrix-react"; + +import ToastContainer from "../../../src/components/structures/ToastContainer"; +import { Kind, showToast } from "../../../src/toasts/SetupEncryptionToast"; + +describe("SetupEncryptionToast", () => { + beforeEach(() => { + render(); + }); + + it("should render the se up recovery toast", async () => { + showToast(Kind.SET_UP_RECOVERY); + + await expect(screen.findByText("Set up recovery")).resolves.toBeInTheDocument(); + }); +});