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 toast for recovery keys being out of sync #28946

Merged
merged 58 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from 53 commits
Commits
Show all changes
58 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
d9d7528
Very early WIP of rejigged e2e error toast code
dbkr 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
484b2af
Fix tests
dbkr Jan 9, 2025
ef76454
Remove unused file!
dbkr Jan 9, 2025
7a55d83
Remove test for unused file
dbkr Jan 9, 2025
9a7f568
Show 'set up encryption' in the 'other' case.
dbkr Jan 9, 2025
33967d4
Test 'key storage out of sync' toast
dbkr Jan 9, 2025
0a52b7c
Update tests
florianduros Jan 9, 2025
cb43672
Fix test & make toast look correct
dbkr Jan 9, 2025
e5dea48
Use new labels when changing the recovery key
florianduros Jan 10, 2025
b7b7a98
Merge branch 'florianduros/encryption-tab' into dbkr/e2e_toasts
dbkr Jan 10, 2025
f78c27a
Fix docs
florianduros Jan 13, 2025
bef3165
Don't reset key backup when creating a recovery key
florianduros Jan 13, 2025
0a6db75
Merge branch 'florianduros/encryption-tab' into dbkr/e2e_toasts
dbkr Jan 15, 2025
9243ae1
Merge remote-tracking branch 'origin/develop' into dbkr/e2e_toasts
dbkr Jan 15, 2025
82ec6ae
Add playwright test for toast
dbkr Jan 15, 2025
1934145
Dismiss the toast as it's now in the way due to being wider
dbkr Jan 15, 2025
57e9c31
Doesn't look like this needs to be async
dbkr Jan 15, 2025
3649b07
Typo
dbkr Jan 15, 2025
5bf0d2d
Typo
dbkr Jan 15, 2025
2c3896c
Override width for just this toast
dbkr Jan 16, 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
3 changes: 3 additions & 0 deletions playwright/e2e/crypto/event-shields.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ test.describe("Cryptography", function () {
// Bob has a second, not cross-signed, device
const bobSecondDevice = await createSecondBotDevice(page, homeserver, bob);

// Dismiss the toast nagging us to set up recovery otherwise it gets in the way of clicking the room list
await page.getByRole("button", { name: "Not now" }).click();

await bob.sendEvent(testRoomId, null, "m.room.encrypted", {
algorithm: "m.megolm.v1.aes-sha2",
ciphertext: "the bird is in the hand",
Expand Down
22 changes: 22 additions & 0 deletions playwright/e2e/crypto/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,3 +413,25 @@ export async function createSecondBotDevice(page: Page, homeserver: HomeserverIn
await bobSecondDevice.prepareClient();
return bobSecondDevice;
}

/**
* Remove the cached secrets from the indexedDB
* This is a workaround to simulate the case where the secrets are not cached.
*/
export 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 } }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be async? Caution is required when mixing Indexed DB with async, so if we can remove asyncs it would make me less nervous. (I think the wider Promise that is awaited is OK, since no async work is done inside an IndexedDB callback.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, not my code (I just moved it) but I agree it doesn't look like it, and the test passes without it.

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();
}
24 changes: 1 addition & 23 deletions playwright/e2e/settings/encryption-user-tab/recovery.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
*/

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

import { test, expect } from ".";
import {
checkDeviceIsConnectedKeyBackup,
checkDeviceIsCrossSigned,
createBot,
deleteCachedSecrets,
verifySession,
} from "../../crypto/utils";

Expand Down Expand Up @@ -154,25 +154,3 @@ 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.
*/
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/structures/_ToastContainer.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ Please see LICENSE files in the repository root for full details.
}

.mx_Toast_description {
max-width: 272px;
max-width: 366px;
Copy link
Member Author

@dbkr dbkr Jan 15, 2025

Choose a reason for hiding this comment

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

This toast wants to be wider according to the designs, so we need a bigger toaster. It does mean some other toasts get wider too: design, is this desired?

Copy link
Member

Choose a reason for hiding this comment

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

@element-hq/design

overflow: hidden;
text-overflow: ellipsis;
margin: 4px 0 11px 0;
Expand Down
72 changes: 39 additions & 33 deletions src/DeviceListener.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,9 @@ import {
hideToast as hideUnverifiedSessionsToast,
showToast as showUnverifiedSessionsToast,
} from "./toasts/UnverifiedSessionToast";
import { accessSecretStorage, isSecretStorageBeingAccessed } from "./SecurityManager";
import { isSecureBackupRequired } from "./utils/WellKnownUtils";
import { isSecretStorageBeingAccessed } from "./SecurityManager";
import { ActionPayload } from "./dispatcher/payloads";
import { Action } from "./dispatcher/actions";
import { isLoggedIn } from "./utils/login";
import SdkConfig from "./SdkConfig";
import PlatformPeg from "./PlatformPeg";
import { recordClientInformation, removeClientInformation } from "./utils/device/clientInformation";
Expand Down Expand Up @@ -283,7 +281,21 @@ export default class DeviceListener {

const crossSigningReady = await crypto.isCrossSigningReady();
const secretStorageReady = await crypto.isSecretStorageReady();
const allSystemsReady = crossSigningReady && secretStorageReady;
const crossSigningStatus = await crypto.getCrossSigningStatus();
const allCrossSigningSecretsCached =
crossSigningStatus.privateKeysCachedLocally.masterKey &&
crossSigningStatus.privateKeysCachedLocally.selfSigningKey &&
crossSigningStatus.privateKeysCachedLocally.userSigningKey;

const defaultKeyId = await cli.secretStorage.getDefaultKeyId();

const isCurrentDeviceTrusted =
crossSigningReady &&
Boolean(
(await crypto.getDeviceVerificationStatus(cli.getSafeUserId(), cli.deviceId!))?.crossSigningVerified,
);

const allSystemsReady = crossSigningReady && secretStorageReady && allCrossSigningSecretsCached;
await this.reportCryptoSessionStateToAnalytics(cli);

if (this.dismissedThisDeviceToast || allSystemsReady) {
Expand All @@ -294,31 +306,31 @@ export default class DeviceListener {
// make sure our keys are finished downloading
await crypto.getUserDeviceInfo([cli.getSafeUserId()]);

// cross signing isn't enabled - nag to enable it
// There are 3 different toasts for:
if (!(await crypto.getCrossSigningKeyId()) && (await crypto.userHasCrossSigningKeys())) {
// Toast 1. Cross-signing on account but this device doesn't trust the master key (verify this session)
if (!crossSigningReady) {
// This account is legacy and doesn't have cross-signing set up at all.
// Prompt the user to set it up.
showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION);
} else if (!isCurrentDeviceTrusted) {
// cross signing is ready but the current device is not trusted: prompt the user to verify
showSetupEncryptionToast(SetupKind.VERIFY_THIS_SESSION);
this.checkKeyBackupStatus();
} else if (!allCrossSigningSecretsCached) {
// cross signing ready & device trusted, but we are missing secrets from our local cached.
dbkr marked this conversation as resolved.
Show resolved Hide resolved
// prompt the user to enter their recovery key.
showSetupEncryptionToast(SetupKind.KEY_STORAGE_OUT_OF_SYNC);
} else if (defaultKeyId === null) {
// the user just hasn't set up 4S yet: prompt them to do so
showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY);
} else {
const backupInfo = await this.getKeyBackupInfo();
if (backupInfo) {
// Toast 2: Key backup is enabled but recovery (4S) is not set up: prompt user to set up recovery.
// Since we now enable key backup at registration time, this will be the common case for
// new users.
showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY);
} else {
// Toast 3: No cross-signing or key backup on account (set up encryption)
await cli.waitForClientWellKnown();
if (isSecureBackupRequired(cli) && isLoggedIn()) {
// If we're meant to set up, and Secure Backup is required,
// trigger the flow directly without a toast once logged in.
hideSetupEncryptionToast();
accessSecretStorage();
} else {
showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION);
}
}
// some other condition... yikes! Show the 'set up encryption' toast: this is what we previously did
// in 'other' situations. Possibly we should consider prompting for a full reset in this case?
logger.warn("Couldn't match encryption state to a know case: showing 'setup encryption' prompt", {
dbkr marked this conversation as resolved.
Show resolved Hide resolved
crossSigningReady,
secretStorageReady,
allCrossSigningSecretsCached,
isCurrentDeviceTrusted,
defaultKeyId,
});
showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION);
Copy link
Member

Choose a reason for hiding this comment

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

Will users end up in an unsolvable loop if we do this? Any guesses as to what state they are in?

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 couldn't say for sure that they won't, some users do report loops at the moment which would imply the process of setting up encryption doesn't completely start from scratch. At least we will have the state of the variables we use here logged in a rageshake now.

}
}

