From 6823ab27db6d70e46c59ea1c46e128faf18ed6fe Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> Date: Tue, 27 Jan 2026 11:28:13 +0100 Subject: [PATCH] [PM-27283] [BEEEP] Reactive `availableVaultTimeoutActions$` in vault timeout settings (#17731) * reactive `availableVaultTimeoutActions$` in vault timeout settings * cleanup * deprecation docs * explicitly provided user id * clearer mocking * better docs --- .../settings/account-security.component.ts | 2 +- .../background-browser-biometrics.service.ts | 2 +- .../extension-lock-component.service.spec.ts | 3 +- .../extension-lock-component.service.ts | 2 +- .../src/app/accounts/settings.component.ts | 2 +- .../account-security-nudge.service.ts | 2 +- .../pin/pin-state.service.abstraction.ts | 21 ++- .../pin/pin-state.service.implementation.ts | 68 +++++--- .../pin/pin-state.service.spec.ts | 50 +++++- .../vault-timeout-settings.service.ts | 5 +- .../vault-timeout-settings.service.spec.ts | 160 ++++++++++++------ .../vault-timeout-settings.service.ts | 127 ++++++-------- .../biometric-state.service.spec.ts | 52 +++--- .../src/biometrics/biometric-state.service.ts | 18 +- 14 files changed, 309 insertions(+), 205 deletions(-) diff --git a/apps/browser/src/auth/popup/settings/account-security.component.ts b/apps/browser/src/auth/popup/settings/account-security.component.ts index 6a3378670bf..1789feebe4e 100644 --- a/apps/browser/src/auth/popup/settings/account-security.component.ts +++ b/apps/browser/src/auth/popup/settings/account-security.component.ts @@ -257,7 +257,7 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { pin: await this.pinService.isPinSet(activeAccount.id), pinLockWithMasterPassword: (await this.pinService.getPinLockType(activeAccount.id)) == "EPHEMERAL", - biometric: await this.vaultTimeoutSettingsService.isBiometricLockSet(), + biometric: await this.vaultTimeoutSettingsService.isBiometricLockSet(activeAccount.id), enableAutoBiometricsPrompt: await firstValueFrom( this.biometricStateService.promptAutomatically$, ), diff --git a/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.ts b/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.ts index c8be58b0bde..d7e755b34ea 100644 --- a/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.ts +++ b/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.ts @@ -35,7 +35,7 @@ export class BackgroundBrowserBiometricsService extends BiometricsService { super(); // Always connect to the native messaging background if biometrics are enabled, not just when it is used // so that there is no wait when used. - const biometricsEnabled = this.biometricStateService.biometricUnlockEnabled$; + const biometricsEnabled = this.biometricStateService.biometricUnlockEnabled$(); combineLatest([timer(0, this.BACKGROUND_POLLING_INTERVAL), biometricsEnabled]) .pipe( diff --git a/apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts b/apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts index ecdb899b9a7..934fb9307ee 100644 --- a/apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts +++ b/apps/browser/src/key-management/lock/services/extension-lock-component.service.spec.ts @@ -375,7 +375,7 @@ describe("ExtensionLockComponentService", () => { platformUtilsService.supportsSecureStorage.mockReturnValue( mockInputs.platformSupportsSecureStorage, ); - biometricStateService.biometricUnlockEnabled$ = of(true); + biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(true)); // PIN pinService.isPinDecryptionAvailable.mockResolvedValue(mockInputs.pinDecryptionAvailable); @@ -386,6 +386,7 @@ describe("ExtensionLockComponentService", () => { const unlockOptions = await firstValueFrom(service.getAvailableUnlockOptions$(userId)); expect(unlockOptions).toEqual(expectedOutput); + expect(biometricStateService.biometricUnlockEnabled$).toHaveBeenCalledWith(userId); }); }); }); diff --git a/apps/browser/src/key-management/lock/services/extension-lock-component.service.ts b/apps/browser/src/key-management/lock/services/extension-lock-component.service.ts index 5e6e564bbc2..1ed9d1ea967 100644 --- a/apps/browser/src/key-management/lock/services/extension-lock-component.service.ts +++ b/apps/browser/src/key-management/lock/services/extension-lock-component.service.ts @@ -69,7 +69,7 @@ export class ExtensionLockComponentService implements LockComponentService { return combineLatest([ // Note: defer is preferable b/c it delays the execution of the function until the observable is subscribed to defer(async () => { - if (!(await firstValueFrom(this.biometricStateService.biometricUnlockEnabled$))) { + if (!(await firstValueFrom(this.biometricStateService.biometricUnlockEnabled$(userId)))) { return BiometricsStatus.NotEnabledLocally; } else { // TODO remove after 2025.3 diff --git a/apps/desktop/src/app/accounts/settings.component.ts b/apps/desktop/src/app/accounts/settings.component.ts index 3952335af48..f2e828b95ce 100644 --- a/apps/desktop/src/app/accounts/settings.component.ts +++ b/apps/desktop/src/app/accounts/settings.component.ts @@ -385,7 +385,7 @@ export class SettingsComponent implements OnInit, OnDestroy { this.vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(activeAccount.id), ), pin: this.userHasPinSet, - biometric: await this.vaultTimeoutSettingsService.isBiometricLockSet(), + biometric: await this.vaultTimeoutSettingsService.isBiometricLockSet(activeAccount.id), requireMasterPasswordOnAppRestart: !(await this.biometricsService.hasPersistentKey( activeAccount.id, )), diff --git a/libs/angular/src/vault/services/custom-nudges-services/account-security-nudge.service.ts b/libs/angular/src/vault/services/custom-nudges-services/account-security-nudge.service.ts index 835c9e35ac7..ab8a1869266 100644 --- a/libs/angular/src/vault/services/custom-nudges-services/account-security-nudge.service.ts +++ b/libs/angular/src/vault/services/custom-nudges-services/account-security-nudge.service.ts @@ -39,7 +39,7 @@ export class AccountSecurityNudgeService extends DefaultSingleNudgeService { this.getNudgeStatus$(nudgeType, userId), of(Date.now() - THIRTY_DAYS_MS), from(this.pinService.isPinSet(userId)), - this.biometricStateService.biometricUnlockEnabled$, + this.biometricStateService.biometricUnlockEnabled$(userId), this.organizationService.organizations$(userId), this.policyService.policiesByType$(PolicyType.RemoveUnlockWithPin, userId), ]).pipe( diff --git a/libs/common/src/key-management/pin/pin-state.service.abstraction.ts b/libs/common/src/key-management/pin/pin-state.service.abstraction.ts index 4aef268c1c4..d577d75ef6f 100644 --- a/libs/common/src/key-management/pin/pin-state.service.abstraction.ts +++ b/libs/common/src/key-management/pin/pin-state.service.abstraction.ts @@ -11,6 +11,20 @@ import { PinLockType } from "./pin-lock-type"; * The PinStateService manages the storage and retrieval of PIN-related state for user accounts. */ export abstract class PinStateServiceAbstraction { + /** + * Checks if a user is enrolled into PIN unlock + * @param userId The user's id + * @throws If the user id is not provided + */ + abstract pinSet$(userId: UserId): Observable; + + /** + * Gets the user's {@link PinLockType} + * @param userId The user's id + * @throws If the user id is not provided + */ + abstract pinLockType$(userId: UserId): Observable; + /** * Gets the user's UserKey encrypted PIN * @deprecated - This is not a public API. DO NOT USE IT @@ -21,17 +35,12 @@ export abstract class PinStateServiceAbstraction { /** * Gets the user's {@link PinLockType} + * @deprecated Use {@link pinLockType$} instead * @param userId The user's id * @throws If the user id is not provided */ abstract getPinLockType(userId: UserId): Promise; - /** - * Checks if a user is enrolled into PIN unlock - * @param userId The user's id - */ - abstract isPinSet(userId: UserId): Promise; - /** * Gets the user's PIN-protected UserKey envelope, either persistent or ephemeral based on the provided PinLockType * @deprecated - This is not a public API. DO NOT USE IT diff --git a/libs/common/src/key-management/pin/pin-state.service.implementation.ts b/libs/common/src/key-management/pin/pin-state.service.implementation.ts index d5b2608f280..10046191c01 100644 --- a/libs/common/src/key-management/pin/pin-state.service.implementation.ts +++ b/libs/common/src/key-management/pin/pin-state.service.implementation.ts @@ -1,4 +1,4 @@ -import { firstValueFrom, map, Observable } from "rxjs"; +import { combineLatest, firstValueFrom, map, Observable } from "rxjs"; import { PasswordProtectedKeyEnvelope } from "@bitwarden/sdk-internal"; import { StateProvider } from "@bitwarden/state"; @@ -26,27 +26,36 @@ export class PinStateService implements PinStateServiceAbstraction { .pipe(map((value) => (value ? new EncString(value) : null))); } - async isPinSet(userId: UserId): Promise { + pinSet$(userId: UserId): Observable { assertNonNullish(userId, "userId"); - return (await this.getPinLockType(userId)) !== "DISABLED"; + return this.pinLockType$(userId).pipe(map((pinLockType) => pinLockType !== "DISABLED")); + } + + pinLockType$(userId: UserId): Observable { + assertNonNullish(userId, "userId"); + + return combineLatest([ + this.pinProtectedUserKeyEnvelope$(userId, "PERSISTENT").pipe(map((key) => key != null)), + this.stateProvider + .getUserState$(USER_KEY_ENCRYPTED_PIN, userId) + .pipe(map((key) => key != null)), + ]).pipe( + map(([isPersistentPinSet, isPinSet]) => { + if (isPersistentPinSet) { + return "PERSISTENT"; + } else if (isPinSet) { + return "EPHEMERAL"; + } else { + return "DISABLED"; + } + }), + ); } async getPinLockType(userId: UserId): Promise { assertNonNullish(userId, "userId"); - const isPersistentPinSet = - (await this.getPinProtectedUserKeyEnvelope(userId, "PERSISTENT")) != null; - const isPinSet = - (await firstValueFrom(this.stateProvider.getUserState$(USER_KEY_ENCRYPTED_PIN, userId))) != - null; - - if (isPersistentPinSet) { - return "PERSISTENT"; - } else if (isPinSet) { - return "EPHEMERAL"; - } else { - return "DISABLED"; - } + return await firstValueFrom(this.pinLockType$(userId)); } async getPinProtectedUserKeyEnvelope( @@ -55,17 +64,7 @@ export class PinStateService implements PinStateServiceAbstraction { ): Promise { assertNonNullish(userId, "userId"); - if (pinLockType === "EPHEMERAL") { - return await firstValueFrom( - this.stateProvider.getUserState$(PIN_PROTECTED_USER_KEY_ENVELOPE_EPHEMERAL, userId), - ); - } else if (pinLockType === "PERSISTENT") { - return await firstValueFrom( - this.stateProvider.getUserState$(PIN_PROTECTED_USER_KEY_ENVELOPE_PERSISTENT, userId), - ); - } else { - throw new Error(`Unsupported PinLockType: ${pinLockType}`); - } + return await firstValueFrom(this.pinProtectedUserKeyEnvelope$(userId, pinLockType)); } async setPinState( @@ -110,4 +109,19 @@ export class PinStateService implements PinStateServiceAbstraction { await this.stateProvider.setUserState(PIN_PROTECTED_USER_KEY_ENVELOPE_EPHEMERAL, null, userId); } + + private pinProtectedUserKeyEnvelope$( + userId: UserId, + pinLockType: PinLockType, + ): Observable { + assertNonNullish(userId, "userId"); + + if (pinLockType === "EPHEMERAL") { + return this.stateProvider.getUserState$(PIN_PROTECTED_USER_KEY_ENVELOPE_EPHEMERAL, userId); + } else if (pinLockType === "PERSISTENT") { + return this.stateProvider.getUserState$(PIN_PROTECTED_USER_KEY_ENVELOPE_PERSISTENT, userId); + } else { + throw new Error(`Unsupported PinLockType: ${pinLockType}`); + } + } } diff --git a/libs/common/src/key-management/pin/pin-state.service.spec.ts b/libs/common/src/key-management/pin/pin-state.service.spec.ts index 7406701c28d..42dcce9fedc 100644 --- a/libs/common/src/key-management/pin/pin-state.service.spec.ts +++ b/libs/common/src/key-management/pin/pin-state.service.spec.ts @@ -1,4 +1,4 @@ -import { firstValueFrom } from "rxjs"; +import { firstValueFrom, of } from "rxjs"; import { PasswordProtectedKeyEnvelope } from "@bitwarden/sdk-internal"; @@ -94,14 +94,50 @@ describe("PinStateService", () => { }); }); - describe("getPinLockType()", () => { + describe("pinSet$", () => { beforeEach(() => { jest.clearAllMocks(); }); it("should throw an error if userId is null", async () => { // Act & Assert - await expect(sut.getPinLockType(null as any)).rejects.toThrow("userId"); + expect(() => sut.pinSet$(null as any)).toThrow("userId"); + }); + + it("should return false when pin lock type is DISABLED", async () => { + // Arrange + jest.spyOn(sut, "pinLockType$").mockReturnValue(of("DISABLED")); + + // Act + const result = await firstValueFrom(sut.pinSet$(mockUserId)); + + // Assert + expect(result).toBe(false); + }); + + it.each([["PERSISTENT" as PinLockType], ["EPHEMERAL" as PinLockType]])( + "should return true when pin lock type is %s", + async (pinLockType) => { + // Arrange + jest.spyOn(sut, "pinLockType$").mockReturnValue(of(pinLockType)); + + // Act + const result = await firstValueFrom(sut.pinSet$(mockUserId)); + + // Assert + expect(result).toBe(true); + }, + ); + }); + + describe("pinLockType$", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it("should throw an error if userId is null", async () => { + // Act & Assert + expect(() => sut.pinLockType$(null as any)).toThrow("userId"); }); it("should return 'PERSISTENT' if a pin protected user key (persistent) is found", async () => { @@ -114,7 +150,7 @@ describe("PinStateService", () => { ); // Act - const result = await sut.getPinLockType(mockUserId); + const result = await firstValueFrom(sut.pinLockType$(mockUserId)); // Assert expect(result).toBe("PERSISTENT"); @@ -125,7 +161,7 @@ describe("PinStateService", () => { await stateProvider.setUserState(USER_KEY_ENCRYPTED_PIN, mockUserKeyEncryptedPin, mockUserId); // Act - const result = await sut.getPinLockType(mockUserId); + const result = await firstValueFrom(sut.pinLockType$(mockUserId)); // Assert expect(result).toBe("EPHEMERAL"); @@ -135,7 +171,7 @@ describe("PinStateService", () => { // Arrange - don't set any PIN-related state // Act - const result = await sut.getPinLockType(mockUserId); + const result = await firstValueFrom(sut.pinLockType$(mockUserId)); // Assert expect(result).toBe("DISABLED"); @@ -151,7 +187,7 @@ describe("PinStateService", () => { await stateProvider.setUserState(USER_KEY_ENCRYPTED_PIN, null, mockUserId); // Act - const result = await sut.getPinLockType(mockUserId); + const result = await firstValueFrom(sut.pinLockType$(mockUserId)); // Assert expect(result).toBe("DISABLED"); diff --git a/libs/common/src/key-management/vault-timeout/abstractions/vault-timeout-settings.service.ts b/libs/common/src/key-management/vault-timeout/abstractions/vault-timeout-settings.service.ts index 697b8a1875c..44108b69513 100644 --- a/libs/common/src/key-management/vault-timeout/abstractions/vault-timeout-settings.service.ts +++ b/libs/common/src/key-management/vault-timeout/abstractions/vault-timeout-settings.service.ts @@ -20,10 +20,9 @@ export abstract class VaultTimeoutSettingsService { /** * Get the available vault timeout actions for the current user * - * **NOTE:** This observable is not yet connected to the state service, so it will not update when the state changes * @param userId The user id to check. If not provided, the current user is used */ - abstract availableVaultTimeoutActions$(userId?: string): Observable; + abstract availableVaultTimeoutActions$(userId?: UserId): Observable; /** * Evaluates the user's available vault timeout actions and returns a boolean representing @@ -55,5 +54,5 @@ export abstract class VaultTimeoutSettingsService { * @param userId The user id to check. If not provided, the current user is used * @returns boolean true if biometric lock is set */ - abstract isBiometricLockSet(userId?: string): Promise; + abstract isBiometricLockSet(userId?: UserId): Promise; } diff --git a/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.spec.ts b/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.spec.ts index 3c391344f04..3fa71598e65 100644 --- a/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.spec.ts +++ b/libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.spec.ts @@ -78,7 +78,8 @@ describe("VaultTimeoutSettingsService", () => { vaultTimeoutSettingsService = createVaultTimeoutSettingsService(defaultVaultTimeout); - biometricStateService.biometricUnlockEnabled$ = of(false); + pinStateService.pinSet$.mockReturnValue(of(false)); + biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false)); }); afterEach(() => { @@ -86,72 +87,121 @@ describe("VaultTimeoutSettingsService", () => { }); describe("availableVaultTimeoutActions$", () => { - it("always returns LogOut", async () => { - const result = await firstValueFrom( - vaultTimeoutSettingsService.availableVaultTimeoutActions$(), - ); + describe("when no userId provided (active user)", () => { + it("always returns LogOut", async () => { + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(), + ); - expect(result).toContain(VaultTimeoutAction.LogOut); + expect(result).toContain(VaultTimeoutAction.LogOut); + }); + + it("contains Lock when the user has a master password", async () => { + userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: true })); + + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(), + ); + + expect(userDecryptionOptionsService.hasMasterPasswordById$).toHaveBeenCalledWith( + mockUserId, + ); + expect(result).toContain(VaultTimeoutAction.Lock); + }); + + it("contains Lock when the user has either a persistent or ephemeral PIN configured", async () => { + pinStateService.pinSet$.mockReturnValue(of(true)); + + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(), + ); + + expect(result).toContain(VaultTimeoutAction.Lock); + }); + + it("contains Lock when the user has biometrics configured", async () => { + biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(true)); + biometricStateService.getBiometricUnlockEnabled.mockResolvedValue(true); + + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(), + ); + + expect(result).toContain(VaultTimeoutAction.Lock); + }); + + it("not contains Lock when the user does not have a master password, PIN, or biometrics", async () => { + userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false })); + pinStateService.pinSet$.mockReturnValue(of(false)); + biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false)); + + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(), + ); + + expect(result).not.toContain(VaultTimeoutAction.Lock); + }); + + it("should throw error when activeAccount$ is null", async () => { + accountService.activeAccountSubject.next(null); + + const result$ = vaultTimeoutSettingsService.availableVaultTimeoutActions$(); + + await expect(firstValueFrom(result$)).rejects.toThrow("Null or undefined account"); + }); }); - it("contains Lock when the user has a master password", async () => { - userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: true })); + describe("with explicit userId parameter", () => { + it("should return Lock and LogOut when provided user has master password", async () => { + userDecryptionOptionsService.hasMasterPasswordById$.mockReturnValue(of(true)); - const result = await firstValueFrom( - vaultTimeoutSettingsService.availableVaultTimeoutActions$(), - ); + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(mockUserId), + ); - expect(result).toContain(VaultTimeoutAction.Lock); - }); + expect(userDecryptionOptionsService.hasMasterPasswordById$).toHaveBeenCalledWith( + mockUserId, + ); + expect(result).toContain(VaultTimeoutAction.Lock); + expect(result).toContain(VaultTimeoutAction.LogOut); + }); - it("contains Lock when the user has either a persistent or ephemeral PIN configured", async () => { - pinStateService.isPinSet.mockResolvedValue(true); + it("should return Lock and LogOut when provided user has PIN configured", async () => { + pinStateService.pinSet$.mockReturnValue(of(true)); - const result = await firstValueFrom( - vaultTimeoutSettingsService.availableVaultTimeoutActions$(), - ); + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(mockUserId), + ); - expect(result).toContain(VaultTimeoutAction.Lock); - }); + expect(pinStateService.pinSet$).toHaveBeenCalledWith(mockUserId); + expect(result).toContain(VaultTimeoutAction.Lock); + expect(result).toContain(VaultTimeoutAction.LogOut); + }); - it("contains Lock when the user has biometrics configured", async () => { - biometricStateService.biometricUnlockEnabled$ = of(true); - biometricStateService.getBiometricUnlockEnabled.mockResolvedValue(true); + it("should return Lock and LogOut when provided user has biometrics configured", async () => { + biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(true)); - const result = await firstValueFrom( - vaultTimeoutSettingsService.availableVaultTimeoutActions$(), - ); + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(mockUserId), + ); - expect(result).toContain(VaultTimeoutAction.Lock); - }); + expect(biometricStateService.biometricUnlockEnabled$).toHaveBeenCalledWith(mockUserId); + expect(result).toContain(VaultTimeoutAction.Lock); + expect(result).toContain(VaultTimeoutAction.LogOut); + }); - it("not contains Lock when the user does not have a master password, PIN, or biometrics", async () => { - userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false })); - pinStateService.isPinSet.mockResolvedValue(false); - biometricStateService.biometricUnlockEnabled$ = of(false); + it("should not return Lock when provided user has no unlock methods", async () => { + userDecryptionOptionsService.hasMasterPasswordById$.mockReturnValue(of(false)); + pinStateService.pinSet$.mockReturnValue(of(false)); + biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false)); - const result = await firstValueFrom( - vaultTimeoutSettingsService.availableVaultTimeoutActions$(), - ); + const result = await firstValueFrom( + vaultTimeoutSettingsService.availableVaultTimeoutActions$(mockUserId), + ); - expect(result).not.toContain(VaultTimeoutAction.Lock); - }); - - it("should return only LogOut when userId is not provided and there is no active account", async () => { - // Set up accountService to return null for activeAccount - accountService.activeAccount$ = of(null); - pinStateService.isPinSet.mockResolvedValue(false); - biometricStateService.biometricUnlockEnabled$ = of(false); - - // Call availableVaultTimeoutActions$ which internally calls userHasMasterPassword without a userId - const result = await firstValueFrom( - vaultTimeoutSettingsService.availableVaultTimeoutActions$(), - ); - - // Since there's no active account, userHasMasterPassword returns false, - // meaning no master password is available, so Lock should not be available - expect(result).toEqual([VaultTimeoutAction.LogOut]); - expect(result).not.toContain(VaultTimeoutAction.Lock); + expect(result).not.toContain(VaultTimeoutAction.Lock); + expect(result).toContain(VaultTimeoutAction.LogOut); + }); }); }); @@ -237,8 +287,8 @@ describe("VaultTimeoutSettingsService", () => { `( "returns $expected when policy is $policy, has PIN unlock method: $hasPinUnlock or Biometric unlock method: $hasBiometricUnlock, and user preference is $userPreference", async ({ hasPinUnlock, hasBiometricUnlock, policy, userPreference, expected }) => { - biometricStateService.getBiometricUnlockEnabled.mockResolvedValue(hasBiometricUnlock); - pinStateService.isPinSet.mockResolvedValue(hasPinUnlock); + biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(hasBiometricUnlock)); + pinStateService.pinSet$.mockReturnValue(of(hasPinUnlock)); userDecryptionOptionsSubject.next( new UserDecryptionOptions({ hasMasterPassword: false }), 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 57e484fd767..5384d6860b7 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 @@ -3,16 +3,15 @@ import { catchError, combineLatest, - defer, distinctUntilChanged, EMPTY, firstValueFrom, from, map, + of, Observable, shareReplay, switchMap, - tap, concatMap, } from "rxjs"; @@ -28,6 +27,7 @@ import { PolicyType } from "../../../admin-console/enums"; import { getFirstPolicy } from "../../../admin-console/services/policy/default-policy.service"; import { AccountService } from "../../../auth/abstractions/account.service"; import { TokenService } from "../../../auth/abstractions/token.service"; +import { getUserId } from "../../../auth/services/account.service"; import { LogService } from "../../../platform/abstractions/log.service"; import { StateProvider } from "../../../platform/state"; import { UserId } from "../../../types/guid"; @@ -101,8 +101,29 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA await this.keyService.refreshAdditionalKeys(userId); } - availableVaultTimeoutActions$(userId?: string): Observable { - return defer(() => this.getAvailableVaultTimeoutActions(userId)); + availableVaultTimeoutActions$(userId?: UserId): Observable { + const userId$ = + userId != null + ? of(userId) + : // TODO remove with https://bitwarden.atlassian.net/browse/PM-10647 + getUserId(this.accountService.activeAccount$); + + return userId$.pipe( + switchMap((userId) => + combineLatest([ + this.userDecryptionOptionsService.hasMasterPasswordById$(userId), + this.biometricStateService.biometricUnlockEnabled$(userId), + this.pinStateService.pinSet$(userId), + ]), + ), + map(([haveMasterPassword, biometricUnlockEnabled, isPinSet]) => { + const canLock = haveMasterPassword || biometricUnlockEnabled || isPinSet; + if (canLock) { + return [VaultTimeoutAction.LogOut, VaultTimeoutAction.Lock]; + } + return [VaultTimeoutAction.LogOut]; + }), + ); } async canLock(userId: UserId): Promise { @@ -112,12 +133,8 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA return availableVaultTimeoutActions?.includes(VaultTimeoutAction.Lock) || false; } - async isBiometricLockSet(userId?: string): Promise { - const biometricUnlockPromise = - userId == null - ? firstValueFrom(this.biometricStateService.biometricUnlockEnabled$) - : this.biometricStateService.getBiometricUnlockEnabled(userId as UserId); - return await biometricUnlockPromise; + async isBiometricLockSet(userId?: UserId): Promise { + return await firstValueFrom(this.biometricStateService.biometricUnlockEnabled$(userId)); } private async setVaultTimeout(userId: UserId, timeout: VaultTimeout): Promise { @@ -262,45 +279,45 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA return combineLatest([ this.stateProvider.getUserState$(VAULT_TIMEOUT_ACTION, userId), this.getMaxSessionTimeoutPolicyDataByUserId$(userId), + this.availableVaultTimeoutActions$(userId), ]).pipe( - switchMap(([currentVaultTimeoutAction, maxSessionTimeoutPolicyData]) => { - return from( - this.determineVaultTimeoutAction( - userId, + concatMap( + async ([ + currentVaultTimeoutAction, + maxSessionTimeoutPolicyData, + availableVaultTimeoutActions, + ]) => { + const vaultTimeoutAction = this.determineVaultTimeoutAction( + availableVaultTimeoutActions, currentVaultTimeoutAction, maxSessionTimeoutPolicyData, - ), - ).pipe( - tap((vaultTimeoutAction: VaultTimeoutAction) => { - // As a side effect, set the new value determined by determineVaultTimeout into state if it's different from the current - // We want to avoid having a null timeout action always so we set it to the default if it is null - // and if the user becomes subject to a policy that requires a specific action, we set it to that - if (vaultTimeoutAction !== currentVaultTimeoutAction) { - return this.stateProvider.setUserState( - VAULT_TIMEOUT_ACTION, - vaultTimeoutAction, - userId, - ); - } - }), - catchError((error: unknown) => { - // Protect outer observable from canceling on error by catching and returning EMPTY - this.logService.error(`Error getting vault timeout: ${error}`); - return EMPTY; - }), - ); + ); + + // As a side effect, set the new value determined by determineVaultTimeout into state if it's different from the current + // We want to avoid having a null timeout action always so we set it to the default if it is null + // and if the user becomes subject to a policy that requires a specific action, we set it to that + if (vaultTimeoutAction !== currentVaultTimeoutAction) { + await this.stateProvider.setUserState(VAULT_TIMEOUT_ACTION, vaultTimeoutAction, userId); + } + + return vaultTimeoutAction; + }, + ), + catchError((error: unknown) => { + // Protect outer observable from canceling on error by catching and returning EMPTY + this.logService.error(`Error getting vault timeout: ${error}`); + return EMPTY; }), distinctUntilChanged(), // Avoid having the set side effect trigger a new emission of the same action shareReplay({ refCount: true, bufferSize: 1 }), ); } - private async determineVaultTimeoutAction( - userId: string, + private determineVaultTimeoutAction( + availableVaultTimeoutActions: VaultTimeoutAction[], currentVaultTimeoutAction: VaultTimeoutAction | null, maxSessionTimeoutPolicyData: MaximumSessionTimeoutPolicyData | null, - ): Promise { - const availableVaultTimeoutActions = await this.getAvailableVaultTimeoutActions(userId); + ): VaultTimeoutAction { if (availableVaultTimeoutActions.length === 1) { return availableVaultTimeoutActions[0]; } @@ -339,38 +356,4 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA map((policy) => (policy?.data ?? null) as MaximumSessionTimeoutPolicyData | null), ); } - - private async getAvailableVaultTimeoutActions(userId?: string): Promise { - userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id; - - const availableActions = [VaultTimeoutAction.LogOut]; - - const canLock = - (await this.userHasMasterPassword(userId)) || - (await this.pinStateService.isPinSet(userId as UserId)) || - (await this.isBiometricLockSet(userId)); - - if (canLock) { - availableActions.push(VaultTimeoutAction.Lock); - } - - return availableActions; - } - - private async userHasMasterPassword(userId: string): Promise { - let resolvedUserId: UserId; - if (userId) { - resolvedUserId = userId as UserId; - } else { - const activeAccount = await firstValueFrom(this.accountService.activeAccount$); - if (!activeAccount) { - return false; // No account, can't have master password - } - resolvedUserId = activeAccount.id; - } - - return await firstValueFrom( - this.userDecryptionOptionsService.hasMasterPasswordById$(resolvedUserId), - ); - } } diff --git a/libs/key-management/src/biometrics/biometric-state.service.spec.ts b/libs/key-management/src/biometrics/biometric-state.service.spec.ts index 32043514ff7..2f1f189a897 100644 --- a/libs/key-management/src/biometrics/biometric-state.service.spec.ts +++ b/libs/key-management/src/biometrics/biometric-state.service.spec.ts @@ -179,18 +179,36 @@ describe("BiometricStateService", () => { }); describe("biometricUnlockEnabled$", () => { - it("emits when biometricUnlockEnabled state is updated", async () => { - const state = stateProvider.activeUser.getFake(BIOMETRIC_UNLOCK_ENABLED); - state.nextState(true); + describe("no user id provided, active user", () => { + it("emits when biometricUnlockEnabled state is updated", async () => { + const state = stateProvider.activeUser.getFake(BIOMETRIC_UNLOCK_ENABLED); + state.nextState(true); - expect(await firstValueFrom(sut.biometricUnlockEnabled$)).toBe(true); + expect(await firstValueFrom(sut.biometricUnlockEnabled$())).toBe(true); + }); + + it("emits false when biometricUnlockEnabled state is undefined", async () => { + const state = stateProvider.activeUser.getFake(BIOMETRIC_UNLOCK_ENABLED); + state.nextState(undefined as unknown as boolean); + + expect(await firstValueFrom(sut.biometricUnlockEnabled$())).toBe(false); + }); }); - it("emits false when biometricUnlockEnabled state is undefined", async () => { - const state = stateProvider.activeUser.getFake(BIOMETRIC_UNLOCK_ENABLED); - state.nextState(undefined as unknown as boolean); + describe("user id provided", () => { + it("returns biometricUnlockEnabled state for the given user", async () => { + stateProvider.singleUser.getFake(userId, BIOMETRIC_UNLOCK_ENABLED).nextState(true); - expect(await firstValueFrom(sut.biometricUnlockEnabled$)).toBe(false); + expect(await firstValueFrom(sut.biometricUnlockEnabled$(userId))).toBe(true); + }); + + it("returns false when the state is not set", async () => { + stateProvider.singleUser + .getFake(userId, BIOMETRIC_UNLOCK_ENABLED) + .nextState(undefined as unknown as boolean); + + expect(await firstValueFrom(sut.biometricUnlockEnabled$(userId))).toBe(false); + }); }); }); @@ -198,7 +216,7 @@ describe("BiometricStateService", () => { it("updates biometricUnlockEnabled$", async () => { await sut.setBiometricUnlockEnabled(true); - expect(await firstValueFrom(sut.biometricUnlockEnabled$)).toBe(true); + expect(await firstValueFrom(sut.biometricUnlockEnabled$())).toBe(true); }); it("updates state", async () => { @@ -210,22 +228,6 @@ describe("BiometricStateService", () => { }); }); - describe("getBiometricUnlockEnabled", () => { - it("returns biometricUnlockEnabled state for the given user", async () => { - stateProvider.singleUser.getFake(userId, BIOMETRIC_UNLOCK_ENABLED).nextState(true); - - expect(await sut.getBiometricUnlockEnabled(userId)).toBe(true); - }); - - it("returns false when the state is not set", async () => { - stateProvider.singleUser - .getFake(userId, BIOMETRIC_UNLOCK_ENABLED) - .nextState(undefined as unknown as boolean); - - expect(await sut.getBiometricUnlockEnabled(userId)).toBe(false); - }); - }); - describe("setFingerprintValidated", () => { it("updates fingerprintValidated$", async () => { await sut.setFingerprintValidated(true); diff --git a/libs/key-management/src/biometrics/biometric-state.service.ts b/libs/key-management/src/biometrics/biometric-state.service.ts index 1488f12b50b..ca1cbcfa871 100644 --- a/libs/key-management/src/biometrics/biometric-state.service.ts +++ b/libs/key-management/src/biometrics/biometric-state.service.ts @@ -18,9 +18,11 @@ import { export abstract class BiometricStateService { /** - * `true` if the currently active user has elected to store a biometric key to unlock their vault. + * Returns whether biometric unlock is enabled for a user. + * @param userId The user id to check. If not provided, returns the state for the currently active user. + * @returns An observable that emits `true` if the user has elected to store a biometric key to unlock their vault. */ - abstract biometricUnlockEnabled$: Observable; // used to be biometricUnlock + abstract biometricUnlockEnabled$(userId?: UserId): Observable; /** * If the user has elected to require a password on first unlock of an application instance, this key will store the * encrypted client key half used to unlock the vault. @@ -53,6 +55,7 @@ export abstract class BiometricStateService { /** * Gets the biometric unlock enabled state for the given user. + * @deprecated Use {@link biometricUnlockEnabled$} instead * @param userId user Id to check */ abstract getBiometricUnlockEnabled(userId: UserId): Promise; @@ -103,7 +106,6 @@ export class DefaultBiometricStateService implements BiometricStateService { private promptAutomaticallyState: ActiveUserState; private fingerprintValidatedState: GlobalState; private lastProcessReloadState: GlobalState; - biometricUnlockEnabled$: Observable; encryptedClientKeyHalf$: Observable; promptCancelled$: Observable; promptAutomatically$: Observable; @@ -112,7 +114,6 @@ export class DefaultBiometricStateService implements BiometricStateService { constructor(private stateProvider: StateProvider) { this.biometricUnlockEnabledState = this.stateProvider.getActive(BIOMETRIC_UNLOCK_ENABLED); - this.biometricUnlockEnabled$ = this.biometricUnlockEnabledState.state$.pipe(map(Boolean)); this.encryptedClientKeyHalfState = this.stateProvider.getActive(ENCRYPTED_CLIENT_KEY_HALF); this.encryptedClientKeyHalf$ = this.encryptedClientKeyHalfState.state$.pipe( @@ -142,6 +143,15 @@ export class DefaultBiometricStateService implements BiometricStateService { await this.biometricUnlockEnabledState.update(() => enabled); } + biometricUnlockEnabled$(userId?: UserId): Observable { + if (userId != null) { + return this.stateProvider.getUser(userId, BIOMETRIC_UNLOCK_ENABLED).state$.pipe(map(Boolean)); + } + // Backwards compatibility for active user state + // TODO remove with https://bitwarden.atlassian.net/browse/PM-12043 + return this.biometricUnlockEnabledState.state$.pipe(map(Boolean)); + } + async getBiometricUnlockEnabled(userId: UserId): Promise { return await firstValueFrom( this.stateProvider.getUser(userId, BIOMETRIC_UNLOCK_ENABLED).state$.pipe(map(Boolean)),