Add key storage toggle to Encryption settings by dbkr · Pull Request #29113 · element-hq/element-web

dbkr · 2025-01-27T16:57:08Z

Checklist

Tests written for new code (and old code if feasible).
New or updated public/exported symbols have accurate TSDoc documentation.
Linter and other CI checks pass.
I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

…oggle

florianduros

Thanks!

florianduros · 2025-02-10T13:16:41Z

+        let setEnabledResolve: () => void;
+        const setEnabledPromise = new Promise<void>((r) => {
+            setEnabledResolve = r;
+        });


FYI, you can use defer here

Or Promise.withResolvers

richvdh

I don't really think this is ready to land as it is.

Although I don't have concerns from the crypto safety PoV, I do have concerns about the usability of the feature, and also the fact that this is code the crypto team will end up maintaining, and I think it needs work.

I'm particularly concerned about the fuzziness around what "enabled" means as regards key storage -- I think the code is making assumptions that do not hold.

Generally though, I'm concerned that this PR is too large to give a meaningful review to. I would urge you to find ways to shave bits off of it so that we can reduce the amount of state we need to keep in our heads at once.

Some random ideas might include:

introducing DestructiveComponent separately
making DeviceListener dependent on m.org.matrix.custom.backup_disabled separately
making the KeyStoragePanel read-only at first

richvdh · 2025-02-14T12:09:22Z

+                ),
+            })}
+        >
+            <Root className="mx_KeyBackupPanel_toggleRow">


shouldn't the class here be about KeyStoragePanel rather than KeyBackupPanel ?

richvdh · 2025-02-14T12:10:30Z

+    test("should warn before turning off key storage", { tag: "@screenshot" }, async ({ page, app, util }) => {
+        await verifySession(app, recoveryKey.encodedPrivateKey);
+        await util.openEncryptionTab();
+
+        await page.getByRole("checkbox", { name: "Allow key storage" }).click();
+
+        await expect(
+            page.getByRole("heading", { name: "Are you sure you want to turn off key storage and delete it?" }),
+        ).toBeVisible();
+
+        await expect(util.getEncryptionTabContent()).toMatchScreenshot("delete-key-storage-confirm.png");
+
+        await page.getByRole("button", { name: "Delete key storage" }).click();
+
+        await expect(page.getByRole("checkbox", { name: "Allow key storage" })).not.toBeChecked();
+    });


it would be good if we confirmed that the switch actually worked, rather than just exposing some nice UI?

richvdh · 2025-02-14T12:16:10Z

I don't have a strong opinion on this, but having a .pcss file which doesn't correspond to a specific component seems like a deviation from our normal way of doing things.

Wouldn't the normal way be to have a DestructiveEncryptionCard or something?

richvdh · 2025-02-14T12:19:44Z

 * If the user needs to set up the encryption, the state will be set to "set_up_encryption".
 * If the user secrets are not cached, the state will be set to "secrets_not_cached".


this comment is now apparently incorrect?

richvdh · 2025-02-14T12:21:56Z

 * Hook to check if the user needs:
 * - to go through the SetupEncryption flow.
 * - to enter their recovery key, if the secrets are not cached locally.
+ * ...and also whether key backup is enabled.


what, exactly, does "enabled" mean here? Is it the same or different to m.org.matrix.custom.backup_disabled ?

And are you sure that checking backupInfo?.version matches that definition?

richvdh · 2025-02-14T12:42:17Z

-                // the user just hasn't set up 4S yet: prompt them to do so
-                showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY);
+                // the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to backups)
+                const disabledEvent = cli.getAccountData("m.org.matrix.custom.backup_disabled");


could we add a constant somewhere for this name, so that we don't need to hardcode it in 15 different places?

richvdh · 2025-02-14T12:42:31Z

            } else if (defaultKeyId === null) {
-                // the user just hasn't set up 4S yet: prompt them to do so
-                showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY);
+                // the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to backups)


Suggested change

// the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to backups)

// the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to key storage)

richvdh · 2025-02-14T12:44:31Z

+        // Recheck the status if this account data has been updated as this implies it has changed
+        if (type === "m.org.matrix.custom.backup_disabled") {
+            checkEncryptionState();
+        }


why specifically this, rather than any of the other things that could cause the initial checks to be invalidated?

richvdh · 2025-02-14T12:45:11Z

