From 1b305c3c239a0fd9f448b161057f132419a7cffd Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> Date: Mon, 15 Dec 2025 11:36:34 +0100 Subject: [PATCH] [PM-26049] Auto key not stored due to vault timeout write vs read race condition for cli (#17707) * auto key not stored due to vault timeout race condition being null for cli * fix unit test default state * neglected electron key service test cleanup * bad merge - fix formatting --- .../electron-key.service.spec.ts | 49 ++++++------------- .../src/key-management/vault-timeout/index.ts | 2 + .../vault-timeout-settings.service.ts | 6 ++- libs/key-management/src/key.service.spec.ts | 4 +- libs/key-management/src/key.service.ts | 4 +- 5 files changed, 28 insertions(+), 37 deletions(-) diff --git a/apps/desktop/src/key-management/electron-key.service.spec.ts b/apps/desktop/src/key-management/electron-key.service.spec.ts index cc1d68ed050..c5e010b1766 100644 --- a/apps/desktop/src/key-management/electron-key.service.spec.ts +++ b/apps/desktop/src/key-management/electron-key.service.spec.ts @@ -13,11 +13,13 @@ import { UserKey } from "@bitwarden/common/types/key"; import { BiometricStateService, KdfConfigService } from "@bitwarden/key-management"; import { - makeSymmetricCryptoKey, FakeAccountService, - mockAccountServiceWith, FakeStateProvider, + makeSymmetricCryptoKey, + mockAccountServiceWith, } from "../../../../libs/common/spec"; +// eslint-disable-next-line no-restricted-imports +import { VAULT_TIMEOUT } from "../../../../libs/common/src/key-management/vault-timeout"; import { DesktopBiometricsService } from "./biometrics/desktop.biometrics.service"; import { ElectronKeyService } from "./electron-key.service"; @@ -40,11 +42,13 @@ describe("ElectronKeyService", () => { let accountService: FakeAccountService; let masterPasswordService: FakeMasterPasswordService; - beforeEach(() => { + beforeEach(async () => { accountService = mockAccountServiceWith(mockUserId); masterPasswordService = new FakeMasterPasswordService(); stateProvider = new FakeStateProvider(accountService); + await stateProvider.setUserState(VAULT_TIMEOUT, 10, mockUserId); + keyService = new ElectronKeyService( masterPasswordService, keyGenerationService, @@ -79,38 +83,17 @@ describe("ElectronKeyService", () => { expect(biometricStateService.getBiometricUnlockEnabled).toHaveBeenCalledWith(mockUserId); }); - describe("biometric unlock enabled", () => { - beforeEach(() => { - biometricStateService.getBiometricUnlockEnabled.mockResolvedValue(true); - }); + it("sets biometric key when biometric unlock enabled", async () => { + biometricStateService.getBiometricUnlockEnabled.mockResolvedValue(true); - it("sets null biometric client key half and biometric unlock key when require password on start disabled", async () => { - biometricStateService.getRequirePasswordOnStart.mockResolvedValue(false); + await keyService.setUserKey(userKey, mockUserId); - await keyService.setUserKey(userKey, mockUserId); - - expect(biometricService.setBiometricProtectedUnlockKeyForUser).toHaveBeenCalledWith( - mockUserId, - userKey, - ); - expect(biometricStateService.setEncryptedClientKeyHalf).not.toHaveBeenCalled(); - expect(biometricStateService.getBiometricUnlockEnabled).toHaveBeenCalledWith(mockUserId); - }); - - describe("require password on start enabled", () => { - beforeEach(() => { - biometricStateService.getRequirePasswordOnStart.mockResolvedValue(true); - }); - - it("sets biometric key", async () => { - await keyService.setUserKey(userKey, mockUserId); - - expect(biometricService.setBiometricProtectedUnlockKeyForUser).toHaveBeenCalledWith( - mockUserId, - userKey, - ); - }); - }); + expect(biometricService.setBiometricProtectedUnlockKeyForUser).toHaveBeenCalledWith( + mockUserId, + userKey, + ); + expect(biometricStateService.setEncryptedClientKeyHalf).not.toHaveBeenCalled(); + expect(biometricStateService.getBiometricUnlockEnabled).toHaveBeenCalledWith(mockUserId); }); }); }); diff --git a/libs/common/src/key-management/vault-timeout/index.ts b/libs/common/src/key-management/vault-timeout/index.ts index ba32c12c9fb..1879de5353c 100644 --- a/libs/common/src/key-management/vault-timeout/index.ts +++ b/libs/common/src/key-management/vault-timeout/index.ts @@ -10,3 +10,5 @@ export { VaultTimeoutNumberType, VaultTimeoutStringType, } from "./types/vault-timeout.type"; +// Only used by desktop's electron-key.service.spec.ts test +export { VAULT_TIMEOUT } from "./services/vault-timeout-settings.state"; diff --git a/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts b/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts index dc0c5620518..b8bc859d11c 100644 --- a/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts +++ b/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts @@ -13,6 +13,7 @@ import { shareReplay, switchMap, tap, + concatMap, } from "rxjs"; // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. @@ -150,7 +151,7 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA return from( this.determineVaultTimeout(currentVaultTimeout, maxSessionTimeoutPolicyData), ).pipe( - tap((vaultTimeout: VaultTimeout) => { + concatMap(async (vaultTimeout: VaultTimeout) => { this.logService.debug( "[VaultTimeoutSettingsService] Determined vault timeout is %o for user id %s", vaultTimeout, @@ -159,8 +160,9 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA // As a side effect, set the new value determined by determineVaultTimeout into state if it's different from the current if (vaultTimeout !== currentVaultTimeout) { - return this.stateProvider.setUserState(VAULT_TIMEOUT, vaultTimeout, userId); + await this.stateProvider.setUserState(VAULT_TIMEOUT, vaultTimeout, userId); } + return vaultTimeout; }), catchError((error: unknown) => { // Protect outer observable from canceling on error by catching and returning EMPTY diff --git a/libs/key-management/src/key.service.spec.ts b/libs/key-management/src/key.service.spec.ts index a5b4eb01c7c..c0af62fe6e9 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/libs/key-management/src/key.service.spec.ts @@ -69,11 +69,13 @@ describe("keyService", () => { let accountService: FakeAccountService; let masterPasswordService: FakeMasterPasswordService; - beforeEach(() => { + beforeEach(async () => { accountService = mockAccountServiceWith(mockUserId); masterPasswordService = new FakeMasterPasswordService(); stateProvider = new FakeStateProvider(accountService); + await stateProvider.setUserState(VAULT_TIMEOUT, VaultTimeoutStringType.Never, mockUserId); + keyService = new DefaultKeyService( masterPasswordService, keyGenerationService, diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index f0e5f6ee08e..621a8135d1e 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -691,7 +691,9 @@ export class DefaultKeyService implements KeyServiceAbstraction { // the VaultTimeoutSettingsSvc and this service. // This should be fixed as part of the PM-7082 - Auto Key Service work. const vaultTimeout = await firstValueFrom( - this.stateProvider.getUserState$(VAULT_TIMEOUT, userId), + this.stateProvider + .getUserState$(VAULT_TIMEOUT, userId) + .pipe(filter((timeout) => timeout != null)), ); shouldStoreKey = vaultTimeout == VaultTimeoutStringType.Never;