diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 77be810cf3..13732ed0f3 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -73,6 +73,8 @@ import { EncryptServiceImplementation } from "@bitwarden/common/platform/service import { MultithreadEncryptServiceImplementation } from "@bitwarden/common/platform/services/cryptography/multithread-encrypt.service.implementation"; import { FileUploadService } from "@bitwarden/common/platform/services/file-upload/file-upload.service"; 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 { SystemService } from "@bitwarden/common/platform/services/system.service"; import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service"; import { @@ -381,6 +383,13 @@ export default class MainBackground { this.stateProvider, this.accountService, ); + + const migrationRunner = new MigrationRunner( + this.storageService, + this.logService, + new MigrationBuilderService(), + ); + this.stateService = new BrowserStateService( this.storageService, this.secureStorageService, @@ -389,6 +398,7 @@ export default class MainBackground { new StateFactory(GlobalState, Account), this.accountService, this.environmentService, + migrationRunner, ); this.platformUtilsService = new BrowserPlatformUtilsService( this.messagingService, diff --git a/apps/browser/src/platform/background/service-factories/migration-runner.factory.ts b/apps/browser/src/platform/background/service-factories/migration-runner.factory.ts new file mode 100644 index 0000000000..a49699a615 --- /dev/null +++ b/apps/browser/src/platform/background/service-factories/migration-runner.factory.ts @@ -0,0 +1,32 @@ +import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service"; +import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; + +import { CachedServices, FactoryOptions, factory } from "./factory-options"; +import { LogServiceInitOptions, logServiceFactory } from "./log-service.factory"; +import { + DiskStorageServiceInitOptions, + diskStorageServiceFactory, +} from "./storage-service.factory"; + +type MigrationRunnerFactory = FactoryOptions; + +export type MigrationRunnerInitOptions = MigrationRunnerFactory & + DiskStorageServiceInitOptions & + LogServiceInitOptions; + +export async function migrationRunnerFactory( + cache: { migrationRunner?: MigrationRunner } & CachedServices, + opts: MigrationRunnerInitOptions, +): Promise { + return factory( + cache, + "migrationRunner", + opts, + async () => + new MigrationRunner( + await diskStorageServiceFactory(cache, opts), + await logServiceFactory(cache, opts), + new MigrationBuilderService(), + ), + ); +} diff --git a/apps/browser/src/platform/background/service-factories/state-service.factory.ts b/apps/browser/src/platform/background/service-factories/state-service.factory.ts index 158fc80b5f..8bcb65a320 100644 --- a/apps/browser/src/platform/background/service-factories/state-service.factory.ts +++ b/apps/browser/src/platform/background/service-factories/state-service.factory.ts @@ -14,6 +14,7 @@ import { } from "./environment-service.factory"; import { CachedServices, factory, FactoryOptions } from "./factory-options"; import { logServiceFactory, LogServiceInitOptions } from "./log-service.factory"; +import { migrationRunnerFactory, MigrationRunnerInitOptions } from "./migration-runner.factory"; import { diskStorageServiceFactory, secureStorageServiceFactory, @@ -36,7 +37,8 @@ export type StateServiceInitOptions = StateServiceFactoryOptions & MemoryStorageServiceInitOptions & LogServiceInitOptions & AccountServiceInitOptions & - EnvironmentServiceInitOptions; + EnvironmentServiceInitOptions & + MigrationRunnerInitOptions; export async function stateServiceFactory( cache: { stateService?: BrowserStateService } & CachedServices, @@ -55,11 +57,11 @@ export async function stateServiceFactory( opts.stateServiceOptions.stateFactory, await accountServiceFactory(cache, opts), await environmentServiceFactory(cache, opts), + await migrationRunnerFactory(cache, opts), opts.stateServiceOptions.useAccountCache, ), ); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - service.init(); + // TODO: If we run migration through a chrome installed/updated event we can turn off running migrations + await service.init(); return service; } diff --git a/apps/browser/src/platform/services/browser-state.service.spec.ts b/apps/browser/src/platform/services/browser-state.service.spec.ts index b421f55a06..0cb7e4fd44 100644 --- a/apps/browser/src/platform/services/browser-state.service.spec.ts +++ b/apps/browser/src/platform/services/browser-state.service.spec.ts @@ -10,6 +10,7 @@ import { import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; import { State } from "@bitwarden/common/platform/models/domain/state"; +import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { SendType } from "@bitwarden/common/tools/send/enums/send-type"; import { SendView } from "@bitwarden/common/tools/send/models/view/send.view"; @@ -31,6 +32,7 @@ describe("Browser State Service", () => { let useAccountCache: boolean; let accountService: MockProxy; let environmentService: MockProxy; + let migrationRunner: MockProxy; let state: State; const userId = "userId"; @@ -44,6 +46,7 @@ describe("Browser State Service", () => { stateFactory = mock(); accountService = mock(); environmentService = mock(); + migrationRunner = mock(); // turn off account cache for tests useAccountCache = false; @@ -70,6 +73,7 @@ describe("Browser State Service", () => { stateFactory, accountService, environmentService, + migrationRunner, useAccountCache, ); }); diff --git a/apps/browser/src/platform/services/browser-state.service.ts b/apps/browser/src/platform/services/browser-state.service.ts index 6641d1e81a..ac5d1a9102 100644 --- a/apps/browser/src/platform/services/browser-state.service.ts +++ b/apps/browser/src/platform/services/browser-state.service.ts @@ -10,6 +10,7 @@ import { import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; 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 { Account } from "../../models/account"; @@ -46,6 +47,7 @@ export class BrowserStateService stateFactory: StateFactory, accountService: AccountService, environmentService: EnvironmentService, + migrationRunner: MigrationRunner, useAccountCache = true, ) { super( @@ -56,6 +58,7 @@ export class BrowserStateService stateFactory, accountService, environmentService, + migrationRunner, useAccountCache, ); diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 99d6c80f69..1389526217 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -72,6 +72,7 @@ import { GlobalState } from "@bitwarden/common/platform/models/domain/global-sta 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 { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { DerivedStateProvider, StateProvider } from "@bitwarden/common/platform/state"; import { SearchService } from "@bitwarden/common/services/search.service"; import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/generator/password"; @@ -482,6 +483,7 @@ function getBgService(service: keyof MainBackground) { logService: LogServiceAbstraction, accountService: AccountServiceAbstraction, environmentService: EnvironmentService, + migrationRunner: MigrationRunner, ) => { return new BrowserStateService( storageService, @@ -491,6 +493,7 @@ function getBgService(service: keyof MainBackground) { new StateFactory(GlobalState, Account), accountService, environmentService, + migrationRunner, ); }, deps: [ @@ -500,6 +503,7 @@ function getBgService(service: keyof MainBackground) { LogServiceAbstraction, AccountServiceAbstraction, EnvironmentService, + MigrationRunner, ], }, { diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index a3f163a77f..05ad43128e 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -51,6 +51,8 @@ import { EncryptServiceImplementation } from "@bitwarden/common/platform/service import { EnvironmentService } from "@bitwarden/common/platform/services/environment.service"; import { FileUploadService } from "@bitwarden/common/platform/services/file-upload/file-upload.service"; 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 { NoopMessagingService } from "@bitwarden/common/platform/services/noop-messaging.service"; import { StateService } from "@bitwarden/common/platform/services/state.service"; import { @@ -276,6 +278,12 @@ export class Main { this.environmentService = new EnvironmentService(this.stateProvider, this.accountService); + const migrationRunner = new MigrationRunner( + this.storageService, + this.logService, + new MigrationBuilderService(), + ); + this.stateService = new StateService( this.storageService, this.secureStorageService, @@ -284,6 +292,7 @@ export class Main { new StateFactory(GlobalState, Account), this.accountService, this.environmentService, + migrationRunner, ); this.cryptoService = new CryptoService( diff --git a/apps/desktop/src/app/services/services.module.ts b/apps/desktop/src/app/services/services.module.ts index 461b254bdd..d1dca936b5 100644 --- a/apps/desktop/src/app/services/services.module.ts +++ b/apps/desktop/src/app/services/services.module.ts @@ -38,6 +38,7 @@ import { BiometricStateService } from "@bitwarden/common/platform/biometrics/bio import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; import { MemoryStorageService } from "@bitwarden/common/platform/services/memory-storage.service"; +import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { SystemService } from "@bitwarden/common/platform/services/system.service"; import { StateProvider } from "@bitwarden/common/platform/state"; // eslint-disable-next-line import/no-restricted-paths -- Implementation for memory storage @@ -134,6 +135,7 @@ const RELOAD_CALLBACK = new InjectionToken<() => any>("RELOAD_CALLBACK"); STATE_FACTORY, AccountServiceAbstraction, EnvironmentService, + MigrationRunner, STATE_SERVICE_USE_CACHE, ], }, diff --git a/apps/desktop/src/main.ts b/apps/desktop/src/main.ts index 53c50a37ed..6d4b2db19d 100644 --- a/apps/desktop/src/main.ts +++ b/apps/desktop/src/main.ts @@ -8,6 +8,8 @@ import { StateFactory } from "@bitwarden/common/platform/factories/state-factory import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; import { EnvironmentService } from "@bitwarden/common/platform/services/environment.service"; 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 { NoopMessagingService } from "@bitwarden/common/platform/services/noop-messaging.service"; /* eslint-disable import/no-restricted-paths -- We need the implementation to inject, but generally this should not be accessed */ import { DefaultActiveUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-active-user-state.provider"; @@ -17,7 +19,6 @@ import { DefaultSingleUserStateProvider } from "@bitwarden/common/platform/state import { DefaultStateProvider } from "@bitwarden/common/platform/state/implementations/default-state.provider"; import { MemoryStorageService as MemoryStorageServiceForStateProviders } from "@bitwarden/common/platform/state/storage/memory-storage.service"; /* eslint-enable import/no-restricted-paths */ -import { migrate } from "@bitwarden/common/state-migrations"; import { MenuMain } from "./main/menu/menu.main"; import { MessagingMain } from "./main/messaging.main"; @@ -46,6 +47,7 @@ export class Main { stateService: ElectronStateService; environmentService: EnvironmentService; desktopCredentialStorageListener: DesktopCredentialStorageListener; + migrationRunner: MigrationRunner; windowMain: WindowMain; messagingMain: MessagingMain; @@ -123,6 +125,12 @@ export class Main { this.environmentService = new EnvironmentService(stateProvider, accountService); + this.migrationRunner = new MigrationRunner( + this.storageService, + this.logService, + new MigrationBuilderService(), + ); + // TODO: this state service will have access to on disk storage, but not in memory storage. // If we could get this to work using the stateService singleton that the rest of the app uses we could save // ourselves from some hacks, like having to manually update the app menu vs. the menu subscribing to events. @@ -134,6 +142,7 @@ export class Main { new StateFactory(GlobalState, Account), accountService, // will not broadcast logouts. This is a hack until we can remove messaging dependency this.environmentService, + this.migrationRunner, false, // Do not use disk caching because this will get out of sync with the renderer service ); @@ -192,7 +201,7 @@ export class Main { bootstrap() { this.desktopCredentialStorageListener.init(); // Run migrations first, then other things - migrate(this.storageService, this.logService).then( + this.migrationRunner.run().then( async () => { await this.windowMain.init(); const locale = await this.stateService.getLocale(); diff --git a/apps/web/src/app/core/core.module.ts b/apps/web/src/app/core/core.module.ts index 4f47862c3a..ee6f53ed01 100644 --- a/apps/web/src/app/core/core.module.ts +++ b/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 { diff --git a/apps/web/src/app/core/state/state.service.ts b/apps/web/src/app/core/state/state.service.ts index e142b3f4fc..a40a24b215 100644 --- a/apps/web/src/app/core/state/state.service.ts +++ b/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 { @Inject(STATE_FACTORY) stateFactory: StateFactory, accountService: AccountService, environmentService: EnvironmentService, + migrationRunner: MigrationRunner, @Inject(STATE_SERVICE_USE_CACHE) useAccountCache = true, ) { super( @@ -43,6 +45,7 @@ export class StateService extends BaseStateService { stateFactory, accountService, environmentService, + migrationRunner, useAccountCache, ); } diff --git a/apps/web/src/app/platform/web-migration-runner.spec.ts b/apps/web/src/app/platform/web-migration-runner.spec.ts new file mode 100644 index 0000000000..4b2949230e --- /dev/null +++ b/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; + let sessionStorageService: MockProxy; + let localStorageService: MockProxy; + let migrationBuilderService: MockProxy; + + 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) => { + 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, + data: Record, + ) => { + 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: null, + }, + otherThing: { + data: 6, + }, + "badd2aff-a380-468f-855a-e476557055d5": 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(); + }); +}); diff --git a/apps/web/src/app/platform/web-migration-runner.ts b/apps/web/src/app/platform/web-migration-runner.ts new file mode 100644 index 0000000000..e8f44f7f41 --- /dev/null +++ b/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 { + // 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("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(): 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; + } +} diff --git a/apps/web/src/app/platform/window-storage.service.ts b/apps/web/src/app/platform/window-storage.service.ts index e011b45538..d5ba1283a9 100644 --- a/apps/web/src/app/platform/window-storage.service.ts +++ b/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); + } } diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index da797aec9f..a3e6d1222e 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -125,6 +125,8 @@ import { EncryptServiceImplementation } from "@bitwarden/common/platform/service import { MultithreadEncryptServiceImplementation } from "@bitwarden/common/platform/services/cryptography/multithread-encrypt.service.implementation"; import { EnvironmentService } from "@bitwarden/common/platform/services/environment.service"; import { FileUploadService } from "@bitwarden/common/platform/services/file-upload/file-upload.service"; +import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service"; +import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { NoopNotificationsService } from "@bitwarden/common/platform/services/noop-notifications.service"; import { StateService } from "@bitwarden/common/platform/services/state.service"; import { ValidationService } from "@bitwarden/common/platform/services/validation.service"; @@ -561,6 +563,7 @@ import { ModalService } from "./modal.service"; STATE_FACTORY, AccountServiceAbstraction, EnvironmentServiceAbstraction, + MigrationRunner, STATE_SERVICE_USE_CACHE, ], }, @@ -929,6 +932,15 @@ import { ModalService } from "./modal.service"; useClass: VaultSettingsService, deps: [StateProvider], }, + { + provide: MigrationRunner, + useClass: MigrationRunner, + deps: [AbstractStorageService, LogService, MigrationBuilderService], + }, + { + provide: MigrationBuilderService, + useClass: MigrationBuilderService, + }, { provide: BillingApiServiceAbstraction, useClass: BillingApiService, diff --git a/libs/common/src/platform/services/migration-builder.service.spec.ts b/libs/common/src/platform/services/migration-builder.service.spec.ts new file mode 100644 index 0000000000..c06d4bf53c --- /dev/null +++ b/libs/common/src/platform/services/migration-builder.service.spec.ts @@ -0,0 +1,89 @@ +import { mock } from "jest-mock-extended"; + +import { FakeStorageService } from "../../../spec/fake-storage.service"; +import { MigrationHelper } from "../../state-migrations/migration-helper"; + +import { MigrationBuilderService } from "./migration-builder.service"; + +describe("MigrationBuilderService", () => { + // All migrations from 10+ should be capable of having a null account object or null global object + const startingStateVersion = 10; + + const noAccounts = { + stateVersion: startingStateVersion, + authenticatedAccounts: [], + }; + + const nullAndUndefinedAccounts = { + stateVersion: startingStateVersion, + authenticatedAccounts: ["account1", "account2"], + account1: null, + account2: undefined, + }; + + const emptyAccountObject = { + stateVersion: startingStateVersion, + authenticatedAccounts: ["account1"], + account1: {}, + }; + + const nullCommonAccountProperties = { + stateVersion: startingStateVersion, + authenticatedAccounts: ["account1"], + account1: { + data: null, + keys: null, + profile: null, + settings: null, + tokens: null, + }, + }; + + const emptyCommonAccountProperties = { + stateVersion: startingStateVersion, + authenticatedAccounts: ["account1"], + account1: { + data: {}, + keys: {}, + profile: {}, + settings: {}, + tokens: {}, + }, + }; + + const nullGlobal = { + stateVersion: startingStateVersion, + global: null, + }; + + const undefinedGlobal = { + stateVersion: startingStateVersion, + global: undefined, + }; + + const emptyGlobalObject = { + stateVersion: startingStateVersion, + global: {}, + }; + + it.each([ + noAccounts, + nullAndUndefinedAccounts, + emptyAccountObject, + nullCommonAccountProperties, + emptyCommonAccountProperties, + nullGlobal, + undefinedGlobal, + emptyGlobalObject, + ])("should not produce migrations that throw when given data: %s", async (startingState) => { + const sut = new MigrationBuilderService(); + + const helper = new MigrationHelper( + startingStateVersion, + new FakeStorageService(startingState), + mock(), + ); + + await sut.build().migrate(helper); + }); +}); diff --git a/libs/common/src/platform/services/migration-builder.service.ts b/libs/common/src/platform/services/migration-builder.service.ts new file mode 100644 index 0000000000..db08cadf4b --- /dev/null +++ b/libs/common/src/platform/services/migration-builder.service.ts @@ -0,0 +1,10 @@ +import { createMigrationBuilder } from "../../state-migrations"; +import { MigrationBuilder } from "../../state-migrations/migration-builder"; + +export class MigrationBuilderService { + private migrationBuilderCache: MigrationBuilder; + + build() { + return (this.migrationBuilderCache ??= createMigrationBuilder()); + } +} diff --git a/libs/common/src/platform/services/migration-runner.spec.ts b/libs/common/src/platform/services/migration-runner.spec.ts new file mode 100644 index 0000000000..3934137f66 --- /dev/null +++ b/libs/common/src/platform/services/migration-runner.spec.ts @@ -0,0 +1,102 @@ +import { mock } from "jest-mock-extended"; + +import { awaitAsync } from "../../../spec"; +import { CURRENT_VERSION } from "../../state-migrations"; +import { MigrationBuilder } from "../../state-migrations/migration-builder"; +import { LogService } from "../abstractions/log.service"; +import { AbstractStorageService } from "../abstractions/storage.service"; + +import { MigrationBuilderService } from "./migration-builder.service"; +import { MigrationRunner } from "./migration-runner"; + +describe("MigrationRunner", () => { + const storage = mock(); + const logService = mock(); + const migrationBuilderService = mock(); + const mockMigrationBuilder = mock(); + + migrationBuilderService.build.mockReturnValue(mockMigrationBuilder); + + const sut = new MigrationRunner(storage, logService, migrationBuilderService); + + describe("migrate", () => { + it("should not run migrations if state is empty", async () => { + storage.get.mockReturnValueOnce(null); + await sut.run(); + expect(migrationBuilderService.build).not.toHaveBeenCalled(); + }); + + it("should set to current version if state is empty", async () => { + storage.get.mockReturnValueOnce(null); + await sut.run(); + expect(storage.save).toHaveBeenCalledWith("stateVersion", CURRENT_VERSION); + }); + + it("should run migration if there is a stateVersion", async () => { + storage.get.mockResolvedValueOnce(12); + + await sut.run(); + + expect(mockMigrationBuilder.migrate).toHaveBeenCalled(); + }); + }); + + describe("waitForCompletion", () => { + it("should wait until stateVersion is current before completing", async () => { + let stateVersion: number | null = null; + + storage.get.mockImplementation((key) => { + if (key === "stateVersion") { + return Promise.resolve(stateVersion); + } + }); + + let promiseCompleted = false; + + const completionPromise = sut.waitForCompletion().then(() => (promiseCompleted = true)); + + await awaitAsync(10); + + expect(promiseCompleted).toBe(false); + + stateVersion = CURRENT_VERSION; + await completionPromise; + }); + + // Skipped for CI since this test takes a while to complete, remove `.skip` to test + it.skip( + "will complete after 8 second step wait if migrations still aren't complete", + async () => { + storage.get.mockImplementation((key) => { + if (key === "stateVersion") { + return Promise.resolve(null); + } + }); + + let promiseCompleted = false; + + void sut.waitForCompletion().then(() => (promiseCompleted = true)); + + await awaitAsync(2 + 4 + 8 + 16); + + expect(promiseCompleted).toBe(false); + + await awaitAsync(32 + 64 + 128 + 256); + + expect(promiseCompleted).toBe(false); + + await awaitAsync(512 + 1024 + 2048 + 4096); + + expect(promiseCompleted).toBe(false); + + const SKEW = 20; + + await awaitAsync(8192 + SKEW); + + expect(promiseCompleted).toBe(true); + }, + // Have to combine all the steps into the timeout to get this to run + 2 + 4 + 8 + 16 + 32 + 64 + 128 + 256 + 512 + 1024 + 2048 + 4096 + 8192 + 100, + ); + }); +}); diff --git a/libs/common/src/platform/services/migration-runner.ts b/libs/common/src/platform/services/migration-runner.ts new file mode 100644 index 0000000000..f900343424 --- /dev/null +++ b/libs/common/src/platform/services/migration-runner.ts @@ -0,0 +1,37 @@ +import { waitForMigrations } from "../../state-migrations"; +import { CURRENT_VERSION, currentVersion } from "../../state-migrations/migrate"; +import { MigrationHelper } from "../../state-migrations/migration-helper"; +import { LogService } from "../abstractions/log.service"; +import { AbstractStorageService } from "../abstractions/storage.service"; + +import { MigrationBuilderService } from "./migration-builder.service"; + +export class MigrationRunner { + constructor( + protected diskStorage: AbstractStorageService, + protected logService: LogService, + protected migrationBuilderService: MigrationBuilderService, + ) {} + + async run(): Promise { + const migrationHelper = new MigrationHelper( + await currentVersion(this.diskStorage, this.logService), + this.diskStorage, + this.logService, + ); + + if (migrationHelper.currentVersion < 0) { + // Cannot determine state, assuming empty so we don't repeatedly apply a migration. + await this.diskStorage.save("stateVersion", CURRENT_VERSION); + return; + } + + const migrationBuilder = this.migrationBuilderService.build(); + + await migrationBuilder.migrate(migrationHelper); + } + + async waitForCompletion(): Promise { + await waitForMigrations(this.diskStorage, this.logService); + } +} diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index 5ad305676f..8086634490 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -14,8 +14,6 @@ import { BiometricKey } from "../../auth/types/biometric-key"; import { VaultTimeoutAction } from "../../enums/vault-timeout-action.enum"; import { EventData } from "../../models/data/event.data"; import { WindowState } from "../../models/domain/window-state"; -import { migrate } from "../../state-migrations"; -import { waitForMigrations } from "../../state-migrations/migrate"; import { GeneratorOptions } from "../../tools/generator/generator-options"; import { GeneratedPasswordHistory, PasswordGeneratorOptions } from "../../tools/generator/password"; import { UsernameGeneratorOptions } from "../../tools/generator/username"; @@ -57,6 +55,8 @@ import { State } from "../models/domain/state"; import { StorageOptions } from "../models/domain/storage-options"; import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key"; +import { MigrationRunner } from "./migration-runner"; + const keys = { state: "state", stateVersion: "stateVersion", @@ -108,6 +108,7 @@ export class StateService< protected stateFactory: StateFactory, protected accountService: AccountService, protected environmentService: EnvironmentService, + private migrationRunner: MigrationRunner, protected useAccountCache: boolean = true, ) { // If the account gets changed, verify the new account is unlocked @@ -136,11 +137,11 @@ export class StateService< } if (runMigrations) { - await migrate(this.storageService, this.logService); + await this.migrationRunner.run(); } else { // It may have been requested to not run the migrations but we should defensively not // continue this method until migrations have a chance to be completed elsewhere. - await waitForMigrations(this.storageService, this.logService); + await this.migrationRunner.waitForCompletion(); } await this.state().then(async (state) => { diff --git a/libs/common/src/state-migrations/index.ts b/libs/common/src/state-migrations/index.ts index c883b1ca81..e51a4e8e93 100644 --- a/libs/common/src/state-migrations/index.ts +++ b/libs/common/src/state-migrations/index.ts @@ -1 +1 @@ -export { migrate, CURRENT_VERSION } from "./migrate"; +export { createMigrationBuilder, waitForMigrations, CURRENT_VERSION } from "./migrate"; diff --git a/libs/common/src/state-migrations/migrate.spec.ts b/libs/common/src/state-migrations/migrate.spec.ts index ade3d261f6..a3e1b7ac57 100644 --- a/libs/common/src/state-migrations/migrate.spec.ts +++ b/libs/common/src/state-migrations/migrate.spec.ts @@ -5,34 +5,7 @@ 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"; -import { CURRENT_VERSION, currentVersion, migrate } from "./migrate"; -import { MigrationBuilder } from "./migration-builder"; - -jest.mock("./migration-builder", () => { - return { - MigrationBuilder: { - create: jest.fn().mockReturnThis(), - }, - }; -}); - -describe("migrate", () => { - it("should not run migrations if state is empty", async () => { - const storage = mock(); - const logService = mock(); - storage.get.mockReturnValueOnce(null); - await migrate(storage, logService); - expect(MigrationBuilder.create).not.toHaveBeenCalled(); - }); - - it("should set to current version if state is empty", async () => { - const storage = mock(); - const logService = mock(); - storage.get.mockReturnValueOnce(null); - await migrate(storage, logService); - expect(storage.save).toHaveBeenCalledWith("stateVersion", CURRENT_VERSION); - }); -}); +import { currentVersion } from "./migrate"; describe("currentVersion", () => { let storage: MockProxy; diff --git a/libs/common/src/state-migrations/migrate.ts b/libs/common/src/state-migrations/migrate.ts index 8ab0f8881b..376aec0026 100644 --- a/libs/common/src/state-migrations/migrate.ts +++ b/libs/common/src/state-migrations/migrate.ts @@ -4,7 +4,6 @@ import { LogService } from "../platform/abstractions/log.service"; import { AbstractStorageService } from "../platform/abstractions/storage.service"; 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"; @@ -27,21 +26,8 @@ export const MIN_VERSION = 2; export const CURRENT_VERSION = 18; export type MinVersion = typeof MIN_VERSION; -export async function migrate( - storageService: AbstractStorageService, - logService: LogService, -): Promise { - const migrationHelper = new MigrationHelper( - await currentVersion(storageService, logService), - storageService, - logService, - ); - if (migrationHelper.currentVersion < 0) { - // Cannot determine state, assuming empty so we don't repeatedly apply a migration. - await storageService.save("stateVersion", CURRENT_VERSION); - return; - } - await MigrationBuilder.create() +export function createMigrationBuilder() { + return MigrationBuilder.create() .with(MinVersionMigrator) .with(FixPremiumMigrator, 2, 3) .with(RemoveEverBeenUnlockedMigrator, 3, 4) @@ -58,9 +44,7 @@ export async function migrate( .with(FolderMigrator, 14, 15) .with(LastSyncMigrator, 15, 16) .with(EnablePasskeysMigrator, 16, 17) - .with(AutofillSettingsKeyMigrator, 17, CURRENT_VERSION) - - .migrate(migrationHelper); + .with(AutofillSettingsKeyMigrator, 17, CURRENT_VERSION); } export async function currentVersion(