[PM-35240] Add sync before forced kdf migration

libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.ts

@@ -34,6 +34,7 @@ export class DefaultEncryptedMigrator implements EncryptedMigrator {
         logService,
         configService,
         masterPasswordService,
+        syncService,
       ),
     });
   }

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<LogService>();
   const mockConfigService = mock<ConfigService>();
   const mockMasterPasswordService = mock<MasterPasswordServiceAbstraction>();
+  const mockSyncService = mock<SyncService>();
 
   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);
 

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<void> {
@@ -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,19 +52,36 @@ 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";
     }
 
     if (!(await this.configService.getFeatureFlag(FeatureFlag.ForceUpdateKDFSettings))) {
       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<boolean> {
+    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
+    );
   }
 }