diff --git a/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.ts b/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.ts index bc91e24070ae..1f1a140ce1d7 100644 --- a/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.ts +++ b/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.ts @@ -34,6 +34,7 @@ export class DefaultEncryptedMigrator implements EncryptedMigrator { logService, configService, masterPasswordService, + syncService, ), }); } diff --git a/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.spec.ts b/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.spec.ts index cf2bd307b6c6..e07222470111 100644 --- a/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.spec.ts +++ b/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.spec.ts @@ -1,5 +1,6 @@ import { mock } from "jest-mock-extended"; +import { SyncService } from "@bitwarden/common/platform/sync"; // eslint-disable-next-line no-restricted-imports import { Argon2KdfConfig, @@ -23,6 +24,7 @@ describe("MinimumKdfMigration", () => { const mockLogService = mock(); const mockConfigService = mock(); const mockMasterPasswordService = mock(); + const mockSyncService = mock(); let sut: MinimumKdfMigration; @@ -38,6 +40,7 @@ describe("MinimumKdfMigration", () => { mockLogService, mockConfigService, mockMasterPasswordService, + mockSyncService, ); }); @@ -116,6 +119,29 @@ describe("MinimumKdfMigration", () => { ); }); + it("should return 'noMigrationNeeded' when sync updates local KDF state to no longer need migration", async () => { + mockMasterPasswordService.userHasMasterPassword.mockResolvedValue(true); + mockConfigService.getFeatureFlag.mockResolvedValue(true); + mockKdfConfigService.getKdfConfig + .mockResolvedValueOnce({ + kdfType: KdfType.PBKDF2_SHA256, + iterations: PBKDF2KdfConfig.ITERATIONS.min - 1000, + } as any) + .mockResolvedValueOnce({ + kdfType: KdfType.PBKDF2_SHA256, + iterations: PBKDF2KdfConfig.ITERATIONS.min, + } as any); + + const result = await sut.needsMigration(mockUserId); + + expect(result).toBe("noMigrationNeeded"); + expect(mockSyncService.fullSync).toHaveBeenCalledWith(false); + expect(mockKdfConfigService.getKdfConfig).toHaveBeenCalledTimes(2); + expect(mockLogService.info).toHaveBeenCalledWith( + `[MinimumKdfMigration] After syncing, user ${mockUserId} does not need migration anymore. This means the migration was likely already performed on another client!`, + ); + }); + it("should throw error when userId is null", async () => { await expect(sut.needsMigration(null as any)).rejects.toThrow("userId"); }); @@ -127,6 +153,13 @@ describe("MinimumKdfMigration", () => { describe("runMigrations", () => { it("should update KDF parameters with minimum PBKDF2 iterations", async () => { + mockKdfConfigService.getKdfConfig.mockResolvedValue({ + kdfType: KdfType.PBKDF2_SHA256, + iterations: PBKDF2KdfConfig.ITERATIONS.min - 1000, + } as any); + mockConfigService.getFeatureFlag.mockResolvedValue(true); + mockMasterPasswordService.userHasMasterPassword.mockResolvedValue(true); + await sut.runMigrations(mockUserId, mockMasterPassword); expect(mockLogService.info).toHaveBeenCalledWith( @@ -137,10 +170,14 @@ describe("MinimumKdfMigration", () => { expect.any(PBKDF2KdfConfig), mockUserId, ); + expect(mockKdfConfigService.setKdfConfig).toHaveBeenCalledWith( + mockUserId, + expect.any(PBKDF2KdfConfig), + ); // Verify the PBKDF2KdfConfig has the correct iteration count const kdfConfigArg = (mockChangeKdfService.updateUserKdfParams as jest.Mock).mock.calls[0][1]; - expect(kdfConfigArg.iterations).toBe(PBKDF2KdfConfig.ITERATIONS.defaultValue); + expect(kdfConfigArg.iterations).toBe(PBKDF2KdfConfig.ITERATIONS.min); }); it("should throw error when userId is null", async () => { @@ -164,6 +201,13 @@ describe("MinimumKdfMigration", () => { }); it("should handle errors from changeKdfService", async () => { + mockKdfConfigService.getKdfConfig.mockResolvedValue({ + kdfType: KdfType.PBKDF2_SHA256, + iterations: PBKDF2KdfConfig.ITERATIONS.min - 1000, + } as any); + mockConfigService.getFeatureFlag.mockResolvedValue(true); + mockMasterPasswordService.userHasMasterPassword.mockResolvedValue(true); + const mockError = new Error("KDF update failed"); mockChangeKdfService.updateUserKdfParams.mockRejectedValue(mockError); diff --git a/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.ts b/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.ts index 0666064b36e9..320eb78d1cb0 100644 --- a/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.ts +++ b/libs/common/src/key-management/encrypted-migrator/migrations/minimum-kdf-migration.ts @@ -1,3 +1,4 @@ +import { SyncService } from "@bitwarden/common/platform/sync"; import { UserId } from "@bitwarden/common/types/guid"; // eslint-disable-next-line no-restricted-imports import { KdfConfigService, KdfType, PBKDF2KdfConfig } from "@bitwarden/key-management"; @@ -23,6 +24,7 @@ export class MinimumKdfMigration implements EncryptedMigration { private readonly logService: LogService, private readonly configService: ConfigService, private readonly masterPasswordService: MasterPasswordServiceAbstraction, + private readonly syncService: SyncService, ) {} async runMigrations(userId: UserId, masterPassword: string | null): Promise { @@ -34,12 +36,12 @@ export class MinimumKdfMigration implements EncryptedMigration { ); await this.changeKdfService.updateUserKdfParams( masterPassword!, - new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.defaultValue), + new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.min), userId, ); await this.kdfConfigService.setKdfConfig( userId, - new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.defaultValue), + new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.min), ); } @@ -50,12 +52,7 @@ export class MinimumKdfMigration implements EncryptedMigration { return "noMigrationNeeded"; } - // Only PBKDF2 users below the minimum iteration count need migration - const kdfConfig = await this.kdfConfigService.getKdfConfig(userId); - if ( - kdfConfig.kdfType !== KdfType.PBKDF2_SHA256 || - kdfConfig.iterations >= PBKDF2KdfConfig.ITERATIONS.min - ) { + if (!(await this.localStateNeedsMigration(userId))) { return "noMigrationNeeded"; } @@ -63,6 +60,28 @@ export class MinimumKdfMigration implements EncryptedMigration { return "noMigrationNeeded"; } - return "needsMigrationWithMasterPassword"; + // This will be replaced by a separate API call that provides the user decryption options. + // This may have a race condition with account switching, since runMigrations is bound to a user-id, but + // sync-service takes the active user-id from state. It ensures we have the latest data from the server. + // It is possible that the current client was offline while the migration happened. This would cause the + // local state to still have the old KDF values and prompt another time. + await this.syncService.fullSync(false); + if (!(await this.localStateNeedsMigration(userId))) { + this.logService.info( + `[MinimumKdfMigration] After syncing, user ${userId} does not need migration anymore. This means the migration was likely already performed on another client!`, + ); + return "noMigrationNeeded"; + } else { + return "needsMigrationWithMasterPassword"; + } + } + + private async localStateNeedsMigration(userId: UserId): Promise { + const kdfConfig = await this.kdfConfigService.getKdfConfig(userId); + // Only PBKDF2 users below the minimum iteration count need migration + return ( + kdfConfig.kdfType === KdfType.PBKDF2_SHA256 && + kdfConfig.iterations < PBKDF2KdfConfig.ITERATIONS.min + ); } }