[PM-2282] Make feature flags type safe (#8612)

Refactors the feature flags in ConfigService to be type safe. It also moves the default value to a centralized location rather than the caller defining it. This ensures consistency across the various places they are used.
2024-12-21 16:18:28 +01:00 · 2024-04-26 12:57:26 +02:00 · 2024-04-26 12:57:26 +02:00 · 14b2eb99a2
commit 14b2eb99a2
parent c7fa376be3
27 changed files with 67 additions and 58 deletions
--- a/apps/browser/src/tools/background/fileless-importer.background.ts
+++ b/apps/browser/src/tools/background/fileless-importer.background.ts
@ -183,7 +183,7 @@ class FilelessImporterBackground implements FilelessImporterBackgroundInterface
      return;
    }

-    const filelessImportFeatureFlagEnabled = await this.configService.getFeatureFlag<boolean>(
+    const filelessImportFeatureFlagEnabled = await this.configService.getFeatureFlag(
      FeatureFlag.BrowserFilelessImport,
    );
    const userAuthStatus = await this.authService.getAuthStatus();
--- a/apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts
+++ b/apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts
@ -58,7 +58,6 @@ export class OrganizationLayoutComponent implements OnInit, OnDestroy {

  protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
    FeatureFlag.ShowPaymentMethodWarningBanners,
-    false,
  );

  constructor(
--- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts
+++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts
@ -218,7 +218,6 @@ export class MemberDialogComponent implements OnDestroy {
      groups: groups$,
      flexibleCollectionsV1Enabled: this.configService.getFeatureFlag$(
        FeatureFlag.FlexibleCollectionsV1,
-        false,
      ),
    })
      .pipe(takeUntil(this.destroy$))
--- a/apps/web/src/app/admin-console/organizations/settings/account.component.ts
+++ b/apps/web/src/app/admin-console/organizations/settings/account.component.ts
@ -44,12 +44,10 @@ export class AccountComponent {

  protected flexibleCollectionsMigrationEnabled$ = this.configService.getFeatureFlag$(
    FeatureFlag.FlexibleCollectionsMigration,
-    false,
  );

  flexibleCollectionsV1Enabled$ = this.configService.getFeatureFlag$(
    FeatureFlag.FlexibleCollectionsV1,
-    false,
  );

  // FormGroup validators taken from server Organization domain object
--- a/apps/web/src/app/auth/key-rotation/user-key-rotation.service.ts
+++ b/apps/web/src/app/auth/key-rotation/user-key-rotation.service.ts
@ -90,7 +90,7 @@ export class UserKeyRotationService {
    request.emergencyAccessKeys = await this.emergencyAccessService.getRotatedKeys(newUserKey);
    request.resetPasswordKeys = await this.resetPasswordService.getRotatedKeys(newUserKey);

-    if (await this.configService.getFeatureFlag<boolean>(FeatureFlag.KeyRotationImprovements)) {
+    if (await this.configService.getFeatureFlag(FeatureFlag.KeyRotationImprovements)) {
      await this.apiService.postUserKeyUpdate(request);
    } else {
      await this.rotateUserKeyAndEncryptedDataLegacy(request);
--- a/apps/web/src/app/billing/organizations/organization-subscription-cloud.component.ts
+++ b/apps/web/src/app/billing/organizations/organization-subscription-cloud.component.ts
@ -84,7 +84,6 @@ export class OrganizationSubscriptionCloudComponent implements OnInit, OnDestroy

    this.showUpdatedSubscriptionStatusSection$ = this.configService.getFeatureFlag$(
      FeatureFlag.AC1795_UpdatedSubscriptionStatusSection,
-      false,
    );
  }

--- a/apps/web/src/app/layouts/user-layout.component.ts
+++ b/apps/web/src/app/layouts/user-layout.component.ts
@ -40,7 +40,6 @@ export class UserLayoutComponent implements OnInit {

  protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
    FeatureFlag.ShowPaymentMethodWarningBanners,
-    false,
  );

  constructor(
--- a/apps/web/src/app/vault/components/collection-dialog/collection-dialog.component.ts
+++ b/apps/web/src/app/vault/components/collection-dialog/collection-dialog.component.ts
@ -76,7 +76,7 @@ export enum CollectionDialogAction {
 })
 export class CollectionDialogComponent implements OnInit, OnDestroy {
  protected flexibleCollectionsV1Enabled$ = this.configService
-    .getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1, false)
+    .getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1)
    .pipe(first());

  private destroy$ = new Subject<void>();
--- a/apps/web/src/app/vault/individual-vault/bulk-action-dialogs/bulk-delete-dialog/bulk-delete-dialog.component.ts
+++ b/apps/web/src/app/vault/individual-vault/bulk-action-dialogs/bulk-delete-dialog/bulk-delete-dialog.component.ts
@ -54,7 +54,6 @@ export class BulkDeleteDialogComponent {

  private flexibleCollectionsV1Enabled$ = this.configService.getFeatureFlag$(
    FeatureFlag.FlexibleCollectionsV1,
-    false,
  );

  constructor(
--- a/apps/web/src/app/vault/individual-vault/vault-onboarding/vault-onboarding.component.ts
+++ b/apps/web/src/app/vault/individual-vault/vault-onboarding/vault-onboarding.component.ts
@ -60,9 +60,8 @@ export class VaultOnboardingComponent implements OnInit, OnChanges, OnDestroy {
  ) {}

  async ngOnInit() {
-    this.showOnboardingAccess$ = await this.configService.getFeatureFlag$<boolean>(
+    this.showOnboardingAccess$ = await this.configService.getFeatureFlag$(
      FeatureFlag.VaultOnboarding,
-      false,
    );
    this.onboardingTasks$ = this.vaultOnboardingService.vaultOnboardingState$;
    await this.setOnboardingTasks();
--- a/apps/web/src/app/vault/individual-vault/vault.component.ts
+++ b/apps/web/src/app/vault/individual-vault/vault.component.ts
@ -148,7 +148,6 @@ export class VaultComponent implements OnInit, OnDestroy {
  protected currentSearchText$: Observable<string>;
  protected flexibleCollectionsV1Enabled$ = this.configService.getFeatureFlag$(
    FeatureFlag.FlexibleCollectionsV1,
-    false,
  );

  private searchText$ = new Subject<string>();
--- a/apps/web/src/app/vault/org-vault/attachments.component.ts
+++ b/apps/web/src/app/vault/org-vault/attachments.component.ts
@ -60,7 +60,7 @@ export class AttachmentsComponent extends BaseAttachmentsComponent implements On
  async ngOnInit() {
    await super.ngOnInit();
    this.flexibleCollectionsV1Enabled = await firstValueFrom(
-      this.configService.getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1, false),
+      this.configService.getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1),
    );
  }

--- a/apps/web/src/app/vault/org-vault/bulk-collection-assignment-dialog/bulk-collection-assignment-dialog.component.ts
+++ b/apps/web/src/app/vault/org-vault/bulk-collection-assignment-dialog/bulk-collection-assignment-dialog.component.ts
@ -70,10 +70,7 @@ export class BulkCollectionAssignmentDialogComponent implements OnDestroy, OnIni
  ) {}

  async ngOnInit() {
-    const v1FCEnabled = await this.configService.getFeatureFlag(
-      FeatureFlag.FlexibleCollectionsV1,
-      false,
-    );
+    const v1FCEnabled = await this.configService.getFeatureFlag(FeatureFlag.FlexibleCollectionsV1);
    const org = await this.organizationService.get(this.params.organizationId);

    if (org.canEditAllCiphers(v1FCEnabled)) {
--- a/apps/web/src/app/vault/org-vault/vault.component.ts
+++ b/apps/web/src/app/vault/org-vault/vault.component.ts
@ -193,7 +193,6 @@ export class VaultComponent implements OnInit, OnDestroy {

    this._flexibleCollectionsV1FlagEnabled = await this.configService.getFeatureFlag(
      FeatureFlag.FlexibleCollectionsV1,
-      false,
    );

    const filter$ = this.routedVaultFilterService.filter$;
--- a/bitwarden_license/bit-web/src/app/admin-console/providers/clients/clients.component.ts
+++ b/bitwarden_license/bit-web/src/app/admin-console/providers/clients/clients.component.ts
@ -42,7 +42,6 @@ export class ClientsComponent extends BaseClientsComponent {

  protected consolidatedBillingEnabled$ = this.configService.getFeatureFlag$(
    FeatureFlag.EnableConsolidatedBilling,
-    false,
  );

  constructor(
--- a/bitwarden_license/bit-web/src/app/admin-console/providers/providers-layout.component.ts
+++ b/bitwarden_license/bit-web/src/app/admin-console/providers/providers-layout.component.ts
@ -37,12 +37,10 @@ export class ProvidersLayoutComponent {

  protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
    FeatureFlag.ShowPaymentMethodWarningBanners,
-    false,
  );

  protected enableConsolidatedBilling$ = this.configService.getFeatureFlag$(
    FeatureFlag.EnableConsolidatedBilling,
-    false,
  );

  constructor(
--- a/bitwarden_license/bit-web/src/app/admin-console/providers/settings/account.component.ts
+++ b/bitwarden_license/bit-web/src/app/admin-console/providers/settings/account.component.ts
@ -30,7 +30,6 @@ export class AccountComponent {

  protected enableDeleteProvider$ = this.configService.getFeatureFlag$(
    FeatureFlag.EnableDeleteProvider,
-    false,
  );

  constructor(
--- a/bitwarden_license/bit-web/src/app/admin-console/providers/setup/setup.component.ts
+++ b/bitwarden_license/bit-web/src/app/admin-console/providers/setup/setup.component.ts
@ -36,12 +36,10 @@ export class SetupComponent implements OnInit {

  protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
    FeatureFlag.ShowPaymentMethodWarningBanners,
-    false,
  );

  protected enableConsolidatedBilling$ = this.configService.getFeatureFlag$(
    FeatureFlag.EnableConsolidatedBilling,
-    false,
  );

  constructor(
--- 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, FeatureFlagValue } from "@bitwarden/common/enums/feature-flag.enum";
+import { AllowedFeatureFlagTypes, FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
 import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";

@ -41,10 +41,8 @@ describe("IfFeatureDirective", () => {
  let content: HTMLElement;
  let mockConfigService: MockProxy<ConfigService>;

-  const mockConfigFlagValue = (flag: FeatureFlag, flagValue: FeatureFlagValue) => {
-    mockConfigService.getFeatureFlag.mockImplementation((f, defaultValue) =>
-      flag == f ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
-    );
+  const mockConfigFlagValue = (flag: FeatureFlag, flagValue: AllowedFeatureFlagTypes) => {
+    mockConfigService.getFeatureFlag.mockImplementation((f) => Promise.resolve(flagValue as any));
  };

  const queryContent = (testId: string) =>
--- a/libs/angular/src/directives/if-feature.directive.ts
+++ b/libs/angular/src/directives/if-feature.directive.ts
@ -1,6 +1,6 @@
 import { Directive, Input, OnInit, TemplateRef, ViewContainerRef } from "@angular/core";

-import { FeatureFlag, FeatureFlagValue } from "@bitwarden/common/enums/feature-flag.enum";
+import { AllowedFeatureFlagTypes, FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
 import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";

@ -23,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: FeatureFlagValue = true;
+  @Input() appIfFeatureValue: AllowedFeatureFlagTypes = true;

  private hasView = false;

--- a/libs/angular/src/platform/guard/feature-flag.guard.spec.ts
+++ b/libs/angular/src/platform/guard/feature-flag.guard.spec.ts
@ -34,12 +34,12 @@ describe("canAccessFeature", () => {
        flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
      );
    } else if (typeof flagValue === "string") {
-      mockConfigService.getFeatureFlag.mockImplementation((flag, defaultValue = "") =>
-        flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
+      mockConfigService.getFeatureFlag.mockImplementation((flag) =>
+        flag == testFlag ? Promise.resolve(flagValue as any) : Promise.resolve(""),
      );
    } else if (typeof flagValue === "number") {
-      mockConfigService.getFeatureFlag.mockImplementation((flag, defaultValue = 0) =>
-        flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
+      mockConfigService.getFeatureFlag.mockImplementation((flag) =>
+        flag == testFlag ? Promise.resolve(flagValue as any) : Promise.resolve(0),
      );
    }

--- a/libs/angular/src/vault/components/add-edit.component.ts
+++ b/libs/angular/src/vault/components/add-edit.component.ts
@ -182,7 +182,6 @@ export class AddEditComponent implements OnInit, OnDestroy {
  async ngOnInit() {
    this.flexibleCollectionsV1Enabled = await this.configService.getFeatureFlag(
      FeatureFlag.FlexibleCollectionsV1,
-      false,
    );

    this.policyService
--- a/libs/common/src/enums/feature-flag.enum.ts
+++ b/libs/common/src/enums/feature-flag.enum.ts
@ -1,3 +1,8 @@
+/**
+ * Feature flags.
+ *
+ * Flags MUST be short lived and SHALL be removed once enabled.
+ */
 export enum FeatureFlag {
  BrowserFilelessImport = "browser-fileless-import",
  ItemShare = "item-share",
@ -13,5 +18,32 @@ export enum FeatureFlag {
  EnableDeleteProvider = "AC-1218-delete-provider",
 }

-// Replace this with a type safe lookup of the feature flag values in PM-2282
-export type FeatureFlagValue = number | string | boolean;
+export type AllowedFeatureFlagTypes = boolean | number | string;
+
+// Helper to ensure the value is treated as a boolean.
+const FALSE = false as boolean;
+
+/**
+ * Default value for feature flags.
+ *
+ * DO NOT enable previously disabled flags, REMOVE them instead.
+ * We support true as a value as we prefer flags to "enable" not "disable".
+ */
+export const DefaultFeatureFlagValue = {
+  [FeatureFlag.BrowserFilelessImport]: FALSE,
+  [FeatureFlag.ItemShare]: FALSE,
+  [FeatureFlag.FlexibleCollectionsV1]: FALSE,
+  [FeatureFlag.VaultOnboarding]: FALSE,
+  [FeatureFlag.GeneratorToolsModernization]: FALSE,
+  [FeatureFlag.KeyRotationImprovements]: FALSE,
+  [FeatureFlag.FlexibleCollectionsMigration]: FALSE,
+  [FeatureFlag.ShowPaymentMethodWarningBanners]: FALSE,
+  [FeatureFlag.EnableConsolidatedBilling]: FALSE,
+  [FeatureFlag.AC1795_UpdatedSubscriptionStatusSection]: FALSE,
+  [FeatureFlag.UnassignedItemsBanner]: FALSE,
+  [FeatureFlag.EnableDeleteProvider]: FALSE,
+} satisfies Record<FeatureFlag, AllowedFeatureFlagTypes>;
+
+export type DefaultFeatureFlagValueType = typeof DefaultFeatureFlagValue;
+
+export type FeatureFlagValueType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];
--- a/libs/common/src/platform/abstractions/config/config.service.ts
+++ b/libs/common/src/platform/abstractions/config/config.service.ts
@ -1,7 +1,7 @@
 import { Observable } from "rxjs";
 import { SemVer } from "semver";

-import { FeatureFlag } from "../../../enums/feature-flag.enum";
+import { FeatureFlag, FeatureFlagValueType } from "../../../enums/feature-flag.enum";
 import { Region } from "../environment.service";

 import { ServerConfig } from "./server-config";
@ -14,23 +14,15 @@ export abstract class ConfigService {
  /**
   * Retrieves the value of a feature flag for the currently active user
   * @param key The feature flag to retrieve
-   * @param defaultValue The default value to return if the feature flag is not set or the server's config is irretrievable
   * @returns An observable that emits the value of the feature flag, updates as the server config changes
   */
-  getFeatureFlag$: <T extends boolean | number | string>(
-    key: FeatureFlag,
-    defaultValue?: T,
-  ) => Observable<T>;
+  getFeatureFlag$: <Flag extends FeatureFlag>(key: Flag) => Observable<FeatureFlagValueType<Flag>>;
  /**
   * Retrieves the value of a feature flag for the currently active user
   * @param key The feature flag to retrieve
-   * @param defaultValue The default value to return if the feature flag is not set or the server's config is irretrievable
   * @returns The value of the feature flag
   */
-  getFeatureFlag: <T extends boolean | number | string>(
-    key: FeatureFlag,
-    defaultValue?: T,
-  ) => Promise<T>;
+  getFeatureFlag: <Flag extends FeatureFlag>(key: Flag) => Promise<FeatureFlagValueType<Flag>>;
  /**
   * Verifies whether the server version meets the minimum required version
   * @param minimumRequiredServerVersion The minimum version required
--- a/libs/common/src/platform/abstractions/config/server-config.ts
+++ b/libs/common/src/platform/abstractions/config/server-config.ts
@ -1,5 +1,6 @@
 import { Jsonify } from "type-fest";

+import { AllowedFeatureFlagTypes } from "../../../enums/feature-flag.enum";
 import {
  ServerConfigData,
  ThirdPartyServerConfigData,
@ -14,7 +15,7 @@ export class ServerConfig {
  server?: ThirdPartyServerConfigData;
  environment?: EnvironmentServerConfigData;
  utcDate: Date;
-  featureStates: { [key: string]: string } = {};
+  featureStates: { [key: string]: AllowedFeatureFlagTypes } = {};

  constructor(serverConfigData: ServerConfigData) {
    this.version = serverConfigData.version;
--- a/libs/common/src/platform/models/data/server-config.data.ts
+++ b/libs/common/src/platform/models/data/server-config.data.ts
@ -1,5 +1,6 @@
 import { Jsonify } from "type-fest";

+import { AllowedFeatureFlagTypes } from "../../../enums/feature-flag.enum";
 import { Region } from "../../abstractions/environment.service";
 import {
  ServerConfigResponse,
@ -13,7 +14,7 @@ export class ServerConfigData {
  server?: ThirdPartyServerConfigData;
  environment?: EnvironmentServerConfigData;
  utcDate: string;
-  featureStates: { [key: string]: string } = {};
+  featureStates: { [key: string]: AllowedFeatureFlagTypes } = {};

  constructor(serverConfigResponse: Partial<ServerConfigResponse>) {
    this.version = serverConfigResponse?.version;
--- a/libs/common/src/platform/services/config/default-config.service.ts
+++ b/libs/common/src/platform/services/config/default-config.service.ts
@ -13,7 +13,11 @@ import {
 } from "rxjs";
 import { SemVer } from "semver";

-import { FeatureFlag, FeatureFlagValue } from "../../../enums/feature-flag.enum";
+import {
+  DefaultFeatureFlagValue,
+  FeatureFlag,
+  FeatureFlagValueType,
+} from "../../../enums/feature-flag.enum";
 import { UserId } from "../../../types/guid";
 import { ConfigApiServiceAbstraction } from "../../abstractions/config/config-api.service.abstraction";
 import { ConfigService } from "../../abstractions/config/config.service";
@ -89,20 +93,21 @@ export class DefaultConfigService implements ConfigService {
      map((config) => config?.environment?.cloudRegion ?? Region.US),
    );
  }
-  getFeatureFlag$<T extends FeatureFlagValue>(key: FeatureFlag, defaultValue?: T) {
+
+  getFeatureFlag$<Flag extends FeatureFlag>(key: Flag) {
    return this.serverConfig$.pipe(
      map((serverConfig) => {
        if (serverConfig?.featureStates == null || serverConfig.featureStates[key] == null) {
-          return defaultValue;
+          return DefaultFeatureFlagValue[key];
        }

-        return serverConfig.featureStates[key] as T;
+        return serverConfig.featureStates[key] as FeatureFlagValueType<Flag>;
      }),
    );
  }

-  async getFeatureFlag<T extends FeatureFlagValue>(key: FeatureFlag, defaultValue?: T) {
-    return await firstValueFrom(this.getFeatureFlag$(key, defaultValue));
+  async getFeatureFlag<Flag extends FeatureFlag>(key: Flag) {
+    return await firstValueFrom(this.getFeatureFlag$(key));
  }

  checkServerMeetsVersionRequirement$(minimumRequiredServerVersion: SemVer) {