[PM-6172] Run localStorage migrations for web (#7900)

* Create MigrationRunner

- Create MigrationRunner Service for running migrations in StateService
- Create web override so that migrations also run against `localStorage`

* Fix Web StateService

* Fix WebMigrationRunner

* Fix CLI

* Fix ElectronStateService

* Update Comment

* More Common Scenarios

apps/web/src/app/core/core.module.ts

@@ -19,12 +19,15 @@ import { LoginService as LoginServiceAbstraction } from "@bitwarden/common/auth/
 import { LoginService } from "@bitwarden/common/auth/services/login.service";
 import { FileDownloadService } from "@bitwarden/common/platform/abstractions/file-download/file-download.service";
 import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { MessagingService as MessagingServiceAbstraction } from "@bitwarden/common/platform/abstractions/messaging.service";
 import { PlatformUtilsService as PlatformUtilsServiceAbstraction } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import { StateService as BaseStateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service";
 import { AbstractStorageService } from "@bitwarden/common/platform/abstractions/storage.service";
 import { StateFactory } from "@bitwarden/common/platform/factories/state-factory";
 import { MemoryStorageService } from "@bitwarden/common/platform/services/memory-storage.service";
+import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service";
+import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner";
 import {
   ActiveUserStateProvider,
   GlobalStateProvider,
@@ -38,6 +41,7 @@ import { HtmlStorageService } from "../core/html-storage.service";
 import { I18nService } from "../core/i18n.service";
 import { WebActiveUserStateProvider } from "../platform/web-active-user-state.provider";
 import { WebGlobalStateProvider } from "../platform/web-global-state.provider";
+import { WebMigrationRunner } from "../platform/web-migration-runner";
 import { WebSingleUserStateProvider } from "../platform/web-single-user-state.provider";
 import { WindowStorageService } from "../platform/window-storage.service";
 import { CollectionAdminService } from "../vault/core/collection-admin.service";
@@ -139,6 +143,16 @@ import { WebPlatformUtilsService } from "./web-platform-utils.service";
       useClass: WebGlobalStateProvider,
       deps: [OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_STORAGE, OBSERVABLE_DISK_LOCAL_STORAGE],
     },
+    {
+      provide: MigrationRunner,
+      useClass: WebMigrationRunner,
+      deps: [
+        AbstractStorageService,
+        LogService,
+        MigrationBuilderService,
+        OBSERVABLE_DISK_LOCAL_STORAGE,
+      ],
+    },
   ],
 })
 export class CoreModule {

apps/web/src/app/core/state/state.service.ts

@@ -15,6 +15,7 @@ import {
 } from "@bitwarden/common/platform/abstractions/storage.service";
 import { StateFactory } from "@bitwarden/common/platform/factories/state-factory";
 import { StorageOptions } from "@bitwarden/common/platform/models/domain/storage-options";
+import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner";
 import { StateService as BaseStateService } from "@bitwarden/common/platform/services/state.service";
 import { SendData } from "@bitwarden/common/tools/send/models/data/send.data";
 import { CipherData } from "@bitwarden/common/vault/models/data/cipher.data";
@@ -33,6 +34,7 @@ export class StateService extends BaseStateService<GlobalState, Account> {
     @Inject(STATE_FACTORY) stateFactory: StateFactory<GlobalState, Account>,
     accountService: AccountService,
     environmentService: EnvironmentService,
+    migrationRunner: MigrationRunner,
     @Inject(STATE_SERVICE_USE_CACHE) useAccountCache = true,
   ) {
     super(
@@ -43,6 +45,7 @@ export class StateService extends BaseStateService<GlobalState, Account> {
       stateFactory,
       accountService,
       environmentService,
+      migrationRunner,
       useAccountCache,
     );
   }

apps/web/src/app/platform/web-migration-runner.spec.ts

@@ -0,0 +1,252 @@
+import { MockProxy, mock } from "jest-mock-extended";
+
+import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
+import { AbstractStorageService } from "@bitwarden/common/platform/abstractions/storage.service";
+import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service";
+import { MigrationBuilder } from "@bitwarden/common/state-migrations/migration-builder";
+import { MigrationHelper } from "@bitwarden/common/state-migrations/migration-helper";
+
+import { WebMigrationRunner } from "./web-migration-runner";
+import { WindowStorageService } from "./window-storage.service";
+
+describe("WebMigrationRunner", () => {
+  let logService: MockProxy<LogService>;
+  let sessionStorageService: MockProxy<AbstractStorageService>;
+  let localStorageService: MockProxy<WindowStorageService>;
+  let migrationBuilderService: MockProxy<MigrationBuilderService>;
+
+  let sut: WebMigrationRunner;
+
+  beforeEach(() => {
+    logService = mock();
+    sessionStorageService = mock();
+    localStorageService = mock();
+    migrationBuilderService = mock();
+
+    sut = new WebMigrationRunner(
+      sessionStorageService,
+      logService,
+      migrationBuilderService,
+      localStorageService,
+    );
+  });
+
+  const mockMigrationBuilder = (migration: (helper: MigrationHelper) => Promise<void>) => {
+    migrationBuilderService.build.mockReturnValue({
+      migrate: async (helper: MigrationHelper) => {
+        await migration(helper);
+      },
+      with: () => {
+        throw new Error("Don't use this in tests.");
+      },
+      rollback: () => {
+        throw new Error("Don't use this in tests.");
+      },
+    } as unknown as MigrationBuilder);
+  };
+
+  const mockGet = (
+    mockStorage: MockProxy<AbstractStorageService>,
+    data: Record<string, unknown>,
+  ) => {
+    mockStorage.get.mockImplementation((key) => {
+      return Promise.resolve(data[key]);
+    });
+  };
+
+  it("should run migration for both storage locations", async () => {
+    mockGet(sessionStorageService, {
+      stateVersion: 4,
+    });
+    mockGet(localStorageService, {});
+
+    mockMigrationBuilder(async (helper) => {
+      await helper.set("something", "something");
+    });
+
+    await sut.run();
+
+    expect(sessionStorageService.save).toHaveBeenCalledWith("something", "something");
+    expect(localStorageService.save).toHaveBeenCalledWith("something", "something");
+  });
+
+  it("should only migrate data in one migration if written defensively", async () => {
+    mockGet(sessionStorageService, {
+      stateVersion: 4,
+    });
+    mockGet(localStorageService, {
+      user1: {
+        settings: {
+          myData: "value",
+        },
+      },
+    });
+
+    mockMigrationBuilder(async (helper) => {
+      const account = await helper.get<{ settings?: { myData?: string } }>("user1");
+      const value = account?.settings?.myData;
+      if (value) {
+        await helper.setToUser("user1", { key: "key", stateDefinition: { name: "state" } }, value);
+      }
+    });
+
+    await sut.run();
+
+    expect(sessionStorageService.save).not.toHaveBeenCalled();
+    expect(localStorageService.save).toHaveBeenCalledWith("user_user1_state_key", "value");
+  });
+
+  it("should gather accounts differently", async () => {
+    mockGet(sessionStorageService, {
+      stateVersion: 10,
+      authenticatedAccounts: ["sessionUser1", "sessionUser2"],
+      sessionUser1: {
+        data: 1,
+      },
+      sessionUser2: {
+        data: null,
+      },
+      sessionUser3: {
+        // User does NOT have authenticated accounts entry
+        data: 3,
+      },
+    });
+
+    const localStorageObject = {
+      "8118af89-a621-4b0f-8dd2-4449569e5067": {
+        data: 4,
+      },
+      "cc202dba-55f8-4cbe-8c66-de37e48e7827": {
+        data: <number>null,
+      },
+      otherThing: {
+        data: 6,
+      },
+      "badd2aff-a380-468f-855a-e476557055d5": <object>null,
+      "01f81ccd-fb18-460c-9a6b-811ef5300d4b": 3,
+    };
+
+    mockGet(localStorageService, localStorageObject);
+    localStorageService.getKeys.mockReturnValue(Object.keys(localStorageObject));
+
+    mockMigrationBuilder(async (helper) => {
+      type ExpectedAccountType = {
+        data?: number;
+      };
+      async function migrateAccount(userId: string, account: ExpectedAccountType) {
+        const value = account?.data;
+        if (value != null) {
+          await helper.setToUser(userId, { key: "key", stateDefinition: { name: "state" } }, value);
+          delete account.data;
+          await helper.set(userId, account);
+        }
+      }
+
+      const accounts = await helper.getAccounts();
+      await Promise.all(accounts.map(({ userId, account }) => migrateAccount(userId, account)));
+    });
+
+    await sut.run();
+
+    // Session storage has two users but only one with data
+    expect(sessionStorageService.save).toHaveBeenCalledTimes(2);
+    // Should move the data to the new location first
+    expect(sessionStorageService.save).toHaveBeenNthCalledWith(1, "user_sessionUser1_state_key", 1);
+    // Should then delete the migrated data and resave object
+    expect(sessionStorageService.save).toHaveBeenNthCalledWith(2, "sessionUser1", {});
+
+    expect(sessionStorageService.get).toHaveBeenCalledTimes(4);
+    // Should first get the state version so it knowns which migrations to run (not really used in this test)
+    expect(sessionStorageService.get).toHaveBeenNthCalledWith(1, "stateVersion");
+    // "base" migration runner should trust the authenticatedAccounts stored value for knowing which accounts to migrate
+    expect(sessionStorageService.get).toHaveBeenNthCalledWith(2, "authenticatedAccounts");
+    // Should get the data for each user
+    expect(sessionStorageService.get).toHaveBeenNthCalledWith(3, "sessionUser1");
+    expect(sessionStorageService.get).toHaveBeenNthCalledWith(4, "sessionUser2");
+
+    expect(localStorageService.save).toHaveBeenCalledTimes(2);
+    // Should migrate data for a user in local storage
+    expect(localStorageService.save).toHaveBeenNthCalledWith(
+      1,
+      "user_8118af89-a621-4b0f-8dd2-4449569e5067_state_key",
+      4,
+    );
+    // Should update object with migrated data deleted
+    expect(localStorageService.save).toHaveBeenNthCalledWith(
+      2,
+      "8118af89-a621-4b0f-8dd2-4449569e5067",
+      {},
+    );
+
+    expect(localStorageService.get).toHaveBeenCalledTimes(5);
+    expect(localStorageService.get).toHaveBeenNthCalledWith(1, "stateVersion");
+    expect(localStorageService.get).toHaveBeenNthCalledWith(
+      2,
+      "8118af89-a621-4b0f-8dd2-4449569e5067",
+    );
+    expect(localStorageService.get).toHaveBeenNthCalledWith(
+      3,
+      "cc202dba-55f8-4cbe-8c66-de37e48e7827",
+    );
+    expect(localStorageService.get).toHaveBeenNthCalledWith(
+      4,
+      "badd2aff-a380-468f-855a-e476557055d5",
+    );
+    expect(localStorageService.get).toHaveBeenNthCalledWith(
+      5,
+      "01f81ccd-fb18-460c-9a6b-811ef5300d4b",
+    );
+  });
+
+  it("should default currentVersion to 12 if no stateVersion exists", async () => {
+    mockGet(sessionStorageService, {
+      stateVersion: 14,
+    });
+    mockGet(localStorageService, {});
+
+    let runCount = 0;
+
+    mockMigrationBuilder(async (helper) => {
+      if (runCount === 0) {
+        // This should be the session storage run
+        expect(helper.currentVersion).toBe(14);
+      } else if (runCount === 1) {
+        // This should be the local storage run, and it should be the default version
+        expect(helper.currentVersion).toBe(12);
+      } else {
+        throw new Error("Should not have been called more than twice");
+      }
+
+      runCount++;
+    });
+
+    await sut.run();
+  });
+
+  it("should respect local storage stateVersion", async () => {
+    mockGet(sessionStorageService, {
+      stateVersion: 14,
+    });
+    mockGet(localStorageService, {
+      stateVersion: 18,
+    });
+
+    let runCount = 0;
+
+    mockMigrationBuilder(async (helper) => {
+      if (runCount === 0) {
+        // This should be the session storage run
+        expect(helper.currentVersion).toBe(14);
+      } else if (runCount === 1) {
+        // This should be the local storage run, and it should be the default version
+        expect(helper.currentVersion).toBe(18);
+      } else {
+        throw new Error("Should not have been called more than twice");
+      }
+
+      runCount++;
+    });
+
+    await sut.run();
+  });
+});

apps/web/src/app/platform/web-migration-runner.ts

@@ -0,0 +1,86 @@
+import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
+import { AbstractStorageService } from "@bitwarden/common/platform/abstractions/storage.service";
+import { Utils } from "@bitwarden/common/platform/misc/utils";
+import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service";
+import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner";
+import { MigrationHelper } from "@bitwarden/common/state-migrations/migration-helper";
+
+import { WindowStorageService } from "./window-storage.service";
+
+export class WebMigrationRunner extends MigrationRunner {
+  constructor(
+    diskStorage: AbstractStorageService,
+    logService: LogService,
+    migrationBuilderService: MigrationBuilderService,
+    private diskLocalStorage: WindowStorageService,
+  ) {
+    super(diskStorage, logService, migrationBuilderService);
+  }
+
+  override async run(): Promise<void> {
+    // Run the default migration against session storage
+    await super.run();
+
+    // run web disk local specific migrations
+    const migrationBuilder = this.migrationBuilderService.build();
+
+    let stateVersion = await this.diskLocalStorage.get<number | null>("stateVersion");
+    if (stateVersion == null) {
+      // Web has never stored a state version in disk local before
+      // TODO: Is this a good number?
+      stateVersion = 12;
+    }
+
+    // Run migrations again specifically for web `localStorage`.
+    const helper = new WebMigrationHelper(stateVersion, this.diskLocalStorage, this.logService);
+
+    await migrationBuilder.migrate(helper);
+  }
+}
+
+class WebMigrationHelper extends MigrationHelper {
+  private readonly diskLocalStorageService: WindowStorageService;
+
+  constructor(
+    currentVersion: number,
+    storageService: WindowStorageService,
+    logService: LogService,
+  ) {
+    super(currentVersion, storageService, logService);
+    this.diskLocalStorageService = storageService;
+  }
+
+  override async getAccounts<ExpectedAccountType>(): Promise<
+    { userId: string; account: ExpectedAccountType }[]
+  > {
+    // Get all the keys of things stored in `localStorage`
+    const keys = this.diskLocalStorageService.getKeys();
+
+    const accounts: { userId: string; account: ExpectedAccountType }[] = [];
+
+    for (const key of keys) {
+      // Is this is likely a userid
+      if (!Utils.isGuid(key)) {
+        continue;
+      }
+
+      const accountCandidate = await this.diskLocalStorageService.get(key);
+
+      // If there isn't data at that key location, don't bother
+      if (accountCandidate == null) {
+        continue;
+      }
+
+      // The legacy account object was always an object, if
+      // it is some other primitive, it's like a false positive.
+      if (typeof accountCandidate !== "object") {
+        continue;
+      }
+
+      accounts.push({ userId: key, account: accountCandidate as ExpectedAccountType });
+    }
+
+    // TODO: Cache this for future calls?
+    return accounts;
+  }
+}

apps/web/src/app/platform/window-storage.service.ts

@@ -50,4 +50,8 @@ export class WindowStorageService implements AbstractStorageService, ObservableS
     this.updatesSubject.next({ key, updateType: "remove" });
     return Promise.resolve();
   }
+
+  getKeys(): string[] {
+    return Object.keys(this.storage);
+  }
 }