Fix extra signalr connection web (#15633)

* Revert "fix(SignalR): Revert "[PM-23062] Fix extra signalr connections""

This reverts commit 97ec9a6339.

* Fix first login on web

apps/web/src/app/platform/web-environment.service.ts

@@ -1,7 +1,7 @@
 // FIXME: Update this file to be type safe and remove this and next line
 // @ts-strict-ignore
 import { Router } from "@angular/router";
-import { firstValueFrom, ReplaySubject } from "rxjs";
+import { firstValueFrom, Observable, ReplaySubject } from "rxjs";
 
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import {
@@ -16,6 +16,7 @@ import {
   SelfHostedEnvironment,
 } from "@bitwarden/common/platform/services/default-environment.service";
 import { StateProvider } from "@bitwarden/common/platform/state";
+import { UserId } from "@bitwarden/user-core";
 
 export type WebRegionConfig = RegionConfig & {
   key: Region | string; // strings are used for custom environments
@@ -27,6 +28,8 @@ export type WebRegionConfig = RegionConfig & {
  * Web specific environment service. Ensures that the urls are set from the window location.
  */
 export class WebEnvironmentService extends DefaultEnvironmentService {
+  private _environmentSubject: ReplaySubject<Environment>;
+
   constructor(
     private win: Window,
     stateProvider: StateProvider,
@@ -60,7 +63,9 @@ export class WebEnvironmentService extends DefaultEnvironmentService {
     // Override the environment observable with a replay subject
     const subject = new ReplaySubject<Environment>(1);
     subject.next(environment);
+    this._environmentSubject = subject;
     this.environment$ = subject.asObservable();
+    this.globalEnvironment$ = subject.asObservable();
   }
 
   // Web setting env means navigating to a new location
@@ -100,6 +105,12 @@ export class WebEnvironmentService extends DefaultEnvironmentService {
     // This return shouldn't matter as we are about to leave the current window
     return chosenRegionConfig.urls;
   }
+
+  getEnvironment$(userId: UserId): Observable<Environment> {
+    // Web does not support account switching, and even if it did, you'd be required to be the environment of where the application
+    // is running.
+    return this._environmentSubject.asObservable();
+  }
 }
 
 export class WebCloudEnvironment extends CloudEnvironment {

libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts

@@ -5,5 +5,5 @@ export abstract class ConfigApiServiceAbstraction {
   /**
    * Fetches the server configuration for the given user. If no user is provided, the configuration will not contain user-specific context.
    */
-  abstract get(userId: UserId | undefined): Promise<ServerConfigResponse>;
+  abstract get(userId: UserId | null): Promise<ServerConfigResponse>;
 }

libs/common/src/platform/abstractions/environment.service.ts

@@ -95,6 +95,13 @@ export interface Environment {
  */
 export abstract class EnvironmentService {
   abstract environment$: Observable<Environment>;
+
+  /**
+   * The environment stored in global state, when a user signs in the state stored here will become
+   * their user environment.
+   */
+  abstract globalEnvironment$: Observable<Environment>;
+
   abstract cloudWebVaultUrl$: Observable<string>;
 
   /**
@@ -125,12 +132,12 @@ export abstract class EnvironmentService {
    * @param userId - The user id to set the cloud web vault app URL for. If null or undefined the global environment is set.
    * @param region - The region of the cloud web vault app.
    */
-  abstract setCloudRegion(userId: UserId, region: Region): Promise<void>;
+  abstract setCloudRegion(userId: UserId | null, region: Region): Promise<void>;
 
   /**
    * Get the environment from state. Useful if you need to get the environment for another user.
    */
-  abstract getEnvironment$(userId: UserId): Observable<Environment | undefined>;
+  abstract getEnvironment$(userId: UserId): Observable<Environment>;
 
   /**
    * @deprecated Use {@link getEnvironment$} instead.

libs/common/src/platform/services/config/config-api.service.ts

@@ -10,7 +10,7 @@ export class ConfigApiService implements ConfigApiServiceAbstraction {
     private tokenService: TokenService,
   ) {}
 
-  async get(userId: UserId | undefined): Promise<ServerConfigResponse> {
+  async get(userId: UserId | null): Promise<ServerConfigResponse> {
     // Authentication adds extra context to config responses, if the user has an access token, we want to use it
     // We don't particularly care about ensuring the token is valid and not expired, just that it exists
     const authed: boolean =

libs/common/src/platform/services/config/config.service.spec.ts

@@ -10,9 +10,9 @@ import {
   FakeGlobalState,
   FakeSingleUserState,
   FakeStateProvider,
-  awaitAsync,
   mockAccountServiceWith,
 } from "../../../../spec";
+import { Matrix } from "../../../../spec/matrix";
 import { subscribeTo } from "../../../../spec/observable-tracker";
 import { AuthService } from "../../../auth/abstractions/auth.service";
 import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
@@ -74,7 +74,8 @@ describe("ConfigService", () => {
     });
 
     beforeEach(() => {
-      environmentService.environment$ = environmentSubject;
+      Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
+      environmentService.globalEnvironment$ = environmentSubject;
       sut = new DefaultConfigService(
         configApiService,
         environmentService,
@@ -98,9 +99,12 @@ describe("ConfigService", () => {
             : serverConfigFactory(activeApiUrl + userId, tooOld);
         const globalStored =
           configStateDescription === "missing"
-            ? {}
+            ? {
+                [activeApiUrl]: null,
+              }
             : {
                 [activeApiUrl]: serverConfigFactory(activeApiUrl, tooOld),
+                [activeApiUrl + "0"]: serverConfigFactory(activeApiUrl + userId, tooOld),
               };
 
         beforeEach(() => {
@@ -108,11 +112,6 @@ describe("ConfigService", () => {
           userState.nextState(userStored);
         });
 
-        // sanity check
-        test("authed and unauthorized state are different", () => {
-          expect(globalStored[activeApiUrl]).not.toEqual(userStored);
-        });
-
         describe("fail to fetch", () => {
           beforeEach(() => {
             configApiService.get.mockRejectedValue(new Error("Unable to fetch"));
@@ -178,6 +177,7 @@ describe("ConfigService", () => {
         beforeEach(() => {
           globalState.stateSubject.next(globalStored);
           userState.nextState(userStored);
+          Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
         });
         it("does not fetch from server", async () => {
           await firstValueFrom(sut.serverConfig$);
@@ -189,21 +189,13 @@ describe("ConfigService", () => {
           const actual = await firstValueFrom(sut.serverConfig$);
           expect(actual).toEqual(activeUserId ? userStored : globalStored[activeApiUrl]);
         });
-
-        it("does not complete after emit", async () => {
-          const emissions = [];
-          const subscription = sut.serverConfig$.subscribe((v) => emissions.push(v));
-          await awaitAsync();
-          expect(emissions.length).toBe(1);
-          expect(subscription.closed).toBe(false);
-        });
       });
     });
   });
 
   it("gets global config when there is an locked active user", async () => {
     await accountService.switchAccount(userId);
-    environmentService.environment$ = of(environmentFactory(activeApiUrl));
+    environmentService.globalEnvironment$ = of(environmentFactory(activeApiUrl));
 
     globalState.stateSubject.next({
       [activeApiUrl]: serverConfigFactory(activeApiUrl + "global"),
@@ -236,7 +228,8 @@ describe("ConfigService", () => {
 
     beforeEach(() => {
       environmentSubject = new Subject<Environment>();
-      environmentService.environment$ = environmentSubject;
+      environmentService.globalEnvironment$ = environmentSubject;
+      Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
       sut = new DefaultConfigService(
         configApiService,
         environmentService,
@@ -327,7 +320,8 @@ describe("ConfigService", () => {
 
     beforeEach(async () => {
       const config = serverConfigFactory("existing-data", tooOld);
-      environmentService.environment$ = environmentSubject;
+      environmentService.globalEnvironment$ = environmentSubject;
+      Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
 
       globalState.stateSubject.next({ [apiUrl(0)]: config });
       userState.stateSubject.next({

libs/common/src/platform/services/config/default-config.service.ts

@@ -1,17 +1,18 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import {
   combineLatest,
+  distinctUntilChanged,
   firstValueFrom,
   map,
   mergeWith,
   NEVER,
   Observable,
   of,
-  shareReplay,
+  ReplaySubject,
+  share,
   Subject,
   switchMap,
   tap,
+  timer,
 } from "rxjs";
 import { SemVer } from "semver";
 
@@ -50,11 +51,15 @@ export const GLOBAL_SERVER_CONFIGURATIONS = KeyDefinition.record<ServerConfig, A
   },
 );
 
+const environmentComparer = (previous: Environment, current: Environment) => {
+  return previous.getApiUrl() === current.getApiUrl();
+};
+
 // FIXME: currently we are limited to api requests for active users. Update to accept a UserId and APIUrl once ApiService supports it.
 export class DefaultConfigService implements ConfigService {
-  private failedFetchFallbackSubject = new Subject<ServerConfig>();
+  private failedFetchFallbackSubject = new Subject<ServerConfig | null>();
 
-  serverConfig$: Observable<ServerConfig>;
+  serverConfig$: Observable<ServerConfig | null>;
 
   serverSettings$: Observable<ServerSettings>;
 
@@ -67,32 +72,61 @@ export class DefaultConfigService implements ConfigService {
     private stateProvider: StateProvider,
     private authService: AuthService,
   ) {
-    const userId$ = this.stateProvider.activeUserId$;
+    const globalConfig$ = this.environmentService.globalEnvironment$.pipe(
-    const authStatus$ = userId$.pipe(
+      distinctUntilChanged(environmentComparer),
-      switchMap((userId) => (userId == null ? of(null) : this.authService.authStatusFor$(userId))),
+      switchMap((environment) =>
+        this.globalConfigFor$(environment.getApiUrl()).pipe(
+          map((config) => {
+            return [config, null as UserId | null, environment, config] as const;
+          }),
+        ),
+      ),
     );
 
-    this.serverConfig$ = combineLatest([
+    this.serverConfig$ = this.stateProvider.activeUserId$.pipe(
-      userId$,
+      distinctUntilChanged(),
-      this.environmentService.environment$,
+      switchMap((userId) => {
-      authStatus$,
+        if (userId == null) {
-    ]).pipe(
+          // Global
-      switchMap(([userId, environment, authStatus]) => {
+          return globalConfig$;
-        if (userId == null || authStatus !== AuthenticationStatus.Unlocked) {
-          return this.globalConfigFor$(environment.getApiUrl()).pipe(
-            map((config) => [config, null, environment] as const),
-          );
         }
 
-        return this.userConfigFor$(userId).pipe(
+        return this.authService.authStatusFor$(userId).pipe(
-          map((config) => [config, userId, environment] as const),
+          map((authStatus) => authStatus === AuthenticationStatus.Unlocked),
+          distinctUntilChanged(),
+          switchMap((isUnlocked) => {
+            if (!isUnlocked) {
+              return globalConfig$;
+            }
+
+            return combineLatest([
+              this.environmentService
+                .getEnvironment$(userId)
+                .pipe(distinctUntilChanged(environmentComparer)),
+              this.userConfigFor$(userId),
+            ]).pipe(
+              switchMap(([environment, config]) => {
+                if (config == null) {
+                  // If the user doesn't have any config yet, use the global config for that url as the fallback
+                  return this.globalConfigFor$(environment.getApiUrl()).pipe(
+                    map(
+                      (globalConfig) =>
+                        [null as ServerConfig | null, userId, environment, globalConfig] as const,
+                    ),
+                  );
+                }
+
+                return of([config, userId, environment, config] as const);
+              }),
+            );
+          }),
         );
       }),
       tap(async (rec) => {
-        const [existingConfig, userId, environment] = rec;
+        const [existingConfig, userId, environment, fallbackConfig] = rec;
         // Grab new config if older retrieval interval
         if (!existingConfig || this.olderThanRetrievalInterval(existingConfig.utcDate)) {
-          await this.renewConfig(existingConfig, userId, environment);
+          await this.renewConfig(existingConfig, userId, environment, fallbackConfig);
         }
       }),
       switchMap(([existingConfig]) => {
@@ -106,7 +140,7 @@ export class DefaultConfigService implements ConfigService {
       }),
       // If fetch fails, we'll emit on this subject to fallback to the existing config
       mergeWith(this.failedFetchFallbackSubject),
-      shareReplay({ refCount: true, bufferSize: 1 }),
+      share({ connector: () => new ReplaySubject(1), resetOnRefCountZero: () => timer(1000) }),
     );
 
     this.cloudRegion$ = this.serverConfig$.pipe(
@@ -155,19 +189,18 @@ export class DefaultConfigService implements ConfigService {
 
   // Updates the on-disk configuration with a newly retrieved configuration
   private async renewConfig(
-    existingConfig: ServerConfig,
+    existingConfig: ServerConfig | null,
-    userId: UserId,
+    userId: UserId | null,
     environment: Environment,
+    fallbackConfig: ServerConfig | null,
   ): Promise<void> {
     try {
       // Feature flags often have a big impact on user experience, lets ensure we return some value
       // somewhat quickly even though it may not be accurate, we won't cancel the HTTP request
       // though so that hopefully it can have finished and hydrated a more accurate value.
       const handle = setTimeout(() => {
-        this.logService.info(
+        this.logService.info("Environment did not respond in time, emitting previous config.");
-          "Self-host environment did not respond in time, emitting previous config.",
+        this.failedFetchFallbackSubject.next(fallbackConfig);
-        );
-        this.failedFetchFallbackSubject.next(existingConfig);
       }, SLOW_EMISSION_GUARD);
       const response = await this.configApiService.get(userId);
       clearTimeout(handle);
@@ -195,17 +228,17 @@ export class DefaultConfigService implements ConfigService {
       // mutate error to be handled by catchError
       this.logService.error(`Unable to fetch ServerConfig from ${environment.getApiUrl()}`, e);
       // Emit the existing config
-      this.failedFetchFallbackSubject.next(existingConfig);
+      this.failedFetchFallbackSubject.next(fallbackConfig);
     }
   }
 
-  private globalConfigFor$(apiUrl: string): Observable<ServerConfig> {
+  private globalConfigFor$(apiUrl: string): Observable<ServerConfig | null> {
     return this.stateProvider
       .getGlobal(GLOBAL_SERVER_CONFIGURATIONS)
-      .state$.pipe(map((configs) => configs?.[apiUrl]));
+      .state$.pipe(map((configs) => configs?.[apiUrl] ?? null));
   }
 
-  private userConfigFor$(userId: UserId): Observable<ServerConfig> {
+  private userConfigFor$(userId: UserId): Observable<ServerConfig | null> {
     return this.stateProvider.getUser(userId, USER_SERVER_CONFIG).state$;
   }
 }

libs/common/src/platform/services/default-environment.service.ts

@@ -133,6 +133,7 @@ export class DefaultEnvironmentService implements EnvironmentService {
   );
 
   environment$: Observable<Environment>;
+  globalEnvironment$: Observable<Environment>;
   cloudWebVaultUrl$: Observable<string>;
 
   constructor(
@@ -148,6 +149,10 @@ export class DefaultEnvironmentService implements EnvironmentService {
       distinctUntilChanged((oldUserId: UserId, newUserId: UserId) => oldUserId == newUserId),
     );
 
+    this.globalEnvironment$ = this.stateProvider
+      .getGlobal(GLOBAL_ENVIRONMENT_KEY)
+      .state$.pipe(map((state) => this.buildEnvironment(state?.region, state?.urls)));
+
     this.environment$ = account$.pipe(
       switchMap((userId) => {
         const t = userId
@@ -263,7 +268,7 @@ export class DefaultEnvironmentService implements EnvironmentService {
     return new SelfHostedEnvironment(urls);
   }
 
-  async setCloudRegion(userId: UserId, region: CloudRegion) {
+  async setCloudRegion(userId: UserId | null, region: CloudRegion) {
     if (userId == null) {
       await this.globalCloudRegionState.update(() => region);
     } else {
@@ -271,7 +276,7 @@ export class DefaultEnvironmentService implements EnvironmentService {
     }
   }
 
-  getEnvironment$(userId: UserId): Observable<Environment | undefined> {
+  getEnvironment$(userId: UserId): Observable<Environment> {
     return this.stateProvider.getUser(userId, USER_ENVIRONMENT_KEY).state$.pipe(
       map((state) => {
         return this.buildEnvironment(state?.region, state?.urls);