From e8970725be472386358c1e2f06f53663c4979e0e Mon Sep 17 00:00:00 2001 From: addison Date: Thu, 11 Nov 2021 16:27:40 -0500 Subject: [PATCH] [bug] Keep up with entire state in storage instead of just accounts and globals Not having access to the last active user was creating issues across clients when restarting the process. For example: when refreshing the page on web we no longer maintain an understanding of who is logged in. To resolve this I converted all storage save operations to get and save an entire state object, instead of specifying accounts and globals. This allows for more flexible saving, like saving activeUserId as a top level storage item. --- common/src/abstractions/state.service.ts | 1 + common/src/models/domain/account.ts | 34 +------ common/src/models/domain/state.ts | 2 +- common/src/services/state.service.ts | 108 ++++++++++++++++------- 4 files changed, 77 insertions(+), 68 deletions(-) diff --git a/common/src/abstractions/state.service.ts b/common/src/abstractions/state.service.ts index 3cbdd594..02140f24 100644 --- a/common/src/abstractions/state.service.ts +++ b/common/src/abstractions/state.service.ts @@ -27,6 +27,7 @@ import { SendView } from '../models/view/sendView'; export abstract class StateService { accounts: BehaviorSubject<{ [userId: string]: Account }>; + init: () => Promise; addAccount: (account: Account) => Promise; setActiveUser: (userId: string) => Promise; clean: (options?: StorageOptions) => Promise; diff --git a/common/src/models/domain/account.ts b/common/src/models/domain/account.ts index e2fb8766..77fa9f30 100644 --- a/common/src/models/domain/account.ts +++ b/common/src/models/domain/account.ts @@ -126,8 +126,7 @@ export class Account { convertAccountToKeyConnector: boolean; usesKeyConnector: boolean; enableFullWidth: boolean; - - private hasPremiumPersonally: boolean; + hasPremiumPersonally: boolean; constructor(userId: string, userEmail: string, kdfType: KdfType, kdfIterations: number, @@ -145,39 +144,8 @@ export class Account { this.hasPremiumPersonally = hasPremium; } - get isAuthenticated(): boolean { - if (!this.accessToken) { - return false; - } - - return this.userId != null; - } - - get canAccessPremium(): boolean { - if (!this.isAuthenticated) { - return false; - } - - return this.hasPremiumPersonally || this.hasPremiumThroughOrganization; - } - get serverUrl(): string { return this.environmentUrls?.base ?? 'bitwarden.com'; } - - private get hasPremiumThroughOrganization(): boolean { - if (this.organizations == null) { - return false; - } - - for (const id of Object.keys(this.organizations)) { - const o = this.organizations[id]; - if (o.enabled && o.usersGetPremium && !o.isProviderUser) { - return true; - } - } - - return false; - } } diff --git a/common/src/models/domain/state.ts b/common/src/models/domain/state.ts index 9a0a32c6..a0099e5d 100644 --- a/common/src/models/domain/state.ts +++ b/common/src/models/domain/state.ts @@ -3,7 +3,7 @@ import { GlobalState } from './globalState'; export class State { accounts: { [userId: string]: Account } = {}; - globals: GlobalState; + globals: GlobalState = new GlobalState(); activeUserId: string; } diff --git a/common/src/services/state.service.ts b/common/src/services/state.service.ts index 5d3a804d..e64f14d8 100644 --- a/common/src/services/state.service.ts +++ b/common/src/services/state.service.ts @@ -63,6 +63,17 @@ export class StateService implements StateServiceAbstraction { private logService: LogService) { } + async init(): Promise { + if (this.state.activeUserId == null) { + await this.loadStateFromDisk(); + } + } + + async loadStateFromDisk() { + const diskState = await this.storageService.get('state', this.defaultOnDiskLocalOptions); + this.state = diskState; + } + async addAccount(account: Account) { this.state.accounts[account.userId] = account; await this.scaffoldNewAccountStorage(account); @@ -70,10 +81,10 @@ export class StateService implements StateServiceAbstraction { } async setActiveUser(userId: string): Promise { - if (!this.state.accounts[userId]) { - return; - } this.state.activeUserId = userId; + const storedState = await this.storageService.get('state', this.defaultOnDiskLocalOptions); + storedState.activeUserId = userId; + await this.storageService.save('state', storedState, this.defaultOnDiskLocalOptions); await this.pushAccounts(); } @@ -85,13 +96,13 @@ export class StateService implements StateServiceAbstraction { } async getAccessToken(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, this.defaultInMemoryOptions)))?.accessToken; + return (await this.getAccount(this.reconcileOptions(options, this.defaultOnDiskLocalOptions)))?.accessToken; } async setAccessToken(value: string, options?: StorageOptions): Promise { - const account = await this.getAccount(this.reconcileOptions(options, this.defaultInMemoryOptions)); + const account = await this.getAccount(this.reconcileOptions(options, this.defaultOnDiskLocalOptions)); account.accessToken = value; - await this.saveAccount(account, this.reconcileOptions(options, this.defaultInMemoryOptions)); + await this.saveAccount(account, this.reconcileOptions(options, this.defaultOnDiskLocalOptions)); } async getAddEditCipherInfo(options?: StorageOptions): Promise { @@ -205,7 +216,28 @@ export class StateService implements StateServiceAbstraction { } async getCanAccessPremium(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, this.defaultInMemoryOptions)))?.canAccessPremium ?? false; + if (!await this.getIsAuthenticated(options)) { + return false; + } + + const account = await this.getAccount(this.reconcileOptions(options, this.defaultOnDiskLocalOptions)); + if (account.hasPremiumPersonally) { + return true; + } + + const organizations = await this.getOrganizations(options); + if (organizations == null) { + return false; + } + + for (const id of Object.keys(organizations)) { + const o = organizations[id]; + if (o.enabled && o.usersGetPremium && !o.isProviderUser) { + return true; + } + } + + return false; } async getClearClipboard(options?: StorageOptions): Promise { @@ -800,7 +832,8 @@ export class StateService implements StateServiceAbstraction { } async getIsAuthenticated(options?: StorageOptions): Promise { - return (await this.getAccount(this.reconcileOptions(options, this.defaultInMemoryOptions)))?.isAuthenticated ?? false; + const account = (await this.getAccount(this.reconcileOptions(options, this.defaultOnDiskLocalOptions))); + return account != null && account?.accessToken != null && account?.userId != null; } async getKdfIterations(options?: StorageOptions): Promise { @@ -1164,7 +1197,7 @@ export class StateService implements StateServiceAbstraction { } private async getGlobalsFromDisk(options: StorageOptions): Promise { - return await this.storageService.get('globals', options); + return (await this.storageService.get('state', options))?.globals; } private saveGlobalsToMemory(globals: GlobalState): void { @@ -1173,16 +1206,17 @@ export class StateService implements StateServiceAbstraction { private async saveGlobalsToDisk(globals: GlobalState, options: StorageOptions): Promise { if (options.useSecureStorage) { - await this.secureStorageService.save('globals', globals, options); + const state = await this.secureStorageService.get('state', options) ?? new State(); + state.globals = globals; + await this.secureStorageService.save('state', state, options); } else { - await this.storageService.save('globals', globals, options); + const state = await this.storageService.get('state', options) ?? new State(); + state.globals = globals; + await this.storageService.save('state', state, options); } } - private async getAccount(options: StorageOptions = { - storageLocation: StorageLocation.Both, - useSecureStorage: false, - }): Promise { + private async getAccount(options: StorageOptions): Promise { try { let account: Account; if (this.useMemory(options.storageLocation)) { @@ -1218,10 +1252,11 @@ export class StateService implements StateServiceAbstraction { return null; } - const account = options?.useSecureStorage ? - await this.secureStorageService.get(options.userId ?? this.state.activeUserId, options) : - await this.storageService.get(options.userId ?? this.state.activeUserId, options); - return account; + const state = options?.useSecureStorage ? + await this.secureStorageService.get('state', options) : + await this.storageService.get('state', options); + + return state.accounts[options?.userId ?? this.state.activeUserId] } private useMemory(storageLocation: StorageLocation) { @@ -1244,11 +1279,14 @@ export class StateService implements StateServiceAbstraction { } private async saveAccountToDisk(account: Account, options: StorageOptions): Promise { - if (options.useSecureStorage) { - await this.secureStorageService.save(account.userId, account, options); - } else { - await this.storageService.save(account.userId, account, options); - } + const storageLocation = options.useSecureStorage ? + this.secureStorageService : + this.storageService; + + const state = await storageLocation.get('state', options); + state.accounts[account.userId] = account; + + await storageLocation.save('state', state, options) await this.pushAccounts(); } @@ -1260,18 +1298,20 @@ export class StateService implements StateServiceAbstraction { } private async scaffoldNewAccountStorage(account: Account): Promise { - const storageAccount = await this.storageService.get(account.userId, this.defaultOnDiskLocalOptions); - if (storageAccount != null) { - storageAccount.accessToken = account.accessToken; - storageAccount.refreshToken = account.refreshToken; - account = storageAccount; + const storedState = await this.storageService.get('state', this.defaultOnDiskLocalOptions) ?? new State(); + const storedAccount = storedState.accounts[account.userId]; + if (storedAccount != null) { + storedAccount.accessToken = account.accessToken; + storedAccount.refreshToken = account.refreshToken; + account = storedAccount; } - await this.storageService.save(account.userId, account, this.defaultOnDiskLocalOptions); - await this.storageService.save(account.userId, account, this.defaultOnDiskMemoryOptions); - await this.storageService.save(account.userId, account); + storedState.accounts[account.userId] = account; + await this.storageService.save('state', storedState, this.defaultOnDiskLocalOptions); + await this.storageService.save('state', storedState, this.defaultOnDiskMemoryOptions); + await this.storageService.save('state', storedState); - if (await this.secureStorageService.get(account.userId) == null) { - await this.secureStorageService.save(account.userId, account); + if (await this.secureStorageService.get('state') == null) { + await this.secureStorageService.save('state', storedState); } }