Skip to content

Commit 099c307

Browse files
richvdhrichvdh
authored
Stop showing a dialog prompting the user to enter an old recovery key (#29143)
* SecurityManager: improve logging * Only prompt user for default 4S key We don't really support the concept of having multiple 4S keys active, so prompting the user to enter a non-default 4S key without even telling them which one we want is rather silly. * playwright: factor out helper for setting up 4S We seem to already have about 5 copies of this code, so before I add another, let's factor it out. * Playwright test for dehydrated device in reset flow This should be fixed by the previous commit, so let's check it stays that way.
1 parent 12932e2 commit 099c307

File tree

6 files changed

+232
-78
lines changed

6 files changed

+232
-78
lines changed

‎playwright/e2e/crypto/backups.spec.ts

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { type Page } from "@playwright/test";
1010

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

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

38-
const currentDialogLocator = page.locator(".mx_Dialog");
39-
40-
// It's the first time and secure storage is not set up, so it will create one
41-
await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
42-
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
43-
await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
44-
await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
45-
// copy the recovery key to use it later
46-
const securityKey = await app.getClipboard();
47-
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
48-
49-
await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
50-
await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
39+
const securityKey = await completeCreateSecretStorageDialog(page);
5140

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

6453
await securityTab.getByRole("button", { name: "Delete Backup", exact: true }).click();
54+
const currentDialogLocator = page.locator(".mx_Dialog");
6555
await expect(currentDialogLocator.getByRole("heading", { name: "Delete Backup" })).toBeVisible();
6656
// Delete it
6757
await currentDialogLocator.getByTestId("dialog-primary-button").click(); // Click "Delete Backup"

‎playwright/e2e/crypto/crypto.spec.ts

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,14 @@ Please see LICENSE files in the repository root for full details.
88

99
import type { Page } from "@playwright/test";
1010
import { expect, test } from "../../element-web-test";
11-
import { autoJoin, copyAndContinue, createSharedRoomWithUser, enableKeyBackup, verify } from "./utils";
11+
import {
12+
autoJoin,
13+
completeCreateSecretStorageDialog,
14+
copyAndContinue,
15+
createSharedRoomWithUser,
16+
enableKeyBackup,
17+
verify,
18+
} from "./utils";
1219
import { Bot } from "../../pages/bot";
1320
import { ElementAppPage } from "../../pages/ElementAppPage";
1421
import { isDendrite } from "../../plugins/homeserver/dendrite";
@@ -111,18 +118,7 @@ test.describe("Cryptography", function () {
111118
await app.settings.openUserSettings("Security & Privacy");
112119
await page.getByRole("button", { name: "Set up Secure Backup" }).click();
113120

114-
const dialog = page.locator(".mx_Dialog");
115-
// Recovery key is selected by default
116-
await dialog.getByRole("button", { name: "Continue" }).click();
117-
await copyAndContinue(page);
118-
119-
// If the device is unverified, there should be a "Setting up keys" step; however, it
120-
// can be quite quick, and playwright can miss it, so we can't test for it.
121-
122-
// Either way, we end up at a success dialog:
123-
await expect(dialog.getByText("Secure Backup successful")).toBeVisible();
124-
await dialog.getByRole("button", { name: "Done" }).click();
125-
await expect(dialog.getByText("Secure Backup successful")).not.toBeVisible();
121+
await completeCreateSecretStorageDialog(page);
126122

127123
// Verify that the SSSS keys are in the account data stored in the server
128124
await verifyKey(app, "master");

‎playwright/e2e/crypto/dehydration.spec.ts

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import { Locator, type Page } from "@playwright/test";
1111
import { test, expect } from "../../element-web-test";
1212
import { viewRoomSummaryByName } from "../right-panel/utils";
1313
import { isDendrite } from "../../plugins/homeserver/dendrite";
14+
import { completeCreateSecretStorageDialog, createBot, logIntoElement } from "./utils.ts";
15+
import { Client } from "../../pages/client.ts";
1416

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

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

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

56-
const currentDialogLocator = page.locator(".mx_Dialog");
57-
58-
// It's the first time and secure storage is not set up, so it will create one
59-
await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
60-
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
61-
await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
62-
await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
63-
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
64-
65-
await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
66-
await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
58+
await completeCreateSecretStorageDialog(page);
6759

6860
// Open the settings again
6961
await app.settings.openUserSettings("Security & Privacy");
@@ -96,4 +88,49 @@ test.describe("Dehydration", () => {
9688
await expect(page.locator(".mx_UserInfo_devices").getByText("Offline device enabled")).toBeVisible();
9789
await expect(page.locator(".mx_UserInfo_devices").getByText("Dehydrated device")).not.toBeVisible();
9890
});
91+
92+
test("Reset recovery key during login re-creates dehydrated device", async ({
93+
page,
94+
homeserver,
95+
app,
96+
credentials,
97+
}) => {
98+
// Set up cross-signing and recovery
99+
const { botClient } = await createBot(page, homeserver, credentials);
100+
// ... and dehydration
101+
await botClient.evaluate(async (client) => await client.getCrypto().startDehydration());
102+
103+
const initialDehydratedDeviceIds = await getDehydratedDeviceIds(botClient);
104+
expect(initialDehydratedDeviceIds.length).toBe(1);
105+
106+
await botClient.evaluate(async (client) => client.stopClient());
107+
108+
// Log in our client
109+
await logIntoElement(page, credentials);
110+
111+
// Oh no, we forgot our recovery key
112+
await page.locator(".mx_AuthPage").getByRole("button", { name: "Reset all" }).click();
113+
await page.locator(".mx_AuthPage").getByRole("button", { name: "Proceed with reset" }).click();
114+
115+
await completeCreateSecretStorageDialog(page, { accountPassword: credentials.password });
116+
117+
// There should be a brand new dehydrated device
118+
const dehydratedDeviceIds = await getDehydratedDeviceIds(app.client);
119+
expect(dehydratedDeviceIds.length).toBe(1);
120+
expect(dehydratedDeviceIds[0]).not.toEqual(initialDehydratedDeviceIds[0]);
121+
});
99122
});
123+
124+
async function getDehydratedDeviceIds(client: Client): Promise<string[]> {
125+
return await client.evaluate(async (client) => {
126+
const userId = client.getUserId();
127+
const devices = await client.getCrypto().getUserDeviceInfo([userId]);
128+
return Array.from(
129+
devices
130+
.get(userId)
131+
.values()
132+
.filter((d) => d.dehydrated)
133+
.map((d) => d.deviceId),
134+
);
135+
});
136+
}

‎playwright/e2e/crypto/utils.ts

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -288,19 +288,52 @@ export async function doTwoWaySasVerification(page: Page, verifier: JSHandle<Ver
288288
export async function enableKeyBackup(app: ElementAppPage): Promise<string> {
289289
await app.settings.openUserSettings("Security & Privacy");
290290
await app.page.getByRole("button", { name: "Set up Secure Backup" }).click();
291-
const dialog = app.page.locator(".mx_Dialog");
292-
// Recovery key is selected by default
293-
await dialog.getByRole("button", { name: "Continue" }).click({ timeout: 60000 });
294291

295-
// copy the text ourselves
296-
const securityKey = await dialog.locator(".mx_CreateSecretStorageDialog_recoveryKey code").textContent();
297-
await copyAndContinue(app.page);
292+
return await completeCreateSecretStorageDialog(app.page);
293+
}
294+
295+
/**
296+
* Go through the "Set up Secure Backup" dialog (aka the `CreateSecretStorageDialog`).
297+
*
298+
* Assumes the dialog is already open for some reason (see also {@link enableKeyBackup}).
299+
*
300+
* @param page - The playwright `Page` fixture.
301+
* @param opts - Options object
302+
* @param opts.accountPassword - The user's account password. If we are also resetting cross-signing, then we will need
303+
* to upload the public cross-signing keys, which will cause the app to prompt for the password.
304+
*
305+
* @returns the new recovery key.
306+
*/
307+
export async function completeCreateSecretStorageDialog(
308+
page: Page,
309+
opts?: { accountPassword?: string },
310+
): Promise<string> {
311+
const currentDialogLocator = page.locator(".mx_Dialog");
312+
313+
await expect(currentDialogLocator.getByRole("heading", { name: "Set up Secure Backup" })).toBeVisible();
314+
// "Generate a Security Key" is selected by default
315+
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
316+
await expect(currentDialogLocator.getByRole("heading", { name: "Save your Security Key" })).toBeVisible();
317+
await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();
318+
// copy the recovery key to use it later
319+
const recoveryKey = await page.evaluate(() => navigator.clipboard.readText());
320+
await currentDialogLocator.getByRole("button", { name: "Continue", exact: true }).click();
321+
322+
// If the device is unverified, there should be a "Setting up keys" step.
323+
// If this is not the first time we are setting up cross-signing, the app will prompt for our password; otherwise
324+
// the step is quite quick, and playwright can miss it, so we can't test for it.
325+
if (opts && Object.hasOwn(opts, "accountPassword")) {
326+
await expect(currentDialogLocator.getByRole("heading", { name: "Setting up keys" })).toBeVisible();
327+
await page.getByPlaceholder("Password").fill(opts!.accountPassword);
328+
await currentDialogLocator.getByRole("button", { name: "Continue" }).click();
329+
}
298330

299-
await expect(dialog.getByText("Secure Backup successful")).toBeVisible();
300-
await dialog.getByRole("button", { name: "Done" }).click();
301-
await expect(dialog.getByText("Secure Backup successful")).not.toBeVisible();
331+
// Either way, we end up at a success dialog:
332+
await expect(currentDialogLocator.getByRole("heading", { name: "Secure Backup successful" })).toBeVisible();
333+
await currentDialogLocator.getByRole("button", { name: "Done", exact: true }).click();
334+
await expect(currentDialogLocator.getByText("Secure Backup successful")).not.toBeVisible();
302335

303-
return securityKey;
336+
return recoveryKey;
304337
}
305338

306339
/**

‎src/SecurityManager.ts

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ Please see LICENSE files in the repository root for full details.
99
import { lazy } from "react";
1010
import { SecretStorage } from "matrix-js-sdk/src/matrix";
1111
import { deriveRecoveryKeyFromPassphrase, decodeRecoveryKey, CryptoCallbacks } from "matrix-js-sdk/src/crypto-api";
12-
import { logger } from "matrix-js-sdk/src/logger";
12+
import { logger as rootLogger } from "matrix-js-sdk/src/logger";
1313

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

32+
const logger = rootLogger.getChild("SecurityManager:");
33+
3234
/**
3335
* This can be used by other components to check if secret storage access is in
3436
* progress, so that we can e.g. avoid intermittently showing toasts during
@@ -70,33 +72,34 @@ function makeInputToKey(
7072
};
7173
}
7274

73-
async function getSecretStorageKey({
74-
keys: keyInfos,
75-
}: {
76-
keys: Record<string, SecretStorage.SecretStorageKeyDescription>;
77-
}): Promise<[string, Uint8Array]> {
75+
async function getSecretStorageKey(
76+
{
77+
keys: keyInfos,
78+
}: {
79+
keys: Record<string, SecretStorage.SecretStorageKeyDescription>;
80+
},
81+
secretName: string,
82+
): Promise<[string, Uint8Array]> {
7883
const cli = MatrixClientPeg.safeGet();
79-
let keyId = await cli.secretStorage.getDefaultKeyId();
80-
let keyInfo!: SecretStorage.SecretStorageKeyDescription;
81-
if (keyId) {
82-
// use the default SSSS key if set
83-
keyInfo = keyInfos[keyId];
84-
if (!keyInfo) {
85-
// if the default key is not available, pretend the default key
86-
// isn't set
87-
keyId = null;
88-
}
89-
}
90-
if (!keyId) {
91-
// if no default SSSS key is set, fall back to a heuristic of using the
84+
const defaultKeyId = await cli.secretStorage.getDefaultKeyId();
85+
86+
let keyId: string;
87+
// If the defaultKey is useful, use that
88+
if (defaultKeyId && keyInfos[defaultKeyId]) {
89+
keyId = defaultKeyId;
90+
} else {
91+
// Fall back to a heuristic of using the
9292
// only available key, if only one key is set
93-
const keyInfoEntries = Object.entries(keyInfos);
94-
if (keyInfoEntries.length > 1) {
93+
const usefulKeys = Object.keys(keyInfos);
94+
if (usefulKeys.length > 1) {
9595
throw new Error("Multiple storage key requests not implemented");
9696
}
97-
[keyId, keyInfo] = keyInfoEntries[0];
97+
keyId = usefulKeys[0];
9898
}
99-
logger.debug(`getSecretStorageKey: request for 4S keys [${Object.keys(keyInfos)}]: looking for key ${keyId}`);
99+
const keyInfo = keyInfos[keyId];
100+
logger.debug(
101+
`getSecretStorageKey: request for 4S keys [${Object.keys(keyInfos)}] for secret \`${secretName}\`: looking for key ${keyId}`,
102+
);
100103

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

107110
const keyFromCustomisations = ModuleRunner.instance.extensions.cryptoSetup.getSecretStorageKey();
108111
if (keyFromCustomisations) {
109-
logger.log("getSecretStorageKey: Using secret storage key from CryptoSetupExtension");
112+
logger.debug("getSecretStorageKey: Using secret storage key from CryptoSetupExtension");
110113
cacheSecretStorageKey(keyId, keyInfo, keyFromCustomisations);
111114
return [keyId, keyFromCustomisations];
112115
}
113116

114-
logger.debug("getSecretStorageKey: prompting user for key");
117+
// We only prompt the user for the default key
118+
if (keyId !== defaultKeyId) {
119+
logger.debug(`getSecretStorageKey: request for non-default key ${keyId}: not prompting user`);
120+
throw new Error("Request for non-default 4S key");
121+
}
122+
123+
logger.debug(`getSecretStorageKey: prompting user for key ${keyId}`);
115124
const inputToKey = makeInputToKey(keyInfo);
116125
const { finished } = Modal.createDialog(
117126
AccessSecretStorageDialog,
@@ -139,7 +148,7 @@ async function getSecretStorageKey({
139148
if (!keyParams) {
140149
throw new AccessCancelledError();
141150
}
142-
logger.debug("getSecretStorageKey: got key from user");
151+
logger.debug(`getSecretStorageKey: got key ${keyId} from user`);
143152
const key = await inputToKey(keyParams);
144153

145154
// Save to cache to avoid future prompts in the current session
@@ -154,6 +163,7 @@ function cacheSecretStorageKey(
154163
key: Uint8Array,
155164
): void {
156165
if (secretStorageBeingAccessed) {
166+
logger.debug(`Caching 4S key ${keyId}`);
157167
secretStorageKeys[keyId] = key;
158168
secretStorageKeyInfo[keyId] = keyInfo;
159169
}
@@ -173,13 +183,13 @@ export const crossSigningCallbacks: CryptoCallbacks = {
173183
* @param func - The operation to be wrapped.
174184
*/
175185
export async function withSecretStorageKeyCache<T>(func: () => Promise<T>): Promise<T> {
176-
logger.debug("SecurityManager: enabling 4S key cache");
186+
logger.debug("enabling 4S key cache");
177187
secretStorageBeingAccessed = true;
178188
try {
179189
return await func();
180190
} finally {
181191
// Clear secret storage key cache now that work is complete
182-
logger.debug("SecurityManager: disabling 4S key cache");
192+
logger.debug("disabling 4S key cache");
183193
secretStorageBeingAccessed = false;
184194
secretStorageKeys = {};
185195
secretStorageKeyInfo = {};

0 commit comments

Comments
 (0)