From 96df6f1a9e1036d2355af93c6933a3c735de4018 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 22 Oct 2025 13:50:55 +0200 Subject: [PATCH] Deduplicate prompts --- .../encrypted-migrations-scheduler.service.ts | 18 ++++++++++++++---- .../src/services/jslib-services.module.ts | 2 +- .../default-encrypted-migrator.spec.ts | 3 +++ .../default-encrypted-migrator.ts | 7 +++++++ .../encrypted-migrator.abstraction.ts | 5 +++++ .../migrations/minimum-kdf-migration.ts | 4 ++++ .../kdf/change-kdf.service.spec.ts | 2 +- .../src/platform/sync/default-sync.service.ts | 4 +--- libs/key-management/src/models/kdf-config.ts | 2 +- 9 files changed, 37 insertions(+), 10 deletions(-) diff --git a/libs/angular/src/key-management/encrypted-migration/encrypted-migrations-scheduler.service.ts b/libs/angular/src/key-management/encrypted-migration/encrypted-migrations-scheduler.service.ts index a7eae5a5136..cea01beb131 100644 --- a/libs/angular/src/key-management/encrypted-migration/encrypted-migrations-scheduler.service.ts +++ b/libs/angular/src/key-management/encrypted-migration/encrypted-migrations-scheduler.service.ts @@ -1,4 +1,4 @@ -import { combineLatest, map, switchMap, of, firstValueFrom, filter, debounceTime, tap } from "rxjs"; +import { combineLatest, map, switchMap, of, firstValueFrom, filter, tap, delay } from "rxjs"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; @@ -26,7 +26,7 @@ export const ENCRYPTED_MIGRATION_DISMISSED = new UserKeyDefinition( clearOn: [], }, ); -const DISMISS_TIME_HOURS = 24; +const DISMISS_TIME_HOURS = 0; type UserSyncData = { userId: UserId; @@ -41,6 +41,8 @@ type UserSyncData = { export class DefaultEncryptedMigrationsSchedulerService implements EncryptedMigrationsSchedulerService { + isMigrating = false; + constructor( private syncService: SyncService, private accountService: AccountService, @@ -68,10 +70,9 @@ export class DefaultEncryptedMigrationsSchedulerService this.authService.authStatusFor$(userId), this.syncService.lastSync$(userId), ]).pipe( - // Prevent double emissions from frequent state changes during login/unlock from showing overlapping prompts - debounceTime(2000), filter(([authStatus]) => authStatus === AuthenticationStatus.Unlocked), map(([, lastSync]) => ({ userId, lastSync }) as UserSyncData), + delay(5_000), tap(({ userId }) => this.runMigrationsIfNeeded(userId)), ), ), @@ -87,6 +88,14 @@ export class DefaultEncryptedMigrationsSchedulerService return; } + if (this.isMigrating || (await this.encryptedMigrator.isRunningMigrations())) { + this.logService.info( + `[EncryptedMigrationsScheduler] Skipping migration check for user ${userId} because migrations are already in progress`, + ); + return; + } + + this.isMigrating = true; switch (await this.encryptedMigrator.needsMigrations(userId)) { case "noMigrationNeeded": this.logService.info( @@ -108,6 +117,7 @@ export class DefaultEncryptedMigrationsSchedulerService await this.runMigrationsWithoutInteraction(userId); break; } + this.isMigrating = false; } private async runMigrationsWithoutInteraction(userId: UserId): Promise { diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 81ca6dc66d5..9394d626fb4 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -520,6 +520,7 @@ const safeProviders: SafeProvider[] = [ LogService, ConfigService, MasterPasswordServiceAbstraction, + SyncService, ], }), safeProvider({ @@ -868,7 +869,6 @@ const safeProviders: SafeProvider[] = [ AuthServiceAbstraction, StateProvider, SecurityStateService, - KdfConfigService, ], }), safeProvider({ diff --git a/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.spec.ts b/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.spec.ts index 10fc8c77944..8f8e6b46945 100644 --- a/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.spec.ts +++ b/libs/common/src/key-management/encrypted-migrator/default-encrypted-migrator.spec.ts @@ -1,6 +1,7 @@ import { mock } from "jest-mock-extended"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { SyncService } from "@bitwarden/common/platform/sync"; import { UserId } from "@bitwarden/common/types/guid"; // eslint-disable-next-line no-restricted-imports import { KdfConfigService } from "@bitwarden/key-management"; @@ -21,6 +22,7 @@ describe("EncryptedMigrator", () => { const mockLogService = mock(); const configService = mock(); const masterPasswordService = mock(); + const syncService = mock(); let sut: DefaultEncryptedMigrator; const mockMigration = mock(); @@ -42,6 +44,7 @@ describe("EncryptedMigrator", () => { mockLogService, configService, masterPasswordService, + syncService, ); }); 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 afc3b1c13d4..e2d04ee4d13 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 @@ -1,5 +1,6 @@ import { assertNonNullish } from "@bitwarden/common/auth/utils"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { SyncService } from "@bitwarden/common/platform/sync"; import { UserId } from "@bitwarden/common/types/guid"; // eslint-disable-next-line no-restricted-imports import { KdfConfigService } from "@bitwarden/key-management"; @@ -22,6 +23,7 @@ export class DefaultEncryptedMigrator implements EncryptedMigrator { private readonly logService: LogService, readonly configService: ConfigService, readonly masterPasswordService: MasterPasswordServiceAbstraction, + readonly syncService: SyncService, ) { // Register migrations here this.migrations.push({ @@ -67,6 +69,7 @@ export class DefaultEncryptedMigrator implements EncryptedMigrator { } this.logService.mark("[Encrypted Migrator] Finish"); this.logService.info(`[Encrypted Migrator] Completed migrations for user: ${userId}`); + await this.syncService.fullSync(true); } catch (error) { this.logService.error( `[Encrypted Migrator] Error running migrations for user: ${userId}`, @@ -93,4 +96,8 @@ export class DefaultEncryptedMigrator implements EncryptedMigrator { return "noMigrationNeeded"; } } + + async isRunningMigrations(): Promise { + return this.isRunningMigration; + } } diff --git a/libs/common/src/key-management/encrypted-migrator/encrypted-migrator.abstraction.ts b/libs/common/src/key-management/encrypted-migrator/encrypted-migrator.abstraction.ts index 09732629ba9..ea032168eb5 100644 --- a/libs/common/src/key-management/encrypted-migrator/encrypted-migrator.abstraction.ts +++ b/libs/common/src/key-management/encrypted-migrator/encrypted-migrator.abstraction.ts @@ -24,4 +24,9 @@ export abstract class EncryptedMigrator { * @param userId The ID of the user to check migrations for. */ abstract needsMigrations(userId: UserId): Promise; + + /** + * Indicates whether migrations are currently running. + */ + abstract isRunningMigrations(): Promise; } 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 52b95472826..4e2490f11f1 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 @@ -42,6 +42,10 @@ export class MinimumKdfMigration implements EncryptedMigration { new PBKDF2KdfConfig(MINIMUM_PBKDF2_ITERATIONS_FOR_UPGRADE), userId, ); + await this.kdfConfigService.setKdfConfig( + userId, + new PBKDF2KdfConfig(MINIMUM_PBKDF2_ITERATIONS_FOR_UPGRADE), + ); } async needsMigration(userId: UserId): Promise { diff --git a/libs/common/src/key-management/kdf/change-kdf.service.spec.ts b/libs/common/src/key-management/kdf/change-kdf.service.spec.ts index c7df90f4790..12096155641 100644 --- a/libs/common/src/key-management/kdf/change-kdf.service.spec.ts +++ b/libs/common/src/key-management/kdf/change-kdf.service.spec.ts @@ -17,7 +17,7 @@ import { } from "../master-password/types/master-password.types"; import { ChangeKdfApiService } from "./change-kdf-api.service.abstraction"; -import { DefaultChangeKdfService } from "./change-kdf-service"; +import { DefaultChangeKdfService } from "./change-kdf.service"; describe("ChangeKdfService", () => { const changeKdfApiService = mock(); diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index e599fbc1c48..d5fa2d0ae68 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -12,7 +12,7 @@ import { // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. import { SecurityStateService } from "@bitwarden/common/key-management/security-state/abstractions/security-state.service"; // eslint-disable-next-line no-restricted-imports -import { KdfConfigService, KeyService } from "@bitwarden/key-management"; +import { KeyService } from "@bitwarden/key-management"; // FIXME: remove `src` and fix import // eslint-disable-next-line no-restricted-imports @@ -100,7 +100,6 @@ export class DefaultSyncService extends CoreSyncService { authService: AuthService, stateProvider: StateProvider, private securityStateService: SecurityStateService, - private kdfConfigService: KdfConfigService, ) { super( tokenService, @@ -435,7 +434,6 @@ export class DefaultSyncService extends CoreSyncService { masterPasswordUnlockData, userId, ); - await this.kdfConfigService.setKdfConfig(userId, masterPasswordUnlockData.kdf); } } } diff --git a/libs/key-management/src/models/kdf-config.ts b/libs/key-management/src/models/kdf-config.ts index ca044845b3c..afa26bab146 100644 --- a/libs/key-management/src/models/kdf-config.ts +++ b/libs/key-management/src/models/kdf-config.ts @@ -160,4 +160,4 @@ export function fromSdkKdfConfig(sdkKdf: Kdf): KdfConfig { } export const DEFAULT_KDF_CONFIG = new PBKDF2KdfConfig(PBKDF2KdfConfig.ITERATIONS.defaultValue); -export const MINIMUM_PBKDF2_ITERATIONS_FOR_UPGRADE = 600_000; +export const MINIMUM_PBKDF2_ITERATIONS_FOR_UPGRADE = 600_500;