From 804ad79877fc4d1cebb2b2e987f12dca8d1130e4 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 7 Aug 2025 08:48:46 -0400 Subject: [PATCH] Fix extra signalr connection web (#15633) * Revert "fix(SignalR): Revert "[PM-23062] Fix extra signalr connections"" This reverts commit 97ec9a633988cbed7790ced508368e37384f5ca9. * Fix first login on web --- .../app/platform/web-environment.service.ts | 13 ++- .../config/config-api.service.abstraction.ts | 2 +- .../abstractions/environment.service.ts | 11 ++- .../services/config/config-api.service.ts | 2 +- .../services/config/config.service.spec.ts | 32 +++--- .../services/config/default-config.service.ts | 99 ++++++++++++------- .../services/default-environment.service.ts | 9 +- 7 files changed, 109 insertions(+), 59 deletions(-) diff --git a/apps/web/src/app/platform/web-environment.service.ts b/apps/web/src/app/platform/web-environment.service.ts index 1df842d6b31..4c4681ff715 100644 --- a/apps/web/src/app/platform/web-environment.service.ts +++ b/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; + 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(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 { + // 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 { diff --git a/libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts b/libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts index 3c191f59ccc..0460e8c715f 100644 --- a/libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts +++ b/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; + abstract get(userId: UserId | null): Promise; } diff --git a/libs/common/src/platform/abstractions/environment.service.ts b/libs/common/src/platform/abstractions/environment.service.ts index b8931656848..86a0fbea242 100644 --- a/libs/common/src/platform/abstractions/environment.service.ts +++ b/libs/common/src/platform/abstractions/environment.service.ts @@ -95,6 +95,13 @@ export interface Environment { */ export abstract class EnvironmentService { abstract environment$: Observable; + + /** + * The environment stored in global state, when a user signs in the state stored here will become + * their user environment. + */ + abstract globalEnvironment$: Observable; + abstract cloudWebVaultUrl$: Observable; /** @@ -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; + abstract setCloudRegion(userId: UserId | null, region: Region): Promise; /** * Get the environment from state. Useful if you need to get the environment for another user. */ - abstract getEnvironment$(userId: UserId): Observable; + abstract getEnvironment$(userId: UserId): Observable; /** * @deprecated Use {@link getEnvironment$} instead. diff --git a/libs/common/src/platform/services/config/config-api.service.ts b/libs/common/src/platform/services/config/config-api.service.ts index f283410acea..b7ecb9c8712 100644 --- a/libs/common/src/platform/services/config/config-api.service.ts +++ b/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 { + async get(userId: UserId | null): Promise { // 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 = diff --git a/libs/common/src/platform/services/config/config.service.spec.ts b/libs/common/src/platform/services/config/config.service.spec.ts index ea3b56a32f1..e8a1872c4c1 100644 --- a/libs/common/src/platform/services/config/config.service.spec.ts +++ b/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(); - 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({ diff --git a/libs/common/src/platform/services/config/default-config.service.ts b/libs/common/src/platform/services/config/default-config.service.ts index 33f86d30885..2dad227876e 100644 --- a/libs/common/src/platform/services/config/default-config.service.ts +++ b/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 { + 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(); + private failedFetchFallbackSubject = new Subject(); - serverConfig$: Observable; + serverConfig$: Observable; serverSettings$: Observable; @@ -67,32 +72,61 @@ export class DefaultConfigService implements ConfigService { private stateProvider: StateProvider, private authService: AuthService, ) { - const userId$ = this.stateProvider.activeUserId$; - const authStatus$ = userId$.pipe( - switchMap((userId) => (userId == null ? of(null) : this.authService.authStatusFor$(userId))), + const globalConfig$ = this.environmentService.globalEnvironment$.pipe( + distinctUntilChanged(environmentComparer), + switchMap((environment) => + this.globalConfigFor$(environment.getApiUrl()).pipe( + map((config) => { + return [config, null as UserId | null, environment, config] as const; + }), + ), + ), ); - this.serverConfig$ = combineLatest([ - userId$, - this.environmentService.environment$, - authStatus$, - ]).pipe( - switchMap(([userId, environment, authStatus]) => { - if (userId == null || authStatus !== AuthenticationStatus.Unlocked) { - return this.globalConfigFor$(environment.getApiUrl()).pipe( - map((config) => [config, null, environment] as const), - ); + this.serverConfig$ = this.stateProvider.activeUserId$.pipe( + distinctUntilChanged(), + switchMap((userId) => { + if (userId == null) { + // Global + return globalConfig$; } - return this.userConfigFor$(userId).pipe( - map((config) => [config, userId, environment] as const), + return this.authService.authStatusFor$(userId).pipe( + 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, - userId: UserId, + existingConfig: ServerConfig | null, + userId: UserId | null, environment: Environment, + fallbackConfig: ServerConfig | null, ): Promise { 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( - "Self-host environment did not respond in time, emitting previous config.", - ); - this.failedFetchFallbackSubject.next(existingConfig); + this.logService.info("Environment did not respond in time, emitting previous config."); + this.failedFetchFallbackSubject.next(fallbackConfig); }, 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 { + private globalConfigFor$(apiUrl: string): Observable { 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 { + private userConfigFor$(userId: UserId): Observable { return this.stateProvider.getUser(userId, USER_SERVER_CONFIG).state$; } } diff --git a/libs/common/src/platform/services/default-environment.service.ts b/libs/common/src/platform/services/default-environment.service.ts index df55693ba0b..4a1af68505a 100644 --- a/libs/common/src/platform/services/default-environment.service.ts +++ b/libs/common/src/platform/services/default-environment.service.ts @@ -133,6 +133,7 @@ export class DefaultEnvironmentService implements EnvironmentService { ); environment$: Observable; + globalEnvironment$: Observable; cloudWebVaultUrl$: Observable; 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 { + getEnvironment$(userId: UserId): Observable { return this.stateProvider.getUser(userId, USER_ENVIRONMENT_KEY).state$.pipe( map((state) => { return this.buildEnvironment(state?.region, state?.urls);