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

@@ -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())) {
+    if (await this.vaultTimeoutService.isLocked()) {
-      this.router.navigate(["login"]);
+      return true;
-      return false;
     }
 
-    if (!(await this.vaultTimeoutService.isLocked())) {
+    const redirectUrl = (await this.stateService.getIsAuthenticated())
-      this.router.navigate([this.homepage]);
+      ? [this.homepage]
-      return false;
+      : [this.loginpage];
-    }
 
-    return true;
+    this.router.navigate(redirectUrl);
+    return false;
   }
 }

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<TEncrypted, TDecrypted> {
   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 };

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;
 }

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 = {
+  environmentUrls: EnvironmentUrls = new EnvironmentUrls();
-    server: "bitwarden.com",
-  };
 }

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;

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<TAccount extends Account = Account>
   implements StateServiceAbstraction<TAccount>
@@ -79,6 +80,7 @@ export class StateService<TAccount extends Account = Account>
   }
 
   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<TAccount extends Account = Account>
 
   async clean(options?: StorageOptions): Promise<void> {
     // 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<TAccount extends Account = Account>
   }
 
   async getEnvironmentUrls(options?: StorageOptions): Promise<any> {
-    return (
+    options = this.reconcileOptions(options, await this.defaultOnDiskOptions());
-      (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions())))
+    if (this.state.activeUserId == null) {
-        ?.environmentUrls ?? {
+      return (await this.getGlobals(options)).environmentUrls ?? new EnvironmentUrls();
-        base: null,
+    }
-        api: null,
+    return (await this.getAccount(options))?.settings?.environmentUrls ?? new EnvironmentUrls();
-        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",
-      }
-    );
   }
 
   async setEnvironmentUrls(value: any, options?: StorageOptions): Promise<void> {
+    // 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<TAccount extends Account = Account>
     state.accounts[account.profile.userId] = account;
 
     await storageLocation.save("state", state, options);
-    await this.pushAccounts();
   }
 
   protected async saveAccountToMemory(account: TAccount): Promise<void> {
@@ -2283,7 +2277,7 @@ export class StateService<TAccount extends Account = Account>
     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<TAccount extends Account = Account>
     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<TAccount extends Account = Account>
     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<TAccount extends Account = Account>
     return {
       storageLocation: StorageLocation.Disk,
       useSecureStorage: true,
-      userId: await this.getActiveUserIdFromStorage(),
+      userId: this.state.activeUserId ?? (await this.getActiveUserIdFromStorage()),
     };
   }
 
@@ -2381,4 +2375,9 @@ export class StateService<TAccount extends Account = Account>
     account.tokens = new AccountTokens();
     return account;
   }
+
+  protected async setAccountEnvironmentUrls(account: TAccount): Promise<TAccount> {
+    account.settings.environmentUrls = await this.getEnvironmentUrls();
+    return account;
+  }
 }

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<any>(v1Keys.environmentUrls, options),
+              environmentUrls:
+                (await this.storageService.get<EnvironmentUrls>(v1Keys.environmentUrls, options)) ??
+                new EnvironmentUrls(),
               installedVersion: await this.storageService.get<string>(
                 v1Keys.installedVersion,
                 options
@@ -442,6 +445,11 @@ export class StateMigrationService {
                     options
                   ),
                   enableTray: await this.storageService.get<boolean>(v1Keys.enableTray, options),
+                  environmentUrls:
+                    (await this.storageService.get<EnvironmentUrls>(
+                      v1Keys.environmentUrls,
+                      options
+                    )) ?? new EnvironmentUrls(),
                   equivalentDomains: await this.storageService.get<any>(
                     v1Keys.equivalentDomains,
                     options