Expand All @@ -334,12 +346,6 @@ export default class DeviceListener {
// Unverified devices that have appeared since then
const newUnverifiedDeviceIds = new Set<string>();

const isCurrentDeviceTrusted =
crossSigningReady &&
Boolean(
(await crypto.getDeviceVerificationStatus(cli.getSafeUserId(), cli.deviceId!))?.crossSigningVerified,
);

// as long as cross-signing isn't ready,
// you can't see or dismiss any device toasts
if (crossSigningReady) {
Expand Down
4 changes: 4 additions & 0 deletions src/i18n/strings/en_EN.json
Original file line number Diff line number Diff line change
Expand Up @@ -879,14 +879,18 @@
"title": "Destroy cross-signing keys?",
"warning": "Deleting cross-signing keys is permanent. Anyone you have verified with will see security alerts. You almost certainly don't want to do this, unless you've lost every device you can cross-sign from."
},
"enter_recovery_key": "Enter recovery key",
"event_shield_reason_authenticity_not_guaranteed": "The authenticity of this encrypted message can't be guaranteed on this device.",
"event_shield_reason_mismatched_sender_key": "Encrypted by an unverified session",
"event_shield_reason_unknown_device": "Encrypted by an unknown or deleted device.",
"event_shield_reason_unsigned_device": "Encrypted by a device not verified by its owner.",
"event_shield_reason_unverified_identity": "Encrypted by an unverified user.",
"export_unsupported": "Your browser does not support the required cryptography extensions",
"forgot_recovery_key": "Forgot recovery key?",
"import_invalid_keyfile": "Not a valid %(brand)s keyfile",
"import_invalid_passphrase": "Authentication check failed: incorrect password?",
"key_storage_out_of_sync": "Your key storage is out of sync.",
"key_storage_out_of_sync_description": "Confirm your recovery key to maintain access to your key storage and message history.",
"messages_not_secure": {
"cause_1": "Your homeserver",
"cause_2": "The homeserver the user you're verifying is connected to",
Expand Down
13 changes: 13 additions & 0 deletions src/toasts/SetupEncryptionToast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const getTitle = (kind: Kind): string => {
return _t("encryption|set_up_recovery");
case Kind.VERIFY_THIS_SESSION:
return _t("encryption|verify_toast_title");
case Kind.KEY_STORAGE_OUT_OF_SYNC:
return _t("encryption|key_storage_out_of_sync");
}
};

Expand All @@ -37,6 +39,7 @@ const getIcon = (kind: Kind): string | undefined => {
case Kind.SET_UP_RECOVERY:
return undefined;
case Kind.VERIFY_THIS_SESSION:
case Kind.KEY_STORAGE_OUT_OF_SYNC:
return "verification_warning";
}
};
Expand All @@ -49,6 +52,8 @@ const getSetupCaption = (kind: Kind): string => {
return _t("action|continue");
case Kind.VERIFY_THIS_SESSION:
return _t("action|verify");
case Kind.KEY_STORAGE_OUT_OF_SYNC:
return _t("encryption|enter_recovery_key");
}
};

