-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Changes from 53 commits
3e77b3d
ae623f8
f9e48b4
0057f57
bb507b0
1aace3f
70c084e
7193998
fec324e
44c6bce
075f6dc
895ad88
ba032a7
7909ac9
618557c
c805cd8
7a372f7
b20579d
24c537c
72adfa5
36c7e0e
52076f1
a0d904e
1a0e6dc
0b254e5
6f236bd
82bf2cc
84d11f8
2fe5555
4b365ba
8a9291a
1c00502
d9d7528
dc940f5
521cebf
7af44cc
8bd5d6a
086f28e
0c18708
484b2af
ef76454
7a55d83
9a7f568
33967d4
0a52b7c
cb43672
e5dea48
b7b7a98
f78c27a
bef3165
0a6db75
9243ae1
82ec6ae
1934145
57e9c31
3649b07
5bf0d2d
2c3896c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,7 +141,7 @@ Please see LICENSE files in the repository root for full details. | |
} | ||
|
||
.mx_Toast_description { | ||
max-width: 272px; | ||
max-width: 366px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
|
@@ -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) { | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
|
||
|
@@ -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) { | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
|
This file was deleted.
There was a problem hiding this comment.
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
async
s 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.)There was a problem hiding this comment.
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.