Bug 1877790 - give new webauthn requests priority over pending requests. r=keeler

Previously, a new webauthn request would only have priority over a pending
request if there had been a visibility change. This was an arbitrary design
decision, and was inconsequential before conditional mediation was implemented.

Differential Revision: https://phabricator.services.mozilla.com/D200349

dom/webauthn/WebAuthnManager.cpp

@@ -203,10 +203,6 @@ nsresult RelaxSameOrigin(nsPIDOMWindowInner* aParent,
  **********************************************************************/
 
 void WebAuthnManager::ClearTransaction() {
-  if (mTransaction.isSome()) {
-    StopListeningForVisibilityEvents();
-  }
-
   mTransaction.reset();
   Unfollow();
 }
@@ -217,12 +213,6 @@ void WebAuthnManager::CancelParent() {
   }
 }
 
-void WebAuthnManager::HandleVisibilityChange() {
-  if (mTransaction.isSome()) {
-    mTransaction.ref().mVisibilityChanged = true;
-  }
-}
-
 WebAuthnManager::~WebAuthnManager() {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -250,15 +240,6 @@ already_AddRefed<Promise> WebAuthnManager::MakeCredential(
   }
 
   if (mTransaction.isSome()) {
-    // If there hasn't been a visibility change during the current
-    // transaction, then let's let that one complete rather than
-    // cancelling it on a subsequent call.
-    if (!mTransaction.ref().mVisibilityChanged) {
-      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
-      return promise.forget();
-    }
-
-    // Otherwise, the user may well have clicked away, so let's
     // abort the old transaction and take over control from here.
     CancelTransaction(NS_ERROR_DOM_ABORT_ERR);
   }
@@ -484,19 +465,11 @@ already_AddRefed<Promise> WebAuthnManager::MakeCredential(
       adjustedTimeout, excludeList, rpInfo, userInfo, coseAlgos, extensions,
       authSelection, attestation, context->Top()->Id());
 
-  // Set up the transaction state (including event listeners, etc). Fallible
-  // operations should not be performed below this line, as we must not leave
-  // the transaction state partially initialized. Once the transaction state is
-  // initialized the only valid ways to end the transaction are
-  // CancelTransaction, RejectTransaction, and FinishMakeCredential.
-#ifdef XP_WIN
-  if (!WinWebAuthnService::AreWebAuthNApisAvailable()) {
-    ListenForVisibilityEvents();
-  }
-#else
-  ListenForVisibilityEvents();
-#endif
-
+  // Set up the transaction state. Fallible operations should not be performed
+  // below this line, as we must not leave the transaction state partially
+  // initialized. Once the transaction state is initialized the only valid ways
+  // to end the transaction are CancelTransaction, RejectTransaction, and
+  // FinishMakeCredential.
   AbortSignal* signal = nullptr;
   if (aSignal.WasPassed()) {
     signal = &aSignal.Value();
@@ -526,15 +499,6 @@ already_AddRefed<Promise> WebAuthnManager::GetAssertion(
   }
 
   if (mTransaction.isSome()) {
-    // If there hasn't been a visibility change during the current
-    // transaction, then let's let that one complete rather than
-    // cancelling it on a subsequent call.
-    if (!mTransaction.ref().mVisibilityChanged) {
-      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
-      return promise.forget();
-    }
-
-    // Otherwise, the user may well have clicked away, so let's
     // abort the old transaction and take over control from here.
     CancelTransaction(NS_ERROR_DOM_ABORT_ERR);
   }
@@ -679,19 +643,11 @@ already_AddRefed<Promise> WebAuthnManager::GetAssertion(
                                 extensions, aOptions.mUserVerification,
                                 aConditionallyMediated, context->Top()->Id());
 
-  // Set up the transaction state (including event listeners, etc). Fallible
-  // operations should not be performed below this line, as we must not leave
-  // the transaction state partially initialized. Once the transaction state is
-  // initialized the only valid ways to end the transaction are
-  // CancelTransaction, RejectTransaction, and FinishGetAssertion.
-#ifdef XP_WIN
-  if (!WinWebAuthnService::AreWebAuthNApisAvailable()) {
-    ListenForVisibilityEvents();
-  }
-#else
-  ListenForVisibilityEvents();
-#endif
-
+  // Set up the transaction state. Fallible operations should not be performed
+  // below this line, as we must not leave the transaction state partially
+  // initialized. Once the transaction state is initialized the only valid ways
+  // to end the transaction are CancelTransaction, RejectTransaction, and
+  // FinishGetAssertion.
   AbortSignal* signal = nullptr;
   if (aSignal.WasPassed()) {
     signal = &aSignal.Value();
@@ -717,15 +673,6 @@ already_AddRefed<Promise> WebAuthnManager::Store(const Credential& aCredential,
   }
 
   if (mTransaction.isSome()) {
-    // If there hasn't been a visibility change during the current
-    // transaction, then let's let that one complete rather than
-    // cancelling it on a subsequent call.
-    if (!mTransaction.ref().mVisibilityChanged) {
-      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
-      return promise.forget();
-    }
-
-    // Otherwise, the user may well have clicked away, so let's
     // abort the old transaction and take over control from here.
     CancelTransaction(NS_ERROR_DOM_ABORT_ERR);
   }

dom/webauthn/WebAuthnManager.h

@@ -51,7 +51,7 @@ class Credential;
 class WebAuthnTransaction {
  public:
   explicit WebAuthnTransaction(const RefPtr<Promise>& aPromise)
-      : mPromise(aPromise), mId(NextId()), mVisibilityChanged(false) {
+      : mPromise(aPromise), mId(NextId()) {
     MOZ_ASSERT(mId > 0);
   }
 
@@ -61,10 +61,6 @@ class WebAuthnTransaction {
   // Unique transaction id.
   uint64_t mId;
 
-  // Whether or not visibility has changed for the window during this
-  // transaction
-  bool mVisibilityChanged;
-
  private:
   // Generates a probabilistically unique ID for the new transaction. IDs are 53
   // bits, as they are used in javascript. We use a random value if possible,
@@ -117,10 +113,6 @@ class WebAuthnManager final : public WebAuthnManagerBase, public AbortFollower {
 
   void RunAbortAlgorithm() override;
 
- protected:
-  // Upon a visibility change, makes note of it in the current transaction.
-  void HandleVisibilityChange() override;
-
  private:
   virtual ~WebAuthnManager();
 

dom/webauthn/WebAuthnManagerBase.cpp

@@ -5,19 +5,12 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/dom/WebAuthnManagerBase.h"
-#include "mozilla/dom/Document.h"
 #include "mozilla/dom/WebAuthnTransactionChild.h"
 #include "mozilla/ipc/BackgroundChild.h"
 #include "mozilla/ipc/PBackgroundChild.h"
-#include "mozilla/dom/Event.h"
-#include "nsGlobalWindowInner.h"
-#include "nsPIWindowRoot.h"
 
 namespace mozilla::dom {
 
-constexpr auto kDeactivateEvent = u"deactivate"_ns;
-constexpr auto kVisibilityChange = u"visibilitychange"_ns;
-
 WebAuthnManagerBase::WebAuthnManagerBase(nsPIDOMWindowInner* aParent)
     : mParent(aParent) {
   MOZ_ASSERT(NS_IsMainThread());
@@ -28,7 +21,6 @@ WebAuthnManagerBase::~WebAuthnManagerBase() { MOZ_ASSERT(NS_IsMainThread()); }
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(WebAuthnManagerBase)
   NS_INTERFACE_MAP_ENTRY(nsISupports)
-  NS_INTERFACE_MAP_ENTRY(nsIDOMEventListener)
 NS_INTERFACE_MAP_END
 
 NS_IMPL_CYCLE_COLLECTION(WebAuthnManagerBase, mParent)
@@ -72,80 +64,4 @@ void WebAuthnManagerBase::ActorDestroyed() {
   mChild = nullptr;
 }
 
-/***********************************************************************
- * Event Handling
- **********************************************************************/
-
-void WebAuthnManagerBase::ListenForVisibilityEvents() {
-  MOZ_ASSERT(NS_IsMainThread());
-
-  nsCOMPtr<nsPIDOMWindowOuter> outer = mParent->GetOuterWindow();
-  if (NS_WARN_IF(!outer)) {
-    return;
-  }
-
-  nsCOMPtr<EventTarget> windowRoot = outer->GetTopWindowRoot();
-  if (NS_WARN_IF(!windowRoot)) {
-    return;
-  }
-
-  nsresult rv = windowRoot->AddEventListener(kDeactivateEvent, this,
-                                             /* use capture */ true,
-                                             /* wants untrusted */ false);
-  Unused << NS_WARN_IF(NS_FAILED(rv));
-
-  rv = windowRoot->AddEventListener(kVisibilityChange, this,
-                                    /* use capture */ true,
-                                    /* wants untrusted */ false);
-  Unused << NS_WARN_IF(NS_FAILED(rv));
-}
-
-void WebAuthnManagerBase::StopListeningForVisibilityEvents() {
-  MOZ_ASSERT(NS_IsMainThread());
-
-  nsCOMPtr<nsPIDOMWindowOuter> outer = mParent->GetOuterWindow();
-  if (NS_WARN_IF(!outer)) {
-    return;
-  }
-
-  nsCOMPtr<EventTarget> windowRoot = outer->GetTopWindowRoot();
-  if (NS_WARN_IF(!windowRoot)) {
-    return;
-  }
-
-  windowRoot->RemoveEventListener(kDeactivateEvent, this,
-                                  /* use capture */ true);
-  windowRoot->RemoveEventListener(kVisibilityChange, this,
-                                  /* use capture */ true);
-}
-
-NS_IMETHODIMP
-WebAuthnManagerBase::HandleEvent(Event* aEvent) {
-  MOZ_ASSERT(NS_IsMainThread());
-  MOZ_ASSERT(aEvent);
-
-  nsAutoString type;
-  aEvent->GetType(type);
-  if (!type.Equals(kDeactivateEvent) && !type.Equals(kVisibilityChange)) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // The "deactivate" event on the root window has no
-  // "current inner window" and thus GetTarget() is always null.
-  if (type.Equals(kVisibilityChange)) {
-    nsCOMPtr<Document> doc = do_QueryInterface(aEvent->GetTarget());
-    if (NS_WARN_IF(!doc) || !doc->Hidden()) {
-      return NS_OK;
-    }
-
-    nsGlobalWindowInner* win = nsGlobalWindowInner::Cast(doc->GetInnerWindow());
-    if (NS_WARN_IF(!win) || !win->IsTopInnerWindow()) {
-      return NS_OK;
-    }
-  }
-
-  HandleVisibilityChange();
-  return NS_OK;
-}
-
 }  // namespace mozilla::dom

dom/webauthn/WebAuthnManagerBase.h

@@ -7,7 +7,6 @@
 #ifndef mozilla_dom_WebAuthnManagerBase_h
 #define mozilla_dom_WebAuthnManagerBase_h
 
-#include "nsIDOMEventListener.h"
 #include "nsCycleCollectionParticipant.h"
 #include "nsCOMPtr.h"
 
@@ -24,10 +23,8 @@ class WebAuthnTransactionChild;
 class WebAuthnMakeCredentialResult;
 class WebAuthnGetAssertionResult;
 
-class WebAuthnManagerBase : public nsIDOMEventListener {
+class WebAuthnManagerBase : public nsISupports {
  public:
-  NS_DECL_NSIDOMEVENTLISTENER
-
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_CLASS(WebAuthnManagerBase)
 
@@ -52,13 +49,6 @@ class WebAuthnManagerBase : public nsIDOMEventListener {
  protected:
   MOZ_CAN_RUN_SCRIPT virtual ~WebAuthnManagerBase();
 
-  // Needed by HandleEvent() to track visibilty changes.
-  MOZ_CAN_RUN_SCRIPT virtual void HandleVisibilityChange() = 0;
-
-  // Visibility event handling.
-  void ListenForVisibilityEvents();
-  void StopListeningForVisibilityEvents();
-
   bool MaybeCreateBackgroundActor();
 
   // The parent window.

dom/webauthn/tests/browser/browser_webauthn_conditional_mediation.js

@@ -8,16 +8,65 @@ const TEST_URL = "https://example.com";
 
 let gAuthenticatorId = add_virtual_authenticator();
 let gExpectNotAllowedError = expectError("NotAllowed");
+let gExpectAbortError = expectError("Abort");
 let gPendingConditionalGetSubject = "webauthn:conditional-get-pending";
 let gWebAuthnService = Cc["@mozilla.org/webauthn/service;1"].getService(
   Ci.nsIWebAuthnService
 );
 
+add_task(async function test_webauthn_modal_request_cancels_conditional_get() {
+  // Open a new tab.
+  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
+
+  let browser = tab.linkedBrowser.browsingContext.embedderElement;
+  let browsingContextId = browser.browsingContext.id;
+
+  let transactionId = gWebAuthnService.hasPendingConditionalGet(
+    browsingContextId,
+    TEST_URL
+  );
+  Assert.equal(transactionId, 0, "should not have a pending conditional get");
+
+  let requestStarted = TestUtils.topicObserved(gPendingConditionalGetSubject);
+
+  let active = true;
+  let condPromise = promiseWebAuthnGetAssertionDiscoverable(tab, "conditional")
+    .then(arrivingHereIsBad)
+    .catch(gExpectAbortError)
+    .then(() => (active = false));
+
+  await requestStarted;
+
+  transactionId = gWebAuthnService.hasPendingConditionalGet(
+    browsingContextId,
+    TEST_URL
+  );
+  Assert.notEqual(transactionId, 0, "should have a pending conditional get");
+
+  ok(active, "conditional request should still be active");
+
+  let promptPromise = promiseNotification("webauthn-prompt-register-direct");
+  let modalPromise = promiseWebAuthnMakeCredential(tab, "direct")
+    .then(arrivingHereIsBad)
+    .catch(gExpectNotAllowedError);
+
+  await condPromise;
+
+  ok(!active, "conditional request should not be active");
+
+  // Cancel the modal request with the button.
+  await promptPromise;
+  PopupNotifications.panel.firstElementChild.secondaryButton.click();
+  await modalPromise;
+
+  // Close tab.
+  await BrowserTestUtils.removeTab(tab);
+});
+
 add_task(async function test_webauthn_resume_conditional_get() {
   // Open a new tab.
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
 
-  // TODO: Is there a better way to get the browsing context id?
   let browser = tab.linkedBrowser.browsingContext.embedderElement;
   let browsingContextId = browser.browsingContext.id;
 
@@ -79,7 +128,6 @@ add_task(async function test_webauthn_select_autofill_entry() {
   let cred1 = await addCredential(gAuthenticatorId, "example.com");
   let cred2 = await addCredential(gAuthenticatorId, "example.com");
 
-  // TODO: Is there a better way to get the browsing context id?
   let browser = tab.linkedBrowser.browsingContext.embedderElement;
   let browsingContextId = browser.browsingContext.id;
 

dom/webauthn/tests/browser/browser_webauthn_prompts.js

@@ -73,19 +73,6 @@ add_task(test_register_direct_proceed);
 add_task(test_register_direct_proceed_anon);
 add_task(test_select_sign_result);
 
-function promiseNotification(id) {
-  return new Promise(resolve => {
-    PopupNotifications.panel.addEventListener("popupshown", function shown() {
-      let notification = PopupNotifications.getNotification(id);
-      if (notification) {
-        ok(true, `${id} prompt visible`);
-        PopupNotifications.panel.removeEventListener("popupshown", shown);
-        resolve();
-      }
-    });
-  });
-}
-
 function promiseNavToolboxStatus(aExpectedStatus) {
   let navToolboxStatus;
   return TestUtils.topicObserved("fullscreen-nav-toolbox", (subject, data) => {

dom/webauthn/tests/browser/head.js

@@ -243,4 +243,17 @@ function checkRpIdHash(rpIdHash, hostname) {
       }
     });
 }
+
+function promiseNotification(id) {
+  return new Promise(resolve => {
+    PopupNotifications.panel.addEventListener("popupshown", function shown() {
+      let notification = PopupNotifications.getNotification(id);
+      if (notification) {
+        ok(true, `${id} prompt visible`);
+        PopupNotifications.panel.removeEventListener("popupshown", shown);
+        resolve();
+      }
+    });
+  });
+}
 /* eslint-enable no-shadow */

dom/webauthn/tests/mochitest.toml

@@ -92,9 +92,6 @@ skip-if = [
   "os == 'android'", # Test disables all tokens, which is an unsupported configuration on android.
 ]
 
-["test_webauthn_override_request.html"]
-skip-if = ["os == 'android'"] # Test sets security.webauth.webauthn_enable_usbtoken to true, which isn't applicable to android
-
 ["test_webauthn_sameorigin.html"]
 fail-if = ["xorigin"] # NotAllowedError
 skip-if = [

dom/webauthn/tests/test_webauthn_override_request.html

@@ -1,96 +0,0 @@
-<!DOCTYPE html>
-<meta charset=utf-8>
-<head>
-  <title>Test for overriding W3C Web Authentication request</title>
-  <script src="/tests/SimpleTest/SimpleTest.js"></script>
-  <script type="text/javascript" src="u2futil.js"></script>
-  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
-</head>
-<body>
-
-  <h1>Test for overriding W3C Web Authentication request</h1>
-  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1415675">Mozilla Bug 1415675</a>
-
-  <script class="testbody" type="text/javascript">
-    "use strict";
-
-    // Last request status.
-    let status = "";
-
-    add_task(async () => {
-      await SpecialPowers.pushPrefEnv({"set": [
-        ["security.webauth.webauthn_enable_softtoken", false],
-        ["security.webauth.webauthn_enable_usbtoken", true],
-      ]});
-    });
-
-    // Start a new MakeCredential() request.
-    async function requestMakeCredential(status_value) {
-      let publicKey = {
-        rp: {id: document.domain, name: "none"},
-        user: {id: new Uint8Array(), name: "none", displayName: "none"},
-        challenge: crypto.getRandomValues(new Uint8Array(16)),
-        timeout: 5000, // the minimum timeout is actually 15 seconds
-        pubKeyCredParams: [{type: "public-key", alg: cose_alg_ECDSA_w_SHA256}],
-      };
-
-      // Start the request, handle failures only.
-      navigator.credentials.create({publicKey}).catch(() => {
-        status = status_value;
-      });
-
-      // Wait a tick to let the statemachine start.
-      await Promise.resolve();
-    }
-
-    // Start a new GetAssertion() request.
-    async function requestGetAssertion(status_value) {
-      let newCredential = {
-        type: "public-key",
-        id: crypto.getRandomValues(new Uint8Array(16)),
-        transports: ["usb"],
-      };
-
-      let publicKey = {
-        challenge: crypto.getRandomValues(new Uint8Array(16)),
-        timeout: 5000, // the minimum timeout is actually 15 seconds
-        rpId: document.domain,
-        allowCredentials: [newCredential]
-      };
-
-      // Start the request, handle failures only.
-      navigator.credentials.get({publicKey}).catch(() => {
-        status = status_value;
-      });
-
-      // Wait a tick to let the statemachine start.
-      await Promise.resolve();
-    }
-
-    // Test that .create() and .get() requests override any pending requests.
-    add_task(async function test_override_pending_requests() {
-      // Request a new credential.
-      await requestMakeCredential("aborted1");
-
-      // Request another credential, the new request will abort.
-      await requestMakeCredential("aborted2");
-      is(status, "aborted2", "second request aborted");
-
-      // Request an assertion, the new request will still abort.
-      await requestGetAssertion("aborted3");
-      is(status, "aborted3", "third request aborted");
-
-      // Request another assertion, this fourth request will abort.
-      await requestGetAssertion("aborted4");
-      is(status, "aborted4", "fourth request aborted");
-
-      // Request another credential, the fifth request will still abort. Why
-      // do we keep trying? Well, the test originally looked like this, and
-      // let's face it, it's kinda funny.
-      await requestMakeCredential("aborted5");
-      is(status, "aborted5", "fifth request aborted");
-    });
-  </script>
-
-</body>
-</html>