Expand All @@ -59,6 +64,8 @@ const getSecondaryButtonLabel = (kind: Kind): string => {
case Kind.SET_UP_ENCRYPTION:
case Kind.VERIFY_THIS_SESSION:
return _t("encryption|verification|unverified_sessions_toast_reject");
case Kind.KEY_STORAGE_OUT_OF_SYNC:
return _t("encryption|forgot_recovery_key");
}
};

Expand All @@ -70,6 +77,8 @@ const getDescription = (kind: Kind): string => {
return _t("encryption|set_up_recovery_toast_description");
case Kind.VERIFY_THIS_SESSION:
return _t("encryption|verify_toast_description");
case Kind.KEY_STORAGE_OUT_OF_SYNC:
return _t("encryption|key_storage_out_of_sync_description");
}
};

Expand All @@ -89,6 +98,10 @@ export enum Kind {
* Prompt the user to verify this session
*/
VERIFY_THIS_SESSION = "verify_this_session",
/**
* Prompt the user to enter their recovery key
*/
KEY_STORAGE_OUT_OF_SYNC = "key_storage_out_of_sync",
}

const onReject = (): void => {
Expand Down
17 changes: 0 additions & 17 deletions src/utils/login.ts

This file was deleted.

19 changes: 10 additions & 9 deletions test/unit-tests/DeviceListener-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,32 +329,33 @@ describe("DeviceListener", () => {
});

it("shows verify session toast when account has cross signing", async () => {
mockCrypto!.userHasCrossSigningKeys.mockResolvedValue(true);
mockCrypto!.isCrossSigningReady.mockResolvedValue(true);
await createAndStart();

expect(mockCrypto!.getUserDeviceInfo).toHaveBeenCalled();
expect(SetupEncryptionToast.showToast).toHaveBeenCalledWith(
SetupEncryptionToast.Kind.VERIFY_THIS_SESSION,
);
});

it("checks key backup status when when account has cross signing", async () => {
mockCrypto!.getCrossSigningKeyId.mockResolvedValue(null);
mockCrypto!.userHasCrossSigningKeys.mockResolvedValue(true);
await createAndStart();

expect(mockCrypto!.getActiveSessionBackupVersion).toHaveBeenCalled();
});
Comment on lines -341 to -347
Copy link
Member Author

Choose a reason for hiding this comment

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

It's not clear why this test was testing this... I don't really understand why it would be important that it did this. Maybe in some circumstances it should, but if so shouldn't we be testing to see if it prompts you to set up key backup or whatever w're expecting?

});

