From a36553336781a3b0da258dc6c53f3805808b6a59 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 14 Feb 2025 16:44:34 +0000 Subject: [PATCH] ChangeRecoveryKey: error handling (#29262) * CreateSecretStorageDialog: error handling I'm fed up with setup operations in EW failing silently. Rather than leaving the user with a mysteriously broken client, let's at least tell them that something has gone wrong, so that they can report the issue and we can investigate. Obviously, showing an unactionable Error dialog is a last resort: ideally, we should handle the error ourselves, or give the user actionable steps to resolve the problem. But that takes significant design and engineering. Just swallowing errors is the worst of all possible options. * Fix typo in test name * Improve test coverage --- .../settings/encryption/ChangeRecoveryKey.tsx | 4 +-- src/utils/ErrorUtils.tsx | 27 +++++++++++++++ .../encryption/ChangeRecoveryKey-test.tsx | 34 ++++++++++++++++++- .../ChangeRecoveryKey-test.tsx.snap | 8 ++--- 4 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/components/views/settings/encryption/ChangeRecoveryKey.tsx b/src/components/views/settings/encryption/ChangeRecoveryKey.tsx index b149b396e1..866ea52d1d 100644 --- a/src/components/views/settings/encryption/ChangeRecoveryKey.tsx +++ b/src/components/views/settings/encryption/ChangeRecoveryKey.tsx @@ -19,7 +19,6 @@ import { } from "@vector-im/compound-web"; import CopyIcon from "@vector-im/compound-design-tokens/assets/web/icons/copy"; import KeyIcon from "@vector-im/compound-design-tokens/assets/web/icons/key-solid"; -import { logger } from "matrix-js-sdk/src/logger"; import { _t } from "../../../../languageHandler"; import { EncryptionCard } from "./EncryptionCard"; @@ -28,6 +27,7 @@ import { useAsyncMemo } from "../../../../hooks/useAsyncMemo"; import { copyPlaintext } from "../../../../utils/strings"; import { initialiseDehydrationIfEnabled } from "../../../../utils/device/dehydration.ts"; import { withSecretStorageKeyCache } from "../../../../SecurityManager"; +import { logErrorAndShowErrorDialog } from "../../../../utils/ErrorUtils.tsx"; /** * The possible states of the component. @@ -132,7 +132,7 @@ export function ChangeRecoveryKey({ }); onFinish(); } catch (e) { - logger.error("Failed to bootstrap secret storage", e); + logErrorAndShowErrorDialog("Failed to set up secret storage", e); } }} submitButtonLabel={ diff --git a/src/utils/ErrorUtils.tsx b/src/utils/ErrorUtils.tsx index 38ba489c7f..2772350a0c 100644 --- a/src/utils/ErrorUtils.tsx +++ b/src/utils/ErrorUtils.tsx @@ -8,11 +8,14 @@ Please see LICENSE files in the repository root for full details. import React, { type ReactNode } from "react"; import { MatrixError, ConnectionError } from "matrix-js-sdk/src/matrix"; +import { logger } from "matrix-js-sdk/src/logger"; import { _t, _td, lookupString, type Tags, type TranslatedString, type TranslationKey } from "../languageHandler"; import SdkConfig from "../SdkConfig"; import { type ValidatedServerConfig } from "./ValidatedServerConfig"; import ExternalLink from "../components/views/elements/ExternalLink"; +import Modal from "../Modal.tsx"; +import ErrorDialog from "../components/views/dialogs/ErrorDialog.tsx"; export const resourceLimitStrings = { "monthly_active_user": _td("error|mau"), @@ -191,3 +194,27 @@ export function messageForConnectionError( return errorText; } + +/** + * Utility for handling unexpected errors: pops up the error dialog. + * + * Example usage: + * ``` + * try { + * /// complicated operation + * } catch (e) { + * logErrorAndShowErrorDialog("Failed complicated operation", e); + * } + * ``` + * + * This isn't particularly intended to be pretty; rather it lets the user know that *something* has gone wrong so that + * they can report a bug. The general idea is that it's better to let the user know of a failure, even if they + * can't do anything about it, than it is to fail silently with the appearance of success. + * + * @param title - Title for the error dialog. + * @param error - The thrown error. Becomes the content of the error dialog. + */ +export function logErrorAndShowErrorDialog(title: string, error: any): void { + logger.error(`${title}:`, error); + Modal.createDialog(ErrorDialog, { title, description: `${error}` }); +} diff --git a/test/unit-tests/components/views/settings/encryption/ChangeRecoveryKey-test.tsx b/test/unit-tests/components/views/settings/encryption/ChangeRecoveryKey-test.tsx index 783aaf701c..706efb2b90 100644 --- a/test/unit-tests/components/views/settings/encryption/ChangeRecoveryKey-test.tsx +++ b/test/unit-tests/components/views/settings/encryption/ChangeRecoveryKey-test.tsx @@ -9,6 +9,7 @@ import React from "react"; import { render, screen, waitFor } from "jest-matrix-react"; import { type MatrixClient } from "matrix-js-sdk/src/matrix"; import userEvent from "@testing-library/user-event"; +import { mocked } from "jest-mock"; import { ChangeRecoveryKey } from "../../../../../../src/components/views/settings/encryption/ChangeRecoveryKey"; import { createTestClient, withClientContextRenderOptions } from "../../../../../test-utils"; @@ -18,6 +19,10 @@ jest.mock("../../../../../../src/utils/strings", () => ({ copyPlaintext: jest.fn(), })); +afterEach(() => { + jest.restoreAllMocks(); +}); + describe("", () => { let matrixClient: MatrixClient; @@ -36,7 +41,7 @@ describe("", () => { ); } - describe("flow to setup a recovery key", () => { + describe("flow to set up a recovery key", () => { it("should display information about the recovery key", async () => { const user = userEvent.setup(); @@ -107,6 +112,33 @@ describe("", () => { await user.click(finishButton); expect(onFinish).toHaveBeenCalledWith(); }); + + it("should display errors from bootstrapSecretStorage", async () => { + const consoleErrorSpy = jest.spyOn(console, "error").mockReturnValue(undefined); + mocked(matrixClient.getCrypto()!).bootstrapSecretStorage.mockRejectedValue(new Error("can't bootstrap")); + + const user = userEvent.setup(); + renderComponent(false); + + // Display the recovery key to save + await waitFor(() => user.click(screen.getByRole("button", { name: "Continue" }))); + // Display the form to confirm the recovery key + await waitFor(() => user.click(screen.getByRole("button", { name: "Continue" }))); + + await waitFor(() => expect(screen.getByText("Enter your recovery key to confirm")).toBeInTheDocument()); + + const finishButton = screen.getByRole("button", { name: "Finish set up" }); + const input = screen.getByRole("textbox"); + await userEvent.type(input, "encoded private key"); + await user.click(finishButton); + + await screen.findByText("Failed to set up secret storage"); + await screen.findByText("Error: can't bootstrap"); + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Failed to set up secret storage:", + new Error("can't bootstrap"), + ); + }); }); describe("flow to change the recovery key", () => { diff --git a/test/unit-tests/components/views/settings/encryption/__snapshots__/ChangeRecoveryKey-test.tsx.snap b/test/unit-tests/components/views/settings/encryption/__snapshots__/ChangeRecoveryKey-test.tsx.snap index 0719c6cc53..882476080e 100644 --- a/test/unit-tests/components/views/settings/encryption/__snapshots__/ChangeRecoveryKey-test.tsx.snap +++ b/test/unit-tests/components/views/settings/encryption/__snapshots__/ChangeRecoveryKey-test.tsx.snap @@ -160,7 +160,7 @@ exports[` flow to change the recovery key should display th `; -exports[` flow to setup a recovery key should ask the user to enter the recovery key 1`] = ` +exports[` flow to set up a recovery key should ask the user to enter the recovery key 1`] = `