[PM-3905] Address PR feedback v2 (#6322)

* [PM-3905] chore: move webauthn utils to vault

* [PM-3905] chore: make static function private

* [PM-3905] chore: add documentation to user interface classes

* [PM-3905] chore: clean up unused abort controllers

* [PM-3905] chore: add documentation to fido2 client and authenticatio

* [PM-3905] chore: extract create credential params mapping to separate function

* [PM-3905] chore: extract get assertion params mapping to separate function

* [PM-3905] chore: assign requireResidentKey as separate variable

* [PM-3905] feat: started rewrite of messenger

Basic message sending implemented, now using message channels instead of rxjs

* [PM-3905] feat: complete rewrite of messenger

* [PM-3905] chore: clarify why we're assigning to window

* [PM-3905] feat: clean up tests

* [PM-3905] docs: document messenger class

* [PM-3905] feat: remove `requestId` which is no longer needed

* [PM-3905] feat: simplify message structure

* [PM-3905] chore: typo

* [PM-3905] chore: clean up old file

* [PM-3905] chore: tweak doc comment

* [PM-3905] feat: create separate class for managing aborts

* [PM-3905] chore: move abort manager to vault

apps/browser/src/background/runtime.background.ts

@@ -12,6 +12,7 @@ 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 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<string, AbortController>();
+  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;
-  }
 }

apps/browser/src/vault/background/abort-manager.ts

@@ -0,0 +1,21 @@
+type Runner<T> = (abortController: AbortController) => Promise<T>;
+
+/**
+ * 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<string, AbortController>();
+
+  runWithAbortController<T>(id: string, runner: Runner<T>): Promise<T> {
+    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();
+  }
+}

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 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/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) {

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,9 @@ describe("Messenger", () => {
   let handlerB: TestMessageHandler;
 
   beforeEach(() => {
+    // jest does not support MessageChannel
+    window.MessageChannel = MockMessageChannel as any;
+
     const channelPair = new TestChannelPair();
     messengerA = new Messenger(channelPair.channelA);
     messengerB = new Messenger(channelPair.channelB);
@@ -81,19 +82,16 @@ class TestChannelPair {
   readonly channelB: Channel;
 
   constructor() {
-    const subjectA = new Subject<MessageWithMetadata>();
-    const subjectB = new Subject<MessageWithMetadata>();
+    const broadcastChannel = new MockMessageChannel<MessageWithMetadata>();
 
     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),
     };
   }
 }
@@ -129,3 +127,28 @@ class TestMessageHandler {
     return received;
   }
 }
+
+class MockMessageChannel<T> {
+  port1 = new MockMessagePort<T>();
+  port2 = new MockMessagePort<T>();
+
+  constructor() {
+    this.port1.remotePort = this.port2;
+    this.port2.remotePort = this.port1;
+  }
+}
+
+class MockMessagePort<T> {
+  onmessage: ((ev: MessageEvent<T>) => any) | null;
+  remotePort: MockMessagePort<T>;
+
+  postMessage(message: T, port?: MessagePort) {
+    this.remotePort.onmessage(
+      new MessageEvent("message", { data: message, ports: port ? [port] : [] })
+    );
+  }
+
+  close() {
+    // Do nothing
+  }
+}