Handle cross-signing keys missing locally and/or from secret storage#31367
Handle cross-signing keys missing locally and/or from secret storage#31367uhoreg merged 24 commits intoelement-hq:developfrom
Conversation
If cross-signing keys are missing both locally and in 4S, show a new toast saying that identity needs resetting, rather than saying that the device needs to be verified.
- move enum from SetupEncryptionToast to DeviceListener - DeviceListener has public method to get device state - DeviceListener emits events to update device state
brings RecoveryPanelOutOfSync in line with SetupEncryptionToast behaviour
rather than using its own logic
Co-authored-by: Skye Elliot <actuallyori@gmail.com>
uhoreg
force-pushed
the
key_storage_out_of_sync_cross_signing
branch
from
fb83e3e to
edc4d81
Compare
kaylendog
left a comment
kaylendog
left a comment
There was a problem hiding this comment.
Wonderful, I think this is fine now!
src/DeviceListener.ts
Outdated
| /** | ||
| * 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", | ||
| } |
There was a problem hiding this comment.
Can we use string union? It allows more flexibility
There was a problem hiding this comment.
I personally like enums better (and this was just copied from SetupEncryptionToast), but if string unions are the currently-preferred style, then I can switch it.
| await deviceListener.whilePaused(async () => { | ||
| await accessSecretStorage(async () => { | ||
| // Reset backup if needed. | ||
| if (needsBackupReset) { | ||
| await resetKeyBackupAndWait(crypto); | ||
| } else if (await matrixClient.isKeyBackupKeyStored()) { | ||
| await crypto.loadSessionBackupPrivateKeyFromSecretStorage(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
what is happening if this promises are rejected?
There was a problem hiding this comment.
I've changed it so that now it does the same thing as SetupEncryptionToast in the same situation
| return render( | ||
| <MatrixClientContext.Provider value={MatrixClientPeg.safeGet()}> | ||
| <RecoveryPanelOutOfSync onFinish={onFinish} onForgotRecoveryKey={onForgotRecoveryKey} /> | ||
| </MatrixClientContext.Provider>, | ||
| ); |
There was a problem hiding this comment.
| return render( | |
| <MatrixClientContext.Provider value={MatrixClientPeg.safeGet()}> | |
| <RecoveryPanelOutOfSync onFinish={onFinish} onForgotRecoveryKey={onForgotRecoveryKey} /> | |
| </MatrixClientContext.Provider>, | |
| ); | |
| return render(<RecoveryPanelOutOfSync onFinish={onFinish} onForgotRecoveryKey={onForgotRecoveryKey} />, withClientContextRenderOptions(matrixClient)); |
| describe("<RecoveyPanelOutOfSync />", () => { | ||
| function renderComponent(onFinish = jest.fn(), onForgotRecoveryKey = jest.fn()) { | ||
| return render(<RecoveryPanelOutOfSync onFinish={onFinish} onForgotRecoveryKey={onForgotRecoveryKey} />); | ||
| stubClient(); |
There was a problem hiding this comment.
Avoid using MatrixClientPeg.safeGet(), prefer to create the matrix client and put it an a context
| stubClient(); | |
| createTestClient(); |
| .mockImplementation(async (func = async (): Promise<void> => {}) => { | ||
| return await func(); | ||
| }); |
There was a problem hiding this comment.
Do we really need to call the function in mockImplementation? Or returning a promise for accessSecretStorage is enough?
| .mockImplementation(async (func = async (): Promise<void> => {}) => { | |
| return await func(); | |
| }); | |
| .mockResolvedValue(undefined) |
There was a problem hiding this comment.
We do need to call the function in mockImplementation because we're checking whether the function calls resetKeyBackup or not. The test will pass without calling the function, but then it makes the expect(matrixClient.getCrypto()!.resetKeyBackup).not.toHaveBeenCalled(); useless.
| } | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks(); |
There was a problem hiding this comment.
| jest.restoreAllMocks(); | |
| jest.clearAllMocks() |
|
|
||
| const user = userEvent.setup(); | ||
| mocked(accessSecretStorage) | ||
| .mockClear() |
There was a problem hiding this comment.
With jest.clearAllMocks() in afteEach, this not necessary
| expect(accessSecretStorage).toHaveBeenCalled(); | ||
| expect(onFinish).toHaveBeenCalled(); | ||
|
|
||
| expect(MatrixClientPeg.safeGet().getCrypto()!.resetKeyBackup).toHaveBeenCalled(); |
| return this.deviceState; | ||
| } | ||
|
|
||
| private async setDeviceState(newState: DeviceState, logSpan: LogSpan): Promise<void> { |
|
@florianduros I think that I've addressed all your concerns |
| subHeading={ | ||
| <SettingsSubheader state="error" stateMessage={_t("encryption|identity_needs_reset_description")} /> | ||
| } | ||
| data-testid="recoveryPanel" |
There was a problem hiding this comment.
Yup, copy-and-paste error
3 participants
Fixes #31187 and parts of #30438
Best to review this commit-by-commit
Checklist
public/exportedsymbols have accurate TSDoc documentation.