Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast #29138

dbkr · 2025-01-29T18:46:53Z

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)

…sync" toast

florianduros · 2025-01-31T13:13:06Z

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

+    const recheckSetupRequired = useCallback(() => {
+        (async () => {
+            const crypto = matrixClient.getCrypto()!;
+            const isCrossSigningReady = await crypto.isCrossSigningReady();
+            if (isCrossSigningReady) {
+                setState("main");
+            } else {
+                setState("set_up_encryption");
+            }
+        })();
+    }, [matrixClient]);


I'm in favour to use a custom hook like useSetUpEncryptionRequired before. It helps the readability of the component imo

Something like this? I find the custom hook a bit awkward in this scenario since you have to pass a lot of functions around, but I don't mind too much.

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

florianduros · 2025-01-31T13:13:51Z

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

+            if (isCrossSigningReady) {
+                setState("main");
+            } else {
+                setState("set_up_encryption");
+            }


Suggested change

if (isCrossSigningReady) {

setState("main");

} else {

setState("set_up_encryption");

}

if (isCrossSigningReady) setState("main");

else setState("set_up_encryption");

I'd really rather not if at all possible: I find these line-ifs split over multiple lines very confusing, and I think our code style permits line ifs only if they're one single line.

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

florianduros · 2025-01-31T13:25:31Z

playwright/e2e/crypto/toasts.spec.ts

+import { registerAccountMas } from "../oidc";
+import { deleteCachedSecrets } from "./utils";
+
+test.use(masHomeserver);


The test needs mas?

florianduros · 2025-01-31T13:28:52Z

playwright/e2e/crypto/toasts.spec.ts

