-
Notifications
You must be signed in to change notification settings - Fork 44
Change Password flow #679
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
base: main
Are you sure you want to change the base?
Change Password flow #679
Changes from 8 commits
218ddb5
bd5be87
e2829d0
4cb30e2
14c2bb8
b66ca10
0f06e35
9248c0c
851d793
ee1e18c
f75393b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,40 @@ | ||
| import { produce } from 'immer'; | ||
| import { PersistentStore } from 'src/modules/persistent-store'; | ||
| import type { Credentials } from '../account/Credentials'; | ||
| import { invariant } from 'src/shared/invariant'; | ||
| import { getError } from 'src/shared/errors/getError'; | ||
| import { ErrorWithEnumerableMessage } from 'src/shared/errors/errors'; | ||
| import type { User } from 'src/shared/types/User'; | ||
| import type { Credentials, SessionCredentials } from '../account/Credentials'; | ||
| import { emitter } from '../events'; | ||
| import { Account } from '../account/Account'; | ||
| import type { WalletRecord } from './model/types'; | ||
| import { WalletRecordModel as Model } from './WalletRecord'; | ||
|
|
||
| type EncryptedWalletRecord = string; | ||
|
|
||
| type WalletStoreState = Record<string, EncryptedWalletRecord | undefined>; | ||
|
|
||
| export class InternalBackupError extends ErrorWithEnumerableMessage { | ||
| didRestore: boolean; | ||
| constructor(error: Error, { didRestore }: { didRestore: boolean }) { | ||
| super(error.message); | ||
| this.name = error.name !== 'Error' ? error.name : 'InternalBackupError'; | ||
| this.didRestore = didRestore; | ||
| } | ||
| } | ||
|
|
||
| type RecordBackup = { user: User; record: string }; | ||
| function stringifyBackup({ user, record }: RecordBackup): string { | ||
| return JSON.stringify({ user, record }); | ||
| } | ||
|
|
||
| function parseBackup(value: string): RecordBackup { | ||
| const parsed = JSON.parse(value) as RecordBackup; | ||
| invariant(parsed.user, 'User not found in backup'); | ||
| invariant(parsed.record, 'Record not found in backup'); | ||
| return parsed; | ||
| } | ||
|
|
||
| export class WalletStore extends PersistentStore<WalletStoreState> { | ||
| static key = 'wallet'; | ||
| /** Store unencrypted "lastRecord" to avoid unnecessary stringifications */ | ||
|
|
@@ -41,17 +68,131 @@ export class WalletStore extends PersistentStore<WalletStoreState> { | |
| return this.lastRecord; | ||
| } | ||
|
|
||
| async save(id: string, encryptionKey: string, record: WalletRecord) { | ||
| /** Prefer WalletStore['save'] unless necessary */ | ||
| private async encryptAndSave( | ||
| id: string, | ||
| credentials: Credentials, | ||
| record: WalletRecord | ||
| ) { | ||
| const encryptedRecord = await Model.encryptRecord( | ||
| credentials.encryptionKey, | ||
| record | ||
| ); | ||
| await this.setState((state) => | ||
| produce(state, (draft) => { | ||
| draft[id] = encryptedRecord; | ||
| }) | ||
| ); | ||
| this.lastRecord = record; | ||
| } | ||
|
|
||
| async save(id: string, credentials: Credentials, record: WalletRecord) { | ||
| if (this.lastRecord === record) { | ||
| return; | ||
| } | ||
| const encryptedRecord = await Model.encryptRecord(encryptionKey, record); | ||
| this.setState((state) => | ||
| await this.encryptAndSave(id, credentials, record); | ||
| } | ||
|
|
||
| async createBackup(id: string) { | ||
|
Collaborator
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. what do you think about calling this param |
||
| /** | ||
| * Accessing user is a cross-concern, but this is the only way | ||
| * to make our backup truly atomic and independent: | ||
| * encrypted record relies on `salt` stored in the user object, | ||
| * and for a robust backup recovery it's best to store this object | ||
| * together with the encrypted record | ||
| */ | ||
| const user = await Account.readCurrentUser(); | ||
| const record = (await this.getSavedState())[id]; | ||
| invariant(record, `Record not found for id: ${id}`); | ||
| invariant(user && user.id === id, `User not found for id: ${id}`); | ||
| return this.setState({ | ||
| ...this.state, | ||
| [`backup:${id}`]: stringifyBackup({ user, record }), | ||
| }); | ||
| } | ||
|
|
||
| async restoreFromBackup(id: string) { | ||
| const key = `backup:${id}`; | ||
| const state = await this.getSavedState(); | ||
| const saved = state[key]; | ||
| invariant(saved, `Backup not found for id: ${id}`); | ||
| const { user, record } = parseBackup(saved); | ||
| await Promise.all([ | ||
| this.setState((state) => | ||
| produce(state, (draft) => { | ||
| draft[id] = record; | ||
| delete draft[key]; | ||
| }) | ||
| ), | ||
| Account.writeCurrentUser(user), | ||
| ]); | ||
| } | ||
|
|
||
| async restoreFromAnyBackup() { | ||
| const state = await this.getSavedState(); | ||
| const key = Object.keys(state).find((key) => key.startsWith('backup:')); | ||
| if (key) { | ||
| await this.restoreFromBackup(key.split(':')[1]); | ||
| } else { | ||
| throw new Error('No backups found'); | ||
| } | ||
| } | ||
|
|
||
| async clearBackup(id: string) { | ||
| return this.setState((state) => | ||
| produce(state, (draft) => { | ||
| draft[id] = encryptedRecord; | ||
| const key = `backup:${id}`; | ||
| delete draft[key]; | ||
| }) | ||
| ); | ||
| this.lastRecord = record; | ||
| } | ||
|
|
||
| /** | ||
| * Executes an operation with a backup and an automatic recovery. | ||
| * Guarantees atomicity by restoring to the previous state if the operation fails. | ||
| */ | ||
| async withBackup(id: string, operation: () => Promise<unknown>) { | ||
| await this.createBackup(id); | ||
| try { | ||
| await operation(); | ||
| await this.clearBackup(id); | ||
| } catch (error) { | ||
| try { | ||
| await this.restoreFromBackup(id); | ||
| emitter.emit('globalError', { | ||
| name: 'internal_error', | ||
| message: 'Atomic wallet update failed. Restored from backup.', | ||
| }); | ||
| console.log('Successfully restored wallet record from backup'); // eslint-disable-line no-console | ||
| } catch { | ||
| emitter.emit('globalError', { | ||
| name: 'internal_error', | ||
| message: 'Atomic wallet update failed. Restore from backup failed.', | ||
| }); | ||
| throw new InternalBackupError(getError(error), { didRestore: false }); | ||
|
Collaborator
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. Do we need this param for this Error? It looks like we only use it with didRestore: false |
||
| } | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| async reEncrypt({ | ||
| id, | ||
| credentials, | ||
| newCredentials, | ||
| }: { | ||
| id: string; | ||
| credentials: SessionCredentials; | ||
| newCredentials: SessionCredentials; | ||
| }) { | ||
| await this.ready(); | ||
| console.log('reading', { id, credentials, state: this.getState() }); | ||
|
Collaborator
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. remove?
Collaborator
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. Looks dangerous to expose credentials :) |
||
| const currentRecord = await this.read(id, credentials); | ||
| invariant(currentRecord, `Record not found for ${id}`); | ||
| const newRecord = await Model.reEncryptRecord(currentRecord, { | ||
| credentials, | ||
| newCredentials, | ||
| }); | ||
| await this.encryptAndSave(id, newCredentials, newRecord); | ||
| } | ||
|
|
||
| deleteMany(keys: string[]) { | ||
|
|
||
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.
do we need to pass all Credentials here?
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.
No, but it makes it more consistent with the other signatures. I think it's more proper to pass credentials, it's easier to abstract them this way