[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

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();

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

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<T>(service: keyof MainBackground) {
         ConfigApiServiceAbstraction,
         AuthServiceAbstraction,
         EnvironmentService,
+        LogService,
       ],
     },
   ],

libs/angular/src/services/jslib-services.module.ts

@@ -647,6 +647,7 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction";
         ConfigApiServiceAbstraction,
         AuthServiceAbstraction,
         EnvironmentServiceAbstraction,
+        LogService,
       ],
     },
     {

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<ConfigApiServiceAbstraction>;
   let authService: MockProxy<AuthService>;
   let environmentService: MockProxy<EnvironmentService>;
+  let logService: MockProxy<LogService>;
 
   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<void>(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" })

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