From fad4e721ceaa50bebcc73a080ea6f6e661fba22c Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Sat, 11 Jul 2026 04:40:14 +0900 Subject: [PATCH] fix(browser): Fix biometrics setup and connection issues (#21782) --- .github/whitelist-capital-letters.txt | 1 - apps/browser/src/_locales/en/messages.json | 3 - .../account-security.component.spec.ts | 165 ++---------------- .../settings/account-security.component.ts | 163 +---------------- .../await-desktop-dialog.component.html | 11 -- .../await-desktop-dialog.component.ts | 23 --- .../browser/src/background/main.background.ts | 4 - .../native-messaging.background.spec.ts | 113 +++++++++++- .../background/nativeMessaging.background.ts | 55 ++++-- ...kground-browser-biometrics.service.spec.ts | 60 ------- .../background-browser-biometrics.service.ts | 66 ++----- apps/browser/src/models/biometricErrors.ts | 54 ------ 12 files changed, 177 insertions(+), 541 deletions(-) delete mode 100644 apps/browser/src/auth/popup/settings/await-desktop-dialog.component.html delete mode 100644 apps/browser/src/auth/popup/settings/await-desktop-dialog.component.ts delete mode 100644 apps/browser/src/models/biometricErrors.ts diff --git a/.github/whitelist-capital-letters.txt b/.github/whitelist-capital-letters.txt index 282fe192267..5e4cd7c4c33 100644 --- a/.github/whitelist-capital-letters.txt +++ b/.github/whitelist-capital-letters.txt @@ -19,7 +19,6 @@ ./apps/cli/stores/chocolatey/tools/VERIFICATION.txt ./apps/browser/store/windows/AppxManifest.xml ./apps/browser/src/background/nativeMessaging.background.ts -./apps/browser/src/models/biometricErrors.ts ./apps/browser/src/browser/safariApp.ts ./apps/browser/src/safari/desktop/ViewController.swift ./apps/browser/src/safari/desktop/Assets.xcassets/AppIcon.appiconset/Contents.json diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index 4a79df6caf2..67286d3c58e 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -6328,9 +6328,6 @@ "unlockPinSet": { "message": "Unlock PIN set" }, - "unlockWithBiometricSet": { - "message": "Unlock with biometrics set" - }, "authenticating": { "message": "Authenticating" }, diff --git a/apps/browser/src/auth/popup/settings/account-security.component.spec.ts b/apps/browser/src/auth/popup/settings/account-security.component.spec.ts index a219852f558..2cb82a3178a 100644 --- a/apps/browser/src/auth/popup/settings/account-security.component.spec.ts +++ b/apps/browser/src/auth/popup/settings/account-security.component.spec.ts @@ -1,5 +1,5 @@ import { ChangeDetectionStrategy, Component, input } from "@angular/core"; -import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testing"; +import { ComponentFixture, TestBed } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; import { ActivatedRoute } from "@angular/router"; import { mock } from "jest-mock-extended"; @@ -35,12 +35,7 @@ import { UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { DialogService, ToastService } from "@bitwarden/components"; import { newGuid } from "@bitwarden/guid"; -import { - BiometricStateService, - BiometricsService, - BiometricsStatus, - KeyService, -} from "@bitwarden/key-management"; +import { BiometricStateService, BiometricsService, KeyService } from "@bitwarden/key-management"; import { SessionTimeoutSettingsComponent } from "@bitwarden/key-management-ui"; import { BrowserApi } from "../../../platform/browser/browser-api"; @@ -372,12 +367,6 @@ describe("AccountSecurityComponent", () => { }); describe("updating to true", () => { - let trySetupBiometricsSpy: jest.SpyInstance; - - beforeEach(() => { - trySetupBiometricsSpy = jest.spyOn(component, "trySetupBiometrics"); - }); - it("displays permission error dialog when nativeMessaging permission is not granted", async () => { browserApiSpy.mockResolvedValue(false); @@ -392,7 +381,7 @@ describe("AccountSecurityComponent", () => { type: "danger", }); expect(component.form.controls.biometric.value).toBe(false); - expect(trySetupBiometricsSpy).not.toHaveBeenCalled(); + expect(biometricsService.unlockWithBiometricsForUser).not.toHaveBeenCalled(); }); it("displays a specific sidebar dialog when nativeMessaging permissions throws an error on firefox + sidebar", async () => { @@ -411,7 +400,7 @@ describe("AccountSecurityComponent", () => { type: "info", }); expect(component.form.controls.biometric.value).toBe(false); - expect(trySetupBiometricsSpy).not.toHaveBeenCalled(); + expect(biometricsService.unlockWithBiometricsForUser).not.toHaveBeenCalled(); }); test.each([ @@ -436,40 +425,24 @@ describe("AccountSecurityComponent", () => { type: "danger", }); expect(component.form.controls.biometric.value).toBe(false); - expect(trySetupBiometricsSpy).not.toHaveBeenCalled(); + expect(biometricsService.unlockWithBiometricsForUser).not.toHaveBeenCalled(); }, ); - it("refreshes additional keys and attempts to setup biometrics when enabled with nativeMessaging permission", async () => { - const setupBiometricsResult = true; - trySetupBiometricsSpy.mockResolvedValue(setupBiometricsResult); + it("refreshes additional keys and enables biometrics when unlock and key validation succeed", async () => { + const userKey = Symbol("userKey"); + biometricsService.unlockWithBiometricsForUser.mockResolvedValue(userKey as any); + keyService.validateUserKey.mockResolvedValue(true); await component.ngOnInit(); await component.updateBiometric(true); expect(keyService.refreshAdditionalKeys).toHaveBeenCalledWith(mockUserId); expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenCalledWith( - setupBiometricsResult, + true, mockUserId, ); - expect(component.form.controls.biometric.value).toBe(setupBiometricsResult); - }); - - it("handles failed biometrics setup", async () => { - const setupBiometricsResult = false; - trySetupBiometricsSpy.mockResolvedValue(setupBiometricsResult); - - await component.ngOnInit(); - await component.updateBiometric(true); - - expect(biometricStateService.setBiometricUnlockEnabled).toHaveBeenCalledWith( - setupBiometricsResult, - mockUserId, - ); - expect(biometricStateService.setFingerprintValidated).toHaveBeenCalledWith( - setupBiometricsResult, - ); - expect(component.form.controls.biometric.value).toBe(setupBiometricsResult); + expect(component.form.controls.biometric.value).toBe(true); }); it("handles error during biometrics setup", async () => { @@ -481,7 +454,7 @@ describe("AccountSecurityComponent", () => { expect(validationService.showError).toHaveBeenCalledWith(new Error("UserId is required")); expect(component.form.controls.biometric.value).toBe(false); - expect(trySetupBiometricsSpy).not.toHaveBeenCalled(); + expect(biometricsService.unlockWithBiometricsForUser).not.toHaveBeenCalled(); }); }); }); @@ -509,118 +482,4 @@ describe("AccountSecurityComponent", () => { ).toHaveBeenCalledWith(false, mockUserId); }); }); - - describe("biometrics polling timer", () => { - let browserApiSpy: jest.SpyInstance; - - beforeEach(() => { - browserApiSpy = jest.spyOn(BrowserApi, "permissionsGranted"); - }); - - afterEach(() => { - component.ngOnDestroy(); - }); - - it("disables biometric control when canEnableBiometricUnlock is false", fakeAsync(async () => { - biometricsService.canEnableBiometricUnlock.mockResolvedValue(false); - - await component.ngOnInit(); - tick(); - - expect(component.form.controls.biometric.disabled).toBe(true); - })); - - it("enables biometric control when canEnableBiometricUnlock is true", fakeAsync(async () => { - biometricsService.canEnableBiometricUnlock.mockResolvedValue(true); - - await component.ngOnInit(); - tick(); - - expect(component.form.controls.biometric.disabled).toBe(false); - })); - - it("skips status check when nativeMessaging permission is not granted and not Safari", fakeAsync(async () => { - biometricsService.canEnableBiometricUnlock.mockResolvedValue(true); - browserApiSpy.mockResolvedValue(false); - platformUtilsService.isSafari.mockReturnValue(false); - - await component.ngOnInit(); - tick(); - - expect(biometricsService.getBiometricsStatusForUser).not.toHaveBeenCalled(); - expect(component.biometricUnavailabilityReason).toBeUndefined(); - })); - - it("checks biometrics status when nativeMessaging permission is granted", fakeAsync(async () => { - biometricsService.canEnableBiometricUnlock.mockResolvedValue(true); - browserApiSpy.mockResolvedValue(true); - platformUtilsService.isSafari.mockReturnValue(false); - biometricsService.getBiometricsStatusForUser.mockResolvedValue( - BiometricsStatus.DesktopDisconnected, - ); - - await component.ngOnInit(); - tick(); - - expect(biometricsService.getBiometricsStatusForUser).toHaveBeenCalledWith(mockUserId); - })); - - it("should check status on Safari", fakeAsync(async () => { - biometricsService.canEnableBiometricUnlock.mockResolvedValue(true); - browserApiSpy.mockResolvedValue(false); - platformUtilsService.isSafari.mockReturnValue(true); - biometricsService.getBiometricsStatusForUser.mockResolvedValue( - BiometricsStatus.DesktopDisconnected, - ); - - await component.ngOnInit(); - tick(); - - expect(biometricsService.getBiometricsStatusForUser).toHaveBeenCalledWith(mockUserId); - })); - - test.each([ - [ - BiometricsStatus.DesktopDisconnected, - "biometricsStatusHelptextDesktopDisconnected-used-i18n", - ], - [ - BiometricsStatus.NotEnabledInConnectedDesktopApp, - "biometricsStatusHelptextNotEnabledInDesktop-used-i18n", - ], - [ - BiometricsStatus.HardwareUnavailable, - "biometricsStatusHelptextHardwareUnavailable-used-i18n", - ], - ])( - "sets expected unavailability reason for %s status when biometric not available", - fakeAsync(async (biometricStatus: BiometricsStatus, expected: string) => { - biometricsService.canEnableBiometricUnlock.mockResolvedValue(false); - browserApiSpy.mockResolvedValue(true); - platformUtilsService.isSafari.mockReturnValue(false); - biometricsService.getBiometricsStatusForUser.mockResolvedValue(biometricStatus); - - await component.ngOnInit(); - tick(); - - expect(component.biometricUnavailabilityReason).toBe(expected); - }), - ); - - it("should not set unavailability reason for error statuses when biometric is available", fakeAsync(async () => { - biometricsService.canEnableBiometricUnlock.mockResolvedValue(true); - browserApiSpy.mockResolvedValue(true); - platformUtilsService.isSafari.mockReturnValue(false); - biometricsService.getBiometricsStatusForUser.mockResolvedValue( - BiometricsStatus.DesktopDisconnected, - ); - - await component.ngOnInit(); - tick(); - - // Status is DesktopDisconnected but biometric IS available, so don't show error - expect(component.biometricUnavailabilityReason).toBe(""); - component.ngOnDestroy(); - })); - }); }); 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 bc27719c8bb..1a8b00b1827 100644 --- a/apps/browser/src/auth/popup/settings/account-security.component.ts +++ b/apps/browser/src/auth/popup/settings/account-security.component.ts @@ -15,7 +15,6 @@ import { Subject, switchMap, takeUntil, - timer, } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; @@ -41,7 +40,6 @@ import { MessagingService } from "@bitwarden/common/platform/abstractions/messag import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; import { - DialogRef, CardComponent, CheckboxModule, DialogService, @@ -59,15 +57,9 @@ import { CalloutModule, SpinnerComponent, } from "@bitwarden/components"; -import { - KeyService, - BiometricsService, - BiometricStateService, - BiometricsStatus, -} from "@bitwarden/key-management"; +import { KeyService, BiometricsService, BiometricStateService } from "@bitwarden/key-management"; import { SessionTimeoutSettingsComponent } from "@bitwarden/key-management-ui"; -import { BiometricErrors, BiometricErrorTypes } from "../../../models/biometricErrors"; import { BrowserApi } from "../../../platform/browser/browser-api"; import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils"; import { PopOutComponent } from "../../../platform/popup/components/pop-out.component"; @@ -76,8 +68,6 @@ import { PopupPageComponent } from "../../../platform/popup/layout/popup-page.co import { SetPinComponent } from "../components/set-pin.component"; import { AuthExtensionRoute } from "../constants/auth-extension-route.constant"; -import { AwaitDesktopDialogComponent } from "./await-desktop-dialog.component"; - // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ @@ -144,7 +134,6 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { protected refreshTimeoutSettings$ = new BehaviorSubject(undefined); private destroy$ = new Subject(); - private readonly BIOMETRICS_POLLING_INTERVAL = 2000; constructor( private accountService: AccountService, @@ -221,52 +210,6 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { this.form.patchValue(initialValues, { emitEvent: false }); this.loading.set(false); - timer(0, this.BIOMETRICS_POLLING_INTERVAL) - .pipe( - switchMap(async () => { - const biometricSettingAvailable = await this.biometricsService.canEnableBiometricUnlock(); - if (!biometricSettingAvailable) { - this.form.controls.biometric.disable({ emitEvent: false }); - } else { - this.form.controls.biometric.enable({ emitEvent: false }); - } - - // Biometrics status shouldn't be checked if permissions are needed. - const needsPermissionPrompt = - !(await BrowserApi.permissionsGranted(["nativeMessaging"])) && - !this.platformUtilsService.isSafari(); - if (needsPermissionPrompt) { - return; - } - - const status = await this.biometricsService.getBiometricsStatusForUser(activeAccount.id); - if (status === BiometricsStatus.DesktopDisconnected && !biometricSettingAvailable) { - this.biometricUnavailabilityReason = this.i18nService.t( - "biometricsStatusHelptextDesktopDisconnected", - ); - } else if ( - status === BiometricsStatus.NotEnabledInConnectedDesktopApp && - !biometricSettingAvailable - ) { - this.biometricUnavailabilityReason = this.i18nService.t( - "biometricsStatusHelptextNotEnabledInDesktop", - activeAccount.email, - ); - } else if ( - status === BiometricsStatus.HardwareUnavailable && - !biometricSettingAvailable - ) { - this.biometricUnavailabilityReason = this.i18nService.t( - "biometricsStatusHelptextHardwareUnavailable", - ); - } else { - this.biometricUnavailabilityReason = ""; - } - }), - takeUntil(this.destroy$), - ) - .subscribe(); - this.showChangeMasterPass = await this.userVerificationService.hasMasterPassword(); this.form.controls.pin.valueChanges @@ -426,19 +369,8 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { try { await this.keyService.refreshAdditionalKeys(userId); - - const successful = await this.trySetupBiometrics(); - this.form.controls.biometric.setValue(successful); - await this.biometricStateService.setBiometricUnlockEnabled(successful, userId); - if (!successful) { - await this.biometricStateService.setFingerprintValidated(false); - return; - } - this.toastService.showToast({ - variant: "success", - title: null, - message: this.i18nService.t("unlockWithBiometricSet"), - }); + this.form.controls.biometric.setValue(true, { emitEvent: false }); + await this.biometricStateService.setBiometricUnlockEnabled(true, userId); } catch (error) { this.form.controls.biometric.setValue(false); this.validationService.showError(error); @@ -449,95 +381,6 @@ export class AccountSecurityComponent implements OnInit, OnDestroy { } } - async trySetupBiometrics(): Promise { - let awaitDesktopDialogRef: DialogRef | undefined; - let biometricsResponseReceived = false; - let setupResult = false; - - const waitForUserDialogPromise = async () => { - // only show waiting dialog if we have waited for 500 msec to prevent double dialog - // the os will respond instantly if the dialog shows successfully, and the desktop app will respond instantly if something is wrong - await new Promise((resolve) => setTimeout(resolve, 500)); - if (biometricsResponseReceived) { - return; - } - - awaitDesktopDialogRef = AwaitDesktopDialogComponent.open(this.dialogService); - await firstValueFrom(awaitDesktopDialogRef.closed); - if (!biometricsResponseReceived) { - setupResult = false; - } - return; - }; - - const biometricsPromise = async () => { - try { - const userId = await firstValueFrom( - this.accountService.activeAccount$.pipe(map((a) => a.id)), - ); - let result = false; - try { - const userKey = await this.biometricsService.unlockWithBiometricsForUser(userId); - result = await this.keyService.validateUserKey(userKey, userId); - // FIXME: Remove when updating file. Eslint update - // eslint-disable-next-line @typescript-eslint/no-unused-vars - } catch (e) { - result = false; - } - - // prevent duplicate dialog - biometricsResponseReceived = true; - if (awaitDesktopDialogRef) { - await awaitDesktopDialogRef.close(result); - } - - if (!result) { - this.platformUtilsService.showToast( - "error", - this.i18nService.t("errorEnableBiometricTitle"), - this.i18nService.t("errorEnableBiometricDesc"), - ); - setupResult = false; - return; - } - setupResult = true; - } catch (e) { - // prevent duplicate dialog - biometricsResponseReceived = true; - if (awaitDesktopDialogRef) { - await awaitDesktopDialogRef.close(true); - } - - if (e.message == "canceled") { - setupResult = false; - return; - } - - const error = BiometricErrors[e.message as BiometricErrorTypes]; - const shouldRetry = await this.dialogService.openSimpleDialog({ - title: { key: error.title }, - content: { key: error.description }, - acceptButtonText: { key: "retry" }, - cancelButtonText: null, - type: "danger", - }); - if (shouldRetry) { - setupResult = await this.trySetupBiometrics(); - } else { - setupResult = false; - return; - } - } finally { - if (awaitDesktopDialogRef) { - await awaitDesktopDialogRef.close(true); - } - } - }; - - await Promise.all([waitForUserDialogPromise(), biometricsPromise()]); - return setupResult; - } - async updateAllowSharingUnlockStateWithDesktop(enabled: boolean) { const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); await this.sharedUnlockSettingsService.setAllowSharingUnlockStateWithDesktop(enabled, userId); diff --git a/apps/browser/src/auth/popup/settings/await-desktop-dialog.component.html b/apps/browser/src/auth/popup/settings/await-desktop-dialog.component.html deleted file mode 100644 index 6e55188833b..00000000000 --- a/apps/browser/src/auth/popup/settings/await-desktop-dialog.component.html +++ /dev/null @@ -1,11 +0,0 @@ - - {{ "awaitDesktop" | i18n }} - - {{ "awaitDesktopDesc" | i18n }} - - - - - diff --git a/apps/browser/src/auth/popup/settings/await-desktop-dialog.component.ts b/apps/browser/src/auth/popup/settings/await-desktop-dialog.component.ts deleted file mode 100644 index 12cf669d89b..00000000000 --- a/apps/browser/src/auth/popup/settings/await-desktop-dialog.component.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { Component } from "@angular/core"; - -import { JslibModule } from "@bitwarden/angular/jslib.module"; -import { - ButtonModule, - CenterPositionStrategy, - DialogModule, - DialogService, -} from "@bitwarden/components"; - -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection -@Component({ - templateUrl: "await-desktop-dialog.component.html", - imports: [JslibModule, ButtonModule, DialogModule], -}) -export class AwaitDesktopDialogComponent { - static open(dialogService: DialogService) { - return dialogService.open(AwaitDesktopDialogComponent, { - positionStrategy: new CenterPositionStrategy(), - }); - } -} diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 1e363aa4f8f..0f0326e446a 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1845,8 +1845,6 @@ export default class MainBackground { if (authStatus === AuthenticationStatus.LoggedOut) { const nextUpAccount = await firstValueFrom(this.accountService.nextUpAccount$); await this.switchAccount(nextUpAccount?.id); - } else { - this.biometricsService.startPolling(active.id); } } @@ -1948,14 +1946,12 @@ export default class MainBackground { await switchPromise; if (userId == null) { - this.biometricsService.stopPolling(); await this.refreshMenu(); await this.updateOverlayCiphers(); this.messagingService.send("goHome"); return; } - this.biometricsService.startPolling(userId); nextAccountStatus = await this.authService.getAuthStatus(userId); await this.systemService.clearPendingClipboard(); diff --git a/apps/browser/src/background/native-messaging.background.spec.ts b/apps/browser/src/background/native-messaging.background.spec.ts index 971aa36d010..b600176aa70 100644 --- a/apps/browser/src/background/native-messaging.background.spec.ts +++ b/apps/browser/src/background/native-messaging.background.spec.ts @@ -97,6 +97,8 @@ describe("NativeMessagingBackground", () => { biometricStateService, accountService, ); + // The constructor starts a reconnection loop; stop it so tests can drive connect() explicitly. + sut.stopConnecting(); }); describe("constructor", () => { @@ -108,14 +110,12 @@ describe("NativeMessagingBackground", () => { }); describe("connect", () => { - it("logs warning and returns if native messaging permission is missing", async () => { + it("does not connect if native messaging permission is missing", async () => { (BrowserApi.permissionsGranted as jest.Mock).mockResolvedValue(false); await sut.connect(); - expect(logService.warning).toHaveBeenCalledWith( - "[Native Messaging IPC] Native messaging permission is missing for biometrics", - ); + expect(BrowserApi.connectNative).not.toHaveBeenCalled(); expect(sut.connected).toBe(false); }); @@ -131,6 +131,106 @@ describe("NativeMessagingBackground", () => { }); }); + describe("startConnecting / stopConnecting", () => { + let connectSpy: jest.SpyInstance; + + beforeEach(() => { + jest.useFakeTimers(); + connectSpy = jest.spyOn(sut, "connect").mockResolvedValue(undefined); + }); + + afterEach(() => { + sut.stopConnecting(); + jest.clearAllTimers(); + jest.useRealTimers(); + }); + + it("attempts to connect immediately", () => { + sut.startConnecting(); + + jest.advanceTimersByTime(0); + + expect(connectSpy).toHaveBeenCalledTimes(1); + }); + + it("starts the reconnection loop on construction", () => { + const instance = new NativeMessagingBackground( + keyService, + encryptService, + cryptoFunctionService, + messagingService, + appIdService, + platformUtilsService, + logService, + biometricStateService, + accountService, + ); + const instanceConnectSpy = jest.spyOn(instance, "connect").mockResolvedValue(undefined); + + jest.advanceTimersByTime(0); + + expect(instanceConnectSpy).toHaveBeenCalledTimes(1); + + instance.stopConnecting(); + }); + + it("retries every 10 seconds while not connected", () => { + sut.startConnecting(); + + jest.advanceTimersByTime(0); + jest.advanceTimersByTime(10_000); + jest.advanceTimersByTime(10_000); + + expect(connectSpy).toHaveBeenCalledTimes(3); + }); + + it("does not attempt to connect while already connected", () => { + sut.connected = true; + + sut.startConnecting(); + jest.advanceTimersByTime(20_000); + + expect(connectSpy).not.toHaveBeenCalled(); + }); + + it("does not attempt to connect while a connection is already in progress", () => { + (sut as any).connecting = true; + + sut.startConnecting(); + jest.advanceTimersByTime(20_000); + + expect(connectSpy).not.toHaveBeenCalled(); + }); + + it("is a no-op when the reconnection loop is already running", () => { + sut.startConnecting(); + sut.startConnecting(); + + jest.advanceTimersByTime(0); + + expect(connectSpy).toHaveBeenCalledTimes(1); + }); + + it("swallows connection errors so the loop keeps retrying", () => { + connectSpy.mockRejectedValue(new Error("startDesktop")); + + sut.startConnecting(); + + expect(() => jest.advanceTimersByTime(20_000)).not.toThrow(); + expect(connectSpy).toHaveBeenCalledTimes(3); + }); + + it("stops attempting to connect after stopConnecting", () => { + sut.startConnecting(); + jest.advanceTimersByTime(0); + + sut.stopConnecting(); + jest.advanceTimersByTime(30_000); + + expect(connectSpy).toHaveBeenCalledTimes(1); + }); + }); + describe("port listeners", () => { let connectPromise: Promise; let messageListener: (msg: unknown) => Promise; @@ -350,11 +450,10 @@ describe("NativeMessagingBackground", () => { await expect(connectPromise).rejects.toThrow("desktopIntegrationDisabled"); }); - it("rejects with an empty message when no error is present", async () => { + it("rejects with 'desktopIntegrationDisabled' even when no explicit error is present", async () => { disconnectListener({ error: { message: undefined } }); - const err = await connectPromise.catch((e: Error) => e); - expect(err instanceof Error ? err.message : "").toBe(""); + await expect(connectPromise).rejects.toThrow("desktopIntegrationDisabled"); }); }); }); diff --git a/apps/browser/src/background/nativeMessaging.background.ts b/apps/browser/src/background/nativeMessaging.background.ts index b78f901c23c..4991b682567 100644 --- a/apps/browser/src/background/nativeMessaging.background.ts +++ b/apps/browser/src/background/nativeMessaging.background.ts @@ -1,4 +1,4 @@ -import { firstValueFrom } from "rxjs"; +import { firstValueFrom, Subscription, timer } from "rxjs"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { CryptoFunctionService } from "@bitwarden/common/key-management/crypto/abstractions/crypto-function.service"; @@ -69,11 +69,15 @@ type SecureChannel = { }; export class NativeMessagingBackground { + private readonly CONNECTION_RETRY_INTERVAL = 10_000; + connected = false; private connecting: boolean = false; private port?: browser.runtime.Port | chrome.runtime.Port; private appId?: string; + private connectionRetrySubscription?: Subscription; + private secureChannel?: SecureChannel; private messageId = 0; @@ -97,13 +101,14 @@ export class NativeMessagingBackground { } }); } + + // Always try to keep a connection to the Bitwarden Desktop app alive so that there is no wait + // when biometrics are used. `connect` is a no-op when the native messaging permission is missing. + this.startConnecting(); } async connect() { if (!(await BrowserApi.permissionsGranted(["nativeMessaging"]))) { - this.logService.warning( - "[Native Messaging IPC] Native messaging permission is missing for biometrics", - ); return; } if (this.connected || this.connecting) { @@ -255,12 +260,7 @@ export class NativeMessagingBackground { }); this.port.onDisconnect.addListener((p: any) => { - let error; - if (BrowserApi.isWebExtensionsApi) { - error = p.error.message; - } else { - error = chrome.runtime.lastError?.message; - } + const error = chrome?.runtime?.lastError?.message ?? p.error?.message ?? "unknown"; this.secureChannel = undefined; this.connected = false; @@ -274,6 +274,41 @@ export class NativeMessagingBackground { }); } + /** + * Starts attempting to keep a connection to the Bitwarden Desktop app alive. If a connection is + * not established, it retries every 10 seconds. Calling this while the loop is already running is + * a no-op. + */ + startConnecting() { + if (this.connectionRetrySubscription != null) { + return; + } + + this.connectionRetrySubscription = timer(0, this.CONNECTION_RETRY_INTERVAL).subscribe(() => { + void this.tryConnect(); + }); + } + + private async tryConnect() { + if (this.connected || this.connecting) { + return; + } + + try { + await this.connect(); + } catch { + // The desktop app may not be running yet; the loop will retry on the next interval. + } + } + + /** + * Stops the reconnection loop started by {@link startConnecting}. + */ + stopConnecting() { + this.connectionRetrySubscription?.unsubscribe(); + this.connectionRetrySubscription = undefined; + } + async callCommand(message: Message): Promise { const messageId = this.messageId++; diff --git a/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.spec.ts b/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.spec.ts index 36b8f2f1b2d..99d6bf14e60 100644 --- a/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.spec.ts +++ b/apps/browser/src/key-management/biometrics/background-browser-biometrics.service.spec.ts @@ -1,11 +1,9 @@ import { mock } from "jest-mock-extended"; -import { BehaviorSubject } from "rxjs"; import { VaultTimeoutSettingsService } from "@bitwarden/common/key-management/vault-timeout"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; -import { UserId } from "@bitwarden/common/types/guid"; import { KeyService, BiometricStateService, BiometricsStatus } from "@bitwarden/key-management"; import { NativeMessagingBackground } from "../../background/nativeMessaging.background"; @@ -15,7 +13,6 @@ import { BackgroundBrowserBiometricsService } from "./background-browser-biometr describe("background browser biometrics service tests", function () { let service: BackgroundBrowserBiometricsService; - const userId = "userId" as UserId; const nativeMessagingBackground = mock(); const logService = mock(); const keyService = mock(); @@ -41,66 +38,9 @@ describe("background browser biometrics service tests", function () { }); afterEach(() => { - service.stopPolling(); jest.useRealTimers(); }); - describe("startPolling", () => { - it("connects to native messaging when biometrics are enabled", () => { - const biometricEnabled$ = new BehaviorSubject(true); - biometricStateService.biometricUnlockEnabled$.mockReturnValue(biometricEnabled$); - nativeMessagingBackground.connected = false; - nativeMessagingBackground.connect.mockResolvedValue(); - - service.startPolling(userId); - jest.advanceTimersByTime(0); - - expect(biometricStateService.biometricUnlockEnabled$).toHaveBeenCalledWith(userId); - expect(nativeMessagingBackground.connect).toHaveBeenCalled(); - }); - - it("does not connect when biometrics are disabled", () => { - const biometricEnabled$ = new BehaviorSubject(false); - biometricStateService.biometricUnlockEnabled$.mockReturnValue(biometricEnabled$); - nativeMessagingBackground.connected = false; - - service.startPolling(userId); - jest.advanceTimersByTime(0); - - expect(nativeMessagingBackground.connect).not.toHaveBeenCalled(); - }); - - it("does not connect when already connected", () => { - const biometricEnabled$ = new BehaviorSubject(true); - biometricStateService.biometricUnlockEnabled$.mockReturnValue(biometricEnabled$); - nativeMessagingBackground.connected = true; - - service.startPolling(userId); - jest.advanceTimersByTime(0); - - expect(nativeMessagingBackground.connect).not.toHaveBeenCalled(); - }); - }); - - describe("stopPolling", () => { - it("stops connecting after stopPolling is called", () => { - const biometricEnabled$ = new BehaviorSubject(true); - biometricStateService.biometricUnlockEnabled$.mockReturnValue(biometricEnabled$); - nativeMessagingBackground.connected = false; - nativeMessagingBackground.connect.mockResolvedValue(); - - service.startPolling(userId); - jest.advanceTimersByTime(0); - expect(nativeMessagingBackground.connect).toHaveBeenCalledTimes(1); - - nativeMessagingBackground.connect.mockClear(); - service.stopPolling(); - jest.advanceTimersByTime(service.BACKGROUND_POLLING_INTERVAL); - - expect(nativeMessagingBackground.connect).not.toHaveBeenCalled(); - }); - }); - describe("canEnableBiometricUnlock", () => { const table: [BiometricsStatus, boolean, boolean][] = [ // status, already enabled, expected 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 efb786e28d2..db66af57654 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 @@ -1,6 +1,3 @@ -import { BehaviorSubject, combineLatest, EMPTY, timer } from "rxjs"; -import { filter, concatMap, switchMap } from "rxjs/operators"; - import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { toTsBiometricsStatus } from "@bitwarden/common/key-management/biometrics-status-mapper"; import { fromTsUserId } from "@bitwarden/common/key-management/utils"; @@ -30,10 +27,6 @@ import { NativeMessagingBackground } from "../../background/nativeMessaging.back import { BrowserApi } from "../../platform/browser/browser-api"; export class BackgroundBrowserBiometricsService extends BiometricsService { - BACKGROUND_POLLING_INTERVAL = 30_000; - - private activePollingUser$ = new BehaviorSubject(null); - constructor( private nativeMessagingBackground: () => NativeMessagingBackground, private configService: () => ConfigService, @@ -45,40 +38,6 @@ export class BackgroundBrowserBiometricsService extends BiometricsService { private ipcService: () => IpcService, ) { 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. - this.activePollingUser$ - .pipe( - switchMap((userId) => { - if (userId == null) { - return EMPTY; - } - return combineLatest([ - timer(0, this.BACKGROUND_POLLING_INTERVAL), - this.biometricStateService.biometricUnlockEnabled$(userId), - ]).pipe( - filter(([_, enabled]) => enabled), - filter(() => !this.nativeMessagingBackground().connected), - concatMap(async () => { - try { - await this.nativeMessagingBackground().connect(); - await this.getBiometricsStatus(); - } catch { - // Ignore - } - }), - ); - }), - ) - .subscribe(); - } - - startPolling(userId: UserId): void { - this.activePollingUser$.next(userId); - } - - stopPolling(): void { - this.activePollingUser$.next(null); } async authenticateWithBiometrics(): Promise { @@ -90,9 +49,11 @@ export class BackgroundBrowserBiometricsService extends BiometricsService { } } - try { - await this.ensureConnected(); + if (!this.nativeMessagingBackground().connected) { + return false; + } + try { const response = await this.nativeMessagingBackground().callCommand({ command: BiometricsCommands.AuthenticateWithBiometrics, }); @@ -166,9 +127,11 @@ export class BackgroundBrowserBiometricsService extends BiometricsService { } } - try { - await this.ensureConnected(); + if (!this.nativeMessagingBackground().connected) { + return null; + } + try { const response = await this.nativeMessagingBackground().callCommand({ command: BiometricsCommands.UnlockWithBiometricsForUser, userId: userId, @@ -211,7 +174,9 @@ export class BackgroundBrowserBiometricsService extends BiometricsService { } try { - await this.ensureConnected(); + if (!this.nativeMessagingBackground().connected) { + return BiometricsStatus.DesktopDisconnected; + } return ( await this.nativeMessagingBackground().callCommand({ @@ -226,15 +191,6 @@ export class BackgroundBrowserBiometricsService extends BiometricsService { } } - // the first time we call, this might use an outdated version of the protocol, so we drop the response - private async ensureConnected() { - if (!this.nativeMessagingBackground().connected) { - await this.nativeMessagingBackground().callCommand({ - command: BiometricsCommands.GetBiometricsStatus, - }); - } - } - async getShouldAutopromptNow(): Promise { return false; } diff --git a/apps/browser/src/models/biometricErrors.ts b/apps/browser/src/models/biometricErrors.ts deleted file mode 100644 index 42d9c679d34..00000000000 --- a/apps/browser/src/models/biometricErrors.ts +++ /dev/null @@ -1,54 +0,0 @@ -type BiometricError = { - title: string; - description: string; -}; - -export type BiometricErrorTypes = - | "startDesktop" - | "desktopIntegrationDisabled" - | "not enabled" - | "not supported" - | "not unlocked" - | "invalidateEncryption" - | "userkey wrong" - | "wrongUserId" - | "not available"; - -export const BiometricErrors: Record = { - startDesktop: { - title: "startDesktopTitle", - description: "startDesktopDesc", - }, - desktopIntegrationDisabled: { - title: "desktopIntegrationDisabledTitle", - description: "desktopIntegrationDisabledDesc", - }, - "not enabled": { - title: "biometricsNotEnabledTitle", - description: "biometricsNotEnabledDesc", - }, - "not supported": { - title: "biometricsNotSupportedTitle", - description: "biometricsNotSupportedDesc", - }, - "not unlocked": { - title: "biometricsUnlockNotUnlockedTitle", - description: "biometricsUnlockNotUnlockedDesc", - }, - invalidateEncryption: { - title: "nativeMessagingInvalidEncryptionTitle", - description: "nativeMessagingInvalidEncryptionDesc", - }, - "userkey wrong": { - title: "nativeMessagingWrongUserKeyTitle", - description: "nativeMessagingWrongUserKeyDesc", - }, - wrongUserId: { - title: "biometricsWrongUserTitle", - description: "biometricsWrongUserDesc", - }, - "not available": { - title: "biometricsNotAvailableTitle", - description: "biometricsNotAvailableDesc", - }, -};