From 931a2258e29fda19b281e417b997dbc31fe3be32 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Thu, 14 Sep 2023 20:29:41 +1000 Subject: [PATCH] [PM-3883] Fix ConfigService.serverConfig$ initial values and error handling (#6272) * Always fetch ServerConfig from server, use stored value as fallback * Handle errors in server fetch --- .../browser/src/background/main.background.ts | 1 + .../services/browser-config.service.ts | 4 +- .../src/popup/services/services.module.ts | 6 ++- .../src/services/jslib-services.module.ts | 1 + .../services/config/config.service.spec.ts | 48 +++++++++++++++---- .../services/config/config.service.ts | 28 +++++------ 6 files changed, 62 insertions(+), 26 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index e646b98d41..3aba0c7767 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -540,6 +540,7 @@ export default class MainBackground { this.configApiService, this.authService, this.environmentService, + this.logService, true ); this.browserPopoutWindowService = new BrowserPopoutWindowService(); diff --git a/apps/browser/src/platform/services/browser-config.service.ts b/apps/browser/src/platform/services/browser-config.service.ts index f928fdd072..39d1fc565e 100644 --- a/apps/browser/src/platform/services/browser-config.service.ts +++ b/apps/browser/src/platform/services/browser-config.service.ts @@ -4,6 +4,7 @@ import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { ConfigApiServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config-api.service.abstraction"; import { ServerConfig } from "@bitwarden/common/platform/abstractions/config/server-config"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { ConfigService } from "@bitwarden/common/platform/services/config/config.service"; @@ -19,8 +20,9 @@ export class BrowserConfigService extends ConfigService { configApiService: ConfigApiServiceAbstraction, authService: AuthService, environmentService: EnvironmentService, + logService: LogService, subscribe = false ) { - super(stateService, configApiService, authService, environmentService, subscribe); + super(stateService, configApiService, authService, environmentService, logService, subscribe); } } diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 4e4f914f23..0ba396af92 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -43,7 +43,10 @@ import { EnvironmentService } from "@bitwarden/common/platform/abstractions/envi import { FileDownloadService } from "@bitwarden/common/platform/abstractions/file-download/file-download.service"; import { FileUploadService } from "@bitwarden/common/platform/abstractions/file-upload/file-upload.service"; import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { LogService as LogServiceAbstraction } from "@bitwarden/common/platform/abstractions/log.service"; +import { + LogService, + LogService as LogServiceAbstraction, +} from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { @@ -502,6 +505,7 @@ function getBgService(service: keyof MainBackground) { ConfigApiServiceAbstraction, AuthServiceAbstraction, EnvironmentService, + LogService, ], }, ], diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 5d27965710..02f6278fec 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -647,6 +647,7 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction"; ConfigApiServiceAbstraction, AuthServiceAbstraction, EnvironmentServiceAbstraction, + LogService, ], }, { 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 511ecfd5c8..2f3a90f70f 100644 --- a/libs/common/src/platform/services/config/config.service.spec.ts +++ b/libs/common/src/platform/services/config/config.service.spec.ts @@ -6,6 +6,7 @@ import { AuthenticationStatus } from "../../../auth/enums/authentication-status" import { ConfigApiServiceAbstraction } from "../../abstractions/config/config-api.service.abstraction"; import { ServerConfig } from "../../abstractions/config/server-config"; import { EnvironmentService } from "../../abstractions/environment.service"; +import { LogService } from "../../abstractions/log.service"; import { StateService } from "../../abstractions/state.service"; import { ServerConfigData } from "../../models/data/server-config.data"; import { @@ -21,6 +22,7 @@ describe("ConfigService", () => { let configApiService: MockProxy; let authService: MockProxy; let environmentService: MockProxy; + let logService: MockProxy; let serverResponseCount: number; // increments to track distinct responses received from server @@ -31,7 +33,8 @@ describe("ConfigService", () => { stateService, configApiService, authService, - environmentService + environmentService, + logService ); configService.init(); return configService; @@ -42,6 +45,8 @@ describe("ConfigService", () => { configApiService = mock(); authService = mock(); environmentService = mock(); + logService = mock(); + environmentService.urls = new ReplaySubject(1); serverResponseCount = 1; @@ -56,10 +61,12 @@ describe("ConfigService", () => { jest.useRealTimers(); }); - it("Loads config from storage", (done) => { + it("Uses storage as fallback", (done) => { const storedConfigData = serverConfigDataFactory("storedConfig"); stateService.getServerConfig.mockResolvedValueOnce(storedConfigData); + configApiService.get.mockRejectedValueOnce(new Error("Unable to fetch")); + const configService = configServiceFactory(); configService.serverConfig$.pipe(take(1)).subscribe((config) => { @@ -68,6 +75,30 @@ describe("ConfigService", () => { expect(stateService.setServerConfig).not.toHaveBeenCalled(); done(); }); + + configService.triggerServerConfigFetch(); + }); + + it("Stream does not error out if fetch fails", (done) => { + const storedConfigData = serverConfigDataFactory("storedConfig"); + stateService.getServerConfig.mockResolvedValueOnce(storedConfigData); + + const configService = configServiceFactory(); + + configService.serverConfig$.pipe(skip(1), take(1)).subscribe((config) => { + try { + expect(config.gitHash).toEqual("server1"); + done(); + } catch (e) { + done(e); + } + }); + + configApiService.get.mockRejectedValueOnce(new Error("Unable to fetch")); + configService.triggerServerConfigFetch(); + + configApiService.get.mockResolvedValueOnce(serverConfigResponseFactory("server1")); + configService.triggerServerConfigFetch(); }); describe("Fetches config from server", () => { @@ -80,8 +111,8 @@ describe("ConfigService", () => { (hours: number, done: jest.DoneCallback) => { const configService = configServiceFactory(); - // skip initial load from storage, plus previous hours (if any) - configService.serverConfig$.pipe(skip(hours), take(1)).subscribe((config) => { + // skip previous hours (if any) + configService.serverConfig$.pipe(skip(hours - 1), take(1)).subscribe((config) => { try { expect(config.gitHash).toEqual("server" + hours); expect(configApiService.get).toHaveBeenCalledTimes(hours); @@ -99,8 +130,7 @@ describe("ConfigService", () => { it("when environment URLs change", (done) => { const configService = configServiceFactory(); - // skip initial load from storage - configService.serverConfig$.pipe(skip(1), take(1)).subscribe((config) => { + configService.serverConfig$.pipe(take(1)).subscribe((config) => { try { expect(config.gitHash).toEqual("server1"); done(); @@ -115,8 +145,7 @@ describe("ConfigService", () => { it("when triggerServerConfigFetch() is called", (done) => { const configService = configServiceFactory(); - // skip initial load from storage - configService.serverConfig$.pipe(skip(1), take(1)).subscribe((config) => { + configService.serverConfig$.pipe(take(1)).subscribe((config) => { try { expect(config.gitHash).toEqual("server1"); done(); @@ -134,8 +163,7 @@ describe("ConfigService", () => { authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked); const configService = configServiceFactory(); - // skip initial load from storage - configService.serverConfig$.pipe(skip(1), take(1)).subscribe(() => { + configService.serverConfig$.pipe(take(1)).subscribe(() => { try { expect(stateService.setServerConfig).toHaveBeenCalledWith( expect.objectContaining({ gitHash: "server1" }) diff --git a/libs/common/src/platform/services/config/config.service.ts b/libs/common/src/platform/services/config/config.service.ts index c3a5fab4d7..008f5a764d 100644 --- a/libs/common/src/platform/services/config/config.service.ts +++ b/libs/common/src/platform/services/config/config.service.ts @@ -1,11 +1,11 @@ import { ReplaySubject, Subject, + catchError, concatMap, + defer, delayWhen, - filter, firstValueFrom, - from, map, merge, timer, @@ -18,6 +18,7 @@ import { ConfigApiServiceAbstraction } from "../../abstractions/config/config-ap import { ConfigServiceAbstraction } from "../../abstractions/config/config.service.abstraction"; import { ServerConfig } from "../../abstractions/config/server-config"; import { EnvironmentService, Region } from "../../abstractions/environment.service"; +import { LogService } from "../../abstractions/log.service"; import { StateService } from "../../abstractions/state.service"; import { ServerConfigData } from "../../models/data/server-config.data"; @@ -38,6 +39,7 @@ export class ConfigService implements ConfigServiceAbstraction { private configApiService: ConfigApiServiceAbstraction, private authService: AuthService, private environmentService: EnvironmentService, + private logService: LogService, // Used to avoid duplicate subscriptions, e.g. in browser between the background and popup private subscribe = true @@ -48,14 +50,16 @@ export class ConfigService implements ConfigServiceAbstraction { return; } - // Get config from storage on initial load - const fromStorage = from(this.stateService.getServerConfig()).pipe( - map((data) => (data == null ? null : new ServerConfig(data))) + const latestServerConfig$ = defer(() => this.configApiService.get()).pipe( + map((response) => new ServerConfigData(response)), + delayWhen((data) => this.saveConfig(data)), + catchError((e: unknown) => { + // fall back to stored ServerConfig (if any) + this.logService.error("Unable to fetch ServerConfig: " + (e as Error)?.message); + return this.stateService.getServerConfig(); + }) ); - fromStorage.subscribe((config) => this._serverConfig.next(config)); - - // Fetch config from server // If you need to fetch a new config when an event occurs, add an observable that emits on that event here merge( timer(ONE_HOUR_IN_MILLISECONDS, ONE_HOUR_IN_MILLISECONDS), // after 1 hour, then every hour @@ -63,12 +67,8 @@ export class ConfigService implements ConfigServiceAbstraction { this._forceFetchConfig // manual ) .pipe( - delayWhen(() => fromStorage), // wait until storage has emitted first to avoid a race condition - concatMap(() => this.configApiService.get()), - filter((response) => response != null), - map((response) => new ServerConfigData(response)), - delayWhen((data) => this.saveConfig(data)), - map((data) => new ServerConfig(data)) + concatMap(() => latestServerConfig$), + map((data) => (data == null ? null : new ServerConfig(data))) ) .subscribe((config) => this._serverConfig.next(config));