[PM-3905] Address PR feedback v2

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) {}
 
@@ -188,12 +192,6 @@ export class BrowserFido2UserInterfaceSession implements Fido2UserInterfaceSessi
     });
   }
 
-  fallbackRequested = false;
-
-  get aborted() {
-    return this.abortController.signal.aborted;
-  }
-
   async pickCredential({
     cipherIds,
     userVerification,

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";

libs/common/src/vault/abstractions/fido2/fido2-authenticator.service.abstraction.ts

@@ -1,16 +1,30 @@
+/**
+ * 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,
     abortController?: AbortController
   ) => Promise<Fido2AuthenticatorMakeCredentialResult>;
 
   /**
-   * 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;

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<CreateCredentialResult>;
+
+  /**
+   * 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<AssertCredentialResult>;
+
   isFido2FeatureEnabled: () => Promise<boolean>;
 }
 
+/**
+ * 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() {

libs/common/src/vault/abstractions/fido2/fido2-user-interface.service.abstraction.ts

@@ -1,38 +1,102 @@
+/**
+ * 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<Fido2UserInterfaceSession>;
 }
 
 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
+    params: PickCredentialParams
   ) => 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
+    params: NewCredentialParams
   ) => 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<void>;
-  informExcludedCredential: (
-    existingCipherIds: string[],
-    abortController?: AbortController
-  ) => Promise<void>;
+
+  /**
+   * 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[]) => Promise<void>;
+
+  /**
+   * Inform the user that the operation was cancelled because their vault does not contain any useable credentials.
+   */
   informCredentialNotFound: (abortController?: AbortController) => Promise<void>;
+
+  /**
+   * Close the session, including any windows that may be open.
+   */
   close: () => void;
 }

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);
         });
       }