From 3a1b56860e4e853b9a60034d03e5589b6006230a Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Thu, 20 Apr 2023 16:34:53 +0200 Subject: [PATCH] [EC-598] feat: add support for user verifiction using MP during assertion --- .../fido2/popup/fido2/fido2.component.html | 13 +------- .../src/fido2/popup/fido2/fido2.component.ts | 19 +++++------- .../browser-fido2-user-interface.service.ts | 31 ++++++------------- ...ido2-user-interface.service.abstraction.ts | 11 +++++-- .../fido2-authenticator.service.spec.ts | 31 ++++++++++++++----- .../services/fido2-authenticator.service.ts | 15 ++++++--- 6 files changed, 60 insertions(+), 60 deletions(-) diff --git a/apps/browser/src/fido2/popup/fido2/fido2.component.html b/apps/browser/src/fido2/popup/fido2/fido2.component.html index 09864a0c34..fcd4aa8621 100644 --- a/apps/browser/src/fido2/popup/fido2/fido2.component.html +++ b/apps/browser/src/fido2/popup/fido2/fido2.component.html @@ -1,17 +1,6 @@
- - A site is asking for authentication using the following credential: -
-
- -
-
- -
- + A passkey already exists in Bitwarden for this account diff --git a/apps/browser/src/fido2/popup/fido2/fido2.component.ts b/apps/browser/src/fido2/popup/fido2/fido2.component.ts index 919f293bc4..2e83b2fc54 100644 --- a/apps/browser/src/fido2/popup/fido2/fido2.component.ts +++ b/apps/browser/src/fido2/popup/fido2/fido2.component.ts @@ -77,9 +77,6 @@ export class Fido2Component implements OnInit, OnDestroy { cipher.fido2Key = new Fido2KeyView(); cipher.fido2Key.userName = data.userName; this.ciphers = [cipher]; - } else if (data?.type === "ConfirmCredentialRequest") { - const cipher = await this.cipherService.get(data.cipherId); - this.ciphers = [await cipher.decrypt()]; } else if (data?.type === "PickCredentialRequest") { this.ciphers = await Promise.all( data.cipherIds.map(async (cipherId) => { @@ -117,10 +114,16 @@ export class Fido2Component implements OnInit, OnDestroy { async pick(cipher: CipherView) { const data = this.data$.value; if (data?.type === "PickCredentialRequest") { + let userVerified = false; + if (data.userVerification) { + userVerified = await this.passwordRepromptService.showPasswordPrompt(); + } + this.send({ sessionId: this.sessionId, cipherId: cipher.id, type: "PickCredentialResponse", + userVerified, }); } else if (data?.type === "ConfirmNewNonDiscoverableCredentialRequest") { let userVerified = false; @@ -139,15 +142,7 @@ export class Fido2Component implements OnInit, OnDestroy { this.loading = true; } - confirm() { - this.send({ - sessionId: this.sessionId, - type: "ConfirmCredentialResponse", - }); - this.loading = true; - } - - async confirmNew() { + async confirm() { const data = this.data$.value; if (data.type !== "ConfirmNewCredentialRequest") { return; diff --git a/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts b/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts index 680d6e07f6..d420478a84 100644 --- a/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts +++ b/apps/browser/src/services/fido2/browser-fido2-user-interface.service.ts @@ -15,6 +15,7 @@ import { Fido2UserInterfaceService as Fido2UserInterfaceServiceAbstraction, Fido2UserInterfaceSession, NewCredentialParams, + PickCredentialParams, } from "@bitwarden/common/fido2/abstractions/fido2-user-interface.service.abstraction"; import { Utils } from "@bitwarden/common/misc/utils"; @@ -46,17 +47,12 @@ export type BrowserFido2Message = { sessionId: string } & ( | { type: "PickCredentialRequest"; cipherIds: string[]; + userVerification: boolean; } | { type: "PickCredentialResponse"; cipherId?: string; - } - | { - type: "ConfirmCredentialRequest"; - cipherId: string; - } - | { - type: "ConfirmCredentialResponse"; + userVerified: boolean; } | { type: "ConfirmNewCredentialRequest"; @@ -179,30 +175,21 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi return this.abortController.signal.aborted; } - async confirmCredential(cipherId: string): Promise { - const data: BrowserFido2Message = { - type: "ConfirmCredentialRequest", - cipherId, - sessionId: this.sessionId, - }; - - await this.send(data); - await this.receive("ConfirmCredentialResponse"); - - return true; - } - - async pickCredential(cipherIds: string[]): Promise { + async pickCredential({ + cipherIds, + userVerification, + }: PickCredentialParams): Promise<{ cipherId: string; userVerified: boolean }> { const data: BrowserFido2Message = { type: "PickCredentialRequest", cipherIds, sessionId: this.sessionId, + userVerification, }; await this.send(data); const response = await this.receive("PickCredentialResponse"); - return response.cipherId; + return { cipherId: response.cipherId, userVerified: response.userVerified }; } async confirmNewCredential({ diff --git a/libs/common/src/fido2/abstractions/fido2-user-interface.service.abstraction.ts b/libs/common/src/fido2/abstractions/fido2-user-interface.service.abstraction.ts index 97f6b2514e..11ab6439aa 100644 --- a/libs/common/src/fido2/abstractions/fido2-user-interface.service.abstraction.ts +++ b/libs/common/src/fido2/abstractions/fido2-user-interface.service.abstraction.ts @@ -4,6 +4,11 @@ export interface NewCredentialParams { userVerification: boolean; } +export interface PickCredentialParams { + cipherIds: string[]; + userVerification: boolean; +} + export abstract class Fido2UserInterfaceService { newSession: (abortController?: AbortController) => Promise; } @@ -12,8 +17,10 @@ export abstract class Fido2UserInterfaceSession { fallbackRequested = false; aborted = false; - confirmCredential: (cipherId: string, abortController?: AbortController) => Promise; - pickCredential: (cipherIds: string[], abortController?: AbortController) => Promise; + pickCredential: ( + params: PickCredentialParams, + abortController?: AbortController + ) => Promise<{ cipherId: string; userVerified: boolean }>; confirmNewCredential: ( params: NewCredentialParams, abortController?: AbortController diff --git a/libs/common/src/fido2/services/fido2-authenticator.service.spec.ts b/libs/common/src/fido2/services/fido2-authenticator.service.spec.ts index 89a32e4ee3..c36e052ca5 100644 --- a/libs/common/src/fido2/services/fido2-authenticator.service.spec.ts +++ b/libs/common/src/fido2/services/fido2-authenticator.service.spec.ts @@ -716,18 +716,30 @@ describe("FidoAuthenticatorService", () => { cipherService.getAllDecrypted.mockResolvedValue(ciphers); }); - /** Spec: Prompt the user to select a public key credential source selectedCredential from credentialOptions. */ - it("should request confirmation from the user", async () => { - userInterfaceSession.pickCredential.mockResolvedValue(ciphers[0].id); + for (const userVerification of [true, false]) { + /** Spec: Prompt the user to select a public key credential source selectedCredential from credentialOptions. */ + it(`should request confirmation from user when user verification is ${userVerification}`, async () => { + params.requireUserVerification = userVerification; + userInterfaceSession.pickCredential.mockResolvedValue({ + cipherId: ciphers[0].id, + userVerified: userVerification, + }); - await authenticator.getAssertion(params); + await authenticator.getAssertion(params); - expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith(ciphers.map((c) => c.id)); - }); + expect(userInterfaceSession.pickCredential).toHaveBeenCalledWith({ + cipherIds: ciphers.map((c) => c.id), + userVerification, + }); + }); + } /** Spec: If the user does not consent, return an error code equivalent to "NotAllowedError" and terminate the operation. */ it("should throw error", async () => { - userInterfaceSession.pickCredential.mockResolvedValue(undefined); + userInterfaceSession.pickCredential.mockResolvedValue({ + cipherId: undefined, + userVerified: false, + }); const result = async () => await authenticator.getAssertion(params); @@ -783,7 +795,10 @@ describe("FidoAuthenticatorService", () => { }); } cipherService.getAllDecrypted.mockResolvedValue(ciphers); - userInterfaceSession.pickCredential.mockResolvedValue(ciphers[0].id); + userInterfaceSession.pickCredential.mockResolvedValue({ + cipherId: ciphers[0].id, + userVerified: false, + }); }; beforeEach(init); diff --git a/libs/common/src/fido2/services/fido2-authenticator.service.ts b/libs/common/src/fido2/services/fido2-authenticator.service.ts index 51a30e21a6..b5b071caf7 100644 --- a/libs/common/src/fido2/services/fido2-authenticator.service.ts +++ b/libs/common/src/fido2/services/fido2-authenticator.service.ts @@ -202,15 +202,22 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.NotAllowed); } - const selectedCipherId = await userInterfaceSession.pickCredential( - cipherOptions.map((cipher) => cipher.id) - ); + const response = await userInterfaceSession.pickCredential({ + cipherIds: cipherOptions.map((cipher) => cipher.id), + userVerification: params.requireUserVerification, + }); + const selectedCipherId = response.cipherId; + const userVerified = response.userVerified; const selectedCipher = cipherOptions.find((c) => c.id === selectedCipherId); if (selectedCipher === undefined) { throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.NotAllowed); } + if (params.requireUserVerification && !userVerified) { + throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.NotAllowed); + } + try { const selectedFido2Key = selectedCipher.type === CipherType.Login @@ -235,7 +242,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr credentialId: Utils.guidToRawFormat(selectedCredentialId), counter: selectedFido2Key.counter, userPresence: true, - userVerification: false, + userVerification: userVerified, }); const signature = await generateSignature({