+        const type = event.getType();
+        // Recheck the status if this account data has been updated as this implies it has changed
+        if (type === "m.org.matrix.custom.backup_disabled") {
+            checkEncryptionState();


won't this reset any flow that the user is in the middle of, causing a computer->window incident?

richvdh · 2025-02-14T12:49:59Z

+                    // also turn off 4S, since this is also storing keys on the server.
+                    // Delete the cross signing keys from secret storage
+                    await matrixClient.deleteAccountData("m.cross_signing.master");
+                    await matrixClient.deleteAccountData("m.cross_signing.self_signing");
+                    await matrixClient.deleteAccountData("m.cross_signing.user_signing");
+                    // and the key backup key (we just turned it off anyway)
+                    await matrixClient.deleteAccountData("m.megolm_backup.v1");
+
+                    // Delete the key information
+                    const defaultKey = await matrixClient.secretStorage.getDefaultKeyId();
+                    if (defaultKey) {
+                        await matrixClient.deleteAccountData(`m.secret_storage.key.${defaultKey}`);
+
+                        // ...and the default key pointer
+                        await matrixClient.deleteAccountData("m.secret_storage.default_key");
+                    }


I think we need to disable dehydration and delete the dehydrated device key here too.

This whole thing could usefully become crypto.deleteSecretStorage.

richvdh · 2025-02-14T13:13:19Z

+        mocked(useKeyStoragePanelViewModel).mockReturnValue({
+            setEnabled,
+            isEnabled: true,
+            loading: false,
+            busy: false,
+        });


this will carry over between tests: please can you jest.restoreAllMocks in an afterEach?

richvdh · 2025-02-14T13:13:55Z

+    });
+
+    it("should call resetKeyBackup if there is no backup currently", async () => {
+        mocked(matrixClient.getCrypto()!.checkKeyBackupAndEnable).mockResolvedValue(null);


again, please remember to restoreAllMocks

dbkr · 2025-02-14T13:16:20Z

Closing in order to re-introduce as individual PRs.

richvdh · 2025-02-14T14:06:27Z

For links: this was part of #26468

Add key storage toggle to Encryption settings

5a74298

dbkr added the T-Enhancement label Jan 27, 2025

github-actions Bot deployed to Netlify January 27, 2025 16:59 View deployment

Keys in the acceptable order

98950de

github-actions Bot deployed to Netlify January 27, 2025 17:03 View deployment

github-actions Bot deployed to EndToEndTests January 27, 2025 17:15 View deployment

dbkr added 2 commits January 28, 2025 15:41

Fix some tests

4eb07d4

Fix import

97057de

github-actions Bot deployed to Netlify January 28, 2025 15:44 View deployment

github-actions Bot deployed to Netlify January 28, 2025 15:46 View deployment

github-actions Bot deployed to EndToEndTests January 28, 2025 15:57 View deployment

Fix toast showing condition

4ec09f0

github-actions Bot deployed to Netlify January 28, 2025 16:38 View deployment

github-actions Bot deployed to EndToEndTests January 28, 2025 16:47 View deployment

Fix import order

4db196e

github-actions Bot deployed to Netlify January 28, 2025 16:53 View deployment

github-actions Bot deployed to EndToEndTests January 28, 2025 17:02 View deployment

Fix playwright tests

98114c8

github-actions Bot deployed to Netlify January 28, 2025 17:18 View deployment

github-actions Bot deployed to EndToEndTests January 28, 2025 17:25 View deployment

Merge remote-tracking branch 'origin/develop' into dbkr/key_storage_t…

6a20703

…oggle

github-actions Bot deployed to Netlify February 5, 2025 10:54 View deployment

Fix bits lost in merge

b7b2ea3

github-actions Bot deployed to Netlify February 5, 2025 11:04 View deployment

github-actions Bot deployed to EndToEndTests February 5, 2025 11:17 View deployment

Add key storage delete confirm screen

e408715

github-actions Bot deployed to Netlify February 5, 2025 13:23 View deployment

Fix hardcoded Element string

aa6de76

github-actions Bot deployed to Netlify February 5, 2025 13:28 View deployment

Fix type imports

5ac2004

github-actions Bot deployed to Netlify February 10, 2025 10:56 View deployment

github-actions Bot deployed to EndToEndTests February 10, 2025 11:03 View deployment

Have "key storage disabled" as a separate state

c4525b9

github-actions Bot deployed to Netlify February 10, 2025 11:08 View deployment

github-actions Bot deployed to EndToEndTests February 10, 2025 11:16 View deployment

Update snapshot

d1aef9f

github-actions Bot deployed to Netlify February 10, 2025 11:19 View deployment

github-actions Bot deployed to EndToEndTests February 10, 2025 11:27 View deployment

Fix... bad merge?

96a70f2

github-actions Bot deployed to Netlify February 10, 2025 11:30 View deployment

github-actions Bot deployed to EndToEndTests February 10, 2025 11:38 View deployment

Add backup enabled mock to more tests

98edffa

github-actions Bot deployed to Netlify February 10, 2025 12:15 View deployment

More snapshots

a5cec9e

github-actions Bot deployed to Netlify February 10, 2025 12:23 View deployment

github-actions Bot deployed to EndToEndTests February 10, 2025 12:34 View deployment

dbkr requested a review from florianduros February 10, 2025 12:35

florianduros approved these changes Feb 10, 2025

View reviewed changes

Use defer util

0162c82

github-actions Bot deployed to Netlify February 10, 2025 14:50 View deployment

github-actions Bot deployed to EndToEndTests February 10, 2025 14:58 View deployment

richvdh requested changes Feb 14, 2025

View reviewed changes

richvdh reviewed Feb 14, 2025

View reviewed changes

dbkr closed this Feb 14, 2025

This was referenced Feb 14, 2025

Factor out duplicated CSS for buttons in encryption settings #29269

Merged

Honour the backup disable flag from Element X #29290

Merged

Add key storage toggle to Encryption settings #29310

Merged

		* If the user needs to set up the encryption, the state will be set to "set_up_encryption".
		* If the user secrets are not cached, the state will be set to "secrets_not_cached".

	// the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to backups)
	// the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to key storage)

Uh oh!

Conversation

dbkr commented Jan 27, 2025

Checklist

Uh oh!

florianduros left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkr commented Feb 14, 2025

Uh oh!

richvdh commented Feb 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants