-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Handle cross-signing keys missing locally and/or from secret storage #31367
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
Open
uhoreg
wants to merge
19
commits into
element-hq:develop
Choose a base branch
from
uhoreg:key_storage_out_of_sync_cross_signing
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
e1ca135
show correct toast when cross-signing keys missing
uhoreg d0ed546
refactor: make DeviceListener in charge of device state
uhoreg 85f8b9c
reset key backup when needed in RecoveryPanelOutOfSync
uhoreg 48743ee
update strings to agree with designs from Figma
uhoreg 303417e
use DeviceListener to determine EncryptionUserSettingsTab display
uhoreg c44916c
prompt to reset identity in Encryption Settings when needed
uhoreg fb2dbe7
Merge branch 'develop' into key_storage_out_of_sync_cross_signing
uhoreg d7a2ac9
fix type
uhoreg 8983cef
calculate device state even if we aren't going to show a toast
uhoreg f1f1f85
Merge branch 'develop' into key_storage_out_of_sync_cross_signing
uhoreg bb3a130
update snapshot
uhoreg ff53fc1
make logs more accurate
uhoreg bccb180
add tests
uhoreg 6f31fed
make the bot use a different access token/device
uhoreg da86841
only log in a new session when requested
uhoreg 46915f7
Mark properties as read-only
uhoreg 3f52a6f
Merge branch 'develop' into key_storage_out_of_sync_cross_signing
uhoreg b09f67c
remove some duplicate strings
uhoreg edc4d81
make accessToken optional instead of using empty string
uhoreg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import { | |
| RoomStateEvent, | ||
| type SyncState, | ||
| ClientStoppedError, | ||
| TypedEventEmitter, | ||
| } from "matrix-js-sdk/src/matrix"; | ||
| import { logger as baseLogger, type BaseLogger, LogSpan } from "matrix-js-sdk/src/logger"; | ||
| import { CryptoEvent, type KeyBackupInfo } from "matrix-js-sdk/src/crypto-api"; | ||
|
|
@@ -29,7 +30,6 @@ import { | |
| } from "./toasts/BulkUnverifiedSessionsToast"; | ||
| import { | ||
| hideToast as hideSetupEncryptionToast, | ||
| Kind as SetupKind, | ||
| showToast as showSetupEncryptionToast, | ||
| } from "./toasts/SetupEncryptionToast"; | ||
| import { | ||
|
|
@@ -65,7 +65,48 @@ export const RECOVERY_ACCOUNT_DATA_KEY = "io.element.recovery"; | |
|
|
||
| const logger = baseLogger.getChild("DeviceListener:"); | ||
|
|
||
| export default class DeviceListener { | ||
| /** | ||
| * The state of the device and the user's account. | ||
| */ | ||
| export enum DeviceState { | ||
| /** | ||
| * The device is in a good state. | ||
| */ | ||
| OK = "ok", | ||
| /** | ||
| * The user needs to set up recovery. | ||
| */ | ||
| SET_UP_RECOVERY = "set_up_recovery", | ||
| /** | ||
| * The device is not verified. | ||
| */ | ||
| VERIFY_THIS_SESSION = "verify_this_session", | ||
| /** | ||
| * Key storage is out of sync (keys are missing locally, from recovery, or both). | ||
| */ | ||
| KEY_STORAGE_OUT_OF_SYNC = "key_storage_out_of_sync", | ||
| /** | ||
| * Key storage is not enabled, and has not been marked as purposely disabled. | ||
| */ | ||
| TURN_ON_KEY_STORAGE = "turn_on_key_storage", | ||
| /** | ||
| * The user's identity needs resetting, due to missing keys. | ||
| */ | ||
| IDENTITY_NEEDS_RESET = "identity_needs_reset", | ||
| } | ||
|
|
||
| /** | ||
| * The events emitted by {@link DeviceListener} | ||
| */ | ||
| export enum DeviceListenerEvents { | ||
| DeviceState = "device_state", | ||
| } | ||
|
|
||
| type EventHandlerMap = { | ||
| [DeviceListenerEvents.DeviceState]: (state: DeviceState) => void; | ||
| }; | ||
|
|
||
| export default class DeviceListener extends TypedEventEmitter<DeviceListenerEvents, EventHandlerMap> { | ||
| private dispatcherRef?: string; | ||
| // device IDs for which the user has dismissed the verify toast ('Later') | ||
| private dismissed = new Set<string>(); | ||
|
|
@@ -87,6 +128,7 @@ export default class DeviceListener { | |
| private shouldRecordClientInformation = false; | ||
| private enableBulkUnverifiedSessionsReminder = true; | ||
| private deviceClientInformationSettingWatcherRef: string | undefined; | ||
| private deviceState: DeviceState = DeviceState.OK; | ||
|
|
||
| // Remember the current analytics state to avoid sending the same event multiple times. | ||
| private analyticsVerificationState?: string; | ||
|
|
@@ -198,8 +240,8 @@ export default class DeviceListener { | |
| } | ||
|
|
||
| /** | ||
| * If a `Kind.KEY_STORAGE_OUT_OF_SYNC` condition from {@link doRecheck} | ||
| * requires a reset of cross-signing keys. | ||
| * If the device is in a `DeviceState.KEY_STORAGE_OUT_OF_SYNC` state, check if | ||
| * it requires a reset of cross-signing keys. | ||
| * | ||
| * We will reset cross-signing keys if both our local cache and 4S don't | ||
| * have all cross-signing keys. | ||
|
|
@@ -227,16 +269,15 @@ export default class DeviceListener { | |
| } | ||
|
|
||
| /** | ||
| * If a `Kind.KEY_STORAGE_OUT_OF_SYNC` condition from {@link doRecheck} | ||
| * requires a reset of key backup. | ||
| * If the device is in a `DeviceState.KEY_STORAGE_OUT_OF_SYNC` state, check if | ||
| * it requires a reset of key backup. | ||
| * | ||
| * If the user has their recovery key, we need to reset backup if: | ||
| * - the user hasn't disabled backup, | ||
| * - we don't have the backup key cached locally, *and* | ||
| * - we don't have the backup key stored in 4S. | ||
| * (The user should already have a key backup created at this point, | ||
| * otherwise `doRecheck` would have triggered a `Kind.TURN_ON_KEY_STORAGE` | ||
| * condition.) | ||
| * (The user should already have a key backup created at this point, the | ||
| * device state would be `DeviceState.TURN_ON_KEY_STORAGE`.) | ||
| * | ||
| * If the user has forgotten their recovery key, we need to reset backup if: | ||
| * - the user hasn't disabled backup, and | ||
|
|
@@ -425,11 +466,9 @@ export default class DeviceListener { | |
|
|
||
| const recoveryIsOk = secretStorageStatus.ready || recoveryDisabled; | ||
|
|
||
| const isCurrentDeviceTrusted = | ||
| crossSigningReady && | ||
| Boolean( | ||
| (await crypto.getDeviceVerificationStatus(cli.getSafeUserId(), cli.deviceId!))?.crossSigningVerified, | ||
| ); | ||
| const isCurrentDeviceTrusted = Boolean( | ||
| (await crypto.getDeviceVerificationStatus(cli.getSafeUserId(), cli.deviceId!))?.crossSigningVerified, | ||
| ); | ||
|
|
||
| const keyBackupUploadActive = await this.isKeyBackupUploadActive(logSpan); | ||
| const backupDisabled = await this.recheckBackupDisabled(cli); | ||
|
|
@@ -448,65 +487,70 @@ export default class DeviceListener { | |
|
|
||
| await this.reportCryptoSessionStateToAnalytics(cli); | ||
|
|
||
| if (this.dismissedThisDeviceToast || allSystemsReady) { | ||
| if (allSystemsReady) { | ||
| logSpan.info("No toast needed"); | ||
| hideSetupEncryptionToast(); | ||
| await this.setDeviceState(DeviceState.OK, logSpan); | ||
|
|
||
| this.checkKeyBackupStatus(); | ||
| } else if (await this.shouldShowSetupEncryptionToast()) { | ||
| } else { | ||
| // make sure our keys are finished downloading | ||
| await crypto.getUserDeviceInfo([cli.getSafeUserId()]); | ||
|
|
||
| if (!isCurrentDeviceTrusted) { | ||
| // the current device is not trusted: prompt the user to verify | ||
| logSpan.info("Current device not verified: showing VERIFY_THIS_SESSION toast"); | ||
| showSetupEncryptionToast(SetupKind.VERIFY_THIS_SESSION); | ||
| logSpan.info("Current device not verified: setting state to VERIFY_THIS_SESSION"); | ||
| await this.setDeviceState(DeviceState.VERIFY_THIS_SESSION, logSpan); | ||
| } else if (!allCrossSigningSecretsCached) { | ||
| // cross signing ready & device trusted, but we are missing secrets from our local cache. | ||
| // prompt the user to enter their recovery key. | ||
| logSpan.info( | ||
| "Some secrets not cached: showing KEY_STORAGE_OUT_OF_SYNC toast", | ||
| "Some secrets not cached: setting state to KEY_STORAGE_OUT_OF_SYNC", | ||
| crossSigningStatus.privateKeysCachedLocally, | ||
| crossSigningStatus.privateKeysInSecretStorage, | ||
| ); | ||
| await this.setDeviceState( | ||
| crossSigningStatus.privateKeysInSecretStorage | ||
| ? DeviceState.KEY_STORAGE_OUT_OF_SYNC | ||
| : DeviceState.IDENTITY_NEEDS_RESET, | ||
| logSpan, | ||
| ); | ||
| showSetupEncryptionToast(SetupKind.KEY_STORAGE_OUT_OF_SYNC); | ||
| } else if (!keyBackupIsOk) { | ||
| logSpan.info("Key backup upload is unexpectedly turned off: showing TURN_ON_KEY_STORAGE toast"); | ||
| showSetupEncryptionToast(SetupKind.TURN_ON_KEY_STORAGE); | ||
| logSpan.info("Key backup upload is unexpectedly turned off: setting state to TURN_ON_KEY_STORAGE"); | ||
| await this.setDeviceState(DeviceState.TURN_ON_KEY_STORAGE, logSpan); | ||
| } else if (secretStorageStatus.defaultKeyId === null) { | ||
| // The user just hasn't set up 4S yet: if they have key | ||
| // backup, prompt them to turn on recovery too. (If not, they | ||
| // have explicitly opted out, so don't hassle them.) | ||
| if (recoveryDisabled) { | ||
| logSpan.info("Recovery disabled: no toast needed"); | ||
| hideSetupEncryptionToast(); | ||
| await this.setDeviceState(DeviceState.OK, logSpan); | ||
| } else if (keyBackupUploadActive) { | ||
| logSpan.info("No default 4S key: showing SET_UP_RECOVERY toast"); | ||
| showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY); | ||
| logSpan.info("No default 4S key: setting state to SET_UP_RECOVERY"); | ||
| await this.setDeviceState(DeviceState.SET_UP_RECOVERY, logSpan); | ||
| } else { | ||
| logSpan.info("No default 4S key but backup disabled: no toast needed"); | ||
| hideSetupEncryptionToast(); | ||
| await this.setDeviceState(DeviceState.OK, logSpan); | ||
| } | ||
| } else { | ||
| // If we get here, then we are verified, have key backup, and | ||
| // 4S, but allSystemsReady is false, which means that either | ||
| // secretStorageStatus.ready is false (which means that 4S | ||
| // doesn't have all the secrets), or we don't have the backup | ||
| // key cached locally. | ||
| // key cached locally. If any of the cross-signing keys are | ||
| // missing locally, that is handled by the | ||
| // `!allCrossSigningSecretsCached` branch above. | ||
| logSpan.warn("4S is missing secrets or backup key not cached", { | ||
| crossSigningReady, | ||
| secretStorageStatus, | ||
| allCrossSigningSecretsCached, | ||
| isCurrentDeviceTrusted, | ||
| backupKeyCached, | ||
| }); | ||
| // We use the right toast variant based on whether the backup | ||
| // key is missing locally. If any of the cross-signing keys are | ||
| // missing locally, that is handled by the | ||
| // `!allCrossSigningSecretsCached` branch above. | ||
| showSetupEncryptionToast(SetupKind.KEY_STORAGE_OUT_OF_SYNC); | ||
| await this.setDeviceState(DeviceState.KEY_STORAGE_OUT_OF_SYNC, logSpan); | ||
| } | ||
| if (this.dismissedThisDeviceToast) { | ||
| this.checkKeyBackupStatus(); | ||
| } | ||
| } else { | ||
| logSpan.info("Not yet ready, but shouldShowSetupEncryptionToast==false"); | ||
| } | ||
|
|
||
| // This needs to be done after awaiting on getUserDeviceInfo() above, so | ||
|
|
@@ -598,6 +642,22 @@ export default class DeviceListener { | |
| return recoveryStatus?.enabled === false; | ||
| } | ||
|
|
||
| public getDeviceState(): DeviceState { | ||
| return this.deviceState; | ||
| } | ||
|
|
||
| private async setDeviceState(newState: DeviceState, logSpan: LogSpan): Promise<void> { | ||
|
Member
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. missing tsdoc |
||
| this.deviceState = newState; | ||
| this.emit(DeviceListenerEvents.DeviceState, newState); | ||
| if (newState === DeviceState.OK || this.dismissedThisDeviceToast) { | ||
| hideSetupEncryptionToast(); | ||
| } else if (await this.shouldShowSetupEncryptionToast()) { | ||
| showSetupEncryptionToast(newState); | ||
| } else { | ||
| logSpan.info("Not yet ready, but shouldShowSetupEncryptionToast==false"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Reports current recovery state to analytics. | ||
| * Checks if the session is verified and if the recovery is correctly set up (i.e all secrets known locally and in 4S). | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can we use string union? It allows more flexibility