[PM-5535] Migrate Environment Service to StateProvider (#7621)

* Migrate EnvironmentService

* Move Migration Test Helper

* Claim StateDefinition

* Add State Migration

* Update StateServices

* Update EnvironmentService Abstraction

* Update DI

* Update Browser Instantiation

* Fix BrowserEnvironmentService

* Update Desktop & CLI Instantiation

* Update Usage

* Create isStringRecord helper

* Fix Old Tests

* Use Existing AccountService

* Don't Rely on Parameter Mutation

* Fix Conflicts

libs/common/src/state-migrations/migrate.ts

@@ -7,6 +7,7 @@ import { MigrationBuilder } from "./migration-builder";
 import { MigrationHelper } from "./migration-helper";
 import { EverHadUserKeyMigrator } from "./migrations/10-move-ever-had-user-key-to-state-providers";
 import { OrganizationKeyMigrator } from "./migrations/11-move-org-keys-to-state-providers";
+import { MoveEnvironmentStateToProviders } from "./migrations/12-move-environment-state-to-providers";
 import { FixPremiumMigrator } from "./migrations/3-fix-premium";
 import { RemoveEverBeenUnlockedMigrator } from "./migrations/4-remove-ever-been-unlocked";
 import { AddKeyTypeToOrgKeysMigrator } from "./migrations/5-add-key-type-to-org-keys";
@@ -17,7 +18,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting
 import { MinVersionMigrator } from "./migrations/min-version";
 
 export const MIN_VERSION = 2;
-export const CURRENT_VERSION = 11;
+export const CURRENT_VERSION = 12;
 export type MinVersion = typeof MIN_VERSION;
 
 export async function migrate(
@@ -44,7 +45,8 @@ export async function migrate(
     .with(MoveStateVersionMigrator, 7, 8)
     .with(MoveBrowserSettingsToGlobal, 8, 9)
     .with(EverHadUserKeyMigrator, 9, 10)
-    .with(OrganizationKeyMigrator, 10, CURRENT_VERSION)
+    .with(OrganizationKeyMigrator, 10, 11)
+    .with(MoveEnvironmentStateToProviders, 11, CURRENT_VERSION)
 
     .migrate(migrationHelper);
 }

libs/common/src/state-migrations/migration-helper.spec.ts

@@ -1,11 +1,16 @@
 import { MockProxy, mock } from "jest-mock-extended";
 
+// eslint-disable-next-line import/no-restricted-paths -- Needed to print log messages
+import { FakeStorageService } from "../../spec/fake-storage.service";
 // eslint-disable-next-line import/no-restricted-paths -- Needed to print log messages
 import { LogService } from "../platform/abstractions/log.service";
 // eslint-disable-next-line import/no-restricted-paths -- Needed to interface with storage locations
 import { AbstractStorageService } from "../platform/abstractions/storage.service";
+// eslint-disable-next-line import/no-restricted-paths -- Needed to generate unique strings for injection
+import { Utils } from "../platform/misc/utils";
 
 import { MigrationHelper } from "./migration-helper";
+import { Migrator } from "./migrator";
 
 const exampleJSON = {
   authenticatedAccounts: [
@@ -172,3 +177,129 @@ export function mockMigrationHelper(
   mockHelper.getAccounts.mockImplementation(() => helper.getAccounts());
   return mockHelper;
 }
+
+// TODO: Use const generic for TUsers in TypeScript 5.0 so consumers don't have to `as const` themselves
+export type InitialDataHint<TUsers extends readonly string[]> = {
+  /**
+   * A string array of the users id who are authenticated
+   *
+   * NOTE: It's recommended to as const this string array so you get type help defining the users data
+   */
+  authenticatedAccounts?: TUsers;
+  /**
+   * Global data
+   */
+  global?: unknown;
+  /**
+   * Other top level data
+   */
+  [key: string]: unknown;
+} & {
+  /**
+   * A users data
+   */
+  [userData in TUsers[number]]?: unknown;
+};
+
+type InjectedData = {
+  propertyName: string;
+  propertyValue: string;
+  originalPath: string[];
+};
+
+// This is a slight lie, technically the type is `Record<string | symbol, unknown>
+// but for the purposes of things in the migrations this is enough.
+function isStringRecord(object: unknown | undefined): object is Record<string, unknown> {
+  return object && typeof object === "object" && !Array.isArray(object);
+}
+
+function injectData(data: Record<string, unknown>, path: string[]): InjectedData[] {
+  if (!data) {
+    return [];
+  }
+
+  const injectedData: InjectedData[] = [];
+
+  // Traverse keys for other objects
+  const keys = Object.keys(data);
+  for (const key of keys) {
+    const currentProperty = data[key];
+    if (isStringRecord(currentProperty)) {
+      injectedData.push(...injectData(currentProperty, [...path, key]));
+    }
+  }
+
+  const propertyName = `__injectedProperty__${Utils.newGuid()}`;
+  const propertyValue = `__injectedValue__${Utils.newGuid()}`;
+
+  injectedData.push({
+    propertyName: propertyName,
+    propertyValue: propertyValue,
+    // Track the path it was originally injected in just for a better error
+    originalPath: path,
+  });
+  data[propertyName] = propertyValue;
+  return injectedData;
+}
+
+function expectInjectedData(
+  data: Record<string, unknown>,
+  injectedData: InjectedData[],
+): [data: Record<string, unknown>, leftoverInjectedData: InjectedData[]] {
+  const keys = Object.keys(data);
+  for (const key of keys) {
+    const propertyValue = data[key];
+    // Injected data does not have to be found exactly where it was injected,
+    // just that it exists at all.
+    const injectedIndex = injectedData.findIndex(
+      (d) =>
+        d.propertyName === key &&
+        typeof propertyValue === "string" &&
+        propertyValue === d.propertyValue,
+    );
+
+    if (injectedIndex !== -1) {
+      // We found something we injected, remove it
+      injectedData.splice(injectedIndex, 1);
+      delete data[key];
+      continue;
+    }
+
+    if (isStringRecord(propertyValue)) {
+      const [updatedData, leftoverInjectedData] = expectInjectedData(propertyValue, injectedData);
+      data[key] = updatedData;
+      injectedData = leftoverInjectedData;
+    }
+  }
+
+  return [data, injectedData];
+}
+
+/**
+ * Runs the {@link Migrator.migrate} method of your migrator. You may pass in your test data and get back the data after the migration.
+ * This also injects extra properties at every level of your state and makes sure that it can be found.
+ * @param migrator Your migrator to use to do the migration
+ * @param initalData The data to start with
+ * @returns State after your migration has ran.
+ */
+// TODO: Use const generic for TUsers in TypeScript 5.0 so consumers don't have to `as const` themselves
+export async function runMigrator<
+  TMigrator extends Migrator<number, number>,
+  TUsers extends readonly string[] = string[],
+>(migrator: TMigrator, initalData?: InitialDataHint<TUsers>): Promise<Record<string, unknown>> {
+  // Inject fake data at every level of the object
+  const allInjectedData = injectData(initalData, []);
+
+  const fakeStorageService = new FakeStorageService(initalData);
+  const helper = new MigrationHelper(migrator.fromVersion, fakeStorageService, mock());
+
+  // Run their migrations
+  await migrator.migrate(helper);
+  const [data, leftoverInjectedData] = expectInjectedData(
+    fakeStorageService.internalStore,
+    allInjectedData,
+  );
+  expect(leftoverInjectedData).toHaveLength(0);
+
+  return data;
+}

libs/common/src/state-migrations/migrations/12-move-environment-state-to-providers.spec.ts

@@ -0,0 +1,157 @@
+import { runMigrator } from "../migration-helper.spec";
+
+import { MoveEnvironmentStateToProviders } from "./12-move-environment-state-to-providers";
+
+describe("MoveEnvironmentStateToProviders", () => {
+  const migrator = new MoveEnvironmentStateToProviders(11, 12);
+
+  it("can migrate all data", async () => {
+    const output = await runMigrator(migrator, {
+      authenticatedAccounts: ["user1", "user2"] as const,
+      global: {
+        region: "US",
+        environmentUrls: {
+          base: "example.com",
+        },
+        extra: "data",
+      },
+      user1: {
+        extra: "data",
+        settings: {
+          extra: "data",
+          region: "US",
+          environmentUrls: {
+            base: "example.com",
+          },
+        },
+      },
+      user2: {
+        extra: "data",
+        settings: {
+          region: "EU",
+          environmentUrls: {
+            base: "other.example.com",
+          },
+          extra: "data",
+        },
+      },
+      extra: "data",
+    });
+
+    expect(output).toEqual({
+      authenticatedAccounts: ["user1", "user2"],
+      global: {
+        extra: "data",
+      },
+      global_environment_region: "US",
+      global_environment_urls: {
+        base: "example.com",
+      },
+      user1: {
+        extra: "data",
+        settings: {
+          extra: "data",
+        },
+      },
+      user2: {
+        extra: "data",
+        settings: {
+          extra: "data",
+        },
+      },
+      extra: "data",
+      user_user1_environment_region: "US",
+      user_user2_environment_region: "EU",
+      user_user1_environment_urls: {
+        base: "example.com",
+      },
+      user_user2_environment_urls: {
+        base: "other.example.com",
+      },
+    });
+  });
+
+  it("handles missing parts", async () => {
+    const output = await runMigrator(migrator, {
+      authenticatedAccounts: ["user1", "user2"],
+      global: {
+        extra: "data",
+      },
+      user1: {
+        extra: "data",
+        settings: {
+          extra: "data",
+        },
+      },
+      user2: null,
+    });
+
+    expect(output).toEqual({
+      authenticatedAccounts: ["user1", "user2"],
+      global: {
+        extra: "data",
+      },
+      user1: {
+        extra: "data",
+        settings: {
+          extra: "data",
+        },
+      },
+      user2: null,
+    });
+  });
+
+  it("can migrate only global data", async () => {
+    const output = await runMigrator(migrator, {
+      authenticatedAccounts: [] as const,
+      global: {
+        region: "Self-Hosted",
+      },
+    });
+
+    expect(output).toEqual({
+      authenticatedAccounts: [],
+      global_environment_region: "Self-Hosted",
+      global: {},
+    });
+  });
+
+  it("can migrate only user state", async () => {
+    const output = await runMigrator(migrator, {
+      authenticatedAccounts: ["user1"] as const,
+      global: null,
+      user1: {
+        settings: {
+          region: "Self-Hosted",
+          environmentUrls: {
+            base: "some-base-url",
+            api: "some-api-url",
+            identity: "some-identity-url",
+            icons: "some-icons-url",
+            notifications: "some-notifications-url",
+            events: "some-events-url",
+            webVault: "some-webVault-url",
+            keyConnector: "some-keyConnector-url",
+          },
+        },
+      },
+    });
+
+    expect(output).toEqual({
+      authenticatedAccounts: ["user1"] as const,
+      global: null,
+      user1: { settings: {} },
+      user_user1_environment_region: "Self-Hosted",
+      user_user1_environment_urls: {
+        base: "some-base-url",
+        api: "some-api-url",
+        identity: "some-identity-url",
+        icons: "some-icons-url",
+        notifications: "some-notifications-url",
+        events: "some-events-url",
+        webVault: "some-webVault-url",
+        keyConnector: "some-keyConnector-url",
+      },
+    });
+  });
+});

libs/common/src/state-migrations/migrations/12-move-environment-state-to-providers.ts

@@ -0,0 +1,132 @@
+import { KeyDefinitionLike, MigrationHelper, StateDefinitionLike } from "../migration-helper";
+import { Migrator } from "../migrator";
+
+type EnvironmentUrls = Record<string, string>;
+
+type ExpectedAccountType = {
+  settings?: { region?: string; environmentUrls?: EnvironmentUrls };
+};
+
+type ExpectedGlobalType = { region?: string; environmentUrls?: EnvironmentUrls };
+
+const ENVIRONMENT_STATE: StateDefinitionLike = { name: "environment" };
+
+const REGION_KEY: KeyDefinitionLike = { key: "region", stateDefinition: ENVIRONMENT_STATE };
+const URLS_KEY: KeyDefinitionLike = { key: "urls", stateDefinition: ENVIRONMENT_STATE };
+
+export class MoveEnvironmentStateToProviders extends Migrator<11, 12> {
+  async migrate(helper: MigrationHelper): Promise<void> {
+    const legacyGlobal = await helper.get<ExpectedGlobalType>("global");
+
+    // Move global data
+    if (legacyGlobal?.region != null) {
+      await helper.setToGlobal(REGION_KEY, legacyGlobal.region);
+    }
+
+    if (legacyGlobal?.environmentUrls != null) {
+      await helper.setToGlobal(URLS_KEY, legacyGlobal.environmentUrls);
+    }
+
+    const legacyAccounts = await helper.getAccounts<ExpectedAccountType>();
+
+    await Promise.all(
+      legacyAccounts.map(async ({ userId, account }) => {
+        // Move account data
+        if (account?.settings?.region != null) {
+          await helper.setToUser(userId, REGION_KEY, account.settings.region);
+        }
+
+        if (account?.settings?.environmentUrls != null) {
+          await helper.setToUser(userId, URLS_KEY, account.settings.environmentUrls);
+        }
+
+        // Delete old account data
+        delete account?.settings?.region;
+        delete account?.settings?.environmentUrls;
+        await helper.set(userId, account);
+      }),
+    );
+
+    // Delete legacy global data
+    delete legacyGlobal?.region;
+    delete legacyGlobal?.environmentUrls;
+    await helper.set("global", legacyGlobal);
+  }
+
+  async rollback(helper: MigrationHelper): Promise<void> {
+    let legacyGlobal = await helper.get<ExpectedGlobalType>("global");
+
+    let updatedLegacyGlobal = false;
+
+    const globalRegion = await helper.getFromGlobal<string>(REGION_KEY);
+
+    if (globalRegion) {
+      if (!legacyGlobal) {
+        legacyGlobal = {};
+      }
+
+      updatedLegacyGlobal = true;
+      legacyGlobal.region = globalRegion;
+      await helper.setToGlobal(REGION_KEY, null);
+    }
+
+    const globalUrls = await helper.getFromGlobal<EnvironmentUrls>(URLS_KEY);
+
+    if (globalUrls) {
+      if (!legacyGlobal) {
+        legacyGlobal = {};
+      }
+
+      updatedLegacyGlobal = true;
+      legacyGlobal.environmentUrls = globalUrls;
+      await helper.setToGlobal(URLS_KEY, null);
+    }
+
+    if (updatedLegacyGlobal) {
+      await helper.set("global", legacyGlobal);
+    }
+
+    async function rollbackUser(userId: string, account: ExpectedAccountType) {
+      let updatedAccount = false;
+      const userRegion = await helper.getFromUser<string>(userId, REGION_KEY);
+
+      if (userRegion) {
+        if (!account) {
+          account = {};
+        }
+
+        if (!account.settings) {
+          account.settings = {};
+        }
+
+        updatedAccount = true;
+        account.settings.region = userRegion;
+        await helper.setToUser(userId, REGION_KEY, null);
+      }
+
+      const userUrls = await helper.getFromUser<EnvironmentUrls>(userId, URLS_KEY);
+
+      if (userUrls) {
+        if (!account) {
+          account = {};
+        }
+
+        if (!account.settings) {
+          account.settings = {};
+        }
+
+        updatedAccount = true;
+        account.settings.environmentUrls = userUrls;
+        await helper.setToUser(userId, URLS_KEY, null);
+      }
+
+      if (updatedAccount) {
+        await helper.set(userId, account);
+      }
+    }
+
+    const accounts = await helper.getAccounts<ExpectedAccountType>();
+
+    await Promise.all(accounts.map(({ userId, account }) => rollbackUser(userId, account)));
+  }
+}

libs/common/src/state-migrations/migrations/9-move-browser-settings-to-global.spec.ts

@@ -1,31 +1,14 @@
-import { mock } from "jest-mock-extended";
-
-import { FakeStorageService } from "../../../spec/fake-storage.service";
-import { MigrationHelper } from "../migration-helper";
-import { Migrator } from "../migrator";
+import { runMigrator } from "../migration-helper.spec";
 
 import { MoveBrowserSettingsToGlobal } from "./9-move-browser-settings-to-global";
 
-type TestState = { authenticatedAccounts: string[] } & { [key: string]: unknown };
-
-// This could become a helper available to anyone
-const runMigrator = async <TMigrator extends Migrator<number, number>>(
-  migrator: TMigrator,
-  initalData?: Record<string, unknown>,
-): Promise<Record<string, unknown>> => {
-  const fakeStorageService = new FakeStorageService(initalData);
-  const helper = new MigrationHelper(migrator.fromVersion, fakeStorageService, mock());
-  await migrator.migrate(helper);
-  return fakeStorageService.internalStore;
-};
-
 describe("MoveBrowserSettingsToGlobal", () => {
   const myMigrator = new MoveBrowserSettingsToGlobal(8, 9);
 
   // This could be the state for a browser client who has never touched the settings or this could
   // be a different client who doesn't make it possible to toggle these settings
   it("doesn't set any value to global if there is no equivalent settings on the account", async () => {
-    const testInput: TestState = {
+    const output = await runMigrator(myMigrator, {
       authenticatedAccounts: ["user1"],
       global: {
         theme: "system", // A real global setting that should persist after migration
@@ -35,9 +18,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
           region: "Self-hosted",
         },
       },
-    };
-
-    const output = await runMigrator(myMigrator, testInput);
+    });
 
     // No additions to the global state
     expect(output["global"]).toEqual({
@@ -55,7 +36,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
   // This could be a user who opened up the settings page and toggled the checkbox, since this setting infers undefined
   // as false this is essentially the default value.
   it("sets the setting from the users settings if they have toggled the setting but placed it back to it's inferred", async () => {
-    const testInput: TestState = {
+    const output = await runMigrator(myMigrator, {
       authenticatedAccounts: ["user1"],
       global: {
         theme: "system", // A real global setting that should persist after migration
@@ -71,9 +52,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
           region: "Self-hosted",
         },
       },
-    };
-
-    const output = await runMigrator(myMigrator, testInput);
+    });
 
     // User settings should have moved to global
     expect(output["global"]).toEqual({
@@ -94,7 +73,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
 
   // The user has set a value and it's not the default, we should respect that choice globally
   it("should take the only users settings", async () => {
-    const testInput: TestState = {
+    const output = await runMigrator(myMigrator, {
       authenticatedAccounts: ["user1"],
       global: {
         theme: "system", // A real global setting that should persist after migration
@@ -110,9 +89,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
           region: "Self-hosted",
         },
       },
-    };
-
-    const output = await runMigrator(myMigrator, testInput);
+    });
 
     // The value for the single user value should be set to global
     expect(output["global"]).toEqual({
@@ -134,7 +111,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
   // but in the bizzare case, we should interpret any user having the feature turned on as the value for
   // all the accounts.
   it("should take the false value if there are conflicting choices", async () => {
-    const testInput: TestState = {
+    const output = await runMigrator(myMigrator, {
       authenticatedAccounts: ["user1", "user2"],
       global: {
         theme: "system", // A real global setting that should persist after migration
@@ -161,9 +138,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
           region: "Self-hosted",
         },
       },
-    };
-
-    const output = await runMigrator(myMigrator, testInput);
+    });
 
     // The false settings should be respected over the true values
     // neverDomains should be combined into a single object
@@ -191,7 +166,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
   // if one user has toggled the setting back to on and one user has never touched the setting,
   // persist the false value into the global state.
   it("should persist the false value if one user has that in their settings", async () => {
-    const testInput: TestState = {
+    const output = await runMigrator(myMigrator, {
       authenticatedAccounts: ["user1", "user2"],
       global: {
         theme: "system", // A real global setting that should persist after migration
@@ -212,9 +187,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
           region: "Self-hosted",
         },
       },
-    };
-
-    const output = await runMigrator(myMigrator, testInput);
+    });
 
     // The false settings should be respected over the true values
     // neverDomains should be combined into a single object
@@ -241,7 +214,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
   // if one user has toggled the setting off and one user has never touched the setting,
   // persist the false value into the global state.
   it("should persist the false value from a user with no settings since undefined is inferred as false", async () => {
-    const testInput: TestState = {
+    const output = await runMigrator(myMigrator, {
       authenticatedAccounts: ["user1", "user2"],
       global: {
         theme: "system", // A real global setting that should persist after migration
@@ -262,9 +235,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
           region: "Self-hosted",
         },
       },
-    };
-
-    const output = await runMigrator(myMigrator, testInput);
+    });
 
     // The false settings should be respected over the true values
     // neverDomains should be combined into a single object
@@ -292,7 +263,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
   // id of the non-current account isn't saved to the authenticatedAccounts array so we don't have a great way to
   // get the state and include it in our calculations for what the global state should be.
   it("only cares about users defined in authenticatedAccounts", async () => {
-    const testInput: TestState = {
+    const output = await runMigrator(myMigrator, {
       authenticatedAccounts: ["user1"],
       global: {
         theme: "system", // A real global setting that should persist after migration
@@ -319,9 +290,7 @@ describe("MoveBrowserSettingsToGlobal", () => {
           region: "Self-hosted",
         },
       },
-    };
-
-    const output = await runMigrator(myMigrator, testInput);
+    });
 
     // The true settings should be respected over the false values because that whole users values
     // shouldn't be respected.