Skip to content

Stop showing a dialog prompting the user to enter an old recovery key #29143

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 30, 2025
Merged

Stop showing a dialog prompting the user to enter an old recovery key #29143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
607e025
SecurityManager: improve logging
richvdh Jan 29, 2025
27c65b5
Only prompt user for default 4S key
richvdh Jan 30, 2025
3d394fb
playwright: factor out helper for setting up 4S
richvdh Jan 29, 2025
dbd3417
Playwright test for dehydrated device in reset flow
richvdh Jan 30, 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
16 changes: 3 additions & 13 deletions playwright/e2e/crypto/backups.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { type Page } from "@playwright/test";

import { test, expect } from "../../element-web-test";
import { isDendrite } from "../../plugins/homeserver/dendrite";
import { completeCreateSecretStorageDialog } from "./utils.ts";

async function expectBackupVersionToBe(page: Page, version: string) {
await expect(page.locator(".mx_SecureBackupPanel_statusList tr:nth-child(5) td")).toHaveText(
Expand All @@ -35,19 +36,7 @@ test.describe("Backups", () => {
await expect(securityTab.getByRole("heading", { name: "Secure Backup" })).toBeVisible();
await securityTab.getByRole("button", { name: "Set up", exact: true }).click();

const currentDialogLocator = page.locator(".mx_Dialog");

// It's the first time and secure storage is not set up, so it will create one
await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
// copy the recovery key to use it later
const securityKey = await app.getClipboard();
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();

await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
const securityKey = await completeCreateSecretStorageDialog(page);

// Open the settings again
await app.settings.openUserSettings("Security & Privacy");
Expand All @@ -62,6 +51,7 @@ test.describe("Backups", () => {
await expectBackupVersionToBe(page, "1");

await securityTab.getByRole("button", { name: "Delete Backup", exact: true }).click();
const currentDialogLocator = page.locator(".mx_Dialog");
await expect(currentDialogLocator.getByRole("heading", { name: "Delete Backup" })).toBeVisible();
// Delete it
await currentDialogLocator.getByTestId("dialog-primary-button").click(); // Click "Delete Backup"
Expand Down
22 changes: 9 additions & 13 deletions playwright/e2e/crypto/crypto.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ Please see LICENSE files in the repository root for full details.

import type { Page } from "@playwright/test";
import { expect, test } from "../../element-web-test";
import { autoJoin, copyAndContinue, createSharedRoomWithUser, enableKeyBackup, verify } from "./utils";
import {
autoJoin,
completeCreateSecretStorageDialog,
copyAndContinue,
createSharedRoomWithUser,
enableKeyBackup,
verify,
} from "./utils";
import { Bot } from "../../pages/bot";
import { ElementAppPage } from "../../pages/ElementAppPage";
import { isDendrite } from "../../plugins/homeserver/dendrite";
Expand Down Expand Up @@ -111,18 +118,7 @@ test.describe("Cryptography", function () {
await app.settings.openUserSettings("Security & Privacy");
await page.getByRole("button", { name: "Set up Secure Backup" }).click();

const dialog = page.locator(".mx_Dialog");
// Recovery key is selected by default
await dialog.getByRole("button", { name: "Continue" }).click();
await copyAndContinue(page);

// If the device is unverified, there should be a "Setting up keys" step; however, it
// can be quite quick, and playwright can miss it, so we can't test for it.

// Either way, we end up at a success dialog:
await expect(dialog.getByText("Secure Backup successful")).toBeVisible();
await dialog.getByRole("button", { name: "Done" }).click();
await expect(dialog.getByText("Secure Backup successful")).not.toBeVisible();
await completeCreateSecretStorageDialog(page);

// Verify that the SSSS keys are in the account data stored in the server
await verifyKey(app, "master");
Expand Down
61 changes: 49 additions & 12 deletions playwright/e2e/crypto/dehydration.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { Locator, type Page } from "@playwright/test";
import { test, expect } from "../../element-web-test";
import { viewRoomSummaryByName } from "../right-panel/utils";
import { isDendrite } from "../../plugins/homeserver/dendrite";
import { completeCreateSecretStorageDialog, createBot, logIntoElement } from "./utils.ts";
import { Client } from "../../pages/client.ts";

const ROOM_NAME = "Test room";
const NAME = "Alice";
Expand Down Expand Up @@ -44,7 +46,7 @@ test.use({
test.describe("Dehydration", () => {
test.skip(isDendrite, "does not yet support dehydration v2");

test("Create dehydrated device", async ({ page, user, app }, workerInfo) => {
test("'Set up secure backup' creates dehydrated device", async ({ page, user, app }, workerInfo) => {
// Create a backup (which will create SSSS, and dehydrated device)

const securityTab = await app.settings.openUserSettings("Security & Privacy");
Expand All @@ -53,17 +55,7 @@ test.describe("Dehydration", () => {
await expect(securityTab.getByText("Offline device enabled")).not.toBeVisible();
await securityTab.getByRole("button", { name: "Set up", exact: true }).click();

const currentDialogLocator = page.locator(".mx_Dialog");

// It's the first time and secure storage is not set up, so it will create one
await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();

await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
await completeCreateSecretStorageDialog(page);

// Open the settings again
await app.settings.openUserSettings("Security & Privacy");
Expand Down Expand Up @@ -96,4 +88,49 @@ test.describe("Dehydration", () => {
await expect(page.locator(".mx_UserInfo_devices").getByText("Offline device enabled")).toBeVisible();
await expect(page.locator(".mx_UserInfo_devices").getByText("Dehydrated device")).not.toBeVisible();
});

test("Reset recovery key during login re-creates dehydrated device", async ({
page,
homeserver,
app,
credentials,
}) => {
// Set up cross-signing and recovery
const { botClient } = await createBot(page, homeserver, credentials);
// ... and dehydration
await botClient.evaluate(async (client) => await client.getCrypto().startDehydration());

const initialDehydratedDeviceIds = await getDehydratedDeviceIds(botClient);
expect(initialDehydratedDeviceIds.length).toBe(1);

await botClient.evaluate(async (client) => client.stopClient());

// Log in our client
await logIntoElement(page, credentials);

// Oh no, we forgot our recovery key
await page.locator(".mx_AuthPage").getByRole("button", { name: "Reset all" }).click();
await page.locator(".mx_AuthPage").getByRole("button", { name: "Proceed with reset" }).click();

await completeCreateSecretStorageDialog(page, { accountPassword: credentials.password });

// There should be a brand new dehydrated device
const dehydratedDeviceIds = await getDehydratedDeviceIds(app.client);
expect(dehydratedDeviceIds.length).toBe(1);
expect(dehydratedDeviceIds[0]).not.toEqual(initialDehydratedDeviceIds[0]);
});
});

async function getDehydratedDeviceIds(client: Client): Promise<string[]> {
return await client.evaluate(async (client) => {
const userId = client.getUserId();
const devices = await client.getCrypto().getUserDeviceInfo([userId]);
return Array.from(
devices
.get(userId)
.values()
.filter((d) => d.dehydrated)
.map((d) => d.deviceId),
);
});
}
53 changes: 43 additions & 10 deletions playwright/e2e/crypto/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,19 +288,52 @@ export async function doTwoWaySasVerification(page: Page, verifier: JSHandle<Ver
export async function enableKeyBackup(app: ElementAppPage): Promise<string> {
await app.settings.openUserSettings("Security & Privacy");
await app.page.getByRole("button", { name: "Set up Secure Backup" }).click();
const dialog = app.page.locator(".mx_Dialog");
// Recovery key is selected by default
await dialog.getByRole("button", { name: "Continue" }).click({ timeout: 60000 });

// copy the text ourselves
const securityKey = await dialog.locator(".mx_CreateSecretStorageDialog_recoveryKey code").textContent();
await copyAndContinue(app.page);
return await completeCreateSecretStorageDialog(app.page);
}

/**
* Go through the "Set up Secure Backup" dialog (aka the `CreateSecretStorageDialog`).
*
* Assumes the dialog is already open for some reason (see also {@link enableKeyBackup}).
*
* @param page - The playwright `Page` fixture.
* @param opts - Options object
* @param opts.accountPassword - The user's account password. If we are also resetting cross-signing, then we will need
* to upload the public cross-signing keys, which will cause the app to prompt for the password.
*
* @returns the new recovery key.
*/
export async function completeCreateSecretStorageDialog(
page: Page,
opts?: { accountPassword?: string },
): Promise<string> {
const currentDialogLocator = page.locator(".mx_Dialog");

await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
// "Generate a Security Key" is selected by default
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
// copy the recovery key to use it later
const recoveryKey = await page.evaluate(() => navigator.clipboard.readText());
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();

// If the device is unverified, there should be a "Setting up keys" step.
// If this is not the first time we are setting up cross-signing, the app will prompt for our password; otherwise
// the step is quite quick, and playwright can miss it, so we can't test for it.
if (opts && Object.hasOwn(opts, "accountPassword")) {
await expect(currentDialogLocator.getByRole("heading", { name: "Setting up keys" })).toBeVisible();
await page.getByPlaceholder("Password").fill(opts!.accountPassword);
await currentDialogLocator.getByRole("button", { name: "Continue" }).click();
}

await expect(dialog.getByText("Secure Backup successful")).toBeVisible();
await dialog.getByRole("button", { name: "Done" }).click();
await expect(dialog.getByText("Secure Backup successful")).not.toBeVisible();
// Either way, we end up at a success dialog:
await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
await expect(currentDialogLocator.getByText("Secure Backup successful")).not.toBeVisible();

return securityKey;
return recoveryKey;
}

/**
Expand Down
66 changes: 38 additions & 28 deletions src/SecurityManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Please see LICENSE files in the repository root for full details.
import { lazy } from "react";
import { SecretStorage } from "matrix-js-sdk/src/matrix";
import { deriveRecoveryKeyFromPassphrase, decodeRecoveryKey, CryptoCallbacks } from "matrix-js-sdk/src/crypto-api";
import { logger } from "matrix-js-sdk/src/logger";
import { logger as rootLogger } from "matrix-js-sdk/src/logger";

import Modal from "./Modal";
import { MatrixClientPeg } from "./MatrixClientPeg";
Expand All @@ -29,6 +29,8 @@ let secretStorageKeys: Record<string, Uint8Array> = {};
let secretStorageKeyInfo: Record<string, SecretStorage.SecretStorageKeyDescription> = {};
let secretStorageBeingAccessed = false;

const logger = rootLogger.getChild("SecurityManager:");

/**
* This can be used by other components to check if secret storage access is in
* progress, so that we can e.g. avoid intermittently showing toasts during
Expand Down Expand Up @@ -70,33 +72,34 @@ function makeInputToKey(
};
}

async function getSecretStorageKey({
keys: keyInfos,
}: {
keys: Record<string, SecretStorage.SecretStorageKeyDescription>;
}): Promise<[string, Uint8Array]> {
async function getSecretStorageKey(
{
keys: keyInfos,
}: {
keys: Record<string, SecretStorage.SecretStorageKeyDescription>;
},
secretName: string,
): Promise<[string, Uint8Array]> {
const cli = MatrixClientPeg.safeGet();
let keyId = await cli.secretStorage.getDefaultKeyId();
let keyInfo!: SecretStorage.SecretStorageKeyDescription;
if (keyId) {
// use the default SSSS key if set
keyInfo = keyInfos[keyId];
if (!keyInfo) {
// if the default key is not available, pretend the default key
// isn't set
keyId = null;
}
}
if (!keyId) {
// if no default SSSS key is set, fall back to a heuristic of using the
const defaultKeyId = await cli.secretStorage.getDefaultKeyId();

let keyId: string;
// If the defaultKey is useful, use that
if (defaultKeyId && keyInfos[defaultKeyId]) {
keyId = defaultKeyId;
} else {
// Fall back to a heuristic of using the
// only available key, if only one key is set
const keyInfoEntries = Object.entries(keyInfos);
if (keyInfoEntries.length > 1) {
const usefulKeys = Object.keys(keyInfos);
if (usefulKeys.length > 1) {
throw new Error("Multiple storage key requests not implemented");
}
[keyId, keyInfo] = keyInfoEntries[0];
keyId = usefulKeys[0];
}
logger.debug(`getSecretStorageKey: request for 4S keys [${Object.keys(keyInfos)}]: looking for key ${keyId}`);
const keyInfo = keyInfos[keyId];
logger.debug(
`getSecretStorageKey: request for 4S keys [${Object.keys(keyInfos)}] for secret \`${secretName}\`: looking for key ${keyId}`,
);

// Check the in-memory cache
if (secretStorageBeingAccessed && secretStorageKeys[keyId]) {
Expand All @@ -106,12 +109,18 @@ async function getSecretStorageKey({

const keyFromCustomisations = ModuleRunner.instance.extensions.cryptoSetup.getSecretStorageKey();
if (keyFromCustomisations) {
logger.log("getSecretStorageKey: Using secret storage key from CryptoSetupExtension");
logger.debug("getSecretStorageKey: Using secret storage key from CryptoSetupExtension");
cacheSecretStorageKey(keyId, keyInfo, keyFromCustomisations);
return [keyId, keyFromCustomisations];
}

logger.debug("getSecretStorageKey: prompting user for key");
// We only prompt the user for the default key
if (keyId !== defaultKeyId) {
logger.debug(`getSecretStorageKey: request for non-default key ${keyId}: not prompting user`);
throw new Error("Request for non-default 4S key");
}

logger.debug(`getSecretStorageKey: prompting user for key ${keyId}`);
const inputToKey = makeInputToKey(keyInfo);
const { finished } = Modal.createDialog(
AccessSecretStorageDialog,
Expand Down Expand Up @@ -139,7 +148,7 @@ async function getSecretStorageKey({
if (!keyParams) {
throw new AccessCancelledError();
}
logger.debug("getSecretStorageKey: got key from user");
logger.debug(`getSecretStorageKey: got key ${keyId} from user`);
const key = await inputToKey(keyParams);

// Save to cache to avoid future prompts in the current session
Expand All @@ -154,6 +163,7 @@ function cacheSecretStorageKey(
key: Uint8Array,
): void {
if (secretStorageBeingAccessed) {
logger.debug(`Caching 4S key ${keyId}`);
secretStorageKeys[keyId] = key;
secretStorageKeyInfo[keyId] = keyInfo;
}
Expand All @@ -173,13 +183,13 @@ export const crossSigningCallbacks: CryptoCallbacks = {
* @param func - The operation to be wrapped.
*/
export async function withSecretStorageKeyCache<T>(func: () => Promise<T>): Promise<T> {
logger.debug("SecurityManager: enabling 4S key cache");
logger.debug("enabling 4S key cache");
secretStorageBeingAccessed = true;
try {
return await func();
} finally {
// Clear secret storage key cache now that work is complete
logger.debug("SecurityManager: disabling 4S key cache");
logger.debug("disabling 4S key cache");
secretStorageBeingAccessed = false;
secretStorageKeys = {};
secretStorageKeyInfo = {};
Expand Down
Loading
Loading