+    test.beforeEach(async ({ page, app, mailpitClient }) => {
+        await page.goto("/#/login");
+        await page.getByRole("button", { name: "Continue" }).click();
+        await registerAccountMas(page, mailpitClient, "alice", "[email protected]", "Pa$sW0rD!");
+
+        await expect(page.getByRole("heading", { level: 1, name: "Welcome alice" })).toBeVisible();
+
+        // We won't be prompted for crypto setup unless we have na e2e room, so make one
+        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();
+
+        // Now set up recovery (otherwise we'll delete the only copy of the secret)
+        await page.getByLabel("User menu").click();
+        await page.getByLabel("All settings").click();
+        await page.getByRole("tab", { name: "Encryption" }).click();
+        await page.getByRole("button", { name: "Set up recovery" }).click();
+        await page.getByRole("button", { name: "Continue" }).click();
+        recoveryKey = await page.getByTestId("recoveryKey").textContent();
+        await page.getByRole("button", { name: "Continue" }).click();
+        await page.getByRole("textbox", { name: "Enter recovery key" }).fill(recoveryKey);
+        await page.getByRole("button", { name: "Finish set up" }).click();
+
+        await expect(page.getByRole("button", { name: "Change recovery key" })).toBeVisible();


Can't we use crypto/utils#createBot to create a second session, bootstrap cross singing and create a recovery key?

crypto/utils#verifySession can be used to verify the current session too

It took a bit but I think this is working with the bot client now.

uhoreg

From a crypto point of view, looks OK

playwright/e2e/crypto/toasts.spec.ts

Co-authored-by: Florian Duros <[email protected]>

…tofsync_forgotpassword

…sync" toast (#29138) * Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast * Unused import & fix test * Test 'forgot' variant * Fix dependencies * Add more toast tests * Unused import * Test initialState in Encryption Tab * Let's see if github has any more luck running this test than me * Working playwright test with screenshot * year * Convert playwright test to use the bot client * Disambiguate Co-authored-by: Florian Duros <[email protected]> * Add doc & do other part of rename * Split out into custom hook * Fix tests --------- Co-authored-by: Florian Duros <[email protected]> (cherry picked from commit 9657d39)

richvdh · 2025-02-05T11:43:07Z

"Wire up", but what actually happens? can we get some screenshots or something?

richvdh · 2025-02-05T11:37:55Z

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

+ *  - "reset_identity_compromised": The panel to show when the user is resetting their identity, in te case where their key is compromised.
+ * - "reset_identity_forgot": The panel to show when the user is resetting their identity, in the case where they forgot their recovery key.
+ * - `secrets_not_cached`: The secrets are not cached locally. This can happen if we verified another device and secret-gossiping failed, or the other device itself lacked the secrets.
 *                          If the "set_up_encryption" and "secrets_not_cached" conditions are both filled, "set_up_encryption" prevails.


what on earth is going on with the indentation here?

richvdh · 2025-02-05T11:39:13Z

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

-    const checkEncryptionState = useCheckEncryptionState(setState);
+interface EncryptionUserSettingsTabProps {
+    /**
+     * If the tab should start in a state other than the deasult


richvdh · 2025-02-05T11:39:51Z

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

-export function EncryptionUserSettingsTab(): JSX.Element {
-    const [state, setState] = useState<State>("loading");
-    const checkEncryptionState = useCheckEncryptionState(setState);
+interface EncryptionUserSettingsTabProps {


FWIW, my reading of https://github.com/element-hq/element-web/blob/develop/code_style.md#react is that the Props interface should be called Props, but I'm perfectly happy for it to have a better name

richvdh · 2025-02-05T11:40:50Z

src/components/views/dialogs/UserSettingsDialog.tsx

@@ -50,6 +50,7 @@ import { EncryptionUserSettingsTab } from "../settings/tabs/user/EncryptionUserS
 interface IProps {
    initialTabId?: UserTab;
    showMsc4108QrCode?: boolean;
+    showResetIdentity?: boolean;


documentation please. What does this do? Is false the same as undefined?

richvdh · 2025-02-05T11:45:31Z

src/toasts/SetupEncryptionToast.ts

+            const payload: OpenToTabPayload = {
+                action: Action.ViewUserSettings,
+                initialTabId: UserTab.Encryption,
+                props: { showResetIdentity: true },
+            };
+            defaultDispatcher.dispatch(payload);


a comment in here might not go amiss. What does this payload actually do?

richvdh · 2025-02-05T11:50:09Z

src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx

@@ -32,23 +32,35 @@ import { RecoveryPanelOutOfSync } from "../../encryption/RecoveryPanelOutOfSync"
 *                           This happens when the user has a recovery key and the user clicks on "Change recovery key" button of the RecoveryPanel.
 *  - "set_recovery_key": The panel to show when the user is setting up their recovery key.
 *                        This happens when the user doesn't have a key a recovery key and the user clicks on "Set up recovery key" button of the RecoveryPanel.
- *  - "reset_identity": The panel to show when the user is resetting their identity.
- *  - `secrets_not_cached`: The secrets are not cached locally. This can happen if we verified another device and secret-gossiping failed, or the other device itself lacked the secrets.
+ *  - "reset_identity_compromised": The panel to show when the user is resetting their identity, in te case where their key is compromised.


richvdh · 2025-02-05T11:52:50Z

src/components/views/settings/encryption/ResetIdentityPanel.tsx

+    /**
+     * The variant of the panel to show. We show more warnings in the 'compromised' variant (no use in showing a user this
+     * warning if they have to reset because they no longer have their key)
+     * "compromised" is shown when the user chooses 'reset' explicitly in settings, usually because they believe their
+     * identity has been compromised.
+     * "forgot" is shown when the user has just forgotten their passphrase.
+     */


Formatting is weird here. Newlines don't do anything in tsdoc; either commit to new paragraphs with a blank line, or match the generated output by removing the linebreaks (presumably the former in this case). You can't just sit on the fence.

richvdh · 2025-02-05T11:59:31Z

playwright/e2e/crypto/utils.ts

+        const useSecurityKey = page.locator(".mx_Dialog").getByRole("button", { name: "use your Security Key" });
+        if (await useSecurityKey.isVisible()) {
+            await useSecurityKey.click();
+        }


comment would be good here. Why is this sometimes necessary and sometimes not?

richvdh · 2025-02-05T12:01:18Z

playwright/e2e/crypto/toasts.spec.ts

+import { test, expect } from "../../element-web-test";
+import { createBot, deleteCachedSecrets, logIntoElement } from "./utils";
+
+test.describe("Key storage out of sync toast", () => {


(don't we already have some tests for this toast somewhere else?)

richvdh · 2025-02-05T12:01:37Z

playwright/e2e/crypto/toasts.spec.ts

+    });
+
+    test("should prompt for recovery key if 'enter recovery key' pressed", { tag: "@screenshot" }, async ({ page }) => {
+        // Need to wait for 2 to appear since playwright only evaluates 'first()' initially, so the waiting won't work


Need to wait for 2 to appear since playwright only evaluates 'first()' initially, so the waiting won't work

wat?

…Key storage out of sync" toast (#29190) * Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast (#29138) * Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast * Unused import & fix test * Test 'forgot' variant * Fix dependencies * Add more toast tests * Unused import * Test initialState in Encryption Tab * Let's see if github has any more luck running this test than me * Working playwright test with screenshot * year * Convert playwright test to use the bot client * Disambiguate Co-authored-by: Florian Duros <[email protected]> * Add doc & do other part of rename * Split out into custom hook * Fix tests --------- Co-authored-by: Florian Duros <[email protected]> (cherry picked from commit 9657d39) * Update fetchdep.sh to understand merge queues --------- Co-authored-by: David Baker <[email protected]> Co-authored-by: Michael Telatynski <[email protected]>

Wire up the "Forgot recovery key" button for the "Key storage out of …

b62aebb

…sync" toast

dbkr added the T-Defect label Jan 29, 2025

github-actions bot deployed to Netlify January 29, 2025 18:49 View deployment

github-actions bot deployed to EndToEndTests January 29, 2025 18:56 View deployment

dbkr added 2 commits January 30, 2025 13:01

Unused import & fix test

8b2aaa8

Test 'forgot' variant

2f0c5d2

github-actions bot deployed to Netlify January 30, 2025 13:06 View deployment

Fix dependencies

33abac3

github-actions bot deployed to Netlify January 30, 2025 13:12 View deployment

github-actions bot deployed to EndToEndTests January 30, 2025 13:20 View deployment

Add more toast tests

d08b013

github-actions bot deployed to Netlify January 30, 2025 13:50 View deployment

Unused import

4d99bef

github-actions bot deployed to Netlify January 30, 2025 13:54 View deployment

github-actions bot deployed to EndToEndTests January 30, 2025 14:02 View deployment

Test initialState in Encryption Tab

bc48eac

github-actions bot deployed to Netlify January 30, 2025 14:29 View deployment

github-actions bot deployed to EndToEndTests January 30, 2025 14:35 View deployment

dbkr added 2 commits January 31, 2025 12:26

Let's see if github has any more luck running this test than me

7ec1f01

Working playwright test with screenshot

823a05e

github-actions bot deployed to Netlify January 31, 2025 12:36 View deployment

github-actions bot deployed to EndToEndTests January 31, 2025 12:44 View deployment

year

6df7df9

dbkr marked this pull request as ready for review January 31, 2025 13:05

dbkr requested review from a team as code owners January 31, 2025 13:05

dbkr requested review from t3chguy, florianduros and uhoreg January 31, 2025 13:05

github-actions bot deployed to Netlify January 31, 2025 13:07 View deployment

github-actions bot deployed to EndToEndTests January 31, 2025 13:14 View deployment

florianduros requested changes Jan 31, 2025

View reviewed changes

uhoreg approved these changes Feb 1, 2025

View reviewed changes

playwright/e2e/crypto/toasts.spec.ts Outdated Show resolved Hide resolved

dbkr and others added 5 commits February 4, 2025 14:36

Convert playwright test to use the bot client

5508c97

Disambiguate

a612305

Co-authored-by: Florian Duros <[email protected]>

Add doc & do other part of rename

939939a

Split out into custom hook

711e4f7

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

6681772

…tofsync_forgotpassword

github-actions bot deployed to Netlify February 4, 2025 16:15 View deployment

florianduros approved these changes Feb 4, 2025

View reviewed changes

github-actions bot deployed to EndToEndTests February 4, 2025 16:29 View deployment

Fix tests

60d1fa1

github-actions bot deployed to Netlify February 4, 2025 17:13 View deployment

github-actions bot deployed to EndToEndTests February 4, 2025 17:20 View deployment

dbkr added this pull request to the merge queue Feb 4, 2025

Merged via the queue into develop with commit 9657d39 Feb 4, 2025
31 checks passed

dbkr deleted the dbkr/keystorage_outofsync_forgotpassword branch February 4, 2025 17:52

dbkr added the backport staging Label to automatically backport PR to staging branch label Feb 4, 2025

RiotRobot mentioned this pull request Feb 4, 2025

[Backport staging] Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast #29190

Merged

richvdh reviewed Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast #29138

Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast #29138

dbkr commented Jan 29, 2025

florianduros Jan 31, 2025

dbkr Feb 4, 2025

florianduros Jan 31, 2025

dbkr Feb 4, 2025

florianduros Jan 31, 2025

florianduros Jan 31, 2025

dbkr Feb 4, 2025

uhoreg left a comment

richvdh commented Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

richvdh Feb 5, 2025

Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast #29138

Wire up the "Forgot recovery key" button for the "Key storage out of sync" toast #29138

Conversation

dbkr commented Jan 29, 2025

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg left a comment

Choose a reason for hiding this comment

richvdh commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment