Element-R: fix repeated requests to enter 4S key during cross-signing reset (#12059)

richvdh · web-flow · commit de5931d5a8e5 · 2023-12-15T14:59:36.000Z
* 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
diff --git 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
@@ -299,6 +299,28 @@ export async function promptForBackupPassphrase(): Promise<Uint8Array> {
     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<T>(func: () => Promise<T>): Promise<T> {
+    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<void> {
-    secretStorageBeingAccessed = true;
+    await withSecretStorageKeyCache(() => doAccessSecretStorage(func, forceReset, setupNewKeyBackup));
+}
+
+/** Helper for {@link #accessSecretStorage} */
+async function doAccessSecretStorage(
+    func: () => Promise<void>,
+    forceReset: boolean,
+    setupNewKeyBackup: boolean,
+): Promise<void> {
     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
@@ -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,45 +118,46 @@ 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<void> => {
+    private async resetCrossSigning(): Promise<void> {
         this.setState({ error: false });
         try {
             const cli = MatrixClientPeg.safeGet();
-            await cli.bootstrapCrossSigning({
-                authUploadDeviceSigningKeys: async (makeRequest): Promise<void> => {
-                    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<void> => {
+                        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 });
             logger.error("Error bootstrapping cross-signing", e);
         }
         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(
-                <AccessibleButton key="reset" kind="danger" onClick={this.resetCrossSigning}>
+                <AccessibleButton key="reset" kind="danger" onClick={this.onResetCrossSigningClick}>
                     {_t("action|reset")}
                 </AccessibleButton>,
             );
@@ -260,54 +261,56 @@ export default class CrossSigningPanel extends React.PureComponent<{}, IState> {
                 <details>
                     <summary className="mx_CrossSigningPanel_advanced">{_t("common|advanced")}</summary>
                     <table className="mx_CrossSigningPanel_statusList">
-                        <tr>
-                            <th scope="row">{_t("settings|security|cross_signing_public_keys")}</th>
-                            <td>
-                                {crossSigningPublicKeysOnDevice
-                                    ? _t("settings|security|cross_signing_in_memory")
-                                    : _t("settings|security|cross_signing_not_found")}
-                            </td>
-                        </tr>
-                        <tr>
-                            <th scope="row">{_t("settings|security|cross_signing_private_keys")}</th>
-                            <td>
-                                {crossSigningPrivateKeysInStorage
-                                    ? _t("settings|security|cross_signing_in_4s")
-                                    : _t("settings|security|cross_signing_not_in_4s")}
-                            </td>
-                        </tr>
-                        <tr>
-                            <th scope="row">{_t("settings|security|cross_signing_master_private_Key")}</th>
-                            <td>
-                                {masterPrivateKeyCached
-                                    ? _t("settings|security|cross_signing_cached")
-                                    : _t("settings|security|cross_signing_not_cached")}
-                            </td>
-                        </tr>
-                        <tr>
-                            <th scope="row">{_t("settings|security|cross_signing_self_signing_private_key")}</th>
-                            <td>
-                                {selfSigningPrivateKeyCached
-                                    ? _t("settings|security|cross_signing_cached")
-                                    : _t("settings|security|cross_signing_not_cached")}
-                            </td>
-                        </tr>
-                        <tr>
-                            <th scope="row">{_t("settings|security|cross_signing_user_signing_private_key")}</th>
-                            <td>
-                                {userSigningPrivateKeyCached
-                                    ? _t("settings|security|cross_signing_cached")
-                                    : _t("settings|security|cross_signing_not_cached")}
-                            </td>
-                        </tr>
-                        <tr>
-                            <th scope="row">{_t("settings|security|cross_signing_homeserver_support")}</th>
-                            <td>
-                                {homeserverSupportsCrossSigning
-                                    ? _t("settings|security|cross_signing_homeserver_support_exists")
-                                    : _t("settings|security|cross_signing_not_found")}
-                            </td>
-                        </tr>
+                        <tbody>
+                            <tr>
+                                <th scope="row">{_t("settings|security|cross_signing_public_keys")}</th>
+                                <td>
+                                    {crossSigningPublicKeysOnDevice
+                                        ? _t("settings|security|cross_signing_in_memory")
+                                        : _t("settings|security|cross_signing_not_found")}
+                                </td>
+                            </tr>
+                            <tr>
+                                <th scope="row">{_t("settings|security|cross_signing_private_keys")}</th>
+                                <td>
+                                    {crossSigningPrivateKeysInStorage
+                                        ? _t("settings|security|cross_signing_in_4s")
+                                        : _t("settings|security|cross_signing_not_in_4s")}
+                                </td>
+                            </tr>
+                            <tr>
+                                <th scope="row">{_t("settings|security|cross_signing_master_private_Key")}</th>
+                                <td>
+                                    {masterPrivateKeyCached
+                                        ? _t("settings|security|cross_signing_cached")
+                                        : _t("settings|security|cross_signing_not_cached")}
+                                </td>
+                            </tr>
+                            <tr>
+                                <th scope="row">{_t("settings|security|cross_signing_self_signing_private_key")}</th>
+                                <td>
+                                    {selfSigningPrivateKeyCached
+                                        ? _t("settings|security|cross_signing_cached")
+                                        : _t("settings|security|cross_signing_not_cached")}
+                                </td>
+                            </tr>
+                            <tr>
+                                <th scope="row">{_t("settings|security|cross_signing_user_signing_private_key")}</th>
+                                <td>
+                                    {userSigningPrivateKeyCached
+                                        ? _t("settings|security|cross_signing_cached")
+                                        : _t("settings|security|cross_signing_not_cached")}
+                                </td>
+                            </tr>
+                            <tr>
+                                <th scope="row">{_t("settings|security|cross_signing_homeserver_support")}</th>
+                                <td>
+                                    {homeserverSupportsCrossSigning
+                                        ? _t("settings|security|cross_signing_homeserver_support_exists")
+                                        : _t("settings|security|cross_signing_not_found")}
+                                </td>
+                            </tr>
+                        </tbody>
                     </table>
                 </details>
                 {errorSection}
diff --git 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("<CrossSigningPanel />", () => {
     const userId = "@alice:server.org";
@@ -43,6 +45,10 @@ describe("<CrossSigningPanel />", () => {
         mockClient.isCrossSigningReady.mockResolvedValue(false);
     });
 
+    afterEach(() => {
+        jest.restoreAllMocks();
+    });
+
     it("should render a spinner while loading", () => {
         getComponent();
 
@@ -85,6 +91,21 @@ describe("<CrossSigningPanel />", () => {
             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
