Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Recovery section in the new user settings Encryption tab #28673

Open
wants to merge 40 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
3e77b3d
Refine `SettingsSection` & `SettingsTab`
florianduros Dec 4, 2024
ae623f8
Add encryption tab
florianduros Dec 4, 2024
f9e48b4
Add recovery section
florianduros Dec 6, 2024
0057f57
Add device verification
florianduros Dec 12, 2024
bb507b0
Rename `Panel` into `State`
florianduros Dec 13, 2024
1aace3f
Update & add tests to user settings common
florianduros Dec 13, 2024
70c084e
Add tests to `RecoveryPanel`
florianduros Dec 18, 2024
7193998
Add tests to `ChangeRecoveryKey`
florianduros Dec 18, 2024
fec324e
Update CreateSecretStorageDialog-test snapshot
florianduros Dec 16, 2024
44c6bce
Add tests to `EncryptionUserSettingsTab`
florianduros Dec 16, 2024
075f6dc
Update existing screenshots of e2e tests
florianduros Dec 16, 2024
895ad88
Add new encryption tab ownership to `@element-hq/element-crypto-web-r…
florianduros Dec 17, 2024
ba032a7
Add e2e tests
florianduros Dec 18, 2024
7909ac9
Fix monospace font and add figma link to hardcoded value
florianduros Dec 19, 2024
618557c
Add unit to Icon
florianduros Dec 19, 2024
c805cd8
Merge branch 'develop' into florianduros/encryption-tab
florianduros Dec 19, 2024
7a372f7
Merge branch 'develop' into florianduros/encryption-tab
florianduros Dec 23, 2024
b20579d
Improve e2e doc
florianduros Dec 23, 2024
24c537c
Assert that the crypto module is defined
florianduros Jan 6, 2025
72adfa5
Add classname doc
florianduros Jan 6, 2025
36c7e0e
Fix typo
florianduros Jan 6, 2025
52076f1
Use `good` state instead of default
florianduros Jan 6, 2025
a0d904e
Rename `ChangeRecoveryKey.isSetupFlow` into `ChangeRecoveryKey.userHa…
florianduros Jan 6, 2025
1a0e6dc
Move `deleteCachedSecrets` fixture in `recovery.spec.ts`
florianduros Jan 6, 2025
0b254e5
Use one callback instead of two in `RecoveryPanel`
florianduros Jan 6, 2025
6f236bd
Fix docs and naming of `utils.createBot`
florianduros Jan 8, 2025
82bf2cc
Fix typo in `RecoveryPanel`
florianduros Jan 8, 2025
84d11f8
Add more doc to the state of the `EncryptionUserSettingsTab`
florianduros Jan 8, 2025
2fe5555
Rename `verification_required` into `set_up_encryption`
florianduros Jan 8, 2025
4b365ba
Merge branch 'develop' into florianduros/encryption-tab
florianduros Jan 8, 2025
8a9291a
Update test
florianduros Jan 8, 2025
1c00502
ADd new license
florianduros Jan 8, 2025
dc940f5
Update comments and doc
florianduros Jan 9, 2025
521cebf
Assert that `recoveryKey.encodedPrivateKey` is always defined
florianduros Jan 9, 2025
7af44cc
Add comments to explain how the secrets could be uncached
florianduros Jan 9, 2025
8bd5d6a
Use `matrixClient.secretStorage.getDefaultKeyId` instead of `matrixCl…
florianduros Jan 9, 2025
086f28e
Merge branch 'develop' into florianduros/encryption-tab
florianduros Jan 9, 2025
0c18708
Update existing screenshot to add encryption tab.
florianduros Jan 9, 2025
0a52b7c
Update tests
florianduros Jan 9, 2025
e5dea48
Use new labels when changing the recovery key
florianduros Jan 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion playwright/e2e/crypto/device-verification.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ test.describe("Device verification", { tag: "@no-webkit" }, () => {

test.beforeEach(async ({ page, homeserver, credentials }) => {
const res = await createBot(page, homeserver, credentials);
aliceBotClient = res.aliceBotClient;
aliceBotClient = res.botClient;
expectedBackupVersion = res.expectedBackupVersion;
});

Expand Down
16 changes: 8 additions & 8 deletions playwright/e2e/crypto/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { ElementAppPage } from "../../pages/ElementAppPage";
import { Bot } from "../../pages/bot";

/**
* Create a new device for the Alice user.
* Create a bot client and wait for the key backup to be ready.
* This function will wait for the key backup to be ready.
* @param page - the page to use
florianduros marked this conversation as resolved.
Show resolved Hide resolved
* @param homeserver - the homeserver to use
Expand All @@ -34,21 +34,21 @@ export async function createBot(
page: Page,
homeserver: HomeserverInstance,
credentials: Credentials,
): Promise<{ aliceBotClient: Bot; recoveryKey: GeneratedSecretStorageKey; expectedBackupVersion: string }> {
): Promise<{ botClient: Bot; recoveryKey: GeneratedSecretStorageKey; expectedBackupVersion: string }> {
// Visit the login page of the app, to load the matrix sdk
await page.goto("/#/login");

// wait for the page to load
await page.waitForSelector(".mx_AuthPage", { timeout: 30000 });

// Create a new device for alice
const aliceBotClient = new Bot(page, homeserver, {
// Create a new bot client
const botClient = new Bot(page, homeserver, {
bootstrapCrossSigning: true,
bootstrapSecretStorage: true,
});
aliceBotClient.setCredentials(credentials);
botClient.setCredentials(credentials);
// Backup is prepared in the background. Poll until it is ready.
const botClientHandle = await aliceBotClient.prepareClient();
const botClientHandle = await botClient.prepareClient();
let expectedBackupVersion: string;
await expect
.poll(async () => {
Expand All @@ -59,9 +59,9 @@ export async function createBot(
})
.not.toBe(null);

const recoveryKey = await aliceBotClient.getRecoveryKey();
const recoveryKey = await botClient.getRecoveryKey();

return { aliceBotClient, recoveryKey, expectedBackupVersion };
return { botClient, recoveryKey, expectedBackupVersion };
}

/**
Expand Down
23 changes: 1 addition & 22 deletions playwright/e2e/settings/encryption-user-tab/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

Expand Down Expand Up @@ -94,25 +94,4 @@ class Helpers {
await dialog.getByRole("button", { name: "Finish set up" }).click();
await expect(dialog).toMatchScreenshot("default-recovery.png");
}

/**
* Remove the cached secrets from the indexedDB
*/
async deleteCachedSecrets() {
await this.page.evaluate(async () => {
const removeCachedSecrets = new Promise((resolve) => {
const request = window.indexedDB.open("matrix-js-sdk::matrix-sdk-crypto");
request.onsuccess = async (event: Event & { target: { result: IDBDatabase } }) => {
const db = event.target.result;
const request = db.transaction("core", "readwrite").objectStore("core").delete("private_identity");
request.onsuccess = () => {
db.close();
resolve(undefined);
};
};
});
await removeCachedSecrets;
});
await this.page.reload();
}
}
29 changes: 26 additions & 3 deletions playwright/e2e/settings/encryption-user-tab/recovery.spec.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

import { GeneratedSecretStorageKey } from "matrix-js-sdk/src/crypto-api";
import { Page } from "@playwright/test";

import { test, expect } from ".";
import {
Expand Down Expand Up @@ -80,7 +81,7 @@ test.describe("Recovery section in Encryption tab", () => {
await verifySession(app, "new passphrase");
await util.deleteKeyBackup(expectedBackupVersion);

// The key backup is deleted and the user needs to set up it
// The key backup is deleted and the user needs to set it up
const dialog = await util.openEncryptionTab();
const setupButton = dialog.getByRole("button", { name: "Set up recovery" });
await expect(setupButton).toBeVisible();
Expand Down Expand Up @@ -114,7 +115,7 @@ test.describe("Recovery section in Encryption tab", () => {
async ({ page, app, util }) => {
await verifySession(app, "new passphrase");
// We need to delete the cached secrets
await util.deleteCachedSecrets();
await deleteCachedSecrets(page);

await util.openEncryptionTab();
// We ask the user to enter the recovery key
Expand All @@ -138,3 +139,25 @@ test.describe("Recovery section in Encryption tab", () => {
},
);
});

/**
* Remove the cached secrets from the indexedDB
* This is a workaround to simulate the case where the secrets are not cached.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but do we know what could cause this to happen in practice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I just know that EX is checking it and we need to do it too, cf this discussion.

(Why I'm doing it that way)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments

*/
async function deleteCachedSecrets(page: Page) {
await page.evaluate(async () => {
const removeCachedSecrets = new Promise((resolve) => {
const request = window.indexedDB.open("matrix-js-sdk::matrix-sdk-crypto");
request.onsuccess = async (event: Event & { target: { result: IDBDatabase } }) => {
const db = event.target.result;
const request = db.transaction("core", "readwrite").objectStore("core").delete("private_identity");
request.onsuccess = () => {
db.close();
resolve(undefined);
};
};
});
await removeCachedSecrets;
});
await page.reload();
}
2 changes: 1 addition & 1 deletion res/css/views/settings/_SettingsHeader.pcss
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

Expand Down
2 changes: 1 addition & 1 deletion res/css/views/settings/_SettingsSubheader.pcss
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

Expand Down
2 changes: 1 addition & 1 deletion res/css/views/settings/encryption/_EncryptionCard.pcss
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

Expand Down
2 changes: 1 addition & 1 deletion src/components/views/settings/SettingsHeader.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

Expand Down
2 changes: 1 addition & 1 deletion src/components/views/settings/SettingsSubheader.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

Expand Down
28 changes: 11 additions & 17 deletions src/components/views/settings/encryption/ChangeRecoveryKey.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,17 @@ import { withSecretStorageKeyCache } from "../../../../SecurityManager";
* The possible states of the component.
* - `inform_user`: The user is informed about the recovery key.
* - `save_key_setup_flow`: The user is asked to save the new recovery key during the setup flow.
* - `save_key_change_flow`: The user is asked to save the new recovery key during the chang key flow.
* - `save_key_change_flow`: The user is asked to save the new recovery key during the change key flow.
* - `confirm`: The user is asked to confirm the new recovery key.
*/
type State = "inform_user" | "save_key_setup_flow" | "save_key_change_flow" | "confirm";

interface ChangeRecoveryKeyProps {
/**
* If true, the component will display the flow to set up a new recovery key.
* If false, the component will display the flow to change the recovery key.
* If true, the component will display the flow to change the recovery key.
* If false,the component will display the flow to set up a new recovery key.
*/
isSetupFlow: boolean;
userHasKeyBackup: boolean;
/**
* Called when the recovery key is successfully changed.
*/
Expand All @@ -56,22 +56,16 @@ interface ChangeRecoveryKeyProps {
* A component to set up or change the recovery key.
*/
export function ChangeRecoveryKey({
isSetupFlow,
userHasKeyBackup,
onFinish,
onCancelClick,
}: ChangeRecoveryKeyProps): JSX.Element | null {
const matrixClient = useMatrixClientContext();

const [state, setState] = useState<State>(isSetupFlow ? "inform_user" : "save_key_change_flow");
const [state, setState] = useState<State>(userHasKeyBackup ? "save_key_change_flow" : "inform_user");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const [state, setState] = useState<State>(userHasKeyBackup ? "save_key_change_flow" : "inform_user");
// If the user is setting up recovery for the first time, we first show them a panel explaining what
// "recovery" is about. Otherwise, we jump straight to showing the user the new key.
const [state, setState] = useState<State>(userHasKeyBackup ? "save_key_change_flow" : "inform_user");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Isn't there supposed to be another panel, if the user is changing key? https://www.figma.com/design/qTWRfItpO3RdCjnTKPu4mL/Settings?node-id=375-77065&t=nVcSrBnetlb3Tg9R-0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Isn't there supposed to be another panel, if the user is changing key? figma.com/design/qTWRfItpO3RdCjnTKPu4mL/Settings?node-id=375-77065&t=nVcSrBnetlb3Tg9R-0)

No, we are only displaying when setting up a recovery key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, per https://www.figma.com/design/qTWRfItpO3RdCjnTKPu4mL?node-id=2442-19834#1085657422, I'm not really sure what the point of that step is

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm displaying the new recovery key in the field during the "set up a recovery key" and "change recovery key" flows

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The figma designs contain an extra screen. But per the figma thread, we're not clear if that screen is useful.


// We create a new recovery key, the recovery key will be displayed to the user
const recoveryKey = useAsyncMemo(() => {
const crypto = matrixClient.getCrypto();
if (!crypto) return Promise.resolve(undefined);

return crypto.createRecoveryKeyFromPassphrase();
}, []);

const recoveryKey = useAsyncMemo(() => matrixClient.getCrypto()!.createRecoveryKeyFromPassphrase(), []);
if (!recoveryKey?.encodedPrivateKey) return null;
florianduros marked this conversation as resolved.
Show resolved Hide resolved

let content: JSX.Element;
Expand Down Expand Up @@ -108,7 +102,7 @@ export function ChangeRecoveryKey({
// when we will try to access the secret storage during the bootstrap
await withSecretStorageKeyCache(() =>
crypto.bootstrapSecretStorage({
setupNewKeyBackup: isSetupFlow,
setupNewKeyBackup: !userHasKeyBackup,
setupNewSecretStorage: true,
createSecretStorageKey: async () => recoveryKey,
}),
Expand All @@ -124,9 +118,9 @@ export function ChangeRecoveryKey({

const pages = [
_t("settings|encryption|title"),
isSetupFlow
? _t("settings|encryption|recovery|set_up_recovery")
: _t("settings|encryption|recovery|change_recovery_key"),
userHasKeyBackup
? _t("settings|encryption|recovery|change_recovery_key")
: _t("settings|encryption|recovery|set_up_recovery"),
];
const labels = getLabels(state);

Expand Down
25 changes: 10 additions & 15 deletions src/components/views/settings/encryption/RecoveryPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

import React, { JSX, MouseEventHandler, useCallback, useEffect, useState } from "react";
import React, { JSX, useCallback, useEffect, useState } from "react";
import { Button, InlineSpinner } from "@vector-im/compound-web";
import KeyIcon from "@vector-im/compound-design-tokens/assets/web/icons/key";

Expand All @@ -27,27 +27,22 @@ type State = "loading" | "missing_backup" | "secrets_not_cached" | "good";

interface RecoveryPanelProps {
/**
* Callback for when the user clicks the button to set up their recovery key.
* Callback for when the user wants to set up or change their recovery key.
*/
onSetUpRecoveryClick: MouseEventHandler<HTMLButtonElement>;
/**
* Callback for when the user clicks the button to change their recovery key.
*/
onChangingRecoveryKeyClick: MouseEventHandler<HTMLButtonElement>;
onChangeRecoveryKeyClick: (setupNewKey: boolean) => void;
}

/**
* This component allows the user to set up or change their recovery key.
*/
export function RecoveryPanel({ onSetUpRecoveryClick, onChangingRecoveryKeyClick }: RecoveryPanelProps): JSX.Element {
export function RecoveryPanel({ onChangeRecoveryKeyClick }: RecoveryPanelProps): JSX.Element {
const [state, setState] = useState<State>("loading");
const isMissingBackup = state === "missing_backup";

const matrixClient = useMatrixClientContext();

const checkEncryption = useCallback(async () => {
const crypto = matrixClient.getCrypto();
if (!crypto) return;
const crypto = matrixClient.getCrypto()!;

// Check if the user has a backup
const hasBackup = Boolean(await crypto.checkKeyBackupAndEnable());
Expand All @@ -72,7 +67,7 @@ export function RecoveryPanel({ onSetUpRecoveryClick, onChangingRecoveryKeyClick
break;
case "missing_backup":
content = (
<Button size="sm" kind="primary" Icon={KeyIcon} onClick={onSetUpRecoveryClick}>
<Button size="sm" kind="primary" Icon={KeyIcon} onClick={() => onChangeRecoveryKeyClick(true)}>
{_t("settings|encryption|recovery|set_up_recovery")}
</Button>
);
Expand All @@ -89,9 +84,9 @@ export function RecoveryPanel({ onSetUpRecoveryClick, onChangingRecoveryKeyClick
</Button>
);
break;
default:
case "good":
content = (
<Button size="sm" kind="secondary" Icon={KeyIcon} onClick={onChangingRecoveryKeyClick}>
<Button size="sm" kind="secondary" Icon={KeyIcon} onClick={() => onChangeRecoveryKeyClick(false)}>
{_t("settings|encryption|recovery|change_recovery_key")}
</Button>
);
Expand Down Expand Up @@ -121,7 +116,7 @@ interface SubheaderProps {
* The subheader for the recovery panel.
*/
function Subheader({ state }: SubheaderProps): JSX.Element {
// If we the secrets are not cached, we display a warning message.
// If the secrets are not cached, we display a warning message.
if (state !== "secrets_not_cached") return <>{_t("settings|encryption|recovery|description")}</>;

return (
Expand Down
4 changes: 4 additions & 0 deletions src/components/views/settings/tabs/SettingsTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import classNames from "classnames";

export interface SettingsTabProps extends HTMLAttributes<HTMLDivElement> {
children?: React.ReactNode;
/**
* Added to the root div
florianduros marked this conversation as resolved.
Show resolved Hide resolved
*/
className?: string;
}

/**
Expand Down
Loading
Loading