From 4b07927d9eb7e24cda9a75c74eb1ef79937d681d Mon Sep 17 00:00:00 2001 From: addison Date: Thu, 18 Nov 2021 16:40:21 -0500 Subject: [PATCH] [bug] Scaffold memory storage for web Not properly creating storage objects on signin was creating weird behavior when logging out, locking, and logging back in. Namely, encrypted data that was recently synced had nowhere to save to and was lost. --- common/src/models/domain/account.ts | 180 +++++++++++++-------------- common/src/services/auth.service.ts | 28 +++-- common/src/services/state.service.ts | 88 +++++++++---- 3 files changed, 168 insertions(+), 128 deletions(-) diff --git a/common/src/models/domain/account.ts b/common/src/models/domain/account.ts index b1cb04de..a1cb1be3 100644 --- a/common/src/models/domain/account.ts +++ b/common/src/models/domain/account.ts @@ -33,101 +33,101 @@ export class DataEncryptionPair { } export class AccountData { - ciphers?: DataEncryptionPair = new DataEncryptionPair(); - folders?: DataEncryptionPair = new DataEncryptionPair(); - localData?: any; - sends?: DataEncryptionPair = new DataEncryptionPair(); - collections?: DataEncryptionPair = new DataEncryptionPair(); - kdfIterations?: number; - kdfType?: KdfType; - policies?: DataEncryptionPair = new DataEncryptionPair(); - passwordGenerationHistory?: EncryptionPair = new EncryptionPair(); - addEditCipherInfo?: any; - collapsedGroupings?: Set; - eventCollection?: EventData[]; - organizations?: { [id: string]: OrganizationData }; - providers?: { [id: string]: ProviderData }; + ciphers: DataEncryptionPair = new DataEncryptionPair(); + folders: DataEncryptionPair = new DataEncryptionPair(); + localData: any; + sends: DataEncryptionPair = new DataEncryptionPair(); + collections: DataEncryptionPair = new DataEncryptionPair(); + policies: DataEncryptionPair = new DataEncryptionPair(); + passwordGenerationHistory: EncryptionPair = new EncryptionPair(); + addEditCipherInfo: any; + collapsedGroupings: Set; + eventCollection: EventData[]; + organizations: { [id: string]: OrganizationData }; + providers: { [id: string]: ProviderData }; } export class AccountKeys { - cryptoMasterKey?: SymmetricCryptoKey; - cryptoMasterKeyAuto?: SymmetricCryptoKey; - cryptoMasterKeyB64?: string; - cryptoMasterKeyBiometric?: SymmetricCryptoKey; - cryptoSymmetricKey?: EncryptionPair = new EncryptionPair(); - organizationKeys?: EncryptionPair> = new EncryptionPair>(); - privateKey?: EncryptionPair = new EncryptionPair(); - providerKeys?: EncryptionPair> = new EncryptionPair>(); - keyHash?: string; - legacyEtmKey?: SymmetricCryptoKey; - publicKey?: ArrayBuffer; + cryptoMasterKey: SymmetricCryptoKey; + cryptoMasterKeyAuto: SymmetricCryptoKey; + cryptoMasterKeyB64: string; + cryptoMasterKeyBiometric: SymmetricCryptoKey; + cryptoSymmetricKey: EncryptionPair = new EncryptionPair(); + organizationKeys: EncryptionPair> = new EncryptionPair>(); + providerKeys: EncryptionPair> = new EncryptionPair>(); + privateKey: EncryptionPair = new EncryptionPair(); + legacyEtmKey: SymmetricCryptoKey; + publicKey: ArrayBuffer; + apiKeyClientSecret: string; } export class AccountProfile { - apiKeyClientId?: string; - apiKeyClientSecret?: string; - authenticationStatus?: AuthenticationStatus; - convertAccountToKeyConnector?: boolean; - email?: string; - emailVerified?: boolean; - entityId?: string; - entityType?: string; - everBeenUnlocked?: boolean; - forcePasswordReset?: boolean; - hasPremiumPersonally?: boolean; - lastActive?: number; - lastSync?: string; - ssoCodeVerifier?: string; - ssoOrganizationIdentifier?: string; - ssoState?: string; - userId?: string; - usesKeyConnector?: boolean; + apiKeyClientId: string; + authenticationStatus: AuthenticationStatus; + convertAccountToKeyConnector: boolean; + email: string; + emailVerified: boolean; + entityId: string; + entityType: string; + everBeenUnlocked: boolean; + forcePasswordReset: boolean; + hasPremiumPersonally: boolean; + lastActive: number; + lastSync: string; + ssoCodeVerifier: string; + ssoOrganizationIdentifier: string; + ssoState: string; + userId: string; + usesKeyConnector: boolean; + keyHash: string; + kdfIterations: number; + kdfType: KdfType; } export class AccountSettings { - alwaysShowDock?: boolean; - autoConfirmFingerPrints?: boolean; - autoFillOnPageLoadDefault?: boolean; - biometricLocked?: boolean; - biometricText?: string; - biometricUnlock?: boolean; - clearClipboard?: number; - defaultUriMatch?: UriMatchType; - disableAddLoginNotification?: boolean; - disableAutoBiometricsPrompt?: boolean; - disableAutoTotpCopy?: boolean; - disableBadgeCounter?: boolean; - disableChangedPasswordNotification?: boolean; - disableContextMenuItem?: boolean; - disableGa?: boolean; - dontShowCardsCurrentTab?: boolean; - dontShowIdentitiesCurrentTab?: boolean; - enableAlwaysOnTop?: boolean; - enableAutoFillOnPageLoad?: boolean; - enableBiometric?: boolean; - enableBiometrics?: boolean; - enableBrowserIntegration?: boolean; - enableBrowserIntegrationFingerprint?: boolean; - enableCloseToTray?: boolean; - enableFullWidth?: boolean; - enableGravitars?: boolean; - enableMinimizeToTray?: boolean; - enableStartToTray?: boolean; - enableTray?: boolean; - environmentUrls?: any; + alwaysShowDock: boolean; + autoConfirmFingerPrints: boolean; + autoFillOnPageLoadDefault: boolean; + biometricLocked: boolean; + biometricText: string; + biometricUnlock: boolean; + clearClipboard: number; + defaultUriMatch: UriMatchType; + disableAddLoginNotification: boolean; + disableAutoBiometricsPrompt: boolean; + disableAutoTotpCopy: boolean; + disableBadgeCounter: boolean; + disableChangedPasswordNotification: boolean; + disableContextMenuItem: boolean; + disableGa: boolean; + dontShowCardsCurrentTab: boolean; + dontShowIdentitiesCurrentTab: boolean; + enableAlwaysOnTop: boolean; + enableAutoFillOnPageLoad: boolean; + enableBiometric: boolean; + enableBiometrics: boolean; + enableBrowserIntegration: boolean; + enableBrowserIntegrationFingerprint: boolean; + enableCloseToTray: boolean; + enableFullWidth: boolean; + enableGravitars: boolean; + enableMinimizeToTray: boolean; + enableStartToTray: boolean; + enableTray: boolean; + environmentUrls: any; equivalentDomains?: any; - locale?: string; - minimizeOnCopyToClipboard?: boolean; - neverDomains?: { [id: string]: any }; - noAutoPromptBiometrics?: boolean; - noAutoPromptBiometricsText?: string; - openAtLogin?: boolean; - passwordGenerationOptions?: any; - pinProtected?: EncryptionPair = new EncryptionPair(); - protectedPin?: string; - settings?: any; // TODO: Merge whatever is going on here into the AccountSettings model properly - vaultTimeout?: number; - vaultTimeoutAction?: string; + locale: string; + minimizeOnCopyToClipboard: boolean; + neverDomains: { [id: string]: any }; + noAutoPromptBiometrics: boolean; + noAutoPromptBiometricsText: string; + openAtLogin: boolean; + passwordGenerationOptions: any; + pinProtected: EncryptionPair = new EncryptionPair(); + protectedPin: string; + settings: any; // TODO: Merge whatever is going on here into the AccountSettings model properly + vaultTimeout: number; + vaultTimeoutAction: string; get serverUrl(): string { return this.environmentUrls?.base ?? 'bitwarden.com'; @@ -152,23 +152,23 @@ export class Account { Object.assign(this, { data: { ...new AccountData(), - ...init.data, + ...init?.data, }, keys: { ...new AccountKeys(), - ...init.keys, + ...init?.keys, }, profile: { ...new AccountProfile(), - ...init.profile, + ...init?.profile, }, settings: { ...new AccountSettings(), - ...init.settings, + ...init?.settings, }, tokens: { ...new AccountTokens(), - ...init.tokens, + ...init?.tokens, }, }); } diff --git a/common/src/services/auth.service.ts b/common/src/services/auth.service.ts index 78621b9c..540d7e5c 100644 --- a/common/src/services/auth.service.ts +++ b/common/src/services/auth.service.ts @@ -2,7 +2,7 @@ import { HashPurpose } from '../enums/hashPurpose'; import { KdfType } from '../enums/kdfType'; import { TwoFactorProviderType } from '../enums/twoFactorProviderType'; -import { Account } from '../models/domain/account'; +import { Account, AccountData, AccountProfile, AccountTokens } from '../models/domain/account'; import { AuthResult } from '../models/domain/authResult'; import { SymmetricCryptoKey } from '../models/domain/symmetricCryptoKey'; @@ -355,19 +355,23 @@ export class AuthService implements AuthServiceAbstraction { const accountInformation = await this.tokenService.decodeToken(tokenResponse.accessToken); await this.stateService.addAccount({ profile: { - userId: accountInformation.sub, - email: accountInformation.email, - apiKeyClientId: clientId, - apiKeyClientSecret: clientSecret, - hasPremiumPersonally: accountInformation.premium, - }, - data: { - kdfIterations: tokenResponse.kdfIterations, - kdfType: tokenResponse.kdf, + ...new AccountProfile(), + ...{ + userId: accountInformation.sub, + email: accountInformation.email, + apiKeyClientId: clientId, + apiKeyClientSecret: clientSecret, + hasPremiumPersonally: accountInformation.premium, + kdfIterations: tokenResponse.kdfIterations, + kdfType: tokenResponse.kdf, + }, }, tokens: { - accessToken: tokenResponse.accessToken, - refreshToken: tokenResponse.refreshToken, + ...new AccountTokens(), + ...{ + accessToken: tokenResponse.accessToken, + refreshToken: tokenResponse.refreshToken, + }, }, }); diff --git a/common/src/services/state.service.ts b/common/src/services/state.service.ts index 8f14dcf1..384afddb 100644 --- a/common/src/services/state.service.ts +++ b/common/src/services/state.service.ts @@ -120,12 +120,12 @@ export class StateService implements StateServiceAbstraction { } async getApiKeyClientSecret(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.profile?.apiKeyClientSecret; + return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.keys?.apiKeyClientSecret; } async setApiKeyClientSecret(value: string, options?: StorageOptions): Promise { const account = await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())); - account.profile.apiKeyClientId = value; + account.keys.apiKeyClientSecret = value; await this.saveAccount(account, this.reconcileOptions(options, await this.defaultOnDiskOptions())); } @@ -619,7 +619,7 @@ export class StateService implements StateServiceAbstraction { } async getEnableGravitars(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskLocalOptions())))?.settings?.enableGravitars ?? true; + return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskLocalOptions())))?.settings?.enableGravitars ?? false; } async setEnableGravitars(value: boolean, options?: StorageOptions): Promise { const account = await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskLocalOptions())); @@ -831,30 +831,30 @@ export class StateService implements StateServiceAbstraction { } async getKdfIterations(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.data?.kdfIterations; + return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.profile?.kdfIterations; } async setKdfIterations(value: number, options?: StorageOptions): Promise { const account = await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())); - account.data.kdfIterations = value; + account.profile.kdfIterations = value; await this.saveAccount(account, this.reconcileOptions(options, await this.defaultOnDiskOptions())); } async getKdfType(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.data?.kdfType; + return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.profile?.kdfType; } async setKdfType(value: KdfType, options?: StorageOptions): Promise { const account = await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())); - account.data.kdfType = value; + account.profile.kdfType = value; await this.saveAccount(account, this.reconcileOptions(options, await this.defaultOnDiskOptions())); } async getKeyHash(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.keys?.keyHash; + return (await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())))?.profile?.keyHash; } async setKeyHash(value: string, options?: StorageOptions): Promise { const account = await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())); - account.keys.keyHash = value; + account.profile.keyHash = value; await this.saveAccount(account, this.reconcileOptions(options, await this.defaultOnDiskOptions())); } @@ -1284,7 +1284,14 @@ export class StateService implements StateServiceAbstraction { } private async scaffoldNewAccountStorage(account: Account): Promise { - const storedState = await this.storageService.get('state', await this.defaultOnDiskOptions()) ?? new State(); + await this.scaffoldNewAccountLocalStorage(account); + await this.scaffoldNewAccountSessionStorage(account); + await this.scaffoldNewAccountMemoryStorage(account); + await this.scaffoldNewAccountSecureStorage(account); + } + + private async scaffoldNewAccountLocalStorage(account: Account): Promise { + const storedState = await this.storageService.get('state', await this.defaultOnDiskLocalOptions()) ?? new State(); const storedAccount = storedState.accounts[account.profile.userId]; if (storedAccount != null) { storedAccount.tokens.accessToken = account.tokens.accessToken; @@ -1293,10 +1300,30 @@ export class StateService implements StateServiceAbstraction { } storedState.accounts[account.profile.userId] = account; await this.storageService.save('state', storedState, await this.defaultOnDiskLocalOptions()); - await this.storageService.save('state', storedState, await this.defaultOnDiskMemoryOptions()); - await this.storageService.save('state', storedState, await this.defaultOnDiskOptions()); + } - await this.scaffoldNewAccountSecureStorage(account); + private async scaffoldNewAccountMemoryStorage(account: Account): Promise { + const storedState = await this.storageService.get('state', await this.defaultOnDiskMemoryOptions()) ?? new State(); + const storedAccount = storedState.accounts[account.profile.userId]; + if (storedAccount != null) { + storedAccount.tokens.accessToken = account.tokens.accessToken; + storedAccount.tokens.refreshToken = account.tokens.refreshToken; + account = storedAccount; + } + storedState.accounts[account.profile.userId] = account; + await this.storageService.save('state', storedState, await this.defaultOnDiskMemoryOptions()); + } + + private async scaffoldNewAccountSessionStorage(account: Account): Promise { + const storedState = await this.storageService.get('state', await this.defaultOnDiskOptions()) ?? new State(); + const storedAccount = storedState.accounts[account.profile.userId]; + if (storedAccount != null) { + storedAccount.tokens.accessToken = account.tokens.accessToken; + storedAccount.tokens.refreshToken = account.tokens.refreshToken; + account = storedAccount; + } + storedState.accounts[account.profile.userId] = account; + await this.storageService.save('state', storedState, await this.defaultOnDiskOptions()); } private async scaffoldNewAccountSecureStorage(account: Account): Promise { @@ -1368,7 +1395,7 @@ export class StateService implements StateServiceAbstraction { return { storageLocation: StorageLocation.Disk, htmlStorageLocation: HtmlStorageLocation.Memory, - userId: await this.storageService.get('activeUserId'), + userId: await this.getUserId(), useSecureStorage: false, }; } @@ -1386,38 +1413,47 @@ export class StateService implements StateServiceAbstraction { return state?.activeUserId; } - private async removeAccountFromLocalStorage(userId: string): Promise { + private async removeAccountFromLocalStorage(userId: string = this.state.activeUserId): Promise { const state = await this.secureStorageService.get('state', { htmlStorageLocation: HtmlStorageLocation.Local }); - if (state?.accounts[userId ?? this.state.activeUserId] == null) { + if (state?.accounts[userId] == null) { return; } - delete state.accounts[userId ?? this.state.activeUserId]; + state.accounts[userId] = new Account({ + profile: state.accounts[userId].profile, + settings: state.accounts[userId].settings, + data: state.accounts[userId].data, + }); + await this.storageService.save('state', state, { htmlStorageLocation: HtmlStorageLocation.Local }); } - private async removeAccountFromSessionStorage(userId: string): Promise { + private async removeAccountFromSessionStorage(userId: string = this.state.activeUserId): Promise { const state = await this.secureStorageService.get('state', { htmlStorageLocation: HtmlStorageLocation.Session }); - const account = state?.accounts[userId ?? this.state.activeUserId]; - if (account == null) { + if (state?.accounts[userId] == null) { return; } - delete state.accounts[userId ?? this.state.activeUserId]; + state.accounts[userId] = new Account({ + profile: state.accounts[userId].profile, + settings: state.accounts[userId].settings, + data: state.accounts[userId].data, + }); + await this.storageService.save('state', state, { htmlStorageLocation: HtmlStorageLocation.Session }); } - private async removeAccountFromSecureStorage(userId: string): Promise { + private async removeAccountFromSecureStorage(userId: string = this.state.activeUserId): Promise { const state = await this.secureStorageService.get('state'); - if (state?.accounts[userId ?? this.state.activeUserId] == null) { + if (state?.accounts[userId] == null) { return; } - delete state.accounts[userId ?? this.state.activeUserId]; + state.accounts[userId] = null; await this.secureStorageService.save('state', state); } - private removeAccountFromMemory(userId: string): void { - if (this.state?.accounts[userId ?? this.state.activeUserId] == null) { + private removeAccountFromMemory(userId: string = this.state.activeUserId): void { + if (this.state?.accounts[userId] == null) { return; } delete this.state.accounts[userId ?? this.state.activeUserId];