From 2827d338ee459baac33526f4aa120ffec8ac6dfe Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Mon, 9 Sep 2024 07:44:08 -0500 Subject: [PATCH] [PM-11419] Fix issues encountered with inline menu passkeys (#10892) * [PM-11419] Login items do not display after adding passkey * [PM-11419] Login items do not display after adding passkey * [PM-11419] Incorporating fixes for deleting a cipher from the inline menu as well as authenticating using passkeys * [PM-11419] Fixing an issue where master password reprompt is ignored for a set passkey cipher * [PM-11419] Fixing an issue where saving a passkey does not trigger a clearing of cached cipher values * [PM-11419] Refactoring implementation * [PM-11419] Ensuring that passkeys must be enabled in order for ciphers to appear * [PM-11419] Adding an abort event from the active request manager * [PM-11419] Adding an abort event from the active request manager * [PM-11419] Working through jest tests within implementation * [PM-11419] Fixing jest tests within Fido2ClientService and Fido2AuthenticatorService * [PM-11419] Adding jest tests for added logic within OverlayBackground * [PM-11419] Adding jest tests for added logic within OverlayBackground * [PM-11419] Reworking how we handle assuming user presence when master password reprompt is required * [PM-11419] Reworking how we handle assuming user presence when master password reprompt is required * [PM-11419] Reworking how we handle assuming user presence when master password reprompt is required * [PM-11419] Refactoring implementation * [PM-11419] Incorporating suggestion for reporting failed passkey authentication from the inline menu * [PM-11419] Reworking positioning of the abort controller that informs the background script of an error * [PM-11419] Scoping down the behavior surrounding master password reprompt a bit more tightly * [PM-11419] Reworking how we handle reacting to active fido2 requests to avoid ambiguity * [PM-11419] Reworking how we handle reacting to active fido2 requests to avoid ambiguity * [PM-11419] Adjusting implementation to ensure we clear any active requests when the passkeys setting is modified --- .../abstractions/overlay.background.ts | 1 + .../background/overlay.background.spec.ts | 129 ++++++++++++++++-- .../autofill/background/overlay.background.ts | 76 +++++++++-- .../fido2/background/fido2.background.spec.ts | 4 + .../fido2/background/fido2.background.ts | 3 + .../fido2/content/fido2-content-script.ts | 4 + .../fido2/content/fido2-page-script.ts | 7 + .../browser-fido2-user-interface.service.ts | 3 +- .../browser/src/background/main.background.ts | 10 +- ...ido2-active-request-manager.abstraction.ts | 18 ++- .../fido2/fido2-client.service.abstraction.ts | 8 -- ...ido2-user-interface.service.abstraction.ts | 5 + .../fido2/fido2-active-request-manager.ts | 19 ++- .../fido2/fido2-authenticator.service.spec.ts | 3 + .../fido2/fido2-authenticator.service.ts | 13 +- .../fido2/fido2-client.service.spec.ts | 35 ++--- .../services/fido2/fido2-client.service.ts | 34 ++--- 17 files changed, 288 insertions(+), 84 deletions(-) diff --git a/apps/browser/src/autofill/background/abstractions/overlay.background.ts b/apps/browser/src/autofill/background/abstractions/overlay.background.ts index 5d2d2a3898..e91a58a84c 100644 --- a/apps/browser/src/autofill/background/abstractions/overlay.background.ts +++ b/apps/browser/src/autofill/background/abstractions/overlay.background.ts @@ -217,6 +217,7 @@ export type OverlayBackgroundExtensionMessageHandlers = { addEditCipherSubmitted: () => void; editedCipher: () => void; deletedCipher: () => void; + fido2AbortRequest: ({ message, sender }: BackgroundOnMessageHandlerParams) => void; }; export type PortMessageParam = { diff --git a/apps/browser/src/autofill/background/overlay.background.spec.ts b/apps/browser/src/autofill/background/overlay.background.spec.ts index 5ac94582ce..30f19e7260 100644 --- a/apps/browser/src/autofill/background/overlay.background.spec.ts +++ b/apps/browser/src/autofill/background/overlay.background.spec.ts @@ -18,12 +18,12 @@ import { EnvironmentService, Region, } from "@bitwarden/common/platform/abstractions/environment.service"; -import { Fido2ClientService } from "@bitwarden/common/platform/abstractions/fido2/fido2-client.service.abstraction"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { ThemeType } from "@bitwarden/common/platform/enums"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { CloudEnvironment } from "@bitwarden/common/platform/services/default-environment.service"; +import { Fido2ActiveRequestManager } from "@bitwarden/common/platform/services/fido2/fido2-active-request-manager"; import { ThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service"; import { FakeAccountService, @@ -32,6 +32,7 @@ import { } from "@bitwarden/common/spec"; import { UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; import { CipherRepromptType, CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { Fido2CredentialView } from "@bitwarden/common/vault/models/view/fido2-credential.view"; @@ -89,8 +90,9 @@ describe("OverlayBackground", () => { let autofillSettingsService: MockProxy; let i18nService: MockProxy; let platformUtilsService: MockProxy; - let availableAutofillCredentialsMock$: BehaviorSubject; - let fido2ClientService: MockProxy; + let enablePasskeysMock$: BehaviorSubject; + let vaultSettingsServiceMock: MockProxy; + let fido2ActiveRequestManager: Fido2ActiveRequestManager; let selectedThemeMock$: BehaviorSubject; let themeStateService: MockProxy; let overlayBackground: OverlayBackground; @@ -159,10 +161,10 @@ describe("OverlayBackground", () => { autofillSettingsService.inlineMenuVisibility$ = inlineMenuVisibilityMock$; i18nService = mock(); platformUtilsService = mock(); - availableAutofillCredentialsMock$ = new BehaviorSubject([]); - fido2ClientService = mock({ - availableAutofillCredentials$: (_tabId) => availableAutofillCredentialsMock$, - }); + enablePasskeysMock$ = new BehaviorSubject(true); + vaultSettingsServiceMock = mock(); + vaultSettingsServiceMock.enablePasskeys$ = enablePasskeysMock$; + fido2ActiveRequestManager = new Fido2ActiveRequestManager(); selectedThemeMock$ = new BehaviorSubject(ThemeType.Light); themeStateService = mock(); themeStateService.selectedTheme$ = selectedThemeMock$; @@ -176,7 +178,8 @@ describe("OverlayBackground", () => { autofillSettingsService, i18nService, platformUtilsService, - fido2ClientService, + vaultSettingsServiceMock, + fido2ActiveRequestManager, themeStateService, ); portKeyForTabSpy = overlayBackground["portKeyForTab"]; @@ -779,6 +782,15 @@ describe("OverlayBackground", () => { expect(cipherService.getAllDecryptedForUrl).not.toHaveBeenCalled(); }); + it("skips updating the inline menu ciphers if the current tab url has non-http protocol", async () => { + const nonHttpTab = createChromeTabMock({ url: "chrome-extension://id/route" }); + getTabFromCurrentWindowIdSpy.mockResolvedValueOnce(nonHttpTab); + + await overlayBackground.updateOverlayCiphers(); + + expect(cipherService.getAllDecryptedForUrl).not.toHaveBeenCalled(); + }); + it("closes the inline menu on the focused field's tab if the user's auth status is not unlocked", async () => { activeAccountStatusMock$.next(AuthenticationStatus.Locked); const previousTab = mock({ id: 1 }); @@ -1113,7 +1125,11 @@ describe("OverlayBackground", () => { }); it("adds available passkey ciphers to the inline menu", async () => { - availableAutofillCredentialsMock$.next(passkeyCipher.login.fido2Credentials); + void fido2ActiveRequestManager.newActiveRequest( + tab.id, + passkeyCipher.login.fido2Credentials, + new AbortController(), + ); overlayBackground["focusedFieldData"] = createFocusedFieldDataMock({ tabId: tab.id, filledByCipherType: CipherType.Login, @@ -1192,10 +1208,15 @@ describe("OverlayBackground", () => { }); it("does not add a passkey to the inline menu when its rpId is part of the neverDomains exclusion list", async () => { - availableAutofillCredentialsMock$.next(passkeyCipher.login.fido2Credentials); + void fido2ActiveRequestManager.newActiveRequest( + tab.id, + passkeyCipher.login.fido2Credentials, + new AbortController(), + ); overlayBackground["focusedFieldData"] = createFocusedFieldDataMock({ tabId: tab.id, filledByCipherType: CipherType.Login, + showPasskeys: true, }); cipherService.getAllDecryptedForUrl.mockResolvedValue([loginCipher1, passkeyCipher]); cipherService.sortCiphersByLastUsedThenName.mockReturnValue(-1); @@ -1248,6 +1269,69 @@ describe("OverlayBackground", () => { showPasskeysLabels: false, }); }); + + it("does not add passkeys to the inline menu if the passkey setting is disabled", async () => { + enablePasskeysMock$.next(false); + void fido2ActiveRequestManager.newActiveRequest( + tab.id, + passkeyCipher.login.fido2Credentials, + new AbortController(), + ); + overlayBackground["focusedFieldData"] = createFocusedFieldDataMock({ + tabId: tab.id, + filledByCipherType: CipherType.Login, + showPasskeys: true, + }); + cipherService.getAllDecryptedForUrl.mockResolvedValue([loginCipher1, passkeyCipher]); + cipherService.sortCiphersByLastUsedThenName.mockReturnValue(-1); + getTabFromCurrentWindowIdSpy.mockResolvedValueOnce(tab); + + await overlayBackground.updateOverlayCiphers(); + + expect(listPortSpy.postMessage).toHaveBeenCalledWith({ + command: "updateAutofillInlineMenuListCiphers", + ciphers: [ + { + id: "inline-menu-cipher-0", + name: passkeyCipher.name, + type: CipherType.Login, + reprompt: passkeyCipher.reprompt, + favorite: passkeyCipher.favorite, + icon: { + fallbackImage: "images/bwi-globe.png", + icon: "bwi-globe", + image: "https://icons.bitwarden.com//jest-testing-website.com/icon.png", + imageEnabled: true, + }, + accountCreationFieldType: undefined, + login: { + username: passkeyCipher.login.username, + passkey: null, + }, + }, + { + id: "inline-menu-cipher-1", + name: loginCipher1.name, + type: CipherType.Login, + reprompt: loginCipher1.reprompt, + favorite: loginCipher1.favorite, + icon: { + fallbackImage: "images/bwi-globe.png", + icon: "bwi-globe", + image: "https://icons.bitwarden.com//jest-testing-website.com/icon.png", + imageEnabled: true, + }, + accountCreationFieldType: undefined, + login: { + username: loginCipher1.login.username, + passkey: null, + }, + }, + ], + showInlineMenuAccountCreation: false, + showPasskeysLabels: false, + }); + }); }); describe("extension message handlers", () => { @@ -2537,6 +2621,25 @@ describe("OverlayBackground", () => { }); }); }); + + describe("fido2AbortRequest", () => { + const sender = mock({ tab: { id: 1 } }); + it("removes an active request associated with the sender tab", () => { + const removeActiveRequestSpy = jest.spyOn(fido2ActiveRequestManager, "removeActiveRequest"); + + sendMockExtensionMessage({ command: "fido2AbortRequest" }, sender); + + expect(removeActiveRequestSpy).toHaveBeenCalledWith(sender.tab.id); + }); + + it("updates the overlay ciphers after removing the active request", () => { + const updateOverlayCiphersSpy = jest.spyOn(overlayBackground, "updateOverlayCiphers"); + + sendMockExtensionMessage({ command: "fido2AbortRequest" }, sender); + + expect(updateOverlayCiphersSpy).toHaveBeenCalledWith(false); + }); + }); }); describe("handle extension onMessage", () => { @@ -2920,6 +3023,7 @@ describe("OverlayBackground", () => { [sender.frameId, pageDetailsForTab], ]); autofillService.isPasswordRepromptRequired.mockResolvedValue(false); + jest.spyOn(fido2ActiveRequestManager, "getActiveRequest"); sendPortMessage(listMessageConnectorSpy, { command: "fillAutofillInlineMenuCipher", @@ -2929,10 +3033,7 @@ describe("OverlayBackground", () => { }); await flushPromises(); - expect(fido2ClientService.autofillCredential).toHaveBeenCalledWith( - sender.tab.id, - fido2Credential.credentialId, - ); + expect(fido2ActiveRequestManager.getActiveRequest).toHaveBeenCalledWith(sender.tab.id); }); }); diff --git a/apps/browser/src/autofill/background/overlay.background.ts b/apps/browser/src/autofill/background/overlay.background.ts index 0c626c6879..0047d1de28 100644 --- a/apps/browser/src/autofill/background/overlay.background.ts +++ b/apps/browser/src/autofill/background/overlay.background.ts @@ -6,6 +6,8 @@ import { throttleTime, switchMap, debounceTime, + Observable, + map, } from "rxjs"; import { parse } from "tldts"; @@ -20,13 +22,17 @@ import { DomainSettingsService } from "@bitwarden/common/autofill/services/domai import { InlineMenuVisibilitySetting } from "@bitwarden/common/autofill/types"; import { NeverDomains } from "@bitwarden/common/models/domain/domain-service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; -import { Fido2ClientService } from "@bitwarden/common/platform/abstractions/fido2/fido2-client.service.abstraction"; +import { + Fido2ActiveRequestEvents, + Fido2ActiveRequestManager, +} from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { ThemeStateService } from "@bitwarden/common/platform/theming/theme-state.service"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; import { CipherType } from "@bitwarden/common/vault/enums"; import { buildCipherIcon } from "@bitwarden/common/vault/icon/build-cipher-icon"; import { CardView } from "@bitwarden/common/vault/models/view/card.view"; @@ -144,6 +150,7 @@ export class OverlayBackground implements OverlayBackgroundInterface { addEditCipherSubmitted: () => this.updateOverlayCiphers(), editedCipher: () => this.updateOverlayCiphers(), deletedCipher: () => this.updateOverlayCiphers(), + fido2AbortRequest: ({ sender }) => this.abortFido2ActiveRequest(sender), }; private readonly inlineMenuButtonPortMessageHandlers: InlineMenuButtonPortMessageHandlers = { triggerDelayedAutofillInlineMenuClosure: () => this.triggerDelayedInlineMenuClosure(), @@ -175,7 +182,8 @@ export class OverlayBackground implements OverlayBackgroundInterface { private autofillSettingsService: AutofillSettingsServiceAbstraction, private i18nService: I18nService, private platformUtilsService: PlatformUtilsService, - private fido2ClientService: Fido2ClientService, + private vaultSettingsService: VaultSettingsService, + private fido2ActiveRequestManager: Fido2ActiveRequestManager, private themeStateService: ThemeStateService, ) { this.initOverlayEventObservables(); @@ -196,7 +204,7 @@ export class OverlayBackground implements OverlayBackgroundInterface { */ private initOverlayEventObservables() { this.storeInlineMenuFido2CredentialsSubject - .pipe(switchMap((tabId) => this.fido2ClientService.availableAutofillCredentials$(tabId))) + .pipe(switchMap((tabId) => this.availablePasskeyAuthCredentials$(tabId))) .subscribe((credentials) => this.storeInlineMenuFido2Credentials(credentials)); this.repositionInlineMenuSubject .pipe( @@ -279,6 +287,11 @@ export class OverlayBackground implements OverlayBackgroundInterface { return; } + const request = this.fido2ActiveRequestManager.getActiveRequest(currentTab.id); + if (request) { + request.subject.next({ type: Fido2ActiveRequestEvents.Refresh }); + } + this.inlineMenuFido2Credentials.clear(); this.storeInlineMenuFido2CredentialsSubject.next(currentTab.id); @@ -452,6 +465,7 @@ export class OverlayBackground implements OverlayBackgroundInterface { if (domainExclusions) { domainExclusionsSet = new Set(Object.keys(await this.getExcludedDomains())); } + const passkeysEnabled = await firstValueFrom(this.vaultSettingsService.enablePasskeys$); for (let cipherIndex = 0; cipherIndex < inlineMenuCiphersArray.length; cipherIndex++) { const [inlineMenuCipherId, cipher] = inlineMenuCiphersArray[cipherIndex]; @@ -459,7 +473,7 @@ export class OverlayBackground implements OverlayBackgroundInterface { continue; } - if (!this.showCipherAsPasskey(cipher, domainExclusionsSet)) { + if (!passkeysEnabled || !(await this.showCipherAsPasskey(cipher, domainExclusionsSet))) { inlineMenuCipherData.push( this.buildCipherData({ inlineMenuCipherId, cipher, showFavicons }), ); @@ -497,7 +511,10 @@ export class OverlayBackground implements OverlayBackgroundInterface { * @param cipher - The cipher to check * @param domainExclusions - The domain exclusions to check against */ - private showCipherAsPasskey(cipher: CipherView, domainExclusions: Set | null): boolean { + private async showCipherAsPasskey( + cipher: CipherView, + domainExclusions: Set | null, + ): Promise { if (cipher.type !== CipherType.Login || !this.focusedFieldData?.showPasskeys) { return false; } @@ -514,10 +531,7 @@ export class OverlayBackground implements OverlayBackgroundInterface { return false; } - return ( - this.inlineMenuFido2Credentials.size === 0 || - this.inlineMenuFido2Credentials.has(credentialId) - ); + return this.inlineMenuFido2Credentials.has(credentialId); } /** @@ -635,12 +649,35 @@ export class OverlayBackground implements OverlayBackgroundInterface { * @param credentials - The FIDO2 credentials to store */ private storeInlineMenuFido2Credentials(credentials: Fido2CredentialView[]) { + this.inlineMenuFido2Credentials.clear(); + credentials.forEach( (credential) => credential?.credentialId && this.inlineMenuFido2Credentials.add(credential.credentialId), ); } + /** + * Gets the passkey credentials available from an active FIDO2 request for a given tab. + * + * @param tabId - The tab id to get the active request for. + */ + private availablePasskeyAuthCredentials$(tabId: number): Observable { + return this.fido2ActiveRequestManager + .getActiveRequest$(tabId) + .pipe(map((request) => request?.credentials ?? [])); + } + + /** + * Aborts an active FIDO2 request for a given tab and updates the inline menu ciphers. + * + * @param sender - The sender of the message + */ + private async abortFido2ActiveRequest(sender: chrome.runtime.MessageSender) { + this.fido2ActiveRequestManager.removeActiveRequest(sender.tab.id); + await this.updateOverlayCiphers(false); + } + /** * Gets the neverDomains setting from the domain settings service. */ @@ -900,11 +937,12 @@ export class OverlayBackground implements OverlayBackgroundInterface { const cipher = this.inlineMenuCiphers.get(inlineMenuCipherId); if (usePasskey && cipher.login?.hasFido2Credentials) { - await this.fido2ClientService.autofillCredential( + await this.authenticatePasskeyCredential( sender.tab.id, cipher.login.fido2Credentials[0].credentialId, ); this.updateLastUsedInlineMenuCipher(inlineMenuCipherId, cipher); + this.closeInlineMenu(sender, { forceCloseInlineMenu: true }); return; } @@ -927,6 +965,24 @@ export class OverlayBackground implements OverlayBackgroundInterface { this.updateLastUsedInlineMenuCipher(inlineMenuCipherId, cipher); } + /** + * Triggers a FIDO2 authentication from the inline menu using the passed credential ID. + * + * @param tabId - The tab ID to trigger the authentication for + * @param credentialId - The credential ID to authenticate + */ + async authenticatePasskeyCredential(tabId: number, credentialId: string) { + const request = this.fido2ActiveRequestManager.getActiveRequest(tabId); + if (!request) { + this.logService.error( + "Could not complete passkey autofill due to missing active Fido2 request", + ); + return; + } + + request.subject.next({ type: Fido2ActiveRequestEvents.Continue, credentialId }); + } + /** * Sets the most recently used cipher at the top of the list of ciphers. * diff --git a/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts b/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts index 2a22a226e3..23d0292e18 100644 --- a/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts +++ b/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts @@ -2,6 +2,7 @@ import { mock, MockProxy } from "jest-mock-extended"; import { BehaviorSubject } from "rxjs"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { Fido2ActiveRequestManager } from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction"; import { AssertCredentialParams, CreateCredentialParams, @@ -52,6 +53,7 @@ describe("Fido2Background", () => { let tabMock!: MockProxy; let senderMock!: MockProxy; let logService!: MockProxy; + let fido2ActiveRequestManager: MockProxy; let fido2ClientService!: MockProxy; let vaultSettingsService!: MockProxy; let scriptInjectorServiceMock!: MockProxy; @@ -77,9 +79,11 @@ describe("Fido2Background", () => { enablePasskeysMock$ = new BehaviorSubject(true); vaultSettingsService.enablePasskeys$ = enablePasskeysMock$; + fido2ActiveRequestManager = mock(); fido2ClientService.isFido2FeatureEnabled.mockResolvedValue(true); fido2Background = new Fido2Background( logService, + fido2ActiveRequestManager, fido2ClientService, vaultSettingsService, scriptInjectorServiceMock, diff --git a/apps/browser/src/autofill/fido2/background/fido2.background.ts b/apps/browser/src/autofill/fido2/background/fido2.background.ts index 800c7d4e0d..a9d1b31477 100644 --- a/apps/browser/src/autofill/fido2/background/fido2.background.ts +++ b/apps/browser/src/autofill/fido2/background/fido2.background.ts @@ -3,6 +3,7 @@ import { pairwise } from "rxjs/operators"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { Fido2ActiveRequestManager } from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction"; import { AssertCredentialParams, AssertCredentialResult, @@ -49,6 +50,7 @@ export class Fido2Background implements Fido2BackgroundInterface { constructor( private logService: LogService, + private fido2ActiveRequestManager: Fido2ActiveRequestManager, private fido2ClientService: Fido2ClientService, private vaultSettingsService: VaultSettingsService, private scriptInjectorService: ScriptInjectorService, @@ -96,6 +98,7 @@ export class Fido2Background implements Fido2BackgroundInterface { previousEnablePasskeysSetting: boolean, enablePasskeys: boolean, ) { + this.fido2ActiveRequestManager.removeAllActiveRequests(); await this.updateContentScriptRegistration(); if (previousEnablePasskeysSetting === undefined) { diff --git a/apps/browser/src/autofill/fido2/content/fido2-content-script.ts b/apps/browser/src/autofill/fido2/content/fido2-content-script.ts index 171aa7cd83..737f8e77ca 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-content-script.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-content-script.ts @@ -60,6 +60,10 @@ import { MessageWithMetadata, Messenger } from "./messaging/messenger"; message.data as InsecureAssertCredentialParams, ); } + + if (message.type === MessageType.AbortRequest) { + return sendExtensionMessage("fido2AbortRequest", { abortedRequestId: requestId }); + } } finally { abortController.signal.removeEventListener("abort", abortHandler); } diff --git a/apps/browser/src/autofill/fido2/content/fido2-page-script.ts b/apps/browser/src/autofill/fido2/content/fido2-page-script.ts index 4631b78ddb..c44c263dd2 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-page-script.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-page-script.ts @@ -131,6 +131,12 @@ import { Messenger } from "./messaging/messenger"; const internalAbortControllers = [new AbortController(), new AbortController()]; const bitwardenResponse = async (internalAbortController: AbortController) => { try { + const abortListener = () => + messenger.request({ + type: MessageType.AbortRequest, + abortedRequestId: abortSignal.toString(), + }); + internalAbortController.signal.addEventListener("abort", abortListener); const response = await messenger.request( { type: MessageType.CredentialGetRequest, @@ -138,6 +144,7 @@ import { Messenger } from "./messaging/messenger"; }, internalAbortController.signal, ); + internalAbortController.signal.removeEventListener("abort", abortListener); if (response.type !== MessageType.CredentialGetResponse) { throw new Error("Something went wrong."); } diff --git a/apps/browser/src/autofill/fido2/services/browser-fido2-user-interface.service.ts b/apps/browser/src/autofill/fido2/services/browser-fido2-user-interface.service.ts index 1e5e880c67..3ab9edf60f 100644 --- a/apps/browser/src/autofill/fido2/services/browser-fido2-user-interface.service.ts +++ b/apps/browser/src/autofill/fido2/services/browser-fido2-user-interface.service.ts @@ -242,12 +242,13 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi cipherIds, userVerification, assumeUserPresence, + masterPasswordRepromptRequired, }: PickCredentialParams): Promise<{ cipherId: string; userVerified: boolean }> { // NOTE: For now, we are defaulting to a userVerified status of `true` when the request // is for a conditionally mediated authentication. This will allow for mediated conditional // authentication to function without requiring user interaction. This is a product // decision, rather than a decision based on the expected technical specifications. - if (assumeUserPresence && cipherIds.length === 1) { + if (assumeUserPresence && cipherIds.length === 1 && !masterPasswordRepromptRequired) { return { cipherId: cipherIds[0], userVerified: userVerification }; } diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index df1e479656..1cb615fe06 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -79,6 +79,7 @@ import { ConfigService } from "@bitwarden/common/platform/abstractions/config/co import { CryptoFunctionService as CryptoFunctionServiceAbstraction } from "@bitwarden/common/platform/abstractions/crypto-function.service"; import { CryptoService as CryptoServiceAbstraction } from "@bitwarden/common/platform/abstractions/crypto.service"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; +import { Fido2ActiveRequestManager as Fido2ActiveRequestManagerAbstraction } from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction"; import { Fido2AuthenticatorService as Fido2AuthenticatorServiceAbstraction } from "@bitwarden/common/platform/abstractions/fido2/fido2-authenticator.service.abstraction"; import { Fido2ClientService as Fido2ClientServiceAbstraction } from "@bitwarden/common/platform/abstractions/fido2/fido2-client.service.abstraction"; import { Fido2UserInterfaceService as Fido2UserInterfaceServiceAbstraction } from "@bitwarden/common/platform/abstractions/fido2/fido2-user-interface.service.abstraction"; @@ -324,6 +325,7 @@ export default class MainBackground { userVerificationApiService: UserVerificationApiServiceAbstraction; fido2UserInterfaceService: Fido2UserInterfaceServiceAbstraction; fido2AuthenticatorService: Fido2AuthenticatorServiceAbstraction; + fido2ActiveRequestManager: Fido2ActiveRequestManagerAbstraction; fido2ClientService: Fido2ClientServiceAbstraction; avatarService: AvatarServiceAbstraction; mainContextMenuHandler: MainContextMenuHandler; @@ -1021,7 +1023,7 @@ export default class MainBackground { this.accountService, this.logService, ); - const fido2ActiveRequestManager = new Fido2ActiveRequestManager(); + this.fido2ActiveRequestManager = new Fido2ActiveRequestManager(); this.fido2ClientService = new Fido2ClientService( this.fido2AuthenticatorService, this.configService, @@ -1029,7 +1031,7 @@ export default class MainBackground { this.vaultSettingsService, this.domainSettingsService, this.taskSchedulerService, - fido2ActiveRequestManager, + this.fido2ActiveRequestManager, this.logService, ); @@ -1057,6 +1059,7 @@ export default class MainBackground { if (!this.popupOnlyContext) { this.fido2Background = new Fido2Background( this.logService, + this.fido2ActiveRequestManager, this.fido2ClientService, this.vaultSettingsService, this.scriptInjectorService, @@ -1605,7 +1608,8 @@ export default class MainBackground { this.autofillSettingsService, this.i18nService, this.platformUtilsService, - this.fido2ClientService, + this.vaultSettingsService, + this.fido2ActiveRequestManager, this.themeStateService, ); } diff --git a/libs/common/src/platform/abstractions/fido2/fido2-active-request-manager.abstraction.ts b/libs/common/src/platform/abstractions/fido2/fido2-active-request-manager.abstraction.ts index 4e164c4577..797133edd2 100644 --- a/libs/common/src/platform/abstractions/fido2/fido2-active-request-manager.abstraction.ts +++ b/libs/common/src/platform/abstractions/fido2/fido2-active-request-manager.abstraction.ts @@ -2,9 +2,22 @@ import { Observable, Subject } from "rxjs"; import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view"; +export const Fido2ActiveRequestEvents = { + Refresh: "refresh-fido2-active-request", + Abort: "abort-fido2-active-request", + Continue: "continue-fido2-active-request", +} as const; + +type Fido2ActiveRequestEvent = typeof Fido2ActiveRequestEvents; + +export type RequestResult = + | { type: Fido2ActiveRequestEvent["Refresh"] } + | { type: Fido2ActiveRequestEvent["Abort"] } + | { type: Fido2ActiveRequestEvent["Continue"]; credentialId: string }; + export interface ActiveRequest { credentials: Fido2CredentialView[]; - subject: Subject; + subject: Subject; } export type RequestCollection = Readonly<{ [tabId: number]: ActiveRequest }>; @@ -16,6 +29,7 @@ export abstract class Fido2ActiveRequestManager { tabId: number, credentials: Fido2CredentialView[], abortController: AbortController, - ) => Promise; + ) => Promise; removeActiveRequest: (tabId: number) => void; + removeAllActiveRequests: () => void; } diff --git a/libs/common/src/platform/abstractions/fido2/fido2-client.service.abstraction.ts b/libs/common/src/platform/abstractions/fido2/fido2-client.service.abstraction.ts index 2ba67a48be..6467af5fc1 100644 --- a/libs/common/src/platform/abstractions/fido2/fido2-client.service.abstraction.ts +++ b/libs/common/src/platform/abstractions/fido2/fido2-client.service.abstraction.ts @@ -1,7 +1,3 @@ -import { Observable } from "rxjs"; - -import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view"; - export const UserRequestedFallbackAbortReason = "UserRequestedFallback"; export type UserVerification = "discouraged" | "preferred" | "required"; @@ -20,10 +16,6 @@ export type UserVerification = "discouraged" | "preferred" | "required"; export abstract class Fido2ClientService { isFido2FeatureEnabled: (hostname: string, origin: string) => Promise; - availableAutofillCredentials$: (tabId: number) => Observable; - - autofillCredential: (tabId: number, credentialId: string) => Promise; - /** * Allows WebAuthn Relying Party scripts to request the creation of a new public key credential source. * For more information please see: https://www.w3.org/TR/webauthn-3/#sctn-createCredential diff --git a/libs/common/src/platform/abstractions/fido2/fido2-user-interface.service.abstraction.ts b/libs/common/src/platform/abstractions/fido2/fido2-user-interface.service.abstraction.ts index 55232162d4..70fe0b092b 100644 --- a/libs/common/src/platform/abstractions/fido2/fido2-user-interface.service.abstraction.ts +++ b/libs/common/src/platform/abstractions/fido2/fido2-user-interface.service.abstraction.ts @@ -45,6 +45,11 @@ export interface PickCredentialParams { * Bypass the UI and assume that the user has already interacted with the authenticator. */ assumeUserPresence?: boolean; + + /** + * Identifies whether a cipher requires a master password reprompt when getting a credential. + */ + masterPasswordRepromptRequired?: boolean; } /** diff --git a/libs/common/src/platform/services/fido2/fido2-active-request-manager.ts b/libs/common/src/platform/services/fido2/fido2-active-request-manager.ts index 0f82d8a09c..d24941ee4e 100644 --- a/libs/common/src/platform/services/fido2/fido2-active-request-manager.ts +++ b/libs/common/src/platform/services/fido2/fido2-active-request-manager.ts @@ -14,6 +14,8 @@ import { ActiveRequest, RequestCollection, Fido2ActiveRequestManager as Fido2ActiveRequestManagerAbstraction, + Fido2ActiveRequestEvents, + RequestResult, } from "../../abstractions/fido2/fido2-active-request-manager.abstraction"; export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstraction { @@ -53,7 +55,7 @@ export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstr tabId: number, credentials: Fido2CredentialView[], abortController: AbortController, - ): Promise { + ): Promise { const newRequest: ActiveRequest = { credentials, subject: new Subject(), @@ -65,10 +67,10 @@ export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstr const abortListener = () => this.abortActiveRequest(tabId); abortController.signal.addEventListener("abort", abortListener); - const credentialId = firstValueFrom(newRequest.subject); + const requestResult = firstValueFrom(newRequest.subject); abortController.signal.removeEventListener("abort", abortListener); - return credentialId; + return requestResult; } /** @@ -85,12 +87,23 @@ export class Fido2ActiveRequestManager implements Fido2ActiveRequestManagerAbstr }); } + /** + * Removes and aborts all active requests. + */ + removeAllActiveRequests() { + Object.keys(this.activeRequests$.value).forEach((tabId) => { + this.abortActiveRequest(Number(tabId)); + }); + this.updateRequests(() => ({})); + } + /** * Aborts the active request associated with a given tab id. * * @param tabId - The tab id to abort the active request for. */ private abortActiveRequest(tabId: number): void { + this.activeRequests$.value[tabId]?.subject.next({ type: Fido2ActiveRequestEvents.Abort }); this.activeRequests$.value[tabId]?.subject.error( new DOMException("The operation either timed out or was not allowed.", "AbortError"), ); diff --git a/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts b/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts index adb1adcce4..289e94e6ef 100644 --- a/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts +++ b/libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts @@ -576,6 +576,7 @@ describe("FidoAuthenticatorService", () => { expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({ cipherIds: ciphers.map((c) => c.id), userVerification: false, + masterPasswordRepromptRequired: false, }); }); @@ -592,6 +593,7 @@ describe("FidoAuthenticatorService", () => { expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({ cipherIds: [discoverableCiphers[0].id], userVerification: false, + masterPasswordRepromptRequired: false, }); }); @@ -609,6 +611,7 @@ describe("FidoAuthenticatorService", () => { expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({ cipherIds: ciphers.map((c) => c.id), userVerification, + masterPasswordRepromptRequired: false, }); }); } diff --git a/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts b/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts index ef09a3d160..ddcc079eb9 100644 --- a/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts +++ b/libs/common/src/platform/services/fido2/fido2-authenticator.service.ts @@ -160,6 +160,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr } const reencrypted = await this.cipherService.encrypt(cipher, activeUserId); await this.cipherService.updateWithServer(reencrypted); + await this.cipherService.clearCache(activeUserId); credentialId = fido2Credential.credentialId; } catch (error) { this.logService?.error( @@ -243,12 +244,18 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr } let response = { cipherId: cipherOptions[0].id, userVerified: false }; + const masterPasswordRepromptRequired = cipherOptions.some( + (cipher) => cipher.reprompt !== CipherRepromptType.None, + ); - if (this.requiresUserVerificationPrompt(params, cipherOptions)) { + if ( + this.requiresUserVerificationPrompt(params, cipherOptions, masterPasswordRepromptRequired) + ) { response = await userInterfaceSession.pickCredential({ cipherIds: cipherOptions.map((cipher) => cipher.id), userVerification: params.requireUserVerification, assumeUserPresence: params.assumeUserPresence, + masterPasswordRepromptRequired, }); } @@ -292,6 +299,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr ); const encrypted = await this.cipherService.encrypt(selectedCipher, activeUserId); await this.cipherService.updateWithServer(encrypted); + await this.cipherService.clearCache(activeUserId); } const authenticatorData = await generateAuthData({ @@ -330,13 +338,14 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr private requiresUserVerificationPrompt( params: Fido2AuthenticatorGetAssertionParams, cipherOptions: CipherView[], + masterPasswordRepromptRequired: boolean, ): boolean { return ( params.requireUserVerification || !params.assumeUserPresence || cipherOptions.length > 1 || cipherOptions.length === 0 || - cipherOptions.some((cipher) => cipher.reprompt !== CipherRepromptType.None) + masterPasswordRepromptRequired ); } diff --git a/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts b/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts index c0ae2cabfb..582849ebc1 100644 --- a/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts +++ b/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts @@ -6,11 +6,12 @@ import { AuthenticationStatus } from "../../../auth/enums/authentication-status" import { DomainSettingsService } from "../../../autofill/services/domain-settings.service"; import { Utils } from "../../../platform/misc/utils"; import { VaultSettingsService } from "../../../vault/abstractions/vault-settings/vault-settings.service"; -import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view"; import { ConfigService } from "../../abstractions/config/config.service"; import { ActiveRequest, + Fido2ActiveRequestEvents, Fido2ActiveRequestManager, + RequestResult, } from "../../abstractions/fido2/fido2-active-request-manager.abstraction"; import { Fido2AuthenticatorError, @@ -56,7 +57,10 @@ describe("FidoAuthenticatorService", () => { domainSettingsService = mock(); taskSchedulerService = mock(); activeRequest = mock({ - subject: new BehaviorSubject(""), + subject: new BehaviorSubject({ + type: Fido2ActiveRequestEvents.Continue, + credentialId: "", + }), }); requestManager = mock({ getActiveRequest$: (tabId: number) => new BehaviorSubject(activeRequest), @@ -615,7 +619,10 @@ describe("FidoAuthenticatorService", () => { }); beforeEach(() => { - requestManager.newActiveRequest.mockResolvedValue(crypto.randomUUID()); + requestManager.newActiveRequest.mockResolvedValue({ + type: Fido2ActiveRequestEvents.Continue, + credentialId: crypto.randomUUID(), + }); authenticator.getAssertion.mockResolvedValue(createAuthenticatorAssertResult()); }); @@ -676,28 +683,6 @@ describe("FidoAuthenticatorService", () => { }; } }); - - describe("autofill of credentials through the active request manager", () => { - it("returns an observable that updates with an array of the credentials for active Fido2 requests", async () => { - const activeRequestCredentials = mock(); - activeRequest.credentials = [activeRequestCredentials]; - - const observable = client.availableAutofillCredentials$(tab.id); - observable.subscribe((credentials) => { - expect(credentials).toEqual([activeRequestCredentials]); - }); - }); - - it("triggers the logic of the next behavior subject of an active request", async () => { - const activeRequestCredentials = mock(); - activeRequest.credentials = [activeRequestCredentials]; - jest.spyOn(activeRequest.subject, "next"); - - await client.autofillCredential(tab.id, activeRequestCredentials.credentialId); - - expect(activeRequest.subject.next).toHaveBeenCalled(); - }); - }); }); /** This is a fake function that always returns the same byte sequence */ diff --git a/libs/common/src/platform/services/fido2/fido2-client.service.ts b/libs/common/src/platform/services/fido2/fido2-client.service.ts index 972d688912..849e0c7256 100644 --- a/libs/common/src/platform/services/fido2/fido2-client.service.ts +++ b/libs/common/src/platform/services/fido2/fido2-client.service.ts @@ -1,13 +1,15 @@ -import { firstValueFrom, map, Observable, Subscription } from "rxjs"; +import { firstValueFrom, Subscription } from "rxjs"; import { parse } from "tldts"; import { AuthService } from "../../../auth/abstractions/auth.service"; import { AuthenticationStatus } from "../../../auth/enums/authentication-status"; import { DomainSettingsService } from "../../../autofill/services/domain-settings.service"; import { VaultSettingsService } from "../../../vault/abstractions/vault-settings/vault-settings.service"; -import { Fido2CredentialView } from "../../../vault/models/view/fido2-credential.view"; import { ConfigService } from "../../abstractions/config/config.service"; -import { Fido2ActiveRequestManager } from "../../abstractions/fido2/fido2-active-request-manager.abstraction"; +import { + Fido2ActiveRequestEvents, + Fido2ActiveRequestManager, +} from "../../abstractions/fido2/fido2-active-request-manager.abstraction"; import { Fido2AuthenticatorError, Fido2AuthenticatorErrorCode, @@ -73,17 +75,6 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { ); } - availableAutofillCredentials$(tabId: number): Observable { - return this.requestManager - .getActiveRequest$(tabId) - .pipe(map((request) => request?.credentials ?? [])); - } - - async autofillCredential(tabId: number, credentialId: string) { - const request = this.requestManager.getActiveRequest(tabId); - request.subject.next(credentialId); - } - async isFido2FeatureEnabled(hostname: string, origin: string): Promise { const isUserLoggedIn = (await this.authService.getAuthStatus()) !== AuthenticationStatus.LoggedOut; @@ -385,12 +376,23 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { this.logService?.info( `[Fido2Client] started mediated request, available credentials: ${availableCredentials.length}`, ); - const credentialId = await this.requestManager.newActiveRequest( + const requestResult = await this.requestManager.newActiveRequest( tab.id, availableCredentials, abortController, ); - params.allowedCredentialIds = [Fido2Utils.bufferToString(guidToRawFormat(credentialId))]; + + if (requestResult.type === Fido2ActiveRequestEvents.Refresh) { + continue; + } + + if (requestResult.type === Fido2ActiveRequestEvents.Abort) { + break; + } + + params.allowedCredentialIds = [ + Fido2Utils.bufferToString(guidToRawFormat(requestResult.credentialId)), + ]; assumeUserPresence = true; const clientDataHash = await crypto.subtle.digest({ name: "SHA-256" }, clientDataJSONBytes);