From 3c651be1a26f0da8a27d6dfb38dd73964e7e18ce Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 13:52:13 +0200 Subject: [PATCH 01/20] [PM-3905] chore: move webauthn utils to vault --- apps/browser/src/vault/fido2/content/page-script.ts | 2 +- apps/browser/src/{browser => vault/fido2}/webauthn-utils.ts | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename apps/browser/src/{browser => vault/fido2}/webauthn-utils.ts (100%) diff --git a/apps/browser/src/vault/fido2/content/page-script.ts b/apps/browser/src/vault/fido2/content/page-script.ts index 1194e0a4e4c3..103b959c59b4 100644 --- a/apps/browser/src/vault/fido2/content/page-script.ts +++ b/apps/browser/src/vault/fido2/content/page-script.ts @@ -1,6 +1,6 @@ import { FallbackRequestedError } from "@bitwarden/common/vault/abstractions/fido2/fido2-client.service.abstraction"; -import { WebauthnUtils } from "../../../browser/webauthn-utils"; +import { WebauthnUtils } from "../webauthn-utils"; import { MessageType } from "./messaging/message"; import { Messenger } from "./messaging/messenger"; diff --git a/apps/browser/src/browser/webauthn-utils.ts b/apps/browser/src/vault/fido2/webauthn-utils.ts similarity index 100% rename from apps/browser/src/browser/webauthn-utils.ts rename to apps/browser/src/vault/fido2/webauthn-utils.ts From 41cba008c491691d195c990a2650887aad51d9d7 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 13:56:34 +0200 Subject: [PATCH 02/20] [PM-3905] chore: make static function private --- libs/common/src/vault/services/fido2/fido2-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/common/src/vault/services/fido2/fido2-utils.ts b/libs/common/src/vault/services/fido2/fido2-utils.ts index 8d4c59c68875..a2de13755071 100644 --- a/libs/common/src/vault/services/fido2/fido2-utils.ts +++ b/libs/common/src/vault/services/fido2/fido2-utils.ts @@ -20,7 +20,7 @@ export class Fido2Utils { } /** Utility function to identify type of bufferSource. Necessary because of differences between runtimes */ - static isArrayBuffer(bufferSource: BufferSource): bufferSource is ArrayBuffer { + private static isArrayBuffer(bufferSource: BufferSource): bufferSource is ArrayBuffer { return bufferSource instanceof ArrayBuffer || bufferSource.buffer === undefined; } } From b502d8860d8081b5ccb7f92c9de0aae4878d5b6b Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 14:39:44 +0200 Subject: [PATCH 03/20] [PM-3905] chore: add documentation to user interface classes --- .../browser-fido2-user-interface.service.ts | 6 +- ...ido2-user-interface.service.abstraction.ts | 73 +++++++++++++++++++ .../noop-fido2-user-interface.service.ts | 9 ++- 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts b/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts index 549ca324afe0..a850c6ceee8e 100644 --- a/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts +++ b/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts @@ -43,7 +43,7 @@ export type BrowserFido2Message = { sessionId: string } & ( } /** * This message is used to announce the creation of a new session. - * It iss used by popouts to know when to close. + * It is used by popouts to know when to close. **/ | { type: "NewSessionCreatedRequest"; @@ -89,6 +89,10 @@ export type BrowserFido2Message = { sessionId: string } & ( } ); +/** + * Browser implementation of the {@link Fido2UserInterfaceService}. + * The user interface is implemented as a popout and the service uses the browser's messaging API to communicate with the it. + */ export class BrowserFido2UserInterfaceService implements Fido2UserInterfaceServiceAbstraction { constructor(private popupUtilsService: PopupUtilsService) {} diff --git a/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts b/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts index 8c2ef2bb28fe..1418fbe04e8a 100644 --- a/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts +++ b/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts @@ -1,38 +1,111 @@ +/** + * Parameters used to ask the user to confirm the creation of a new credential. + */ export interface NewCredentialParams { + /** + * The name of the credential. + */ credentialName: string; + + /** + * The name of the user. + */ userName: string; + + /** + * Whether or not the user must be verified before completing the operation. + */ userVerification: boolean; } +/** + * Parameters used to ask the user to pick a credential from a list of existing credentials. + */ export interface PickCredentialParams { + /** + * The IDs of the credentials that the user can pick from. + */ cipherIds: string[]; + + /** + * Whether or not the user must be verified before completing the operation. + */ userVerification: boolean; } +/** + * This service is used to provide a user interface with which the user can control FIDO2 operations. + * It acts as a way to remote control the user interface from the background script. + * + * The service is session based and is intended to be used by the FIDO2 authenticator to open a window, + * and then use this window to ask the user for input and/or display messages to the user. + */ export abstract class Fido2UserInterfaceService { + /** + * Creates a new session. + * Note: This will not necessarily open a window until it is needed to request something from the user. + * + * @param fallbackSupported Whether or not the browser natively supports WebAuthn. + * @param abortController An abort controller that can be used to cancel/close the session. + */ newSession: ( fallbackSupported: boolean, abortController?: AbortController ) => Promise; } +// TODO: Remove abortController from all the function signatures. export abstract class Fido2UserInterfaceSession { fallbackRequested = false; aborted = false; + /** + * Ask the user to pick a credential from a list of existing credentials. + * + * @param params The parameters to use when asking the user to pick a credential. + * @param abortController An abort controller that can be used to cancel/close the session. + * @returns The ID of the cipher that contains the credentials the user picked. + */ pickCredential: ( params: PickCredentialParams, abortController?: AbortController ) => Promise<{ cipherId: string; userVerified: boolean }>; + + /** + * Ask the user to confirm the creation of a new credential. + * + * @param params The parameters to use when asking the user to confirm the creation of a new credential. + * @param abortController An abort controller that can be used to cancel/close the session. + * @returns The ID of the cipher where the new credential should be saved. + */ confirmNewCredential: ( params: NewCredentialParams, abortController?: AbortController ) => Promise<{ cipherId: string; userVerified: boolean }>; + + /** + * Make sure that the vault is unlocked. + * This will open a window and ask the user to login or unlock the vault if necessary. + */ ensureUnlockedVault: () => Promise; + + /** + * Inform the user that the operation was cancelled because their vault contains excluded credentials. + * + * @param existingCipherIds The IDs of the excluded credentials. + */ informExcludedCredential: ( existingCipherIds: string[], abortController?: AbortController ) => Promise; + + /** + * Inform the user that the operation was cancelled because their vault does not contain any useable credentials. + */ informCredentialNotFound: (abortController?: AbortController) => Promise; + + /** + * Close the session, including any windows that may be open. + */ close: () => void; } diff --git a/libs/common/src/vault/services/fido2/noop-fido2-user-interface.service.ts b/libs/common/src/vault/services/fido2/noop-fido2-user-interface.service.ts index 4ed14831e228..440bd5190027 100644 --- a/libs/common/src/vault/services/fido2/noop-fido2-user-interface.service.ts +++ b/libs/common/src/vault/services/fido2/noop-fido2-user-interface.service.ts @@ -3,11 +3,12 @@ import { Fido2UserInterfaceSession, } from "../../abstractions/fido2/fido2-user-interface.service.abstraction"; +/** + * Noop implementation of the {@link Fido2UserInterfaceService}. + * This implementation does not provide any user interface. + */ export class Fido2UserInterfaceService implements Fido2UserInterfaceServiceAbstraction { - newSession( - fallbackSupported: boolean, - abortController?: AbortController - ): Promise { + newSession(): Promise { throw new Error("Not implemented exception"); } } From 44b3753b13d267d1a26474f02cb7e36c7f74b2ab Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 15:03:43 +0200 Subject: [PATCH 04/20] [PM-3905] chore: clean up unused abort controllers --- .../fido2/browser-fido2-user-interface.service.ts | 6 ------ .../fido2-user-interface.service.abstraction.ts | 15 +++------------ .../fido2/fido2-authenticator.service.spec.ts | 15 ++++++--------- .../services/fido2/fido2-authenticator.service.ts | 15 ++++++--------- 4 files changed, 15 insertions(+), 36 deletions(-) diff --git a/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts b/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts index a850c6ceee8e..50320cb04f43 100644 --- a/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts +++ b/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts @@ -192,12 +192,6 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi }); } - fallbackRequested = false; - - get aborted() { - return this.abortController.signal.aborted; - } - async pickCredential({ cipherIds, userVerification, diff --git a/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts b/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts index 1418fbe04e8a..f1e18b4a3a78 100644 --- a/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts +++ b/libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts @@ -54,11 +54,7 @@ export abstract class Fido2UserInterfaceService { ) => Promise; } -// TODO: Remove abortController from all the function signatures. export abstract class Fido2UserInterfaceSession { - fallbackRequested = false; - aborted = false; - /** * Ask the user to pick a credential from a list of existing credentials. * @@ -67,8 +63,7 @@ export abstract class Fido2UserInterfaceSession { * @returns The ID of the cipher that contains the credentials the user picked. */ pickCredential: ( - params: PickCredentialParams, - abortController?: AbortController + params: PickCredentialParams ) => Promise<{ cipherId: string; userVerified: boolean }>; /** @@ -79,8 +74,7 @@ export abstract class Fido2UserInterfaceSession { * @returns The ID of the cipher where the new credential should be saved. */ confirmNewCredential: ( - params: NewCredentialParams, - abortController?: AbortController + params: NewCredentialParams ) => Promise<{ cipherId: string; userVerified: boolean }>; /** @@ -94,10 +88,7 @@ export abstract class Fido2UserInterfaceSession { * * @param existingCipherIds The IDs of the excluded credentials. */ - informExcludedCredential: ( - existingCipherIds: string[], - abortController?: AbortController - ) => Promise; + informExcludedCredential: (existingCipherIds: string[]) => Promise; /** * Inform the user that the operation was cancelled because their vault does not contain any useable credentials. diff --git a/libs/common/src/vault/services/fido2/fido2-authenticator.service.spec.ts b/libs/common/src/vault/services/fido2/fido2-authenticator.service.spec.ts index 0e030630d02a..3b91760bf301 100644 --- a/libs/common/src/vault/services/fido2/fido2-authenticator.service.spec.ts +++ b/libs/common/src/vault/services/fido2/fido2-authenticator.service.spec.ts @@ -207,16 +207,13 @@ describe("FidoAuthenticatorService", () => { userVerified: userVerification, }); - await authenticator.makeCredential(params, new AbortController()); + await authenticator.makeCredential(params); - expect(userInterfaceSession.confirmNewCredential).toHaveBeenCalledWith( - { - credentialName: params.rpEntity.name, - userName: params.userEntity.displayName, - userVerification, - } as NewCredentialParams, - expect.anything() - ); + expect(userInterfaceSession.confirmNewCredential).toHaveBeenCalledWith({ + credentialName: params.rpEntity.name, + userName: params.userEntity.displayName, + userVerification, + } as NewCredentialParams); }); } diff --git a/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts b/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts index e322bf7e752f..2eba7bc8b980 100644 --- a/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts @@ -92,7 +92,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr this.logService?.info( `[Fido2Authenticator] Aborting due to excluded credential found in vault.` ); - await userInterfaceSession.informExcludedCredential(existingCipherIds, abortController); + await userInterfaceSession.informExcludedCredential(existingCipherIds); throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.NotAllowed); } @@ -101,14 +101,11 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr let keyPair: CryptoKeyPair; let userVerified = false; let credentialId: string; - const response = await userInterfaceSession.confirmNewCredential( - { - credentialName: params.rpEntity.name, - userName: params.userEntity.displayName, - userVerification: params.requireUserVerification, - }, - abortController - ); + const response = await userInterfaceSession.confirmNewCredential({ + credentialName: params.rpEntity.name, + userName: params.userEntity.displayName, + userVerification: params.requireUserVerification, + }); const cipherId = response.cipherId; userVerified = response.userVerified; From 8e2bb9d9e7df552c19f3e3c4a36f7ccb46225729 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 16:16:47 +0200 Subject: [PATCH 05/20] [PM-3905] chore: add documentation to fido2 client and authenticatio --- ...fido2-authenticator.service.abstraction.ts | 27 +++++- .../fido2/fido2-client.service.abstraction.ts | 82 +++++++++++++++++++ .../fido2/fido2-authenticator.service.ts | 5 +- .../services/fido2/fido2-client.service.ts | 6 ++ 4 files changed, 115 insertions(+), 5 deletions(-) diff --git a/libs/common/src/vault/abstractions/fido2/fido2-authenticator.service.abstraction.ts b/libs/common/src/vault/abstractions/fido2/fido2-authenticator.service.abstraction.ts index 521c332344ca..04ec7123afde 100644 --- a/libs/common/src/vault/abstractions/fido2/fido2-authenticator.service.abstraction.ts +++ b/libs/common/src/vault/abstractions/fido2/fido2-authenticator.service.abstraction.ts @@ -1,8 +1,17 @@ +/** + * This class represents an abstraction of the WebAuthn Authenticator model as described by W3C: + * https://www.w3.org/TR/webauthn-3/#sctn-authenticator-model + * + * The authenticator provides key management and cryptographic signatures. + */ export abstract class Fido2AuthenticatorService { /** - * Create and save a new credential + * Create and save a new credential as described in: + * https://www.w3.org/TR/webauthn-3/#sctn-op-make-cred * - * @return {Uint8Array} Attestation object + * @param params Parameters for creating a new credential + * @param abortController An AbortController that can be used to abort the operation. + * @returns A promise that resolves with the new credential and an attestation signature. **/ makeCredential: ( params: Fido2AuthenticatorMakeCredentialsParams, @@ -10,7 +19,12 @@ export abstract class Fido2AuthenticatorService { ) => Promise; /** - * Generate an assertion using an existing credential + * Generate an assertion using an existing credential as describe in: + * https://www.w3.org/TR/webauthn-3/#sctn-op-get-assertion + * + * @param params Parameters for generating an assertion + * @param abortController An AbortController that can be used to abort the operation. + * @returns A promise that resolves with the asserted credential and an assertion signature. */ getAssertion: ( params: Fido2AuthenticatorGetAssertionParams, @@ -46,7 +60,6 @@ export interface PublicKeyCredentialDescriptor { /** * Parameters for {@link Fido2AuthenticatorService.makeCredential} * - * @note * This interface represents the input parameters described in * https://www.w3.org/TR/webauthn-3/#sctn-op-make-cred */ @@ -97,6 +110,12 @@ export interface Fido2AuthenticatorMakeCredentialResult { publicKeyAlgorithm: number; } +/** + * Parameters for {@link Fido2AuthenticatorService.getAssertion} + + * This interface represents the input parameters described in + * https://www.w3.org/TR/webauthn-3/#sctn-op-get-assertion + */ export interface Fido2AuthenticatorGetAssertionParams { /** The caller’s RP ID, as determined by the user agent and the client. */ rpId: string; diff --git a/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts b/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts index f888b5d68929..a87168e3ebcf 100644 --- a/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts +++ b/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts @@ -2,53 +2,118 @@ export const UserRequestedFallbackAbortReason = "UserRequestedFallback"; export type UserVerification = "discouraged" | "preferred" | "required"; +/** + * This class represents an abstraction of the WebAuthn Client as described by W3C: + * https://www.w3.org/TR/webauthn-3/#webauthn-client + * + * The WebAuthn Client is an intermediary entity typically implemented in the user agent + * (in whole, or in part). Conceptually, it underlies the Web Authentication API and embodies + * the implementation of the Web Authentication API's operations. + * + * It is responsible for both marshalling the inputs for the underlying authenticator operations, + * and for returning the results of the latter operations to the Web Authentication API's callers. + */ export abstract class Fido2ClientService { + /** + * 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 + * + * @param params The parameters for the credential creation operation. + * @param abortController An AbortController that can be used to abort the operation. + * @returns A promise that resolves with the new credential. + */ createCredential: ( params: CreateCredentialParams, abortController?: AbortController ) => Promise; + + /** + * Allows WebAuthn Relying Party scripts to discover and use an existing public key credential, with the user’s consent. + * Relying Party script can optionally specify some criteria to indicate what credential sources are acceptable to it. + * For more information please see: https://www.w3.org/TR/webauthn-3/#sctn-getAssertion + * + * @param params The parameters for the credential assertion operation. + * @param abortController An AbortController that can be used to abort the operation. + * @returns A promise that resolves with the asserted credential. + */ assertCredential: ( params: AssertCredentialParams, abortController?: AbortController ) => Promise; + isFido2FeatureEnabled: () => Promise; } +/** + * Parameters for creating a new credential. + */ export interface CreateCredentialParams { + /** The Relaying Parties origin, see: https://html.spec.whatwg.org/multipage/browsers.html#concept-origin */ origin: string; + /** + * A value which is true if and only if the caller’s environment settings object is same-origin with its ancestors. + * It is false if caller is cross-origin. + * */ sameOriginWithAncestors: boolean; + /** The Relying Party's preference for attestation conveyance */ attestation?: "direct" | "enterprise" | "indirect" | "none"; + /** The Relying Party's requirements of the authenticator used in the creation of the credential. */ authenticatorSelection?: { // authenticatorAttachment?: AuthenticatorAttachment; // not used requireResidentKey?: boolean; residentKey?: "discouraged" | "preferred" | "required"; userVerification?: UserVerification; }; + /** Challenge intended to be used for generating the newly created credential's attestation object. */ challenge: string; // b64 encoded + /** + * This member is intended for use by Relying Parties that wish to limit the creation of multiple credentials for + * the same account on a single authenticator. The client is requested to return an error if the new credential would + * be created on an authenticator that also contains one of the credentials enumerated in this parameter. + * */ excludeCredentials?: { id: string; // b64 encoded transports?: ("ble" | "hybrid" | "internal" | "nfc" | "usb")[]; type: "public-key"; }[]; + /** + * This member contains additional parameters requesting additional processing by the client and authenticator. + * Not currently supported. + **/ extensions?: { appid?: string; appidExclude?: string; credProps?: boolean; uvm?: boolean; }; + /** + * This member contains information about the desired properties of the credential to be created. + * The sequence is ordered from most preferred to least preferred. + * The client makes a best-effort to create the most preferred credential that it can. + */ pubKeyCredParams: PublicKeyCredentialParam[]; + /** Data about the Relying Party responsible for the request. */ rp: { id?: string; name: string; }; + /** Data about the user account for which the Relying Party is requesting attestation. */ user: { id: string; // b64 encoded displayName: string; }; + /** Forwarded to user interface */ fallbackSupported: boolean; + /** + * This member specifies a time, in milliseconds, that the caller is willing to wait for the call to complete. + * This is treated as a hint, and MAY be overridden by the client. + **/ timeout?: number; } +/** + * The result of creating a new credential. + */ export interface CreateCredentialResult { credentialId: string; clientDataJSON: string; @@ -58,6 +123,9 @@ export interface CreateCredentialResult { transports: string[]; } +/** + * Parameters for asserting a credential. + */ export interface AssertCredentialParams { allowedCredentialIds: string[]; rpId: string; @@ -69,6 +137,9 @@ export interface AssertCredentialParams { fallbackSupported: boolean; } +/** + * The result of asserting a credential. + */ export interface AssertCredentialResult { credentialId: string; clientDataJSON: string; @@ -77,11 +148,22 @@ export interface AssertCredentialResult { userHandle: string; } +/** + * A description of a key type and algorithm. + * + * @example { + * alg: -7, // ES256 + * type: "public-key" + * } + */ export interface PublicKeyCredentialParam { alg: number; type: "public-key"; } +/** + * Error thrown when the user requests a fallback to the browser's built-in WebAuthn implementation. + */ export class FallbackRequestedError extends Error { readonly fallbackRequested = true; constructor() { diff --git a/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts b/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts index 2eba7bc8b980..42fb619a6156 100644 --- a/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-authenticator.service.ts @@ -31,8 +31,10 @@ export const AAGUID = new Uint8Array([ const KeyUsages: KeyUsage[] = ["sign"]; /** - * Bitwarden implementation of the WebAuthn Authenticator Model described by W3C + * Bitwarden implementation of the WebAuthn Authenticator Model as described by W3C * https://www.w3.org/TR/webauthn-3/#sctn-authenticator-model + * + * It is highly recommended that the W3C specification is used a reference when reading this code. */ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstraction { constructor( @@ -41,6 +43,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr private syncService: SyncService, private logService?: LogService ) {} + async makeCredential( params: Fido2AuthenticatorMakeCredentialsParams, abortController?: AbortController diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.ts b/libs/common/src/vault/services/fido2/fido2-client.service.ts index b9432dae14c4..ead8c4ab76f7 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.ts @@ -27,6 +27,12 @@ import { import { isValidRpId } from "./domain-utils"; import { Fido2Utils } from "./fido2-utils"; +/** + * Bitwarden implementation of the Web Authentication API as described by W3C + * https://www.w3.org/TR/webauthn-3/#sctn-api + * + * It is highly recommended that the W3C specification is used a reference when reading this code. + */ export class Fido2ClientService implements Fido2ClientServiceAbstraction { constructor( private authenticator: Fido2AuthenticatorService, From b71513a5ee52ca5202324cf6cb06173fe65231d9 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 16:26:59 +0200 Subject: [PATCH 06/20] [PM-3905] chore: extract create credential params mapping to separate function --- .../services/fido2/fido2-client.service.ts | 80 ++++++++++++------- 1 file changed, 52 insertions(+), 28 deletions(-) diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.ts b/libs/common/src/vault/services/fido2/fido2-client.service.ts index ead8c4ab76f7..be36488ebc72 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.ts @@ -87,10 +87,12 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { let credTypesAndPubKeyAlgs: PublicKeyCredentialParam[]; if (params.pubKeyCredParams?.length > 0) { + // Filter out all unsupported algorithms credTypesAndPubKeyAlgs = params.pubKeyCredParams.filter( (kp) => kp.alg === -7 && kp.type === "public-key" ); } else { + // Assign default algorithms credTypesAndPubKeyAlgs = [ { alg: -7, type: "public-key" }, { alg: -257, type: "public-key" }, @@ -115,6 +117,13 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { const clientDataJSON = JSON.stringify(collectedClientData); const clientDataJSONBytes = Utils.fromByteStringToArray(clientDataJSON); const clientDataHash = await crypto.subtle.digest({ name: "SHA-256" }, clientDataJSONBytes); + const makeCredentialParams = mapToMakeCredentialParams({ + params, + credTypesAndPubKeyAlgs, + clientDataHash, + }); + + // Set timeout before invoking authenticator if (abortController.signal.aborted) { this.logService?.info(`[Fido2Client] Aborted with AbortController`); throw new DOMException(undefined, "AbortError"); @@ -124,34 +133,7 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { params.authenticatorSelection?.userVerification, params.timeout ); - const excludeCredentialDescriptorList: PublicKeyCredentialDescriptor[] = - params.excludeCredentials?.map((credential) => ({ - id: Fido2Utils.stringToBuffer(credential.id), - transports: credential.transports, - type: credential.type, - })) ?? []; - - const makeCredentialParams: Fido2AuthenticatorMakeCredentialsParams = { - requireResidentKey: - params.authenticatorSelection?.residentKey === "required" || - params.authenticatorSelection?.residentKey === "preferred" || - (params.authenticatorSelection?.residentKey === undefined && - params.authenticatorSelection?.requireResidentKey === true), - requireUserVerification: params.authenticatorSelection?.userVerification === "required", - enterpriseAttestationPossible: params.attestation === "enterprise", - excludeCredentialDescriptorList, - credTypesAndPubKeyAlgs, - hash: clientDataHash, - rpEntity: { - id: rpId, - name: params.rp.name, - }, - userEntity: { - id: Fido2Utils.stringToBuffer(params.user.id), - displayName: params.user.displayName, - }, - fallbackSupported: params.fallbackSupported, - }; + let makeCredentialResult; try { makeCredentialResult = await this.authenticator.makeCredential( @@ -349,3 +331,45 @@ function setAbortTimeout( return window.setTimeout(() => abortController.abort(), clampedTimeout); } + +/** + * Convert data gathered by the WebAuthn Client to a format that can be used by the authenticator. + */ +function mapToMakeCredentialParams({ + params, + credTypesAndPubKeyAlgs, + clientDataHash, +}: { + params: CreateCredentialParams; + credTypesAndPubKeyAlgs: PublicKeyCredentialParam[]; + clientDataHash: ArrayBuffer; +}): Fido2AuthenticatorMakeCredentialsParams { + const excludeCredentialDescriptorList: PublicKeyCredentialDescriptor[] = + params.excludeCredentials?.map((credential) => ({ + id: Fido2Utils.stringToBuffer(credential.id), + transports: credential.transports, + type: credential.type, + })) ?? []; + + return { + requireResidentKey: + params.authenticatorSelection?.residentKey === "required" || + params.authenticatorSelection?.residentKey === "preferred" || + (params.authenticatorSelection?.residentKey === undefined && + params.authenticatorSelection?.requireResidentKey === true), + requireUserVerification: params.authenticatorSelection?.userVerification === "required", + enterpriseAttestationPossible: params.attestation === "enterprise", + excludeCredentialDescriptorList, + credTypesAndPubKeyAlgs, + hash: clientDataHash, + rpEntity: { + id: params.rp.id, + name: params.rp.name, + }, + userEntity: { + id: Fido2Utils.stringToBuffer(params.user.id), + displayName: params.user.displayName, + }, + fallbackSupported: params.fallbackSupported, + }; +} From 5d275da4b4a0b4c635434a98c2a0f0fff3f276c5 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 16:39:54 +0200 Subject: [PATCH 07/20] [PM-3905] chore: extract get assertion params mapping to separate function --- .../services/fido2/fido2-client.service.ts | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.ts b/libs/common/src/vault/services/fido2/fido2-client.service.ts index be36488ebc72..2807e64ec011 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.ts @@ -226,6 +226,7 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { const clientDataJSON = JSON.stringify(collectedClientData); const clientDataJSONBytes = Utils.fromByteStringToArray(clientDataJSON); const clientDataHash = await crypto.subtle.digest({ name: "SHA-256" }, clientDataJSONBytes); + const getAssertionParams = mapToGetAssertionParams({ params, clientDataHash }); if (abortController.signal.aborted) { this.logService?.info(`[Fido2Client] Aborted with AbortController`); @@ -234,21 +235,6 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { const timeout = setAbortTimeout(abortController, params.userVerification, params.timeout); - const allowCredentialDescriptorList: PublicKeyCredentialDescriptor[] = - params.allowedCredentialIds.map((id) => ({ - id: Fido2Utils.stringToBuffer(id), - type: "public-key", - })); - - const getAssertionParams: Fido2AuthenticatorGetAssertionParams = { - rpId, - requireUserVerification: params.userVerification === "required", - hash: clientDataHash, - allowCredentialDescriptorList, - extensions: {}, - fallbackSupported: params.fallbackSupported, - }; - let getAssertionResult; try { getAssertionResult = await this.authenticator.getAssertion( @@ -373,3 +359,29 @@ function mapToMakeCredentialParams({ fallbackSupported: params.fallbackSupported, }; } + +/** + * Convert data gathered by the WebAuthn Client to a format that can be used by the authenticator. + */ +function mapToGetAssertionParams({ + params, + clientDataHash, +}: { + params: AssertCredentialParams; + clientDataHash: ArrayBuffer; +}): Fido2AuthenticatorGetAssertionParams { + const allowCredentialDescriptorList: PublicKeyCredentialDescriptor[] = + params.allowedCredentialIds.map((id) => ({ + id: Fido2Utils.stringToBuffer(id), + type: "public-key", + })); + + return { + rpId: params.rpId, + requireUserVerification: params.userVerification === "required", + hash: clientDataHash, + allowCredentialDescriptorList, + extensions: {}, + fallbackSupported: params.fallbackSupported, + }; +} From 565e05032a40401e9de024d387cb5d0cd2b0b547 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Mon, 18 Sep 2023 16:49:28 +0200 Subject: [PATCH 08/20] [PM-3905] chore: assign requireResidentKey as separate variable --- .../src/vault/services/fido2/fido2-client.service.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.ts b/libs/common/src/vault/services/fido2/fido2-client.service.ts index 2807e64ec011..0057e0c0c087 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.ts @@ -337,12 +337,14 @@ function mapToMakeCredentialParams({ type: credential.type, })) ?? []; + const requireResidentKey = + params.authenticatorSelection?.residentKey === "required" || + params.authenticatorSelection?.residentKey === "preferred" || + (params.authenticatorSelection?.residentKey === undefined && + params.authenticatorSelection?.requireResidentKey === true); + return { - requireResidentKey: - params.authenticatorSelection?.residentKey === "required" || - params.authenticatorSelection?.residentKey === "preferred" || - (params.authenticatorSelection?.residentKey === undefined && - params.authenticatorSelection?.requireResidentKey === true), + requireResidentKey, requireUserVerification: params.authenticatorSelection?.userVerification === "required", enterpriseAttestationPossible: params.attestation === "enterprise", excludeCredentialDescriptorList, From 1944d02b4c4d9404fdb0cf6035e0a08523775267 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 11:44:39 +0200 Subject: [PATCH 09/20] [PM-3905] feat: started rewrite of messenger Basic message sending implemented, now using message channels instead of rxjs --- .../fido2/content/messaging/messenger.bak.ts | 174 +++++++++++++++++ .../fido2/content/messaging/messenger.spec.ts | 66 ++++++- .../fido2/content/messaging/messenger.ts | 179 +++++++++++------- 3 files changed, 345 insertions(+), 74 deletions(-) create mode 100644 apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts new file mode 100644 index 000000000000..383efb1ebf16 --- /dev/null +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts @@ -0,0 +1,174 @@ +// import { concatMap, filter, firstValueFrom, Observable } from "rxjs"; + +import { Message, MessageType } from "./message"; + +const SENDER = "bitwarden-webauthn"; + +type PostMessageFunction = (message: MessageWithMetadata, remotePort: MessagePort) => void; + +export type Channel = { + // messages$: Observable; + addEventListener: (listener: (message: MessageEvent) => void) => void; + postMessage: PostMessageFunction; +}; + +export type Metadata = { SENDER: typeof SENDER; requestId: string }; +export type MessageWithMetadata = Message & { metadata: Metadata }; +type Handler = ( + message: MessageWithMetadata, + abortController?: AbortController +) => Promise; + +// TODO: This class probably duplicates functionality but I'm not especially familiar with +// the inner workings of the browser extension yet. +// If you see this in a code review please comment on it! + +export class Messenger { + static forDOMCommunication(window: Window) { + const windowOrigin = window.location.origin; + + return new Messenger({ + postMessage: (message, port) => window.postMessage(message, windowOrigin, [port]), + addEventListener: (listener) => + window.addEventListener("message", (event: MessageEvent) => { + if (event.origin !== windowOrigin) { + return; + } + + listener(event as MessageEvent); + }), + // messages$: new Observable((subscriber) => { + // const eventListener = (event: MessageEvent) => { + // if (event.origin !== windowOrigin) { + // return; + // } + + // subscriber.next(event.data); + // }; + + // window.addEventListener("message", eventListener); + + // return () => window.removeEventListener("message", eventListener); + // }), + }); + } + + handler?: Handler; + private abortControllers = new Map(); + + constructor(private broadcastChannel: Channel) { + this.broadcastChannel.addEventListener(async (event) => { + const abortController = new AbortController(); + const message = event.data; + const port = event.ports[0]; + + const handlerResponse = await this.handler(message, abortController); + if (handlerResponse === undefined) { + return; + } + + const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + port.postMessage({ ...handlerResponse, metadata }); + }); + + // this.channel.messages$ + // .pipe( + // filter((message) => message?.metadata?.SENDER === SENDER), + // concatMap(async (message) => { + // if (this.handler === undefined) { + // return; + // } + // const abortController = new AbortController(); + // this.abortControllers.set(message.metadata.requestId, abortController); + // try { + // const handlerResponse = await this.handler(message, abortController); + // if (handlerResponse === undefined) { + // return; + // } + // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + // this.channel.postMessage({ ...handlerResponse, metadata }); + // } catch (error) { + // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + // this.channel.postMessage({ + // type: MessageType.ErrorResponse, + // metadata, + // error: JSON.stringify(error, Object.getOwnPropertyNames(error)), + // }); + // } finally { + // this.abortControllers.delete(message.metadata.requestId); + // } + // }) + // ) + // .subscribe(); + // this.channel.messages$.subscribe((message) => { + // if (message.type !== MessageType.AbortRequest) { + // return; + // } + // this.abortControllers.get(message.abortedRequestId)?.abort(); + // }); + } + + async request(request: Message, abortController?: AbortController): Promise { + const requestId = Date.now().toString(); + const metadata: Metadata = { SENDER, requestId }; + + const requestChannel = new MessageChannel(); + const { port1: localPort, port2: remotePort } = requestChannel; + + const promise = new Promise((resolve) => { + localPort.onmessage = (event: MessageEvent) => resolve(event.data); + }); + + this.broadcastChannel.postMessage({ ...request, metadata }, remotePort); + const response = await promise; + + if (response.type === MessageType.ErrorResponse) { + const error = new Error(); + Object.assign(error, JSON.parse(response.error)); + throw error; + } + + return response; + + // const promise = new Promise((resolve, reject) => { + // const eventListener = (event: MessageEvent) => { + // const message = event.data; + // if (event.origin !== window.location.origin || message == null || message.metadata?.requestId !== requestId) { + // return; + // } + + // resolve(message); + // }; + + // localPort.addEventListener("message", eventListener); + // }); + // const promise = firstValueFrom( + // this.channel.messages$.pipe( + // filter( + // (m) => m != undefined && m.metadata?.requestId === requestId && m.type !== request.type + // ) + // ) + // ); + + // const abortListener = () => + // this.channel.postMessage({ + // metadata: { SENDER, requestId: `${requestId}-abort` }, + // type: MessageType.AbortRequest, + // abortedRequestId: requestId, + // }); + // abortController?.signal.addEventListener("abort", abortListener); + + // this.channel.postMessage({ ...request, metadata }, remotePort); + + // const response = await promise; + // abortController?.signal.removeEventListener("abort", abortListener); + + // if (response.type === MessageType.ErrorResponse) { + // const error = new Error(); + // Object.assign(error, JSON.parse(response.error)); + // throw error; + // } + + // return response; + } +} diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts index 6d3ffe50effb..a3ada14a56da 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts @@ -1,5 +1,3 @@ -import { Subject } from "rxjs"; - import { Utils } from "@bitwarden/common/platform/misc/utils"; import { Message } from "./message"; @@ -12,6 +10,8 @@ describe("Messenger", () => { let handlerB: TestMessageHandler; beforeEach(() => { + window.MessageChannel = MockMessageChannel as any; + const channelPair = new TestChannelPair(); messengerA = new Messenger(channelPair.channelA); messengerB = new Messenger(channelPair.channelB); @@ -81,20 +81,45 @@ class TestChannelPair { readonly channelB: Channel; constructor() { - const subjectA = new Subject(); - const subjectB = new Subject(); + const broadcastChannel = new MockMessageChannel(); this.channelA = { - messages$: subjectA, - postMessage: (message) => { - subjectB.next(message); - }, + addEventListener: (listener) => (broadcastChannel.port1.onmessage = listener), + postMessage: (message, port) => broadcastChannel.port1.postMessage(message, port), }; this.channelB = { - messages$: subjectB, - postMessage: (message) => subjectA.next(message), + addEventListener: (listener) => (broadcastChannel.port2.onmessage = listener), + postMessage: (message, port) => broadcastChannel.port2.postMessage(message, port), }; + + // this.channelB = { + // addEventListener: (listener) => (broadcastChannel.port2.onmessage = listener), + // postMessage: (message, port) => broadcastChannel.port1.postMessage(message, port), + // }; + + // const subjectA = new Subject<{ message: MessageWithMetadata; port: MessagePort }>(); + // const subjectB = new Subject<{ message: MessageWithMetadata; port: MessagePort }>(); + + // this.channelA = { + // addEventListener: (listener) => + // subjectA.subscribe(({ message, port }) => { + // listener(new MessageEvent("message", { data: message, ports: [port] })); + // }), + // postMessage: (message, port) => { + // subjectB.next({ message, port }); + // }, + // }; + + // this.channelB = { + // addEventListener: (listener) => + // subjectB.subscribe(({ message, port }) => { + // listener(new MessageEvent("message", { data: message, ports: [port] })); + // }), + // postMessage: (message, port) => { + // subjectA.next({ message, port }); + // }, + // }; } } @@ -129,3 +154,24 @@ class TestMessageHandler { return received; } } + +class MockMessageChannel { + port1 = new MockMessagePort(); + port2 = new MockMessagePort(); + + constructor() { + this.port1.remotePort = this.port2; + this.port2.remotePort = this.port1; + } +} + +class MockMessagePort { + onmessage: ((ev: MessageEvent) => any) | null; + remotePort: MockMessagePort; + + postMessage(message: T, port?: MessagePort) { + this.remotePort.onmessage( + new MessageEvent("message", { data: message, ports: port ? [port] : [] }) + ); + } +} diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.ts index 103a0c23191f..6b6d3530e6b8 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.ts @@ -1,13 +1,14 @@ -import { concatMap, filter, firstValueFrom, Observable } from "rxjs"; +// import { concatMap, filter, firstValueFrom, Observable } from "rxjs"; import { Message, MessageType } from "./message"; const SENDER = "bitwarden-webauthn"; -type PostMessageFunction = (message: MessageWithMetadata) => void; +type PostMessageFunction = (message: MessageWithMetadata, remotePort: MessagePort) => void; export type Channel = { - messages$: Observable; + // messages$: Observable; + addEventListener: (listener: (message: MessageEvent) => void) => void; postMessage: PostMessageFunction; }; @@ -27,94 +28,103 @@ export class Messenger { const windowOrigin = window.location.origin; return new Messenger({ - postMessage: (message) => window.postMessage(message, windowOrigin), - messages$: new Observable((subscriber) => { - const eventListener = (event: MessageEvent) => { + postMessage: (message, port) => window.postMessage(message, windowOrigin, [port]), + addEventListener: (listener) => + window.addEventListener("message", (event: MessageEvent) => { if (event.origin !== windowOrigin) { return; } - subscriber.next(event.data); - }; + listener(event as MessageEvent); + }), + // messages$: new Observable((subscriber) => { + // const eventListener = (event: MessageEvent) => { + // if (event.origin !== windowOrigin) { + // return; + // } - window.addEventListener("message", eventListener); + // subscriber.next(event.data); + // }; - return () => window.removeEventListener("message", eventListener); - }), + // window.addEventListener("message", eventListener); + + // return () => window.removeEventListener("message", eventListener); + // }), }); } handler?: Handler; private abortControllers = new Map(); - constructor(private channel: Channel) { - this.channel.messages$ - .pipe( - filter((message) => message?.metadata?.SENDER === SENDER), - concatMap(async (message) => { - if (this.handler === undefined) { - return; - } + constructor(private broadcastChannel: Channel) { + this.broadcastChannel.addEventListener(async (event) => { + if (this.handler === undefined) { + return; + } - const abortController = new AbortController(); - this.abortControllers.set(message.metadata.requestId, abortController); - - try { - const handlerResponse = await this.handler(message, abortController); - - if (handlerResponse === undefined) { - return; - } - - const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - this.channel.postMessage({ ...handlerResponse, metadata }); - } catch (error) { - const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - this.channel.postMessage({ - type: MessageType.ErrorResponse, - metadata, - error: JSON.stringify(error, Object.getOwnPropertyNames(error)), - }); - } finally { - this.abortControllers.delete(message.metadata.requestId); - } - }) - ) - .subscribe(); + const abortController = new AbortController(); + const message = event.data; + const port = event.ports[0]; - this.channel.messages$.subscribe((message) => { - if (message.type !== MessageType.AbortRequest) { + const handlerResponse = await this.handler(message, abortController); + if (handlerResponse === undefined) { return; } - this.abortControllers.get(message.abortedRequestId)?.abort(); + const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + port.postMessage({ ...handlerResponse, metadata }); }); + + // this.channel.messages$ + // .pipe( + // filter((message) => message?.metadata?.SENDER === SENDER), + // concatMap(async (message) => { + // if (this.handler === undefined) { + // return; + // } + // const abortController = new AbortController(); + // this.abortControllers.set(message.metadata.requestId, abortController); + // try { + // const handlerResponse = await this.handler(message, abortController); + // if (handlerResponse === undefined) { + // return; + // } + // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + // this.channel.postMessage({ ...handlerResponse, metadata }); + // } catch (error) { + // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + // this.channel.postMessage({ + // type: MessageType.ErrorResponse, + // metadata, + // error: JSON.stringify(error, Object.getOwnPropertyNames(error)), + // }); + // } finally { + // this.abortControllers.delete(message.metadata.requestId); + // } + // }) + // ) + // .subscribe(); + // this.channel.messages$.subscribe((message) => { + // if (message.type !== MessageType.AbortRequest) { + // return; + // } + // this.abortControllers.get(message.abortedRequestId)?.abort(); + // }); } async request(request: Message, abortController?: AbortController): Promise { const requestId = Date.now().toString(); const metadata: Metadata = { SENDER, requestId }; - const promise = firstValueFrom( - this.channel.messages$.pipe( - filter( - (m) => m != undefined && m.metadata?.requestId === requestId && m.type !== request.type - ) - ) - ); - - const abortListener = () => - this.channel.postMessage({ - metadata: { SENDER, requestId: `${requestId}-abort` }, - type: MessageType.AbortRequest, - abortedRequestId: requestId, - }); - abortController?.signal.addEventListener("abort", abortListener); + const requestChannel = new MessageChannel(); + const { port1: localPort, port2: remotePort } = requestChannel; - this.channel.postMessage({ ...request, metadata }); + const promise = new Promise((resolve) => { + localPort.onmessage = (event: MessageEvent) => resolve(event.data); + }); + this.broadcastChannel.postMessage({ ...request, metadata }, remotePort); const response = await promise; - abortController?.signal.removeEventListener("abort", abortListener); if (response.type === MessageType.ErrorResponse) { const error = new Error(); @@ -123,5 +133,46 @@ export class Messenger { } return response; + + // const promise = new Promise((resolve, reject) => { + // const eventListener = (event: MessageEvent) => { + // const message = event.data; + // if (event.origin !== window.location.origin || message == null || message.metadata?.requestId !== requestId) { + // return; + // } + + // resolve(message); + // }; + + // localPort.addEventListener("message", eventListener); + // }); + // const promise = firstValueFrom( + // this.channel.messages$.pipe( + // filter( + // (m) => m != undefined && m.metadata?.requestId === requestId && m.type !== request.type + // ) + // ) + // ); + + // const abortListener = () => + // this.channel.postMessage({ + // metadata: { SENDER, requestId: `${requestId}-abort` }, + // type: MessageType.AbortRequest, + // abortedRequestId: requestId, + // }); + // abortController?.signal.addEventListener("abort", abortListener); + + // this.channel.postMessage({ ...request, metadata }, remotePort); + + // const response = await promise; + // abortController?.signal.removeEventListener("abort", abortListener); + + // if (response.type === MessageType.ErrorResponse) { + // const error = new Error(); + // Object.assign(error, JSON.parse(response.error)); + // throw error; + // } + + // return response; } } From 638334729198aa12130798775e8580d196c05903 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 13:38:32 +0200 Subject: [PATCH 10/20] [PM-3905] feat: complete rewrite of messenger --- .../fido2/content/messaging/messenger.spec.ts | 4 + .../fido2/content/messaging/messenger.ts | 135 +++++------------- 2 files changed, 37 insertions(+), 102 deletions(-) diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts index a3ada14a56da..93b9569929ac 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts @@ -174,4 +174,8 @@ class MockMessagePort { new MessageEvent("message", { data: message, ports: port ? [port] : [] }) ); } + + close() { + // Do nothing + } } diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.ts index 6b6d3530e6b8..07bddcdea87f 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.ts @@ -1,5 +1,3 @@ -// import { concatMap, filter, firstValueFrom, Observable } from "rxjs"; - import { Message, MessageType } from "./message"; const SENDER = "bitwarden-webauthn"; @@ -7,7 +5,6 @@ const SENDER = "bitwarden-webauthn"; type PostMessageFunction = (message: MessageWithMetadata, remotePort: MessagePort) => void; export type Channel = { - // messages$: Observable; addEventListener: (listener: (message: MessageEvent) => void) => void; postMessage: PostMessageFunction; }; @@ -37,79 +34,44 @@ export class Messenger { listener(event as MessageEvent); }), - // messages$: new Observable((subscriber) => { - // const eventListener = (event: MessageEvent) => { - // if (event.origin !== windowOrigin) { - // return; - // } - - // subscriber.next(event.data); - // }; - - // window.addEventListener("message", eventListener); - - // return () => window.removeEventListener("message", eventListener); - // }), }); } handler?: Handler; - private abortControllers = new Map(); - constructor(private broadcastChannel: Channel) { this.broadcastChannel.addEventListener(async (event) => { if (this.handler === undefined) { return; } - const abortController = new AbortController(); const message = event.data; - const port = event.ports[0]; - - const handlerResponse = await this.handler(message, abortController); - if (handlerResponse === undefined) { + const port = event.ports?.[0]; + if (message?.metadata?.SENDER !== SENDER || message == null || port == null) { return; } - const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - port.postMessage({ ...handlerResponse, metadata }); + const abortController = new AbortController(); + port.onmessage = (event: MessageEvent) => { + if (event.data.type === MessageType.AbortRequest) { + abortController.abort(); + } + }; + + try { + const handlerResponse = await this.handler(message, abortController); + const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + port.postMessage({ ...handlerResponse, metadata }); + } catch (error) { + const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + port.postMessage({ + type: MessageType.ErrorResponse, + metadata, + error: JSON.stringify(error, Object.getOwnPropertyNames(error)), + }); + } finally { + port.close(); + } }); - - // this.channel.messages$ - // .pipe( - // filter((message) => message?.metadata?.SENDER === SENDER), - // concatMap(async (message) => { - // if (this.handler === undefined) { - // return; - // } - // const abortController = new AbortController(); - // this.abortControllers.set(message.metadata.requestId, abortController); - // try { - // const handlerResponse = await this.handler(message, abortController); - // if (handlerResponse === undefined) { - // return; - // } - // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - // this.channel.postMessage({ ...handlerResponse, metadata }); - // } catch (error) { - // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - // this.channel.postMessage({ - // type: MessageType.ErrorResponse, - // metadata, - // error: JSON.stringify(error, Object.getOwnPropertyNames(error)), - // }); - // } finally { - // this.abortControllers.delete(message.metadata.requestId); - // } - // }) - // ) - // .subscribe(); - // this.channel.messages$.subscribe((message) => { - // if (message.type !== MessageType.AbortRequest) { - // return; - // } - // this.abortControllers.get(message.abortedRequestId)?.abort(); - // }); } async request(request: Message, abortController?: AbortController): Promise { @@ -123,9 +85,19 @@ export class Messenger { localPort.onmessage = (event: MessageEvent) => resolve(event.data); }); + const abortListener = () => + localPort.postMessage({ + metadata: { SENDER, requestId: `${requestId}-abort` }, + type: MessageType.AbortRequest, + abortedRequestId: requestId, + }); + abortController?.signal.addEventListener("abort", abortListener); + this.broadcastChannel.postMessage({ ...request, metadata }, remotePort); const response = await promise; + abortController?.signal.removeEventListener("abort", abortListener); + if (response.type === MessageType.ErrorResponse) { const error = new Error(); Object.assign(error, JSON.parse(response.error)); @@ -133,46 +105,5 @@ export class Messenger { } return response; - - // const promise = new Promise((resolve, reject) => { - // const eventListener = (event: MessageEvent) => { - // const message = event.data; - // if (event.origin !== window.location.origin || message == null || message.metadata?.requestId !== requestId) { - // return; - // } - - // resolve(message); - // }; - - // localPort.addEventListener("message", eventListener); - // }); - // const promise = firstValueFrom( - // this.channel.messages$.pipe( - // filter( - // (m) => m != undefined && m.metadata?.requestId === requestId && m.type !== request.type - // ) - // ) - // ); - - // const abortListener = () => - // this.channel.postMessage({ - // metadata: { SENDER, requestId: `${requestId}-abort` }, - // type: MessageType.AbortRequest, - // abortedRequestId: requestId, - // }); - // abortController?.signal.addEventListener("abort", abortListener); - - // this.channel.postMessage({ ...request, metadata }, remotePort); - - // const response = await promise; - // abortController?.signal.removeEventListener("abort", abortListener); - - // if (response.type === MessageType.ErrorResponse) { - // const error = new Error(); - // Object.assign(error, JSON.parse(response.error)); - // throw error; - // } - - // return response; } } From ef24a5a2ac94300faa43767d871837d28620c528 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 13:52:21 +0200 Subject: [PATCH 11/20] [PM-3905] chore: clarify why we're assigning to window --- apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts index 93b9569929ac..641d4e330aa6 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts @@ -10,6 +10,7 @@ describe("Messenger", () => { let handlerB: TestMessageHandler; beforeEach(() => { + // jest does not support MessageChannel window.MessageChannel = MockMessageChannel as any; const channelPair = new TestChannelPair(); From a7eef6717db4418a9a631578d2892e7f3f5c5b13 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 13:53:20 +0200 Subject: [PATCH 12/20] [PM-3905] feat: clean up tests --- .../fido2/content/messaging/messenger.spec.ts | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts index 641d4e330aa6..505682d997d8 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.spec.ts @@ -93,34 +93,6 @@ class TestChannelPair { addEventListener: (listener) => (broadcastChannel.port2.onmessage = listener), postMessage: (message, port) => broadcastChannel.port2.postMessage(message, port), }; - - // this.channelB = { - // addEventListener: (listener) => (broadcastChannel.port2.onmessage = listener), - // postMessage: (message, port) => broadcastChannel.port1.postMessage(message, port), - // }; - - // const subjectA = new Subject<{ message: MessageWithMetadata; port: MessagePort }>(); - // const subjectB = new Subject<{ message: MessageWithMetadata; port: MessagePort }>(); - - // this.channelA = { - // addEventListener: (listener) => - // subjectA.subscribe(({ message, port }) => { - // listener(new MessageEvent("message", { data: message, ports: [port] })); - // }), - // postMessage: (message, port) => { - // subjectB.next({ message, port }); - // }, - // }; - - // this.channelB = { - // addEventListener: (listener) => - // subjectB.subscribe(({ message, port }) => { - // listener(new MessageEvent("message", { data: message, ports: [port] })); - // }), - // postMessage: (message, port) => { - // subjectA.next({ message, port }); - // }, - // }; } } From ea7cb185d277cc8ff4347012b5bb404071641299 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 14:20:52 +0200 Subject: [PATCH 13/20] [PM-3905] docs: document messenger class --- .../fido2/content/messaging/messenger.ts | 73 +++++++++++++------ 1 file changed, 50 insertions(+), 23 deletions(-) diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.ts index 07bddcdea87f..eba3a2bf6b89 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.ts @@ -16,11 +16,20 @@ type Handler = ( abortController?: AbortController ) => Promise; -// TODO: This class probably duplicates functionality but I'm not especially familiar with -// the inner workings of the browser extension yet. -// If you see this in a code review please comment on it! - +/** + * A class that handles communication between the page and content script. It converts + * the browser's broadcasting API into a request/response API with support for seamlessly + * handling aborts and exceptions across separate execution contexts. + */ export class Messenger { + /** + * Creates a messenger that uses the browser's `window.postMessage` API to initiate + * requests in the content script. Every request will then create it's own + * `MessageChannel` through which all subsequent communication will be sent through. + * + * @param window the window object to use for communication + * @returns a `Messenger` instance + */ static forDOMCommunication(window: Window) { const windowOrigin = window.location.origin; @@ -37,7 +46,13 @@ export class Messenger { }); } + /** + * The handler that will be called when a message is recieved. The handler should return + * a promise that resolves to the response message. If the handler throws an error, the + * error will be sent back to the sender. + */ handler?: Handler; + constructor(private broadcastChannel: Channel) { this.broadcastChannel.addEventListener(async (event) => { if (this.handler === undefined) { @@ -74,6 +89,14 @@ export class Messenger { }); } + /** + * Sends a request to the content script and returns the response. If the request is + * aborted, the request will be aborted in the content script as well. + * + * @param request data to send to the content script + * @param abortController the abort controller that might be used to abort the request + * @returns the response from the content script + */ async request(request: Message, abortController?: AbortController): Promise { const requestId = Date.now().toString(); const metadata: Metadata = { SENDER, requestId }; @@ -81,29 +104,33 @@ export class Messenger { const requestChannel = new MessageChannel(); const { port1: localPort, port2: remotePort } = requestChannel; - const promise = new Promise((resolve) => { - localPort.onmessage = (event: MessageEvent) => resolve(event.data); - }); - - const abortListener = () => - localPort.postMessage({ - metadata: { SENDER, requestId: `${requestId}-abort` }, - type: MessageType.AbortRequest, - abortedRequestId: requestId, + try { + const promise = new Promise((resolve) => { + localPort.onmessage = (event: MessageEvent) => resolve(event.data); }); - abortController?.signal.addEventListener("abort", abortListener); - this.broadcastChannel.postMessage({ ...request, metadata }, remotePort); - const response = await promise; + const abortListener = () => + localPort.postMessage({ + metadata: { SENDER, requestId: `${requestId}-abort` }, + type: MessageType.AbortRequest, + abortedRequestId: requestId, + }); + abortController?.signal.addEventListener("abort", abortListener); - abortController?.signal.removeEventListener("abort", abortListener); + this.broadcastChannel.postMessage({ ...request, metadata }, remotePort); + const response = await promise; - if (response.type === MessageType.ErrorResponse) { - const error = new Error(); - Object.assign(error, JSON.parse(response.error)); - throw error; - } + abortController?.signal.removeEventListener("abort", abortListener); + + if (response.type === MessageType.ErrorResponse) { + const error = new Error(); + Object.assign(error, JSON.parse(response.error)); + throw error; + } - return response; + return response; + } finally { + localPort.close(); + } } } From a1e6d16f11bcfc5427798c9b9240d3333ecee86d Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 14:40:03 +0200 Subject: [PATCH 14/20] [PM-3905] feat: remove `requestId` which is no longer needed --- .../src/vault/fido2/content/content-script.ts | 7 ++++--- .../src/vault/fido2/content/messaging/messenger.ts | 12 +++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/apps/browser/src/vault/fido2/content/content-script.ts b/apps/browser/src/vault/fido2/content/content-script.ts index eb266aff4d07..bf147c4b58fa 100644 --- a/apps/browser/src/vault/fido2/content/content-script.ts +++ b/apps/browser/src/vault/fido2/content/content-script.ts @@ -20,10 +20,11 @@ function initializeFido2ContentScript(isFido2FeatureEnabled: boolean) { const messenger = Messenger.forDOMCommunication(window); messenger.handler = async (message, abortController) => { + const requestId = Date.now().toString(); const abortHandler = () => chrome.runtime.sendMessage({ command: "fido2AbortRequest", - abortedRequestId: message.metadata.requestId, + abortedRequestId: requestId, }); abortController.signal.addEventListener("abort", abortHandler); @@ -33,7 +34,7 @@ function initializeFido2ContentScript(isFido2FeatureEnabled: boolean) { { command: "fido2RegisterCredentialRequest", data: message.data, - requestId: message.metadata.requestId, + requestId: requestId, }, (response) => { if (response.error !== undefined) { @@ -55,7 +56,7 @@ function initializeFido2ContentScript(isFido2FeatureEnabled: boolean) { { command: "fido2GetCredentialRequest", data: message.data, - requestId: message.metadata.requestId, + requestId: requestId, }, (response) => { if (response.error !== undefined) { diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.ts index eba3a2bf6b89..5f0f6e835f78 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.ts @@ -9,7 +9,7 @@ export type Channel = { postMessage: PostMessageFunction; }; -export type Metadata = { SENDER: typeof SENDER; requestId: string }; +export type Metadata = { SENDER: typeof SENDER }; export type MessageWithMetadata = Message & { metadata: Metadata }; type Handler = ( message: MessageWithMetadata, @@ -74,10 +74,10 @@ export class Messenger { try { const handlerResponse = await this.handler(message, abortController); - const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + const metadata: Metadata = { SENDER }; port.postMessage({ ...handlerResponse, metadata }); } catch (error) { - const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; + const metadata: Metadata = { SENDER }; port.postMessage({ type: MessageType.ErrorResponse, metadata, @@ -98,8 +98,7 @@ export class Messenger { * @returns the response from the content script */ async request(request: Message, abortController?: AbortController): Promise { - const requestId = Date.now().toString(); - const metadata: Metadata = { SENDER, requestId }; + const metadata: Metadata = { SENDER }; const requestChannel = new MessageChannel(); const { port1: localPort, port2: remotePort } = requestChannel; @@ -111,9 +110,8 @@ export class Messenger { const abortListener = () => localPort.postMessage({ - metadata: { SENDER, requestId: `${requestId}-abort` }, + metadata: { SENDER }, type: MessageType.AbortRequest, - abortedRequestId: requestId, }); abortController?.signal.addEventListener("abort", abortListener); From 3b34b244e5e637a17e6ed189a1fbc121b6ca570b Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 14:43:05 +0200 Subject: [PATCH 15/20] [PM-3905] feat: simplify message structure --- .../src/vault/fido2/content/messaging/messenger.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.ts index 5f0f6e835f78..a02a3b70205a 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.ts @@ -10,7 +10,7 @@ export type Channel = { }; export type Metadata = { SENDER: typeof SENDER }; -export type MessageWithMetadata = Message & { metadata: Metadata }; +export type MessageWithMetadata = Message & Metadata; type Handler = ( message: MessageWithMetadata, abortController?: AbortController @@ -61,7 +61,7 @@ export class Messenger { const message = event.data; const port = event.ports?.[0]; - if (message?.metadata?.SENDER !== SENDER || message == null || port == null) { + if (message?.SENDER !== SENDER || message == null || port == null) { return; } @@ -74,13 +74,11 @@ export class Messenger { try { const handlerResponse = await this.handler(message, abortController); - const metadata: Metadata = { SENDER }; - port.postMessage({ ...handlerResponse, metadata }); + port.postMessage({ ...handlerResponse, SENDER }); } catch (error) { - const metadata: Metadata = { SENDER }; port.postMessage({ + SENDER, type: MessageType.ErrorResponse, - metadata, error: JSON.stringify(error, Object.getOwnPropertyNames(error)), }); } finally { @@ -98,8 +96,6 @@ export class Messenger { * @returns the response from the content script */ async request(request: Message, abortController?: AbortController): Promise { - const metadata: Metadata = { SENDER }; - const requestChannel = new MessageChannel(); const { port1: localPort, port2: remotePort } = requestChannel; @@ -115,7 +111,7 @@ export class Messenger { }); abortController?.signal.addEventListener("abort", abortListener); - this.broadcastChannel.postMessage({ ...request, metadata }, remotePort); + this.broadcastChannel.postMessage({ ...request, SENDER }, remotePort); const response = await promise; abortController?.signal.removeEventListener("abort", abortListener); From da032784f7c3efd19b98ad33e37eaf68716627c4 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 14:46:08 +0200 Subject: [PATCH 16/20] [PM-3905] chore: typo --- .../src/vault/fido2/browser-fido2-user-interface.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts b/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts index 50320cb04f43..272c3d44d454 100644 --- a/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts +++ b/apps/browser/src/vault/fido2/browser-fido2-user-interface.service.ts @@ -91,7 +91,7 @@ export type BrowserFido2Message = { sessionId: string } & ( /** * Browser implementation of the {@link Fido2UserInterfaceService}. - * The user interface is implemented as a popout and the service uses the browser's messaging API to communicate with the it. + * The user interface is implemented as a popout and the service uses the browser's messaging API to communicate with it. */ export class BrowserFido2UserInterfaceService implements Fido2UserInterfaceServiceAbstraction { constructor(private popupUtilsService: PopupUtilsService) {} From b710fe0ad66e156de64e38fe6b0c2d01f10b7c36 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 14:47:11 +0200 Subject: [PATCH 17/20] [PM-3905] chore: clean up old file --- .../fido2/content/messaging/messenger.bak.ts | 174 ------------------ 1 file changed, 174 deletions(-) delete mode 100644 apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts deleted file mode 100644 index 383efb1ebf16..000000000000 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.bak.ts +++ /dev/null @@ -1,174 +0,0 @@ -// import { concatMap, filter, firstValueFrom, Observable } from "rxjs"; - -import { Message, MessageType } from "./message"; - -const SENDER = "bitwarden-webauthn"; - -type PostMessageFunction = (message: MessageWithMetadata, remotePort: MessagePort) => void; - -export type Channel = { - // messages$: Observable; - addEventListener: (listener: (message: MessageEvent) => void) => void; - postMessage: PostMessageFunction; -}; - -export type Metadata = { SENDER: typeof SENDER; requestId: string }; -export type MessageWithMetadata = Message & { metadata: Metadata }; -type Handler = ( - message: MessageWithMetadata, - abortController?: AbortController -) => Promise; - -// TODO: This class probably duplicates functionality but I'm not especially familiar with -// the inner workings of the browser extension yet. -// If you see this in a code review please comment on it! - -export class Messenger { - static forDOMCommunication(window: Window) { - const windowOrigin = window.location.origin; - - return new Messenger({ - postMessage: (message, port) => window.postMessage(message, windowOrigin, [port]), - addEventListener: (listener) => - window.addEventListener("message", (event: MessageEvent) => { - if (event.origin !== windowOrigin) { - return; - } - - listener(event as MessageEvent); - }), - // messages$: new Observable((subscriber) => { - // const eventListener = (event: MessageEvent) => { - // if (event.origin !== windowOrigin) { - // return; - // } - - // subscriber.next(event.data); - // }; - - // window.addEventListener("message", eventListener); - - // return () => window.removeEventListener("message", eventListener); - // }), - }); - } - - handler?: Handler; - private abortControllers = new Map(); - - constructor(private broadcastChannel: Channel) { - this.broadcastChannel.addEventListener(async (event) => { - const abortController = new AbortController(); - const message = event.data; - const port = event.ports[0]; - - const handlerResponse = await this.handler(message, abortController); - if (handlerResponse === undefined) { - return; - } - - const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - port.postMessage({ ...handlerResponse, metadata }); - }); - - // this.channel.messages$ - // .pipe( - // filter((message) => message?.metadata?.SENDER === SENDER), - // concatMap(async (message) => { - // if (this.handler === undefined) { - // return; - // } - // const abortController = new AbortController(); - // this.abortControllers.set(message.metadata.requestId, abortController); - // try { - // const handlerResponse = await this.handler(message, abortController); - // if (handlerResponse === undefined) { - // return; - // } - // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - // this.channel.postMessage({ ...handlerResponse, metadata }); - // } catch (error) { - // const metadata: Metadata = { SENDER, requestId: message.metadata.requestId }; - // this.channel.postMessage({ - // type: MessageType.ErrorResponse, - // metadata, - // error: JSON.stringify(error, Object.getOwnPropertyNames(error)), - // }); - // } finally { - // this.abortControllers.delete(message.metadata.requestId); - // } - // }) - // ) - // .subscribe(); - // this.channel.messages$.subscribe((message) => { - // if (message.type !== MessageType.AbortRequest) { - // return; - // } - // this.abortControllers.get(message.abortedRequestId)?.abort(); - // }); - } - - async request(request: Message, abortController?: AbortController): Promise { - const requestId = Date.now().toString(); - const metadata: Metadata = { SENDER, requestId }; - - const requestChannel = new MessageChannel(); - const { port1: localPort, port2: remotePort } = requestChannel; - - const promise = new Promise((resolve) => { - localPort.onmessage = (event: MessageEvent) => resolve(event.data); - }); - - this.broadcastChannel.postMessage({ ...request, metadata }, remotePort); - const response = await promise; - - if (response.type === MessageType.ErrorResponse) { - const error = new Error(); - Object.assign(error, JSON.parse(response.error)); - throw error; - } - - return response; - - // const promise = new Promise((resolve, reject) => { - // const eventListener = (event: MessageEvent) => { - // const message = event.data; - // if (event.origin !== window.location.origin || message == null || message.metadata?.requestId !== requestId) { - // return; - // } - - // resolve(message); - // }; - - // localPort.addEventListener("message", eventListener); - // }); - // const promise = firstValueFrom( - // this.channel.messages$.pipe( - // filter( - // (m) => m != undefined && m.metadata?.requestId === requestId && m.type !== request.type - // ) - // ) - // ); - - // const abortListener = () => - // this.channel.postMessage({ - // metadata: { SENDER, requestId: `${requestId}-abort` }, - // type: MessageType.AbortRequest, - // abortedRequestId: requestId, - // }); - // abortController?.signal.addEventListener("abort", abortListener); - - // this.channel.postMessage({ ...request, metadata }, remotePort); - - // const response = await promise; - // abortController?.signal.removeEventListener("abort", abortListener); - - // if (response.type === MessageType.ErrorResponse) { - // const error = new Error(); - // Object.assign(error, JSON.parse(response.error)); - // throw error; - // } - - // return response; - } -} From be077eab6ebf4b2e906953283dafac1347ee0dec Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 14:50:43 +0200 Subject: [PATCH 18/20] [PM-3905] chore: tweak doc comment --- apps/browser/src/vault/fido2/content/messaging/messenger.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/browser/src/vault/fido2/content/messaging/messenger.ts b/apps/browser/src/vault/fido2/content/messaging/messenger.ts index a02a3b70205a..aeb835e2d5f7 100644 --- a/apps/browser/src/vault/fido2/content/messaging/messenger.ts +++ b/apps/browser/src/vault/fido2/content/messaging/messenger.ts @@ -88,8 +88,8 @@ export class Messenger { } /** - * Sends a request to the content script and returns the response. If the request is - * aborted, the request will be aborted in the content script as well. + * Sends a request to the content script and returns the response. + * AbortController signals will be forwarded to the content script. * * @param request data to send to the content script * @param abortController the abort controller that might be used to abort the request From e4e016e1f488e4f0b4ddfe266a1851f79766ed73 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 20 Sep 2023 15:16:52 +0200 Subject: [PATCH 19/20] [PM-3905] feat: create separate class for managing aborts --- apps/browser/src/background/abort-manager.ts | 21 +++++++++++++++++ .../src/background/runtime.background.ts | 23 ++++++++----------- 2 files changed, 30 insertions(+), 14 deletions(-) create mode 100644 apps/browser/src/background/abort-manager.ts diff --git a/apps/browser/src/background/abort-manager.ts b/apps/browser/src/background/abort-manager.ts new file mode 100644 index 000000000000..8e61ca7a7b47 --- /dev/null +++ b/apps/browser/src/background/abort-manager.ts @@ -0,0 +1,21 @@ +type Runner = (abortController: AbortController) => Promise; + +/** + * Manages abort controllers for long running tasks and allow separate + * execution contexts to abort each other by using ids. + */ +export class AbortManager { + private abortControllers = new Map(); + + runWithAbortController(id: string, runner: Runner): Promise { + const abortController = new AbortController(); + this.abortControllers.set(id, abortController); + return runner(abortController).finally(() => { + this.abortControllers.delete(id); + }); + } + + abort(id: string) { + this.abortControllers.get(id)?.abort(); + } +} diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index 2abd4662d1a6..c73c84e76bb6 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -13,6 +13,7 @@ import { BrowserPopoutWindowService } from "../platform/popup/abstractions/brows import { BrowserEnvironmentService } from "../platform/services/browser-environment.service"; import BrowserPlatformUtilsService from "../platform/services/browser-platform-utils.service"; +import { AbortManager } from "./abort-manager"; import MainBackground from "./main.background"; import LockedVaultPendingNotificationsItem from "./models/lockedVaultPendingNotificationsItem"; @@ -21,7 +22,7 @@ export default class RuntimeBackground { private pageDetailsToAutoFill: any[] = []; private onInstalledReason: string = null; private lockedVaultPendingNotifications: LockedVaultPendingNotificationsItem[] = []; - private abortControllers = new Map(); + private abortManager = new AbortManager(); constructor( private main: MainBackground, @@ -253,18 +254,18 @@ export default class RuntimeBackground { this.platformUtilsService.copyToClipboard(msg.identifier, { window: window }); break; case "fido2AbortRequest": - this.abortControllers.get(msg.abortedRequestId)?.abort(); + this.abortManager.abort(msg.abortedRequestId); break; case "checkFido2FeatureEnabled": return await this.main.fido2ClientService.isFido2FeatureEnabled(); case "fido2RegisterCredentialRequest": - return await this.main.fido2ClientService - .createCredential(msg.data, this.createAbortController(msg.requestId)) - .finally(() => this.abortControllers.delete(msg.requestId)); + return await this.abortManager.runWithAbortController(msg.requestId, (abortController) => + this.main.fido2ClientService.createCredential(msg.data, abortController) + ); case "fido2GetCredentialRequest": - return await this.main.fido2ClientService - .assertCredential(msg.data, this.createAbortController(msg.requestId)) - .finally(() => this.abortControllers.delete(msg.requestId)); + return await this.abortManager.runWithAbortController(msg.requestId, (abortController) => + this.main.fido2ClientService.assertCredential(msg.data, abortController) + ); } } @@ -301,10 +302,4 @@ export default class RuntimeBackground { } }, 100); } - - private createAbortController(id: string): AbortController { - const abortController = new AbortController(); - this.abortControllers.set(id, abortController); - return abortController; - } } From 5d6968a20ba868ce780cccd2d273a40a4ec48563 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Thu, 21 Sep 2023 10:25:36 +0200 Subject: [PATCH 20/20] [PM-3905] chore: move abort manager to vault --- apps/browser/src/background/runtime.background.ts | 2 +- apps/browser/src/{ => vault}/background/abort-manager.ts | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename apps/browser/src/{ => vault}/background/abort-manager.ts (100%) diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index c73c84e76bb6..2b1814372a04 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -12,8 +12,8 @@ import { BrowserApi } from "../platform/browser/browser-api"; import { BrowserPopoutWindowService } from "../platform/popup/abstractions/browser-popout-window.service"; import { BrowserEnvironmentService } from "../platform/services/browser-environment.service"; import BrowserPlatformUtilsService from "../platform/services/browser-platform-utils.service"; +import { AbortManager } from "../vault/background/abort-manager"; -import { AbortManager } from "./abort-manager"; import MainBackground from "./main.background"; import LockedVaultPendingNotificationsItem from "./models/lockedVaultPendingNotificationsItem"; diff --git a/apps/browser/src/background/abort-manager.ts b/apps/browser/src/vault/background/abort-manager.ts similarity index 100% rename from apps/browser/src/background/abort-manager.ts rename to apps/browser/src/vault/background/abort-manager.ts