From de5931d5a8e509aa4b411bc07326bdc01211a793 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 15 Dec 2023 14:59:36 +0000 Subject: [PATCH] Element-R: fix repeated requests to enter 4S key during cross-signing reset (#12059) * Remove redundant `forceReset` parameter This was always true, so let's get rid of it. Also some function renames. * Factor out new `withSecretStorageKeyCache` helper ... so that we can use the cache without the whole of `accessSecretStorage`. * Cache secret storage key during cross-signing reset * Playwright test for resetting cross-signing * CrossSigningPanel: Silence annoying react warnings React complains if we don't include an explicit `tbody`. * Simple unit test of reset button --- playwright/e2e/crypto/crypto.spec.ts | 37 +++++ src/SecurityManager.ts | 39 ++++- .../views/settings/CrossSigningPanel.tsx | 153 +++++++++--------- .../views/settings/CrossSigningPanel-test.tsx | 21 +++ .../SecurityUserSettingsTab-test.tsx.snap | 122 +++++++------- 5 files changed, 229 insertions(+), 143 deletions(-) diff --git a/playwright/e2e/crypto/crypto.spec.ts b/playwright/e2e/crypto/crypto.spec.ts index 83d1383675..6bd2f8e27a 100644 --- a/playwright/e2e/crypto/crypto.spec.ts +++ b/playwright/e2e/crypto/crypto.spec.ts @@ -212,6 +212,43 @@ test.describe("Cryptography", function () { }); } + test("Can reset cross-signing keys", async ({ page, app, user: aliceCredentials }) => { + const secretStorageKey = await enableKeyBackup(app); + + // Fetch the current cross-signing keys + async function fetchMasterKey() { + return await test.step("Fetch master key from server", async () => { + const k = await app.client.evaluate(async (cli) => { + const userId = cli.getUserId(); + const keys = await cli.downloadKeysForUsers([userId]); + return Object.values(keys.master_keys[userId].keys)[0]; + }); + console.log(`fetchMasterKey: ${k}`); + return k; + }); + } + const masterKey1 = await fetchMasterKey(); + + // Find the "reset cross signing" button, and click it + await app.settings.openUserSettings("Security & Privacy"); + await page.locator("div.mx_CrossSigningPanel_buttonRow").getByRole("button", { name: "Reset" }).click(); + + // Confirm + await page.getByRole("button", { name: "Clear cross-signing keys" }).click(); + + // Enter the 4S key + await page.getByPlaceholder("Security Key").fill(secretStorageKey); + await page.getByRole("button", { name: "Continue" }).click(); + + await expect(async () => { + const masterKey2 = await fetchMasterKey(); + expect(masterKey1).not.toEqual(masterKey2); + }).toPass(); + + // The dialog should have gone away + await expect(page.locator(".mx_Dialog")).toHaveCount(1); + }); + test("creating a DM should work, being e2e-encrypted / user verification", async ({ page, app, diff --git a/src/SecurityManager.ts b/src/SecurityManager.ts index 056be7a2a9..f6fff3628e 100644 --- a/src/SecurityManager.ts +++ b/src/SecurityManager.ts @@ -299,6 +299,28 @@ export async function promptForBackupPassphrase(): Promise { return key; } +/** + * Carry out an operation that may require multiple accesses to secret storage, caching the key. + * + * Use this helper to wrap an operation that may require multiple accesses to secret storage; the user will be prompted + * to enter the 4S key or passphrase on the first access, and the key will be cached for the rest of the operation. + * + * @param func - The operation to be wrapped. + */ +export async function withSecretStorageKeyCache(func: () => Promise): Promise { + secretStorageBeingAccessed = true; + try { + return await func(); + } finally { + // Clear secret storage key cache now that work is complete + secretStorageBeingAccessed = false; + if (!isCachingAllowed()) { + secretStorageKeys = {}; + secretStorageKeyInfo = {}; + } + } +} + /** * This helper should be used whenever you need to access secret storage. It * ensures that secret storage (and also cross-signing since they each depend on @@ -326,7 +348,15 @@ export async function accessSecretStorage( forceReset = false, setupNewKeyBackup = true, ): Promise { - secretStorageBeingAccessed = true; + await withSecretStorageKeyCache(() => doAccessSecretStorage(func, forceReset, setupNewKeyBackup)); +} + +/** Helper for {@link #accessSecretStorage} */ +async function doAccessSecretStorage( + func: () => Promise, + forceReset: boolean, + setupNewKeyBackup: boolean, +): Promise { try { const cli = MatrixClientPeg.safeGet(); if (!(await cli.hasSecretStorageKey()) || forceReset) { @@ -403,13 +433,6 @@ export async function accessSecretStorage( logger.error(e); // Re-throw so that higher level logic can abort as needed throw e; - } finally { - // Clear secret storage key cache now that work is complete - secretStorageBeingAccessed = false; - if (!isCachingAllowed()) { - secretStorageKeys = {}; - secretStorageKeyInfo = {}; - } } } diff --git a/src/components/views/settings/CrossSigningPanel.tsx b/src/components/views/settings/CrossSigningPanel.tsx index 6588690254..91791e6372 100644 --- a/src/components/views/settings/CrossSigningPanel.tsx +++ b/src/components/views/settings/CrossSigningPanel.tsx @@ -26,7 +26,7 @@ import Spinner from "../elements/Spinner"; import InteractiveAuthDialog from "../dialogs/InteractiveAuthDialog"; import ConfirmDestroyCrossSigningDialog from "../dialogs/security/ConfirmDestroyCrossSigningDialog"; import SetupEncryptionDialog from "../dialogs/security/SetupEncryptionDialog"; -import { accessSecretStorage } from "../../../SecurityManager"; +import { accessSecretStorage, withSecretStorageKeyCache } from "../../../SecurityManager"; import AccessibleButton from "../elements/AccessibleButton"; import { SettingsSubsectionText } from "./shared/SettingsSubsection"; @@ -118,31 +118,27 @@ export default class CrossSigningPanel extends React.PureComponent<{}, IState> { } /** - * Bootstrapping cross-signing take one of these paths: - * 1. Create cross-signing keys locally and store in secret storage (if it - * already exists on the account). - * 2. Access existing secret storage by requesting passphrase and accessing - * cross-signing keys as needed. - * 3. All keys are loaded and there's nothing to do. - * @param {bool} [forceReset] Bootstrap again even if keys already present + * Reset the user's cross-signing keys. */ - private bootstrapCrossSigning = async ({ forceReset = false }): Promise => { + private async resetCrossSigning(): Promise { this.setState({ error: false }); try { const cli = MatrixClientPeg.safeGet(); - await cli.bootstrapCrossSigning({ - authUploadDeviceSigningKeys: async (makeRequest): Promise => { - const { finished } = Modal.createDialog(InteractiveAuthDialog, { - title: _t("encryption|bootstrap_title"), - matrixClient: cli, - makeRequest, - }); - const [confirmed] = await finished; - if (!confirmed) { - throw new Error("Cross-signing key upload auth canceled"); - } - }, - setupNewCrossSigning: forceReset, + await withSecretStorageKeyCache(async () => { + await cli.getCrypto()!.bootstrapCrossSigning({ + authUploadDeviceSigningKeys: async (makeRequest): Promise => { + const { finished } = Modal.createDialog(InteractiveAuthDialog, { + title: _t("encryption|bootstrap_title"), + matrixClient: cli, + makeRequest, + }); + const [confirmed] = await finished; + if (!confirmed) { + throw new Error("Cross-signing key upload auth canceled"); + } + }, + setupNewCrossSigning: true, + }); }); } catch (e) { this.setState({ error: true }); @@ -150,13 +146,18 @@ export default class CrossSigningPanel extends React.PureComponent<{}, IState> { } if (this.unmounted) return; this.getUpdatedStatus(); - }; + } - private resetCrossSigning = (): void => { + /** + * Callback for when the user clicks the "reset cross signing" button. + * + * Shows a confirmation dialog, and then does the reset if confirmed. + */ + private onResetCrossSigningClick = (): void => { Modal.createDialog(ConfirmDestroyCrossSigningDialog, { - onFinished: (act) => { + onFinished: async (act) => { if (!act) return; - this.bootstrapCrossSigning({ forceReset: true }); + this.resetCrossSigning(); }, }); }; @@ -243,7 +244,7 @@ export default class CrossSigningPanel extends React.PureComponent<{}, IState> { if (keysExistAnywhere) { actions.push( - + {_t("action|reset")} , ); @@ -260,54 +261,56 @@ export default class CrossSigningPanel extends React.PureComponent<{}, IState> {
{_t("common|advanced")} - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + +
{_t("settings|security|cross_signing_public_keys")} - {crossSigningPublicKeysOnDevice - ? _t("settings|security|cross_signing_in_memory") - : _t("settings|security|cross_signing_not_found")} -
{_t("settings|security|cross_signing_private_keys")} - {crossSigningPrivateKeysInStorage - ? _t("settings|security|cross_signing_in_4s") - : _t("settings|security|cross_signing_not_in_4s")} -
{_t("settings|security|cross_signing_master_private_Key")} - {masterPrivateKeyCached - ? _t("settings|security|cross_signing_cached") - : _t("settings|security|cross_signing_not_cached")} -
{_t("settings|security|cross_signing_self_signing_private_key")} - {selfSigningPrivateKeyCached - ? _t("settings|security|cross_signing_cached") - : _t("settings|security|cross_signing_not_cached")} -
{_t("settings|security|cross_signing_user_signing_private_key")} - {userSigningPrivateKeyCached - ? _t("settings|security|cross_signing_cached") - : _t("settings|security|cross_signing_not_cached")} -
{_t("settings|security|cross_signing_homeserver_support")} - {homeserverSupportsCrossSigning - ? _t("settings|security|cross_signing_homeserver_support_exists") - : _t("settings|security|cross_signing_not_found")} -
{_t("settings|security|cross_signing_public_keys")} + {crossSigningPublicKeysOnDevice + ? _t("settings|security|cross_signing_in_memory") + : _t("settings|security|cross_signing_not_found")} +
{_t("settings|security|cross_signing_private_keys")} + {crossSigningPrivateKeysInStorage + ? _t("settings|security|cross_signing_in_4s") + : _t("settings|security|cross_signing_not_in_4s")} +
{_t("settings|security|cross_signing_master_private_Key")} + {masterPrivateKeyCached + ? _t("settings|security|cross_signing_cached") + : _t("settings|security|cross_signing_not_cached")} +
{_t("settings|security|cross_signing_self_signing_private_key")} + {selfSigningPrivateKeyCached + ? _t("settings|security|cross_signing_cached") + : _t("settings|security|cross_signing_not_cached")} +
{_t("settings|security|cross_signing_user_signing_private_key")} + {userSigningPrivateKeyCached + ? _t("settings|security|cross_signing_cached") + : _t("settings|security|cross_signing_not_cached")} +
{_t("settings|security|cross_signing_homeserver_support")} + {homeserverSupportsCrossSigning + ? _t("settings|security|cross_signing_homeserver_support_exists") + : _t("settings|security|cross_signing_not_found")} +
{errorSection} diff --git a/test/components/views/settings/CrossSigningPanel-test.tsx b/test/components/views/settings/CrossSigningPanel-test.tsx index 96c65fd5c6..501b0fe901 100644 --- a/test/components/views/settings/CrossSigningPanel-test.tsx +++ b/test/components/views/settings/CrossSigningPanel-test.tsx @@ -26,6 +26,8 @@ import { mockClientMethodsCrypto, mockClientMethodsUser, } from "../../../test-utils"; +import Modal from "../../../../src/Modal"; +import ConfirmDestroyCrossSigningDialog from "../../../../src/components/views/dialogs/security/ConfirmDestroyCrossSigningDialog"; describe("", () => { const userId = "@alice:server.org"; @@ -43,6 +45,10 @@ describe("", () => { mockClient.isCrossSigningReady.mockResolvedValue(false); }); + afterEach(() => { + jest.restoreAllMocks(); + }); + it("should render a spinner while loading", () => { getComponent(); @@ -85,6 +91,21 @@ describe("", () => { expect(screen.getByTestId("summarised-status").innerHTML).toEqual("✅ Cross-signing is ready for use."); expect(screen.getByText("Cross-signing private keys:").parentElement!).toMatchSnapshot(); }); + + it("should allow reset of cross-signing", async () => { + mockClient.getCrypto()!.bootstrapCrossSigning = jest.fn().mockResolvedValue(undefined); + getComponent(); + await flushPromises(); + + const modalSpy = jest.spyOn(Modal, "createDialog"); + + screen.getByRole("button", { name: "Reset" }).click(); + expect(modalSpy).toHaveBeenCalledWith(ConfirmDestroyCrossSigningDialog, expect.any(Object)); + modalSpy.mock.lastCall![1]!.onFinished(true); + expect(mockClient.getCrypto()!.bootstrapCrossSigning).toHaveBeenCalledWith( + expect.objectContaining({ setupNewCrossSigning: true }), + ); + }); }); describe("when cross signing is not ready", () => { diff --git a/test/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap b/test/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap index 562d8c3952..ea53192542 100644 --- a/test/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap +++ b/test/components/views/settings/tabs/user/__snapshots__/SecurityUserSettingsTab-test.tsx.snap @@ -175,66 +175,68 @@ exports[` renders security section 1`] = ` - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + +
- Cross-signing public keys: - - not found -
- Cross-signing private keys: - - not found in storage -
- Master private key: - - not found locally -
- Self signing private key: - - not found locally -
- User signing private key: - - not found locally -
- Homeserver feature support: - - not found -
+ Cross-signing public keys: + + not found +
+ Cross-signing private keys: + + not found in storage +
+ Master private key: + + not found locally +
+ Self signing private key: + + not found locally +
+ User signing private key: + + not found locally +
+ Homeserver feature support: + + not found +