Extensions logic should not participate in LoginHandler dedup.

In the network service path, the extensions onAuthRequired hook is
implemented up in LoginHandler, rather than deep in the NetworkDelegate.

This causes it to inherit the login prompt deduplication logic, which
results in strange behavior for an extension. Fix this by only
deduplicating after the extension stuff is resolved, matching the
pre-network-service behavior.

This also fixes the test that was disabled in issue #928465. (Mostly.
That test is still making some *slightly* incorrect timing assumptions,
but this no longer makes the issue net-svc-specific, so remove the test
suppression and add a TODO to the effect.)

Also add a regression test. In doing so, generalize retval_function to
control when the callback is called, and fix a bug around finding the
right retval_function to use.

Bug: 931479, 928465
Change-Id: I0431b115f64f145ed4201b8944e86b56994ec1fd
Reviewed-on: https://chromium-review.googlesource.com/c/1469289
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634445}

chrome/browser/ui/login/login_handler.cc

@@ -193,19 +193,10 @@ LoginHandler::LoginHandler(net::AuthChallengeInfo* auth_info,
     : WebContentsObserver(web_contents),
       auth_info_(auth_info),
       auth_required_callback_(std::move(auth_required_callback)),
+      prompt_started_(false),
       weak_factory_(this) {
   DCHECK(web_contents);
   DCHECK(auth_info_) << "LoginHandler constructed with NULL auth info";
-
-  // This is probably OK; we need to listen to everything and we break out of
-  // the Observe() if we aren't handling the same auth_info().
-  //
-  // TODO(davidben): We only need to listen to notifications within a single
-  // BrowserContext.
-  registrar_.Add(this, chrome::NOTIFICATION_AUTH_SUPPLIED,
-                 content::NotificationService::AllBrowserContextsAndSources());
-  registrar_.Add(this, chrome::NOTIFICATION_AUTH_CANCELLED,
-                 content::NotificationService::AllBrowserContextsAndSources());
 }
 
 LoginHandler::~LoginHandler() {
@@ -362,7 +353,7 @@ void LoginHandler::Observe(int type,
 
 void LoginHandler::NotifyAuthNeeded() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (WasAuthHandled())
+  if (WasAuthHandled() || !prompt_started_)
     return;
 
   content::NotificationService* service =
@@ -381,7 +372,7 @@ void LoginHandler::NotifyAuthSupplied(const base::string16& username,
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(WasAuthHandled());
 
-  if (!web_contents())
+  if (!web_contents() || !prompt_started_)
     return;
 
   content::NotificationService* service =
@@ -399,6 +390,9 @@ void LoginHandler::NotifyAuthCancelled() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(WasAuthHandled());
 
+  if (!prompt_started_)
+    return;
+
   content::NotificationService* service =
       content::NotificationService::current();
   NavigationController* controller =
@@ -528,6 +522,17 @@ void LoginHandler::MaybeSetUpLoginPrompt(
     return;
   }
 
+  // This is OK; we break out of the Observe() if we aren't handling the same
+  // auth_info() or BrowserContext.
+  //
+  // TODO(davidben): Only listen to notifications within a single
+  // BrowserContext.
+  registrar_.Add(this, chrome::NOTIFICATION_AUTH_SUPPLIED,
+                 content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this, chrome::NOTIFICATION_AUTH_CANCELLED,
+                 content::NotificationService::AllBrowserContextsAndSources());
+  prompt_started_ = true;
+
   // Check if this is a main frame navigation and
   // (a) if the request is cross origin or
   // (b) if an interstitial is already being shown or

chrome/browser/ui/login/login_handler.h

@@ -181,6 +181,9 @@ class LoginHandler : public content::NotificationObserver,
   LoginAuthRequiredCallback auth_required_callback_;
 
   base::WeakPtr<LoginInterstitialDelegate> interstitial_delegate_;
+
+  // True if the extensions logic has run and the prompt logic has started.
+  bool prompt_started_;
   base::WeakPtrFactory<LoginHandler> weak_factory_;
 };
 

chrome/browser/ui/login/login_handler_browsertest.cc

@@ -646,15 +646,6 @@ IN_PROC_BROWSER_TEST_F(MultiRealmLoginPromptBrowserTest,
 // Testing for recovery from an incorrect password for the case where
 // there are multiple authenticated resources.
 IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, IncorrectConfirmation) {
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-  // TODO(http://crbug.com/928465): This test has some timing issues that cause
-  // the asserts to fail when the webRequest proxy is enabled.
-  if (base::FeatureList::IsEnabled(
-          extensions_features::kForceWebRequestProxyForTest)) {
-    return;
-  }
-#endif
-
   ASSERT_TRUE(embedded_test_server()->Start());
   GURL test_page = embedded_test_server()->GetURL(kSingleRealmTestPage);
 

chrome/browser/ui/login/login_handler_test_utils.cc

@@ -41,6 +41,11 @@ void LoginPromptBrowserTestObserver::RemoveHandler(LoginHandler* handler) {
   auto i = std::find(handlers_.begin(), handlers_.end(), handler);
   // Cannot use ASSERT_NE, because gTest on Android confuses iterators with
   // containers.
+  //
+  // TODO(davidben): NOTIFICATION_AUTH_SUPPLIED and NOTIFICATION_AUTH_CANCELLED
+  // are not quite guaranteed to come after NOTIFICATION_AUTH_NEEDED. Either
+  // remove this assumption from the test class or fix things so this assumption
+  // holds.
   ASSERT_TRUE(i != handlers_.end());
   handlers_.erase(i);
 }

chrome/test/data/extensions/api_test/webrequest/auth_parallel/index.html

+<html>
+<body>
+<script>
+  const params = new URLSearchParams(location.search);
+
+  const img1 = new Image();
+  img1.src = params.get('img1');
+  document.body.appendChild(img1);
+
+  const img2 = new Image();
+  img2.src = params.get('img2');
+  document.body.appendChild(img2);
+</script>
+</body>
+</html>

chrome/test/data/extensions/api_test/webrequest/framework.js

@@ -10,6 +10,7 @@ var capturedUnexpectedData;
 var expectedEventOrder;
 var networkServiceState = "unknown";
 var tabId;
+var tabIsIncognito;
 var tabIdMap;
 var frameIdMap;
 var testWebSocketPort;
@@ -50,6 +51,7 @@ function runTestsForTab(tests, tab) {
   tabId = tab.id;
   tabIdMap = {"-1": -1};
   tabIdMap[tabId] = 0;
+  tabIsIncognito = tab.incognito;
   chrome.test.getConfig(function(config) {
     testServerPort = config.testServer.port;
     testWebSocketPort = config.testWebSocketPort;
@@ -132,11 +134,36 @@ function navigateAndWait(url, callback) {
   chrome.tabs.update(tabId, {url: url});
 }
 
+function deepCopy(obj) {
+  if (obj === null)
+    return null;
+  if (typeof(obj) != 'object')
+    return obj;
+  if (Array.isArray(obj)) {
+    var tmp_array = new Array;
+    for (var i = 0; i < obj.length; i++) {
+      tmp_array.push(deepCopy(obj[i]));
+    }
+    return tmp_array;
+  }
+
+  var tmp_object = {}
+  for (var p in obj) {
+    tmp_object[p] = deepCopy(obj[p]);
+  }
+  return tmp_object;
+}
+
 // data: array of expected events, each one is a dictionary:
 //     { label: "<unique identifier>",
 //       event: "<webrequest event type>",
 //       details: { <expected details of the webrequest event> },
 //       retval: { <dictionary that the event handler shall return> } (optional)
+//       retval_function: <function to run when the event occurs, this overrides
+//                         any retval handling. The function takes
+//                         (name, details, optional callback). The value it
+//                         returns is returned out of the event
+//                         handler> (optional)
 //     }
 // order: an array of sequences, e.g. [ ["a", "b", "c"], ["d", "e"] ] means that
 //     event with label "a" needs to occur before event with label "b". The
@@ -288,17 +315,7 @@ function captureEvent(name, details, callback) {
     return;
   }
 
-  // Pull the extra per-event options out of the expected data. These let
-  // us specify special return values per event.
-  var currentIndex = capturedEventData.length;
-  var extraOptions;
-  var retval;
-  if (expectedEventData.length > currentIndex) {
-    retval =
-        expectedEventData[currentIndex].retval_function ?
-        expectedEventData[currentIndex].retval_function(name, details) :
-        expectedEventData[currentIndex].retval;
-  }
+  var originalDetails = deepCopy(details);
 
   // Check that the frameId can be used to reliably determine the URL of the
   // frame that caused requests.
@@ -356,41 +373,51 @@ function captureEvent(name, details, callback) {
     }
   });
 
-  // find |details| in expectedEventData
-  var found = false;
-  var label = undefined;
+  // find |details| in matchingExpectedEventData
+  var matchingExpectedEvent = undefined;
   expectedEventData.forEach(function (exp) {
     if (deepEq(exp.event, name) && deepEq(exp.details, details)) {
-      if (found) {
+      if (matchingExpectedEvent) {
         chrome.test.fail("Duplicated expectation entry '" + exp.label +
         "' should be identified by |eventCount|: " + JSON.stringify(details));
       } else {
-        found = true;
-        label = exp.label;
+        matchingExpectedEvent = exp;
       }
     }
   });
-  if (!found && !ignoreUnexpected) {
+  if (!matchingExpectedEvent && !ignoreUnexpected) {
     console.log("Expected events: " +
         JSON.stringify(expectedEventData, null, 2));
     chrome.test.fail("Received unexpected event '" + name + "':" +
         JSON.stringify(details, null, 2));
   }
 
-  if (found) {
+  var retval;
+  var retval_function;
+  if (matchingExpectedEvent) {
     if (logAllRequests) {
       console.log("Expected: " + name + ": " + JSON.stringify(details));
     }
-    capturedEventData.push({label: label, event: name, details: details});
+    capturedEventData.push(
+        {label: matchingExpectedEvent.label, event: name, details: details});
 
     // checkExpecations decrements the counter of pending events. We may only
     // call it if an expected event has occurred.
     checkExpectations();
+
+    // Pull the extra per-event options out of the expected data. These let us
+    // specify special return values per event.
+    retval = matchingExpectedEvent.retval;
+    retval_function = matchingExpectedEvent.retval_function;
   } else {
     if (logAllRequests) {
-      console.log("NOT Expected: " + name + ": " + JSON.stringify(details));
+      console.log('NOT Expected: ' + name + ': ' + JSON.stringify(details));
     }
-    capturedUnexpectedData.push({label: label, event: name, details: details});
+    capturedUnexpectedData.push({event: name, details: details});
+  }
+
+  if (retval_function) {
+    return retval_function(name, originalDetails, callback);
   }
 
   if (callback) {

chrome/test/data/extensions/api_test/webrequest/test_auth_required.js

@@ -5,8 +5,9 @@
 // Generates a unique authentication URL so each test can run
 // without hitting the HTTP authentication cache. Each test
 // must use a unique realm, however.
-function getURLAuthRequired(realm) {
-  return getServerURL('auth-basic/' + realm + '/subpath?realm=' + realm);
+function getURLAuthRequired(realm, subpath = 'subpath') {
+  return getServerURL(
+      'auth-basic/' + realm + '/' + subpath + '?realm=' + realm);
 }
 
 runTests([
@@ -585,4 +586,255 @@ runTests([
       ["responseHeaders", "asyncBlocking"]);
     navigateAndWait(url);
   },
+
+  // Test that two parallel onAuthRequired signals do not interfere with each
+  // other. This is a regression test for https://crbug.com/931479.
+  function authRequiredParallel() {
+    const realm = 'parallelasync';
+
+    const imgUrl1 = getURLAuthRequired(realm, '1');
+    const imgUrl2 = getURLAuthRequired(realm, '2');
+    // TODO(https://crbug.com/934398): We incorrectly filter out the initiator
+    // for requests from incognito contexts for split mode extensions.
+    const initiator =
+        tabIsIncognito ? undefined : getServerDomain(initiators.WEB_INITIATED);
+    const parallelAuthRequestsUrl = getServerURL(
+        'extensions/api_test/webrequest/auth_parallel?img1=' +
+        encodeURIComponent(imgUrl1) + '&img2=' + encodeURIComponent(imgUrl2));
+
+    function createExternallyResolvablePromise() {
+      let _resolve;
+      const promise = new Promise((resolve, reject) => _resolve = resolve);
+      promise.resolve = _resolve;
+      return promise;
+    }
+
+    // Create some promises that will resolved to callbacks when onAuthRequired
+    // and onComplete fire.
+    const authRequired1 = createExternallyResolvablePromise();
+    const completed1 = createExternallyResolvablePromise();
+    const authRequired2 = createExternallyResolvablePromise();
+
+    // responseStarted2 is whether the request for |imgUrl2| has signalled
+    // |onResponseStarted| yet.
+    let responseStarted2 = false;
+
+    expect(
+      [  // events
+        { label: 'onBeforeRequest-1',
+          event: 'onBeforeRequest',
+          details: {
+            url: imgUrl1,
+            type: 'image',
+            initiator: initiator,
+            // The testing framework cannot recover the frame URL because the
+            // URL filter below does not capture the top-level request.
+            frameUrl: 'unknown frame URL',
+          },
+        },
+        { label: 'onBeforeSendHeaders-1',
+          event: 'onBeforeSendHeaders',
+          details: {
+            url: imgUrl1,
+            type: 'image',
+            initiator: initiator,
+            // Note: no requestHeaders because we don't ask for them.
+          },
+          retval: {}
+        },
+        { label: 'onSendHeaders-1',
+          event: 'onSendHeaders',
+          details: {
+            url: imgUrl1,
+            type: 'image',
+            initiator: initiator,
+          }
+        },
+        { label: 'onHeadersReceived-1',
+          event: 'onHeadersReceived',
+          details: {
+            url: imgUrl1,
+            type: 'image',
+            initiator: initiator,
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 401 Unauthorized',
+            statusCode: 401,
+          }
+        },
+        { label: 'onAuthRequired-1',
+          event: 'onAuthRequired',
+          details: {
+            url: imgUrl1,
+            type: 'image',
+            initiator: initiator,
+            isProxy: false,
+            scheme: 'basic',
+            realm: realm,
+            challenger: {host: testServer, port: testServerPort},
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 401 Unauthorized',
+            statusCode: 401,
+          },
+          retval_function:
+              (name, details, callback) => authRequired1.resolve(callback),
+        },
+        { label: 'onResponseStarted-1',
+          event: 'onResponseStarted',
+          details: {
+            url: imgUrl1,
+            type: 'image',
+            initiator: initiator,
+            fromCache: false,
+            statusCode: 200,
+            ip: '127.0.0.1',
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 200 OK',
+          }
+        },
+        { label: 'onCompleted-1',
+          event: 'onCompleted',
+          details: {
+            url: imgUrl1,
+            type: 'image',
+            initiator: initiator,
+            fromCache: false,
+            statusCode: 200,
+            ip: '127.0.0.1',
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 200 OK',
+          },
+          retval_function: (name, details) => completed1.resolve(),
+        },
+        { label: 'onBeforeRequest-2',
+          event: 'onBeforeRequest',
+          details: {
+            url: imgUrl2,
+            type: 'image',
+            initiator: initiator,
+            // The testing framework cannot recover the frame URL because the
+            // URL filter below does not capture the top-level request.
+            frameUrl: 'unknown frame URL',
+          },
+        },
+        { label: 'onBeforeSendHeaders-2',
+          event: 'onBeforeSendHeaders',
+          details: {
+            url: imgUrl2,
+            type: 'image',
+            initiator: initiator,
+            // Note: no requestHeaders because we don't ask for them.
+          },
+          retval: {}
+        },
+        { label: 'onSendHeaders-2',
+          event: 'onSendHeaders',
+          details: {
+            url: imgUrl2,
+            type: 'image',
+            initiator: initiator,
+          }
+        },
+        { label: 'onHeadersReceived-2',
+          event: 'onHeadersReceived',
+          details: {
+            url: imgUrl2,
+            type: 'image',
+            initiator: initiator,
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 401 Unauthorized',
+            statusCode: 401,
+          }
+        },
+        { label: 'onAuthRequired-2',
+          event: 'onAuthRequired',
+          details: {
+            url: imgUrl2,
+            type: 'image',
+            initiator: initiator,
+            isProxy: false,
+            scheme: 'basic',
+            realm: realm,
+            challenger: {host: testServer, port: testServerPort},
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 401 Unauthorized',
+            statusCode: 401,
+          },
+          retval_function:
+              (name, details, callback) => authRequired2.resolve(callback),
+        },
+        { label: 'onResponseStarted-2',
+          event: 'onResponseStarted',
+          details: {
+            url: imgUrl2,
+            type: 'image',
+            initiator: initiator,
+            fromCache: false,
+            statusCode: 401,
+            ip: '127.0.0.1',
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 401 Unauthorized',
+          },
+          retval_function: (name, details) => responseStarted2 = true,
+        },
+        { label: 'onCompleted-2',
+          event: 'onCompleted',
+          details: {
+            url: imgUrl2,
+            type: 'image',
+            initiator: initiator,
+            fromCache: false,
+            statusCode: 401,
+            ip: '127.0.0.1',
+            responseHeadersExist: true,
+            statusLine: 'HTTP/1.1 401 Unauthorized',
+          }
+        },
+      ],
+      [  // event order
+        ['onBeforeRequest-1', 'onBeforeSendHeaders-1', 'onSendHeaders-1',
+         'onHeadersReceived-1', 'onAuthRequired-1', 'onResponseStarted-1',
+         'onCompleted-1'],
+        ['onBeforeRequest-2', 'onBeforeSendHeaders-2', 'onSendHeaders-2',
+         'onHeadersReceived-2', 'onAuthRequired-2', 'onResponseStarted-2',
+         'onCompleted-2']
+      ],
+      // Only pay attention to |imgUrl1| and |imgUrl2|, not
+      // |parallelAuthRequestsUrl|.
+      {urls: ['<all_urls>'], types: ['image']},
+      ['responseHeaders', 'asyncBlocking']);
+
+    navigateAndWait(parallelAuthRequestsUrl);
+    (async function() {
+      // Wait for onAuthRequired to be signaled for both requests before doing
+      // anything.
+      const callback1 = await authRequired1;
+      const callback2 = await authRequired2;
+
+      // Resolve the first request and let it complete.
+      callback1({authCredentials: {username: 'foo', password: 'secret'}});
+      await completed1;
+
+      // We wish to test that this did not resolve |callback2| with the same
+      // credentials internally. This would cause |imgUrl2| to signal
+      // |onResponseStarted| and |onCompleted| though it should be blocked.
+      chrome.test.assertFalse(responseStarted2);
+
+      // However it's possible that the second request may be much slower than
+      // the first, causing the above check to pass even if we don't correctly
+      // isolate the two authentication requests.
+      //
+      // Per the webRequest documentation, |onBeforeSendHeaders| should be
+      // signaled after a resolved |onAuthRequired|. This happens without a
+      // network delay, so it would make a fairly reliable test. However,
+      // webRequest does not match the documentation and does not signal it. See
+      // https://crbug.com/809761.
+      //
+      // Instead, resolve the second request, this time canceling the
+      // authentication request. This should result in |imgUrl2| completing with
+      // a 401 response. If the credentials were incorrectly propagated from
+      // |imgUrl1|, this will be ignored and |imgUrl2| will complete with a 200
+      // response.
+      callback2({cancel: true});
+    })();
+  },
 ]);