From 57d60bdfa6355571f4e2e738ce0a56a48bd19414 Mon Sep 17 00:00:00 2001 From: Addison Beck Date: Fri, 7 Jan 2022 09:30:54 -0500 Subject: [PATCH] Misc Account Switching Fixes & Refactors (#600) * [refactor] Restructure EnvironmentUrls in state * Patch up (add missing fields) and more extensivly use the EnvironmentUrls class instead of passing around an any * Add environmentUrls to the AccountSettings model in addition to GlobalState for use in both scopes * Move EnvironmentUrls initialization to the model level and out of StateSerice * Adjust the StateMigrationService to account for these changes * [refactor] Improve order of operations for LockGuardService We currently jump through a bunch of hoops to verify users can access the Lock page, like checking authentication first. If a user is not authenticated, they are not locked, so we can improve performance for the happy path of this serivice by checking isLocked first and using isAuthenticated to deviate from the normal flow if needed. * [bug] Subscribe to State.accounts in EnvironmentService and set urls accordingly The EnvironmentService has no context for account changes currently and does not update actively used urls based on active account. This commit addresses this issue by subscribing to State.accounts and resetting the service's urls on account change. * [bug] Clear AccessToken from State on clean In order for logout flows to function as expected we need to deauthenticate users when cleaning up state before checking for the next active user Otherwise the service will continue to think the user being logged out is active * [refactor] Stop pushing accounts when modifying disk state There is no reason to push new accounts to subscribers when updating disk state. Subscribers recieve a copy of in memory state, so changes to disk will not be refelected and have to be fetched seperatly from the service. Pushing when saving disk state is just creating an unecassary performance burden. * [refactor] Default to in memory active user if availible, even when accessing disk state Sometimes we need to pull activeUserId from storage to access a bit of data, like on initial boot, but most of the time this isn't necassary. Since we pull this userId a lot, checking disk each time is a performance burden. Defaulting to the in memory user ID if avaible helps alleviate this. * [style] Ran prettier * [style] Change a let to a const --- angular/src/services/lock-guard.service.ts | 16 ++++---- common/src/models/domain/account.ts | 2 + common/src/models/domain/environmentUrls.ts | 4 ++ common/src/models/domain/globalState.ts | 5 +-- common/src/services/environment.service.ts | 6 ++- common/src/services/state.service.ts | 39 +++++++++---------- common/src/services/stateMigration.service.ts | 10 ++++- 7 files changed, 49 insertions(+), 33 deletions(-) diff --git a/angular/src/services/lock-guard.service.ts b/angular/src/services/lock-guard.service.ts index b0598c34ea..517e440658 100644 --- a/angular/src/services/lock-guard.service.ts +++ b/angular/src/services/lock-guard.service.ts @@ -7,6 +7,7 @@ import { VaultTimeoutService } from "jslib-common/abstractions/vaultTimeout.serv @Injectable() export class LockGuardService implements CanActivate { protected homepage = "vault"; + protected loginpage = ""; constructor( private vaultTimeoutService: VaultTimeoutService, private router: Router, @@ -14,16 +15,15 @@ export class LockGuardService implements CanActivate { ) {} async canActivate() { - if (!(await this.stateService.getIsAuthenticated())) { - this.router.navigate(["login"]); - return false; + if (await this.vaultTimeoutService.isLocked()) { + return true; } - if (!(await this.vaultTimeoutService.isLocked())) { - this.router.navigate([this.homepage]); - return false; - } + const redirectUrl = (await this.stateService.getIsAuthenticated()) + ? [this.homepage] + : [this.loginpage]; - return true; + this.router.navigate(redirectUrl); + return false; } } diff --git a/common/src/models/domain/account.ts b/common/src/models/domain/account.ts index 6a99813870..565b86e6a3 100644 --- a/common/src/models/domain/account.ts +++ b/common/src/models/domain/account.ts @@ -21,6 +21,7 @@ import { FolderData } from "../data/folderData"; import { PolicyData } from "../data/policyData"; import { ProviderData } from "../data/providerData"; import { SendData } from "../data/sendData"; +import { EnvironmentUrls } from "./environmentUrls"; export class EncryptionPair { encrypted?: TEncrypted; @@ -130,6 +131,7 @@ export class AccountSettings { enableMinimizeToTray?: boolean; enableStartToTray?: boolean; enableTray?: boolean; + environmentUrls: EnvironmentUrls = new EnvironmentUrls(); equivalentDomains?: any; minimizeOnCopyToClipboard?: boolean; neverDomains?: { [id: string]: any }; diff --git a/common/src/models/domain/environmentUrls.ts b/common/src/models/domain/environmentUrls.ts index 6426fef484..c80265924c 100644 --- a/common/src/models/domain/environmentUrls.ts +++ b/common/src/models/domain/environmentUrls.ts @@ -2,5 +2,9 @@ export class EnvironmentUrls { base: string; api: string; identity: string; + icons: string; + notifications: string; events: string; + webVault: string; + keyConnector: string; } diff --git a/common/src/models/domain/globalState.ts b/common/src/models/domain/globalState.ts index 0dcf493902..24dc54ae68 100644 --- a/common/src/models/domain/globalState.ts +++ b/common/src/models/domain/globalState.ts @@ -1,4 +1,5 @@ import { StateVersion } from "../../enums/stateVersion"; +import { EnvironmentUrls } from "./environmentUrls"; export class GlobalState { enableAlwaysOnTop?: boolean; @@ -26,7 +27,5 @@ export class GlobalState { noAutoPromptBiometrics?: boolean; noAutoPromptBiometricsText?: string; stateVersion: StateVersion = StateVersion.Latest; - environmentUrls?: any = { - server: "bitwarden.com", - }; + environmentUrls: EnvironmentUrls = new EnvironmentUrls(); } diff --git a/common/src/services/environment.service.ts b/common/src/services/environment.service.ts index cf435d878d..cf59d38959 100644 --- a/common/src/services/environment.service.ts +++ b/common/src/services/environment.service.ts @@ -21,7 +21,11 @@ export class EnvironmentService implements EnvironmentServiceAbstraction { private eventsUrl: string; private keyConnectorUrl: string; - constructor(private stateService: StateService) {} + constructor(private stateService: StateService) { + this.stateService.activeAccount.subscribe(async (_userId) => { + await this.setUrlsFromStorage(); + }); + } hasBaseUrl() { return this.baseUrl != null; diff --git a/common/src/services/state.service.ts b/common/src/services/state.service.ts index 852809969d..aefdf6652a 100644 --- a/common/src/services/state.service.ts +++ b/common/src/services/state.service.ts @@ -41,6 +41,7 @@ import { SendData } from "../models/data/sendData"; import { BehaviorSubject } from "rxjs"; import { StateMigrationService } from "../abstractions/stateMigration.service"; +import { EnvironmentUrls } from "../models/domain/environmentUrls"; export class StateService implements StateServiceAbstraction @@ -79,6 +80,7 @@ export class StateService } async addAccount(account: TAccount) { + await this.setAccountEnvironmentUrls(account); this.state.accounts[account.profile.userId] = account; await this.scaffoldNewAccountStorage(account); await this.setActiveUser(account.profile.userId); @@ -99,6 +101,7 @@ export class StateService async clean(options?: StorageOptions): Promise { // Find and set the next active user if any exists + await this.setAccessToken(null, { userId: options?.userId }); if (options?.userId == null || options.userId === (await this.getUserId())) { for (const userId in this.state.accounts) { if (userId == null) { @@ -1359,24 +1362,16 @@ export class StateService } async getEnvironmentUrls(options?: StorageOptions): Promise { - return ( - (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) - ?.environmentUrls ?? { - base: null, - api: null, - identity: null, - icons: null, - notifications: null, - events: null, - webVault: null, - keyConnector: null, - // TODO: this is a bug and we should use base instead for the server detail in the account switcher, otherwise self hosted urls will not show correctly - server: "bitwarden.com", - } - ); + options = this.reconcileOptions(options, await this.defaultOnDiskOptions()); + if (this.state.activeUserId == null) { + return (await this.getGlobals(options)).environmentUrls ?? new EnvironmentUrls(); + } + return (await this.getAccount(options))?.settings?.environmentUrls ?? new EnvironmentUrls(); } async setEnvironmentUrls(value: any, options?: StorageOptions): Promise { + // Global values are set on each change and the current global settings are passed to any newly authed accounts. + // This is to allow setting environement values before an account is active, while still allowing individual accounts to have their own environments. const globals = await this.getGlobals( this.reconcileOptions(options, await this.defaultOnDiskOptions()) ); @@ -2189,7 +2184,6 @@ export class StateService state.accounts[account.profile.userId] = account; await storageLocation.save("state", state, options); - await this.pushAccounts(); } protected async saveAccountToMemory(account: TAccount): Promise { @@ -2283,7 +2277,7 @@ export class StateService return { storageLocation: StorageLocation.Disk, htmlStorageLocation: HtmlStorageLocation.Session, - userId: await this.getActiveUserIdFromStorage(), + userId: this.state.activeUserId ?? (await this.getActiveUserIdFromStorage()), useSecureStorage: false, }; } @@ -2292,7 +2286,7 @@ export class StateService return { storageLocation: StorageLocation.Disk, htmlStorageLocation: HtmlStorageLocation.Local, - userId: await this.getActiveUserIdFromStorage(), + userId: this.state.activeUserId ?? (await this.getActiveUserIdFromStorage()), useSecureStorage: false, }; } @@ -2301,7 +2295,7 @@ export class StateService return { storageLocation: StorageLocation.Disk, htmlStorageLocation: HtmlStorageLocation.Memory, - userId: await this.getUserId(), + userId: this.state.activeUserId ?? (await this.getUserId()), useSecureStorage: false, }; } @@ -2310,7 +2304,7 @@ export class StateService return { storageLocation: StorageLocation.Disk, useSecureStorage: true, - userId: await this.getActiveUserIdFromStorage(), + userId: this.state.activeUserId ?? (await this.getActiveUserIdFromStorage()), }; } @@ -2381,4 +2375,9 @@ export class StateService account.tokens = new AccountTokens(); return account; } + + protected async setAccountEnvironmentUrls(account: TAccount): Promise { + account.settings.environmentUrls = await this.getEnvironmentUrls(); + return account; + } } diff --git a/common/src/services/stateMigration.service.ts b/common/src/services/stateMigration.service.ts index 864a243abb..5c5ee37fa8 100644 --- a/common/src/services/stateMigration.service.ts +++ b/common/src/services/stateMigration.service.ts @@ -18,6 +18,7 @@ import { SendData } from "../models/data/sendData"; import { HtmlStorageLocation } from "../enums/htmlStorageLocation"; import { KdfType } from "../enums/kdfType"; import { StateVersion } from "../enums/stateVersion"; +import { EnvironmentUrls } from "../models/domain/environmentUrls"; // Originally (before January 2022) storage was handled as a flat key/value pair store. // With the move to a typed object for state storage these keys should no longer be in use anywhere outside of this migration. @@ -176,7 +177,9 @@ export class StateMigrationService { v1Keys.enableBiometric, options ), - environmentUrls: await this.storageService.get(v1Keys.environmentUrls, options), + environmentUrls: + (await this.storageService.get(v1Keys.environmentUrls, options)) ?? + new EnvironmentUrls(), installedVersion: await this.storageService.get( v1Keys.installedVersion, options @@ -442,6 +445,11 @@ export class StateMigrationService { options ), enableTray: await this.storageService.get(v1Keys.enableTray, options), + environmentUrls: + (await this.storageService.get( + v1Keys.environmentUrls, + options + )) ?? new EnvironmentUrls(), equivalentDomains: await this.storageService.get( v1Keys.equivalentDomains, options