From 61e1bc1a1c1c36f29426003f8425337dcf8191a4 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Sat, 9 Sep 2023 00:05:37 +1000 Subject: [PATCH] [AC-1479][BEEEP] Refactor ConfigService to improve observable usage (#5602) * refactor ConfigService to use observables * make environmentService.urls a ReplaySubject --------- Co-authored-by: Hinton --- .../browser/src/background/main.background.ts | 13 +- .../src/background/runtime.background.ts | 4 +- .../services/browser-config.service.ts | 18 +- .../src/popup/services/init.service.ts | 6 +- .../src/popup/services/services.module.ts | 4 +- apps/desktop/src/app/app.component.ts | 2 +- apps/desktop/src/app/services/init.service.ts | 6 +- apps/web/src/app/app.component.ts | 2 +- ...ganization-subscription-cloud.component.ts | 2 +- .../billing/settings/add-credit.component.ts | 3 +- .../settings/organization-plans.component.ts | 2 +- .../environment-selector.component.ts | 2 +- apps/web/src/app/core/init.service.ts | 6 +- .../bit-web/src/app/auth/sso/sso.component.ts | 2 +- .../environment-selector.component.ts | 2 +- .../src/auth/components/sso.component.spec.ts | 2 +- .../src/auth/components/sso.component.ts | 2 +- .../components/two-factor.component.spec.ts | 2 +- .../auth/components/two-factor.component.ts | 2 +- .../directives/if-feature.directive.spec.ts | 23 +-- .../src/directives/if-feature.directive.ts | 17 +- .../src/guard/feature-flag.guard.spec.ts | 8 +- libs/angular/src/guard/feature-flag.guard.ts | 10 +- .../src/services/jslib-services.module.ts | 6 +- libs/common/src/enums/feature-flag.enum.ts | 3 + .../config/config.service.abstraction.ts | 22 ++- .../services/config/config.service.spec.ts | 174 ++++++++++++++++++ .../services/config/config.service.ts | 165 +++++++++-------- .../platform/services/environment.service.ts | 4 +- 29 files changed, 356 insertions(+), 158 deletions(-) create mode 100644 libs/common/src/platform/services/config/config.service.spec.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index f9963bcf7d..e646b98d41 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -35,7 +35,6 @@ import { UserVerificationApiService } from "@bitwarden/common/auth/services/user import { UserVerificationService } from "@bitwarden/common/auth/services/user-verification/user-verification.service"; import { AppIdService as AppIdServiceAbstraction } from "@bitwarden/common/platform/abstractions/app-id.service"; import { ConfigApiServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config-api.service.abstraction"; -import { ConfigServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config.service.abstraction"; import { CryptoFunctionService as CryptoFunctionServiceAbstraction } from "@bitwarden/common/platform/abstractions/crypto-function.service"; import { CryptoService as CryptoServiceAbstraction } from "@bitwarden/common/platform/abstractions/crypto.service"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; @@ -53,7 +52,6 @@ import { StateFactory } from "@bitwarden/common/platform/factories/state-factory import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; import { AppIdService } from "@bitwarden/common/platform/services/app-id.service"; import { ConfigApiService } from "@bitwarden/common/platform/services/config/config-api.service"; -import { ConfigService } from "@bitwarden/common/platform/services/config/config.service"; import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; import { EncryptServiceImplementation } from "@bitwarden/common/platform/services/cryptography/encrypt.service.implementation"; @@ -123,6 +121,7 @@ import { flagEnabled } from "../platform/flags"; import { UpdateBadge } from "../platform/listeners/update-badge"; import BrowserPopoutWindowService from "../platform/popup/browser-popout-window.service"; import { BrowserStateService as StateServiceAbstraction } from "../platform/services/abstractions/browser-state.service"; +import { BrowserConfigService } from "../platform/services/browser-config.service"; import { BrowserCryptoService } from "../platform/services/browser-crypto.service"; import { BrowserEnvironmentService } from "../platform/services/browser-environment.service"; import { BrowserI18nService } from "../platform/services/browser-i18n.service"; @@ -200,7 +199,7 @@ export default class MainBackground { avatarUpdateService: AvatarUpdateServiceAbstraction; mainContextMenuHandler: MainContextMenuHandler; cipherContextMenuHandler: CipherContextMenuHandler; - configService: ConfigServiceAbstraction; + configService: BrowserConfigService; configApiService: ConfigApiServiceAbstraction; devicesApiService: DevicesApiServiceAbstraction; devicesService: DevicesServiceAbstraction; @@ -533,12 +532,15 @@ export default class MainBackground { this.authService, this.messagingService ); + this.configApiService = new ConfigApiService(this.apiService, this.authService); - this.configService = new ConfigService( + + this.configService = new BrowserConfigService( this.stateService, this.configApiService, this.authService, - this.environmentService + this.environmentService, + true ); this.browserPopoutWindowService = new BrowserPopoutWindowService(); @@ -682,6 +684,7 @@ export default class MainBackground { await this.notificationBackground.init(); await this.commandsBackground.init(); + this.configService.init(); this.twoFactorService.init(); await this.tabsBackground.init(); diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index c1cfdf0420..dcf828ef4a 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -103,7 +103,7 @@ export default class RuntimeBackground { await this.main.refreshMenu(); }, 2000); this.main.avatarUpdateService.loadColorFromState(); - this.configService.fetchServerConfig(); + this.configService.triggerServerConfigFetch(); } break; case "openPopup": @@ -139,7 +139,7 @@ export default class RuntimeBackground { case "triggerAutofillScriptInjection": await this.autofillService.injectAutofillScripts( sender, - await this.configService.getFeatureFlagBool(FeatureFlag.AutofillV2) + await this.configService.getFeatureFlag(FeatureFlag.AutofillV2) ); break; case "bgCollectPageDetails": diff --git a/apps/browser/src/platform/services/browser-config.service.ts b/apps/browser/src/platform/services/browser-config.service.ts index 68237b4c20..f928fdd072 100644 --- a/apps/browser/src/platform/services/browser-config.service.ts +++ b/apps/browser/src/platform/services/browser-config.service.ts @@ -1,6 +1,10 @@ -import { BehaviorSubject } from "rxjs"; +import { ReplaySubject } from "rxjs"; +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 { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { ConfigService } from "@bitwarden/common/platform/services/config/config.service"; import { browserSession, sessionSync } from "../decorators/session-sync-observable"; @@ -8,5 +12,15 @@ import { browserSession, sessionSync } from "../decorators/session-sync-observab @browserSession export class BrowserConfigService extends ConfigService { @sessionSync({ initializer: ServerConfig.fromJSON }) - protected _serverConfig: BehaviorSubject; + protected _serverConfig: ReplaySubject; + + constructor( + stateService: StateService, + configApiService: ConfigApiServiceAbstraction, + authService: AuthService, + environmentService: EnvironmentService, + subscribe = false + ) { + super(stateService, configApiService, authService, environmentService, subscribe); + } } diff --git a/apps/browser/src/popup/services/init.service.ts b/apps/browser/src/popup/services/init.service.ts index 23ae6e8e89..c9a1ffb720 100644 --- a/apps/browser/src/popup/services/init.service.ts +++ b/apps/browser/src/popup/services/init.service.ts @@ -4,6 +4,7 @@ import { AbstractThemingService } from "@bitwarden/angular/services/theming/them import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService as LogServiceAbstraction } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; +import { ConfigService } from "@bitwarden/common/platform/services/config/config.service"; import { BrowserStateService as StateServiceAbstraction } from "../../platform/services/abstractions/browser-state.service"; @@ -17,7 +18,8 @@ export class InitService { private popupUtilsService: PopupUtilsService, private stateService: StateServiceAbstraction, private logService: LogServiceAbstraction, - private themingService: AbstractThemingService + private themingService: AbstractThemingService, + private configService: ConfigService ) {} init() { @@ -50,6 +52,8 @@ export class InitService { htmlEl.classList.add("force_redraw"); this.logService.info("Force redraw is on"); } + + this.configService.init(); }; } } diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 261f6abe37..4e4f914f23 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -36,7 +36,6 @@ import { AuthService } from "@bitwarden/common/auth/services/auth.service"; import { LoginService } from "@bitwarden/common/auth/services/login.service"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; import { ConfigApiServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config-api.service.abstraction"; -import { ConfigServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config.service.abstraction"; import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service"; import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; @@ -57,6 +56,7 @@ import { } from "@bitwarden/common/platform/abstractions/storage.service"; import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; +import { ConfigService } from "@bitwarden/common/platform/services/config/config.service"; import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; import { SearchService } from "@bitwarden/common/services/search.service"; @@ -495,7 +495,7 @@ function getBgService(service: keyof MainBackground) { deps: [StateServiceAbstraction, PlatformUtilsService], }, { - provide: ConfigServiceAbstraction, + provide: ConfigService, useClass: BrowserConfigService, deps: [ StateServiceAbstraction, diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index b5321a2bc8..39e2d2f8b1 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -232,7 +232,7 @@ export class AppComponent implements OnInit, OnDestroy { break; case "syncCompleted": await this.updateAppMenu(); - await this.configService.fetchServerConfig(); + this.configService.triggerServerConfigFetch(); break; case "openSettings": await this.openModal(SettingsComponent, this.settingsRef); diff --git a/apps/desktop/src/app/services/init.service.ts b/apps/desktop/src/app/services/init.service.ts index 34300aed93..0d60a1140f 100644 --- a/apps/desktop/src/app/services/init.service.ts +++ b/apps/desktop/src/app/services/init.service.ts @@ -11,6 +11,7 @@ import { EnvironmentService as EnvironmentServiceAbstraction } from "@bitwarden/ import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService as PlatformUtilsServiceAbstraction } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService as StateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service"; +import { ConfigService } from "@bitwarden/common/platform/services/config/config.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; import { EventUploadService } from "@bitwarden/common/services/event/event-upload.service"; import { VaultTimeoutService } from "@bitwarden/common/services/vault-timeout/vault-timeout.service"; @@ -35,7 +36,8 @@ export class InitService { private cryptoService: CryptoServiceAbstraction, private nativeMessagingService: NativeMessagingService, private themingService: AbstractThemingService, - private encryptService: EncryptService + private encryptService: EncryptService, + private configService: ConfigService ) {} init() { @@ -71,6 +73,8 @@ export class InitService { const containerService = new ContainerService(this.cryptoService, this.encryptService); containerService.attachToGlobal(this.win); + + this.configService.init(); }; } } diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index 3066dbe093..5b26527842 100644 --- a/apps/web/src/app/app.component.ts +++ b/apps/web/src/app/app.component.ts @@ -138,7 +138,7 @@ export class AppComponent implements OnDestroy, OnInit { case "syncStarted": break; case "syncCompleted": - await this.configService.fetchServerConfig(); + this.configService.triggerServerConfigFetch(); break; case "upgradeOrganization": { const upgradeConfirmed = await this.dialogService.openSimpleDialog({ diff --git a/apps/web/src/app/billing/organizations/organization-subscription-cloud.component.ts b/apps/web/src/app/billing/organizations/organization-subscription-cloud.component.ts index dedc99edcb..b419f74326 100644 --- a/apps/web/src/app/billing/organizations/organization-subscription-cloud.component.ts +++ b/apps/web/src/app/billing/organizations/organization-subscription-cloud.component.ts @@ -135,7 +135,7 @@ export class OrganizationSubscriptionCloudComponent implements OnInit, OnDestroy this.loading = false; // Remove the remaining lines when the sm-ga-billing flag is deleted - const smBillingEnabled = await this.configService.getFeatureFlagBool( + const smBillingEnabled = await this.configService.getFeatureFlag( FeatureFlag.SecretsManagerBilling ); this.showSecretsManagerSubscribe = this.showSecretsManagerSubscribe && smBillingEnabled; diff --git a/apps/web/src/app/billing/settings/add-credit.component.ts b/apps/web/src/app/billing/settings/add-credit.component.ts index c59886b4b4..2a60e65f2a 100644 --- a/apps/web/src/app/billing/settings/add-credit.component.ts +++ b/apps/web/src/app/billing/settings/add-credit.component.ts @@ -7,6 +7,7 @@ import { Output, ViewChild, } from "@angular/core"; +import { firstValueFrom } from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; @@ -79,7 +80,7 @@ export class AddCreditComponent implements OnInit { this.email = this.subject; this.ppButtonCustomField = "user_id:" + this.userId; } - this.region = await this.configService.getCloudRegion(); + this.region = await firstValueFrom(this.configService.cloudRegion$); this.ppButtonCustomField += ",account_credit:1"; this.ppButtonCustomField += `,region:${this.region}`; this.returnUrl = window.location.href; diff --git a/apps/web/src/app/billing/settings/organization-plans.component.ts b/apps/web/src/app/billing/settings/organization-plans.component.ts index c84d9511a1..6b0bd65d19 100644 --- a/apps/web/src/app/billing/settings/organization-plans.component.ts +++ b/apps/web/src/app/billing/settings/organization-plans.component.ts @@ -169,7 +169,7 @@ export class OrganizationPlansComponent implements OnInit, OnDestroy { this.singleOrgPolicyAppliesToActiveUser = policyAppliesToActiveUser; }); - this.showSecretsManagerSubscribe = await this.configService.getFeatureFlagBool( + this.showSecretsManagerSubscribe = await this.configService.getFeatureFlag( FeatureFlag.SecretsManagerBilling, false ); diff --git a/apps/web/src/app/components/environment-selector/environment-selector.component.ts b/apps/web/src/app/components/environment-selector/environment-selector.component.ts index 99146134b6..3c1c5a1973 100644 --- a/apps/web/src/app/components/environment-selector/environment-selector.component.ts +++ b/apps/web/src/app/components/environment-selector/environment-selector.component.ts @@ -25,7 +25,7 @@ export class EnvironmentSelectorComponent implements OnInit { routeAndParams: string; async ngOnInit() { - this.euServerFlagEnabled = await this.configService.getFeatureFlagBool( + this.euServerFlagEnabled = await this.configService.getFeatureFlag( FeatureFlag.DisplayEuEnvironmentFlag ); const domain = Utils.getDomain(window.location.href); diff --git a/apps/web/src/app/core/init.service.ts b/apps/web/src/app/core/init.service.ts index 3437c4f3e9..f171217d3c 100644 --- a/apps/web/src/app/core/init.service.ts +++ b/apps/web/src/app/core/init.service.ts @@ -13,6 +13,7 @@ import { } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service"; import { StateService as StateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service"; +import { ConfigService } from "@bitwarden/common/platform/services/config/config.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; import { EventUploadService } from "@bitwarden/common/services/event/event-upload.service"; import { VaultTimeoutService } from "@bitwarden/common/services/vault-timeout/vault-timeout.service"; @@ -32,7 +33,8 @@ export class InitService { private stateService: StateServiceAbstraction, private cryptoService: CryptoServiceAbstraction, private themingService: AbstractThemingService, - private encryptService: EncryptService + private encryptService: EncryptService, + private configService: ConfigService ) {} init() { @@ -57,6 +59,8 @@ export class InitService { await this.themingService.monitorThemeChanges(); const containerService = new ContainerService(this.cryptoService, this.encryptService); containerService.attachToGlobal(this.win); + + this.configService.init(); }; } } diff --git a/bitwarden_license/bit-web/src/app/auth/sso/sso.component.ts b/bitwarden_license/bit-web/src/app/auth/sso/sso.component.ts index e159d4744b..21cda4bbb0 100644 --- a/bitwarden_license/bit-web/src/app/auth/sso/sso.component.ts +++ b/bitwarden_license/bit-web/src/app/auth/sso/sso.component.ts @@ -235,7 +235,7 @@ export class SsoComponent implements OnInit, OnDestroy { ) .subscribe(); - const tdeFeatureFlag = await this.configService.getFeatureFlagBool( + const tdeFeatureFlag = await this.configService.getFeatureFlag( FeatureFlag.TrustedDeviceEncryption ); diff --git a/libs/angular/src/auth/components/environment-selector.component.ts b/libs/angular/src/auth/components/environment-selector.component.ts index 498d72b01e..19dc95dbcf 100644 --- a/libs/angular/src/auth/components/environment-selector.component.ts +++ b/libs/angular/src/auth/components/environment-selector.component.ts @@ -89,7 +89,7 @@ export class EnvironmentSelectorComponent implements OnInit, OnDestroy { async updateEnvironmentInfo() { this.selectedEnvironment = this.environmentService.selectedRegion; - this.euServerFlagEnabled = await this.configService.getFeatureFlagBool( + this.euServerFlagEnabled = await this.configService.getFeatureFlag( FeatureFlag.DisplayEuEnvironmentFlag ); } diff --git a/libs/angular/src/auth/components/sso.component.spec.ts b/libs/angular/src/auth/components/sso.component.spec.ts index 68c41b6611..894232af44 100644 --- a/libs/angular/src/auth/components/sso.component.spec.ts +++ b/libs/angular/src/auth/components/sso.component.spec.ts @@ -331,7 +331,7 @@ describe("SsoComponent", () => { describe("Trusted Device Encryption scenarios", () => { beforeEach(() => { - mockConfigService.getFeatureFlagBool.mockResolvedValue(true); // TDE enabled + mockConfigService.getFeatureFlag.mockResolvedValue(true); // TDE enabled }); describe("Given Trusted Device Encryption is enabled and user needs to set a master password", () => { diff --git a/libs/angular/src/auth/components/sso.component.ts b/libs/angular/src/auth/components/sso.component.ts index 8030e880a5..7e6aca7ec0 100644 --- a/libs/angular/src/auth/components/sso.component.ts +++ b/libs/angular/src/auth/components/sso.component.ts @@ -242,7 +242,7 @@ export class SsoComponent { private async isTrustedDeviceEncEnabled( trustedDeviceOption: TrustedDeviceUserDecryptionOption ): Promise { - const trustedDeviceEncryptionFeatureActive = await this.configService.getFeatureFlagBool( + const trustedDeviceEncryptionFeatureActive = await this.configService.getFeatureFlag( FeatureFlag.TrustedDeviceEncryption ); diff --git a/libs/angular/src/auth/components/two-factor.component.spec.ts b/libs/angular/src/auth/components/two-factor.component.spec.ts index 470c9d4eb7..9e147f3357 100644 --- a/libs/angular/src/auth/components/two-factor.component.spec.ts +++ b/libs/angular/src/auth/components/two-factor.component.spec.ts @@ -376,7 +376,7 @@ describe("TwoFactorComponent", () => { describe("Trusted Device Encryption scenarios", () => { beforeEach(() => { - mockConfigService.getFeatureFlagBool.mockResolvedValue(true); + mockConfigService.getFeatureFlag.mockResolvedValue(true); }); describe("Given Trusted Device Encryption is enabled and user needs to set a master password", () => { diff --git a/libs/angular/src/auth/components/two-factor.component.ts b/libs/angular/src/auth/components/two-factor.component.ts index 0a06d39315..9a6352283a 100644 --- a/libs/angular/src/auth/components/two-factor.component.ts +++ b/libs/angular/src/auth/components/two-factor.component.ts @@ -257,7 +257,7 @@ export class TwoFactorComponent extends CaptchaProtectedComponent implements OnI trustedDeviceOption: TrustedDeviceUserDecryptionOption ): Promise { const ssoTo2faFlowActive = this.route.snapshot.queryParamMap.get("sso") === "true"; - const trustedDeviceEncryptionFeatureActive = await this.configService.getFeatureFlagBool( + const trustedDeviceEncryptionFeatureActive = await this.configService.getFeatureFlag( FeatureFlag.TrustedDeviceEncryption ); diff --git a/libs/angular/src/directives/if-feature.directive.spec.ts b/libs/angular/src/directives/if-feature.directive.spec.ts index bf73a172a5..9d492b7f01 100644 --- a/libs/angular/src/directives/if-feature.directive.spec.ts +++ b/libs/angular/src/directives/if-feature.directive.spec.ts @@ -3,7 +3,7 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; import { mock, MockProxy } from "jest-mock-extended"; -import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { FeatureFlag, FeatureFlagValue } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config.service.abstraction"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; @@ -41,21 +41,12 @@ describe("IfFeatureDirective", () => { let content: HTMLElement; let mockConfigService: MockProxy; - const mockConfigFlagValue = (flag: FeatureFlag, flagValue: any) => { - if (typeof flagValue === "boolean") { - mockConfigService.getFeatureFlagBool.mockImplementation((f, defaultValue = false) => - flag == f ? Promise.resolve(flagValue) : Promise.resolve(defaultValue) - ); - } else if (typeof flagValue === "string") { - mockConfigService.getFeatureFlagString.mockImplementation((f, defaultValue = "") => - flag == f ? Promise.resolve(flagValue) : Promise.resolve(defaultValue) - ); - } else if (typeof flagValue === "number") { - mockConfigService.getFeatureFlagNumber.mockImplementation((f, defaultValue = 0) => - flag == f ? Promise.resolve(flagValue) : Promise.resolve(defaultValue) - ); - } + const mockConfigFlagValue = (flag: FeatureFlag, flagValue: FeatureFlagValue) => { + mockConfigService.getFeatureFlag.mockImplementation((f, defaultValue) => + flag == f ? Promise.resolve(flagValue) : Promise.resolve(defaultValue) + ); }; + const queryContent = (testId: string) => fixture.debugElement.query(By.css(`[data-testid="${testId}"]`))?.nativeElement; @@ -126,7 +117,7 @@ describe("IfFeatureDirective", () => { }); it("hides content when the directive throws an unexpected exception", async () => { - mockConfigService.getFeatureFlagBool.mockImplementation(() => Promise.reject("Some error")); + mockConfigService.getFeatureFlag.mockImplementation(() => Promise.reject("Some error")); fixture.detectChanges(); await fixture.whenStable(); diff --git a/libs/angular/src/directives/if-feature.directive.ts b/libs/angular/src/directives/if-feature.directive.ts index 1a0ee35dc6..e9aca531bb 100644 --- a/libs/angular/src/directives/if-feature.directive.ts +++ b/libs/angular/src/directives/if-feature.directive.ts @@ -1,12 +1,9 @@ import { Directive, Input, OnInit, TemplateRef, ViewContainerRef } from "@angular/core"; -import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { FeatureFlag, FeatureFlagValue } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config.service.abstraction"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; -// Replace this with a type safe lookup of the feature flag values in PM-2282 -type FlagValue = boolean | number | string; - /** * Directive that conditionally renders the element when the feature flag is enabled and/or * matches the value specified by {@link appIfFeatureValue}. @@ -26,7 +23,7 @@ export class IfFeatureDirective implements OnInit { * Optional value to compare against the value of the feature flag in the config service. * @default true */ - @Input() appIfFeatureValue: FlagValue = true; + @Input() appIfFeatureValue: FeatureFlagValue = true; private hasView = false; @@ -39,15 +36,7 @@ export class IfFeatureDirective implements OnInit { async ngOnInit() { try { - let flagValue: FlagValue; - - if (typeof this.appIfFeatureValue === "boolean") { - flagValue = await this.configService.getFeatureFlagBool(this.appIfFeature); - } else if (typeof this.appIfFeatureValue === "number") { - flagValue = await this.configService.getFeatureFlagNumber(this.appIfFeature); - } else if (typeof this.appIfFeatureValue === "string") { - flagValue = await this.configService.getFeatureFlagString(this.appIfFeature); - } + const flagValue = await this.configService.getFeatureFlag(this.appIfFeature); if (this.appIfFeatureValue === flagValue) { if (!this.hasView) { diff --git a/libs/angular/src/guard/feature-flag.guard.spec.ts b/libs/angular/src/guard/feature-flag.guard.spec.ts index bba879278f..1ac2a90ae0 100644 --- a/libs/angular/src/guard/feature-flag.guard.spec.ts +++ b/libs/angular/src/guard/feature-flag.guard.spec.ts @@ -30,15 +30,15 @@ describe("canAccessFeature", () => { // Mock the correct getter based on the type of flagValue; also mock default values if one is not provided if (typeof flagValue === "boolean") { - mockConfigService.getFeatureFlagBool.mockImplementation((flag, defaultValue = false) => + mockConfigService.getFeatureFlag.mockImplementation((flag, defaultValue = false) => flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue) ); } else if (typeof flagValue === "string") { - mockConfigService.getFeatureFlagString.mockImplementation((flag, defaultValue = "") => + mockConfigService.getFeatureFlag.mockImplementation((flag, defaultValue = "") => flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue) ); } else if (typeof flagValue === "number") { - mockConfigService.getFeatureFlagNumber.mockImplementation((flag, defaultValue = 0) => + mockConfigService.getFeatureFlag.mockImplementation((flag, defaultValue = 0) => flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue) ); } @@ -143,7 +143,7 @@ describe("canAccessFeature", () => { it("fails to navigate when the config service throws an unexpected exception", async () => { const { router } = setup(canAccessFeature(testFlag), true); - mockConfigService.getFeatureFlagBool.mockImplementation(() => Promise.reject("Some error")); + mockConfigService.getFeatureFlag.mockImplementation(() => Promise.reject("Some error")); await router.navigate([featureRoute]); diff --git a/libs/angular/src/guard/feature-flag.guard.ts b/libs/angular/src/guard/feature-flag.guard.ts index f4596f3cf4..d9297cbd97 100644 --- a/libs/angular/src/guard/feature-flag.guard.ts +++ b/libs/angular/src/guard/feature-flag.guard.ts @@ -29,16 +29,8 @@ export const canAccessFeature = ( const i18nService = inject(I18nService); const logService = inject(LogService); - let flagValue: FlagValue; - try { - if (typeof requiredFlagValue === "boolean") { - flagValue = await configService.getFeatureFlagBool(featureFlag); - } else if (typeof requiredFlagValue === "number") { - flagValue = await configService.getFeatureFlagNumber(featureFlag); - } else if (typeof requiredFlagValue === "string") { - flagValue = await configService.getFeatureFlagString(featureFlag); - } + const flagValue = await configService.getFeatureFlag(featureFlag); if (flagValue === requiredFlagValue) { return true; diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index df64c25c91..5d27965710 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -640,7 +640,7 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction"; useClass: SyncNotifierService, }, { - provide: ConfigServiceAbstraction, + provide: ConfigService, useClass: ConfigService, deps: [ StateServiceAbstraction, @@ -649,6 +649,10 @@ import { AbstractThemingService } from "./theming/theming.service.abstraction"; EnvironmentServiceAbstraction, ], }, + { + provide: ConfigServiceAbstraction, + useExisting: ConfigService, + }, { provide: ConfigApiServiceAbstraction, useClass: ConfigApiService, diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index 7016849b3b..8f30478ced 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -5,3 +5,6 @@ export enum FeatureFlag { AutofillV2 = "autofill-v2", SecretsManagerBilling = "sm-ga-billing", } + +// Replace this with a type safe lookup of the feature flag values in PM-2282 +export type FeatureFlagValue = number | string | boolean; diff --git a/libs/common/src/platform/abstractions/config/config.service.abstraction.ts b/libs/common/src/platform/abstractions/config/config.service.abstraction.ts index 13e44b9f5e..59f87b0fa2 100644 --- a/libs/common/src/platform/abstractions/config/config.service.abstraction.ts +++ b/libs/common/src/platform/abstractions/config/config.service.abstraction.ts @@ -1,14 +1,26 @@ import { Observable } from "rxjs"; import { FeatureFlag } from "../../../enums/feature-flag.enum"; +import { Region } from "../environment.service"; import { ServerConfig } from "./server-config"; export abstract class ConfigServiceAbstraction { serverConfig$: Observable; - fetchServerConfig: () => Promise; - getFeatureFlagBool: (key: FeatureFlag, defaultValue?: boolean) => Promise; - getFeatureFlagString: (key: FeatureFlag, defaultValue?: string) => Promise; - getFeatureFlagNumber: (key: FeatureFlag, defaultValue?: number) => Promise; - getCloudRegion: (defaultValue?: string) => Promise; + cloudRegion$: Observable; + getFeatureFlag$: ( + key: FeatureFlag, + defaultValue?: T + ) => Observable; + getFeatureFlag: ( + key: FeatureFlag, + defaultValue?: T + ) => Promise; + + /** + * Force ConfigService to fetch an updated config from the server and emit it from serverConfig$ + * @deprecated The service implementation should subscribe to an observable and use that to trigger a new fetch from + * server instead + */ + triggerServerConfigFetch: () => void; } diff --git a/libs/common/src/platform/services/config/config.service.spec.ts b/libs/common/src/platform/services/config/config.service.spec.ts new file mode 100644 index 0000000000..511ecfd5c8 --- /dev/null +++ b/libs/common/src/platform/services/config/config.service.spec.ts @@ -0,0 +1,174 @@ +import { MockProxy, mock } from "jest-mock-extended"; +import { ReplaySubject, skip, take } from "rxjs"; + +import { AuthService } from "../../../auth/abstractions/auth.service"; +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 { StateService } from "../../abstractions/state.service"; +import { ServerConfigData } from "../../models/data/server-config.data"; +import { + EnvironmentServerConfigResponse, + ServerConfigResponse, + ThirdPartyServerConfigResponse, +} from "../../models/response/server-config.response"; + +import { ConfigService } from "./config.service"; + +describe("ConfigService", () => { + let stateService: MockProxy; + let configApiService: MockProxy; + let authService: MockProxy; + let environmentService: MockProxy; + + let serverResponseCount: number; // increments to track distinct responses received from server + + // Observables will start emitting as soon as this is created, so only create it + // after everything is mocked + const configServiceFactory = () => { + const configService = new ConfigService( + stateService, + configApiService, + authService, + environmentService + ); + configService.init(); + return configService; + }; + + beforeEach(() => { + stateService = mock(); + configApiService = mock(); + authService = mock(); + environmentService = mock(); + environmentService.urls = new ReplaySubject(1); + + serverResponseCount = 1; + configApiService.get.mockImplementation(() => + Promise.resolve(serverConfigResponseFactory("server" + serverResponseCount++)) + ); + + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("Loads config from storage", (done) => { + const storedConfigData = serverConfigDataFactory("storedConfig"); + stateService.getServerConfig.mockResolvedValueOnce(storedConfigData); + + const configService = configServiceFactory(); + + configService.serverConfig$.pipe(take(1)).subscribe((config) => { + expect(config).toEqual(new ServerConfig(storedConfigData)); + expect(stateService.getServerConfig).toHaveBeenCalledTimes(1); + expect(stateService.setServerConfig).not.toHaveBeenCalled(); + done(); + }); + }); + + describe("Fetches config from server", () => { + beforeEach(() => { + stateService.getServerConfig.mockResolvedValueOnce(null); + }); + + it.each([1, 2, 3])( + "after %p hour/s", + (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) => { + try { + expect(config.gitHash).toEqual("server" + hours); + expect(configApiService.get).toHaveBeenCalledTimes(hours); + done(); + } catch (e) { + done(e); + } + }); + + const oneHourInMs = 1000 * 3600; + jest.advanceTimersByTime(oneHourInMs * hours + 1); + } + ); + + it("when environment URLs change", (done) => { + const configService = configServiceFactory(); + + // skip initial load from storage + configService.serverConfig$.pipe(skip(1), take(1)).subscribe((config) => { + try { + expect(config.gitHash).toEqual("server1"); + done(); + } catch (e) { + done(e); + } + }); + + (environmentService.urls as ReplaySubject).next(); + }); + + it("when triggerServerConfigFetch() is called", (done) => { + const configService = configServiceFactory(); + + // skip initial load from storage + configService.serverConfig$.pipe(skip(1), take(1)).subscribe((config) => { + try { + expect(config.gitHash).toEqual("server1"); + done(); + } catch (e) { + done(e); + } + }); + + configService.triggerServerConfigFetch(); + }); + }); + + it("Saves server config to storage when the user is logged in", (done) => { + stateService.getServerConfig.mockResolvedValueOnce(null); + authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Locked); + const configService = configServiceFactory(); + + // skip initial load from storage + configService.serverConfig$.pipe(skip(1), take(1)).subscribe(() => { + try { + expect(stateService.setServerConfig).toHaveBeenCalledWith( + expect.objectContaining({ gitHash: "server1" }) + ); + done(); + } catch (e) { + done(e); + } + }); + + configService.triggerServerConfigFetch(); + }); +}); + +function serverConfigDataFactory(gitHash: string) { + return new ServerConfigData(serverConfigResponseFactory(gitHash)); +} + +function serverConfigResponseFactory(gitHash: string) { + return new ServerConfigResponse({ + version: "myConfigVersion", + gitHash: gitHash, + server: new ThirdPartyServerConfigResponse({ + name: "myThirdPartyServer", + url: "www.example.com", + }), + environment: new EnvironmentServerConfigResponse({ + vault: "vault.example.com", + }), + featureStates: { + feat1: "off", + feat2: "on", + feat3: "off", + }, + }); +} diff --git a/libs/common/src/platform/services/config/config.service.ts b/libs/common/src/platform/services/config/config.service.ts index 5b0325b68a..c3a5fab4d7 100644 --- a/libs/common/src/platform/services/config/config.service.ts +++ b/libs/common/src/platform/services/config/config.service.ts @@ -1,103 +1,106 @@ -import { BehaviorSubject, concatMap, from, timer } from "rxjs"; +import { + ReplaySubject, + Subject, + concatMap, + delayWhen, + filter, + firstValueFrom, + from, + map, + merge, + timer, +} from "rxjs"; import { AuthService } from "../../../auth/abstractions/auth.service"; import { AuthenticationStatus } from "../../../auth/enums/authentication-status"; -import { FeatureFlag } from "../../../enums/feature-flag.enum"; +import { FeatureFlag, FeatureFlagValue } from "../../../enums/feature-flag.enum"; import { ConfigApiServiceAbstraction } from "../../abstractions/config/config-api.service.abstraction"; import { ConfigServiceAbstraction } from "../../abstractions/config/config.service.abstraction"; import { ServerConfig } from "../../abstractions/config/server-config"; -import { EnvironmentService } from "../../abstractions/environment.service"; +import { EnvironmentService, Region } from "../../abstractions/environment.service"; import { StateService } from "../../abstractions/state.service"; import { ServerConfigData } from "../../models/data/server-config.data"; +const ONE_HOUR_IN_MILLISECONDS = 1000 * 3600; + export class ConfigService implements ConfigServiceAbstraction { - protected _serverConfig = new BehaviorSubject(null); + protected _serverConfig = new ReplaySubject(1); serverConfig$ = this._serverConfig.asObservable(); + private _forceFetchConfig = new Subject(); + private inited = false; + + cloudRegion$ = this.serverConfig$.pipe( + map((config) => config?.environment?.cloudRegion ?? Region.US) + ); constructor( private stateService: StateService, private configApiService: ConfigApiServiceAbstraction, private authService: AuthService, - private environmentService: EnvironmentService - ) { - // Re-fetch the server config every hour - timer(0, 1000 * 3600) - .pipe(concatMap(() => from(this.fetchServerConfig()))) - .subscribe((serverConfig) => { - this._serverConfig.next(serverConfig); - }); + private environmentService: EnvironmentService, - this.environmentService.urls.subscribe(() => { - this.fetchServerConfig(); - }); - } + // Used to avoid duplicate subscriptions, e.g. in browser between the background and popup + private subscribe = true + ) {} - async fetchServerConfig(): Promise { - try { - const response = await this.configApiService.get(); - - if (response == null) { - return; - } - - const data = new ServerConfigData(response); - const serverConfig = new ServerConfig(data); - this._serverConfig.next(serverConfig); - - const userAuthStatus = await this.authService.getAuthStatus(); - if (userAuthStatus !== AuthenticationStatus.LoggedOut) { - // Store the config for offline use if the user is logged in - await this.stateService.setServerConfig(data); - this.environmentService.setCloudWebVaultUrl(data.environment?.cloudRegion); - } - // Always return new server config from server to calling method - // to ensure up to date information - // This change is specifically for the getFeatureFlag > buildServerConfig flow - // for locked or logged in users. - return serverConfig; - } catch { - return null; - } - } - - async getFeatureFlagBool(key: FeatureFlag, defaultValue = false): Promise { - return await this.getFeatureFlag(key, defaultValue); - } - - async getFeatureFlagString(key: FeatureFlag, defaultValue = ""): Promise { - return await this.getFeatureFlag(key, defaultValue); - } - - async getFeatureFlagNumber(key: FeatureFlag, defaultValue = 0): Promise { - return await this.getFeatureFlag(key, defaultValue); - } - - async getCloudRegion(defaultValue = "US"): Promise { - const serverConfig = await this.buildServerConfig(); - return serverConfig.environment?.cloudRegion ?? defaultValue; - } - - private async getFeatureFlag(key: FeatureFlag, defaultValue: T): Promise { - const serverConfig = await this.buildServerConfig(); - if ( - serverConfig == null || - serverConfig.featureStates == null || - serverConfig.featureStates[key] == null - ) { - return defaultValue; - } - return serverConfig.featureStates[key] as T; - } - - private async buildServerConfig(): Promise { - const data = await this.stateService.getServerConfig(); - const domain = data ? new ServerConfig(data) : this._serverConfig.getValue(); - - if (domain == null || !domain.isValid() || domain.expiresSoon()) { - const value = await this.fetchServerConfig(); - return value ?? domain; + init() { + if (!this.subscribe || this.inited) { + return; } - return domain; + // Get config from storage on initial load + const fromStorage = from(this.stateService.getServerConfig()).pipe( + map((data) => (data == null ? null : new ServerConfig(data))) + ); + + 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 + this.environmentService.urls, // when environment URLs change (including when app is started) + 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)) + ) + .subscribe((config) => this._serverConfig.next(config)); + + this.inited = true; + } + + getFeatureFlag$(key: FeatureFlag, defaultValue?: T) { + return this.serverConfig$.pipe( + map((serverConfig) => { + if (serverConfig?.featureStates == null || serverConfig.featureStates[key] == null) { + return defaultValue; + } + + return serverConfig.featureStates[key] as T; + }) + ); + } + + async getFeatureFlag(key: FeatureFlag, defaultValue?: T) { + return await firstValueFrom(this.getFeatureFlag$(key, defaultValue)); + } + + triggerServerConfigFetch() { + this._forceFetchConfig.next(); + } + + private async saveConfig(data: ServerConfigData) { + if ((await this.authService.getAuthStatus()) === AuthenticationStatus.LoggedOut) { + return; + } + + await this.stateService.setServerConfig(data); + this.environmentService.setCloudWebVaultUrl(data.environment?.cloudRegion); } } diff --git a/libs/common/src/platform/services/environment.service.ts b/libs/common/src/platform/services/environment.service.ts index 770de20f83..d85341a68c 100644 --- a/libs/common/src/platform/services/environment.service.ts +++ b/libs/common/src/platform/services/environment.service.ts @@ -1,4 +1,4 @@ -import { concatMap, Observable, Subject } from "rxjs"; +import { concatMap, Observable, ReplaySubject } from "rxjs"; import { EnvironmentUrls } from "../../auth/models/domain/environment-urls"; import { @@ -9,7 +9,7 @@ import { import { StateService } from "../abstractions/state.service"; export class EnvironmentService implements EnvironmentServiceAbstraction { - private readonly urlsSubject = new Subject(); + private readonly urlsSubject = new ReplaySubject(1); urls: Observable = this.urlsSubject.asObservable(); selectedRegion?: Region; initialized = false;