describe("when user does have a cross signing id on this device", () => {
beforeEach(() => {
mockCrypto!.isCrossSigningReady.mockResolvedValue(true);
mockCrypto!.getCrossSigningKeyId.mockResolvedValue("abc");
mockCrypto!.getDeviceVerificationStatus.mockResolvedValue(
new DeviceVerificationStatus({
trustCrossSignedDevices: true,
crossSigningVerified: true,
}),
);
});

it("shows set up recovery toast when user has a key backup available", async () => {
// non falsy response
mockCrypto.getKeyBackupInfo.mockResolvedValue({} as unknown as KeyBackupInfo);
mockClient.secretStorage.getDefaultKeyId.mockResolvedValue(null);

await createAndStart();

expect(SetupEncryptionToast.showToast).toHaveBeenCalledWith(
Expand Down
8 changes: 7 additions & 1 deletion test/unit-tests/toasts/SetupEncryptionToast-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,15 @@ describe("SetupEncryptionToast", () => {
render(<ToastContainer />);
});

it("should render the se up recovery toast", async () => {
it("should render the 'set up recovery' toast", async () => {
showToast(Kind.SET_UP_RECOVERY);

await expect(screen.findByText("Set up recovery")).resolves.toBeInTheDocument();
});

it("should render the 'key storage out of sync' toast", async () => {
showToast(Kind.KEY_STORAGE_OUT_OF_SYNC);

await expect(screen.findByText("Your key storage is out of sync.")).resolves.toBeInTheDocument();
});
});
22 changes: 0 additions & 22 deletions test/unit-tests/utils/login-test.ts

This file was deleted.

Loading