https://developer.chrome.com/extensions/webRequest#event-onAuthRequired https://developer.chrome.com/extensions/webRequest#event-onBeforeRedirect

Half of this bug is actually Bug 1215197, which I fixed. Morphing accordingly.

Girogio, still working on this?

Actually not, because I don't know directly about extensions which actually depend on it (I fixed the half I needed for mine). I could in future, after finishing NoScript as a WebExtension, so if it's a priority feel free to take it.

Hi, we are planning to port our produxt's firefox plugin to webextension. We are blocked on this API. Is there an ETA on when the API would be available. Another reason we want to port the plugin to the webextension mode is due to the E10s feature.

WIP patch for onAuthRequired. Looking for feedback on the way I've hooked into notificationCallbacks and modified runChannelListener. This works, need to write tests, but it has one difference from the chromium implementation. Per https://developer.chrome.com/extensions/webRequest The flow after onAuthRequired goes back to onBeforeSendHeaders, however our implementation goes back to onBeforeRequest. Haven't looked into how to identify this easily to skip that call, also not sure if it's necessary.

Comment on attachment 8819484 [details] [diff] [review] WIP authRequired Review of attachment 8819484 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html @@ +28,5 @@ > + // SelfSupport has a tendency to fire when running this test alone, without > + // a good way to turn it off we just set the url to "" > + yield SpecialPowers.pushPrefEnv({ > + set: [["browser.selfsupport.url", ""]], > + }); Please move this to the head_webrequest file. @@ +50,5 @@ > + yield extension.awaitMessage("continue"); > + > + yield testXHR(SimpleTest.getTestFileURL("auth.sjs")); > + > + yield extension.awaitMessage("done"); We need to test the behavior with cached auth credentials, fallback to the default auth prompt, multiple listeners, ... ::: toolkit/modules/addons/WebRequest.jsm @@ +540,5 @@ > + // "ONLY_PASSWORD":8,"PREVIOUS_FAILED":16,"CROSS_ORIGIN_SUB_RESOURCE":32} > + let data = { > + scheme: authInfo.authenticationScheme, > + realm: authInfo.realm, > + isProxy: authInfo.flags & authInfo.AUTH_PROXY, This needs to be a boolean. @@ +550,5 @@ > + this.runChannelListener(channel, this.getLoadContext(channel), > + "authRequired", data, (result) => { > + dump(`authRequired callback gets ${JSON.stringify(result)}\n`); > + if (!result) { > + callback.onAuthCancelled(context, true); We need to fall back to default auth handlers here, and correctly handle multiple authRequired listeners. @@ +560,5 @@ > + } > + }); > + } catch (e) { > + dump(`---- asyncPromptAuth failure ${e.message}\n`); > + } We need to return an nsICancelable here. @@ +710,5 @@ > > return Object.assign(data, extraData); > }, > > + runChannelListener(channel, loadContext = null, kind, extraData = null, channelCallback = null) { I'd rather we just have `runChannelListeners` return the listener results for authRequired listeners than pass a callback and add all these special cases. @@ +888,5 @@ > this.runChannelListener(channel, loadContext, "headersReceived"); > + > + if (this.listeners.authRequired.size) { > + dump(`=== set notificationCallbacks in examine, TODO better place to do this?\n`); > + channel.notificationCallbacks = this; If we're going to replace channel notificationCallbacks, we need to only do it if there are auth listeners that actually match this request, and to forward interfaces that we don't support to the original callbacks, and to forward to the original auth handler when a listener doesn't provide credentials. But actually finding the correct auth handler to forward to is non-trivial, and could easily get out of sync with necko's logic, so I'd much rather we come up with a cleaner way to add global auth listeners, similar to what we do for "net-channel-event-sinks" category entries.

> If we're going to replace channel notificationCallbacks, we need to only do > it if there are auth listeners that actually match this request, and to > forward interfaces that we don't support to the original callbacks, and to > forward to the original auth handler when a listener doesn't provide > credentials. > > But actually finding the correct auth handler to forward to is non-trivial, > and could easily get out of sync with necko's logic, so I'd much rather we > come up with a cleaner way to add global auth listeners, similar to what we > do for "net-channel-event-sinks" category entries. I've opted to continue with notificationCallbacks for now as there are a bunch of paths using it to prompt for authorization. I think a followup would be to fix/replace notificationCallbacks or at least provide an alternative to it.

This improves on the previous and should be handling the comments you made before. I still have to add tests for proxy auth, but would like to see if you have further comments on the direction of the patch.

Comment on attachment 8821259 [details] [diff] [review] WIP authRequired V2 Review of attachment 8821259 [details] [diff] [review]: ----------------------------------------------------------------- If we're going to go this route, please file a follow-up to handle this in necko without having to wrap notification callbacks on every channel it affects. ::: toolkit/components/extensions/test/mochitest/mochitest.ini @@ +1,4 @@ > [DEFAULT] > tags = webextensions in-process-webextensions > > + Not necessary. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html @@ +14,5 @@ > + > +let baseUrl = "http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/authenticate.sjs"; > +function testXHR(url) { > + return new Promise(resolve => { > + let xhr = new XMLHttpRequest(); Please use fetch instead. @@ +87,5 @@ > + // expecting origin == undefined > + extension.sendMessage("set-expected", {expect, origin: location.href}); > + yield extension.awaitMessage("continue"); > + > + yield testXHR(baseUrl + `?realm=webRequest_auth&user=${authInfo.username}&pass=${authInfo.password}`); `${baseUrl}?realm=...` ::: toolkit/modules/addons/WebRequest.jsm @@ +437,5 @@ > + throw Cr.NS_ERROR_NO_INTERFACE; > + }, > + > + promptAuth(channel, level, authInfo) { > + throw Cr.NS_ERROR_NOT_IMPLEMENTED; We should fallback to the previous callbacks here. @@ +456,5 @@ > + port: uri.port, > + }, > + }; > + > + channel.notificationCallbacks = this.notificationCallbacks; This is going to cause problems if some other caller tries to chain callbacks the same way that we're doing. @@ +463,5 @@ > + // In the case that no listener provides credentials, we fallback to the > + // previously set callback class for authentication. > + channelData.authPromptForward = () => { > + let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL; > + let provider = channel.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider); We should probably stick to our saved callbacks here. And we need to handle the other cases that necko handles: http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#362-383 @@ +471,5 @@ > + channelData.authPromptCallback = (authCredentials) => { > + // The API allows for canceling the request, providing credentials or > + // doing nothing, so we do not provide a way to call onAuthCanceled. > + // Canceling the request will result in canceling the authentication. > + if (authCredentials) { We need to verify that username and password are valid string properties. @@ +478,5 @@ > + > + callback.onAuthAvailable(context, authInfo); > + // At least one addon has responded, so we wont forward to the regular > + // prompt handlers. > + delete channelData.authPromptForward; Set to null instead so we don't change the shape of the object. We also need to clear the authPromptCallback. @@ +488,5 @@ > + > + return { > + QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]), > + callback: callback, > + context: context, callback, context, cancel() { ...; } However, given that this is already a closure, I'm not sure why we need the `context` and `callback` member properties. @@ +893,5 @@ > if (opts.responseHeaders && result.responseHeaders && responseHeaders) { > responseHeaders.applyChanges(result.responseHeaders); > } > + > + if (opts.blocking && result.authCredentials) { We should only be doing this for authRequired listeners. @@ +949,3 @@ > } > > + if (this.listeners.authRequired.size) { We need to check whether these listeners actually apply to this channel. Same for headersReceived. @@ +1008,5 @@ > > var onBeforeSendHeaders = new HttpEvent("modify", ["requestHeaders", "blocking"]); > var onSendHeaders = new HttpEvent("afterModify", ["requestHeaders"]); > var onHeadersReceived = new HttpEvent("headersReceived", ["blocking", "responseHeaders"]); > +var onAuthRequired = new HttpEvent("authRequired", ["blocking", "responseHeaders"]); // TODO asyncBlocking We already handle async blocking for all listeners. No need to treat this any differently.

(In reply to Kris Maglione [:kmag] from comment #9) > @@ +949,3 @@ > > } > > > > + if (this.listeners.authRequired.size) { > > We need to check whether these listeners actually apply to this channel. > Same for headersReceived. runChannelListener does that, do you feel we should additionally check before that? That seems to make sense for auth since we're hooking into notificationCallbacks, but I'm not sure it makes sense to do for headersReceived. > @@ +1008,5 @@ > > > > var onBeforeSendHeaders = new HttpEvent("modify", ["requestHeaders", "blocking"]); > > var onSendHeaders = new HttpEvent("afterModify", ["requestHeaders"]); > > var onHeadersReceived = new HttpEvent("headersReceived", ["blocking", "responseHeaders"]); > > +var onAuthRequired = new HttpEvent("authRequired", ["blocking", "responseHeaders"]); // TODO asyncBlocking > > We already handle async blocking for all listeners. No need to treat this > any differently. I'm not clear about your comment. "asyncBlocking" is an option documented for onAuthRequired that can be used instead of "blocking", it's the only webrequest listener with that option. If you use that you are given a callback to use in the listener. While we achieve the same thing by returning a promise from the listener, we'd still want to support asyncBlocking if we want to get to full compatibility.

(In reply to Shane Caraveo (:mixedpuppy) from comment #10) > (In reply to Kris Maglione [:kmag] from comment #9) > > > @@ +949,3 @@ > > > } > > > > > > + if (this.listeners.authRequired.size) { > > > > We need to check whether these listeners actually apply to this channel. > > Same for headersReceived. > > runChannelListener does that, do you feel we should additionally check > before that? That seems to make sense for auth since we're hooking into > notificationCallbacks, but I'm not sure it makes sense to do for > headersReceived. Yes. My point is that we shouldn't be replacing the channel's notificationCallbacks if there are no listeners that require it. > > @@ +1008,5 @@ > > > > > > var onBeforeSendHeaders = new HttpEvent("modify", ["requestHeaders", "blocking"]); > > > var onSendHeaders = new HttpEvent("afterModify", ["requestHeaders"]); > > > var onHeadersReceived = new HttpEvent("headersReceived", ["blocking", "responseHeaders"]); > > > +var onAuthRequired = new HttpEvent("authRequired", ["blocking", "responseHeaders"]); // TODO asyncBlocking > > > > We already handle async blocking for all listeners. No need to treat this > > any differently. > > I'm not clear about your comment. "asyncBlocking" is an option documented > for onAuthRequired that can be used instead of "blocking", it's the only > webrequest listener with that option. If you use that you are given a > callback to use in the listener. While we achieve the same thing by > returning a promise from the listener, we'd still want to support > asyncBlocking if we want to get to full compatibility. I'm not worried about 100% compatibility. If at some point we decide we need to support that convention in the `chrome` namespace, we can add a wrapper at the binding layer.

(In reply to Kris Maglione [:kmag] from comment #9) > ::: > toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html > @@ +14,5 @@ > > + > > +let baseUrl = "http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/authenticate.sjs"; > > +function testXHR(url) { > > + return new Promise(resolve => { > > + let xhr = new XMLHttpRequest(); > > Please use fetch instead. Fetch doesn't support authentication. Bug 1120722. https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#289

Comment on attachment 8821259 [details] [diff] [review] WIP authRequired V2 I believe I have everything covered now, so pushed to review.

Comment on attachment 8821372 [details] Bug 1190689 implement onAuthRequired for WebRequest, https://reviewboard.mozilla.org/r/100680/#review104758 Looks good for the most part. I'm mainly just worried that we have insufficient tests for some corner cases at this point. ::: toolkit/components/extensions/test/mochitest/head_webrequest.js:185 (Diff revision 1) > expectedEvent = expected.optional_events.includes(name); > } > browser.test.assertTrue(expectedEvent, `received ${name}`); > browser.test.assertEq(expected.type, details.type, "resource type is correct"); > + // ignore origin test for generated background page > + if (details.originUrl.split("/").pop() != "_generated_background_page.html") { if (!details.originUrl.endsWith("/_generated_background_page.html")) ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:23 (Diff revision 1) > + xhr.onabort = () => { > + resolve(); > + }; > + xhr.onerror = () => { > + resolve(); > + }; Shouldn't we reject in these cases? ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:43 (Diff revision 1) > + }, {urls: ["<all_urls>"]}, ["blocking"]); > + browser.webRequest.onErrorOccurred.addListener((details) => { > + browser.test.succeed("request canceled"); > + browser.test.sendMessage("done"); > + }, {urls: ["<all_urls>"]}); > + browser.test.sendMessage("started"); No need for this. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:143 (Diff revision 1) > + browser.webRequest.onAuthRequired.addListener((details) => { > + browser.test.log(`handlingExt called with ${details.requestId} ${details.url}`); > + browser.test.sendMessage("done"); > + return {authCredentials: {username: "foobar", password: "testpass"}}; > + }, {urls: ["<all_urls>"]}, ["blocking"]); > + browser.test.sendMessage("started"); Not necessary. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:207 (Diff revision 1) > + function background() { > + browser.webRequest.onAuthRequired.addListener((details) => { > + browser.test.succeed(`handlingExt onAuthRequired called with ${details.requestId} ${details.url}`); > + browser.test.sendMessage("done"); > + }, {urls: ["<all_urls>"]}, ["blocking"]); > + browser.test.sendMessage("started"); Not necessary. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:233 (Diff revision 1) > + > + yield handlingExt.awaitMessage("done"); > + yield handlingExt.unload(); > + yield canceler.awaitMessage("done"); > + yield canceler.unload(); > +}); I think we're still missing the case where there are two blocking listeners, where the first doesn't return any credentials and the second does. Also, ideally separately for cases where it returns an empty object and where it returns undefined. We should also probably have listeners that return no credentials that are called after, and two listeners that both return credentials for the same request. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:249 (Diff revision 1) > + const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > + Cu.import("resource://gre/modules/Services.jsm"); > + Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > + let observer = channel => { > + if (channel instanceof Ci.nsIHttpChannel && channel.URI.host === "mochi.test") { Can we return early here instead just to save a level of indentation? ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:267 (Diff revision 1) > + getInterface(iid) { > + if (iid.equals(Ci.nsIAuthPromptProvider) || iid.equals(Ci.nsIAuthPrompt2)) { > + return this; > + } > + return callbacks.getInterface(iid); > + }, Please make sure to test this separately with the nsIAuthPromptProvider and nsIAuthPrompt2 cases, and make sure that code gets complete coverage in coverage tests. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:275 (Diff revision 1) > + }, > + getAuthPrompt(reason, iid) { > + return this; > + }, > + asyncPromptAuth(channel, callback, context, level, authInfo) { > + channel.notificationCallbacks = callbacks; Why? ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:298 (Diff revision 1) > + }, {urls: ["<all_urls>"]}, ["blocking"]); > + browser.webRequest.onErrorOccurred.addListener((details) => { > + browser.test.succeed("request canceled"); > + browser.test.sendMessage("done"); > + }, {urls: ["<all_urls>"]}); > + browser.test.sendMessage("started"); Not necessary. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:337 (Diff revision 1) > + return {authCredentials: {username: "puser", password: "ppass"}}; > + } > + browser.test.assertTrue(proxyOk, "providing www authorization after proxy auth"); > + browser.test.sendMessage("done"); > + return {authCredentials: {username: "auser", password: "apass"}}; > + }, {urls: ["<all_urls>"]}, ["blocking"]); We should probably use a better URL filter in all of these. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:338 (Diff revision 1) > + } > + browser.test.assertTrue(proxyOk, "providing www authorization after proxy auth"); > + browser.test.sendMessage("done"); > + return {authCredentials: {username: "auser", password: "apass"}}; > + }, {urls: ["<all_urls>"]}, ["blocking"]); > + browser.test.sendMessage("started"); Not necessary. ::: toolkit/modules/addons/WebRequest.jsm:404 (Diff revision 1) > }; > > ChannelEventSink.init(); > > +// nsIAuthPrompt2 implementation for onAuthRequired > +function AuthRequestor(notificationCallbacks, httpObserver) { Please use an ES6 class. ::: toolkit/modules/addons/WebRequest.jsm:440 (Diff revision 1) > + // This should never get called without getInterface having been called first. > + if (iid.equals(Ci.nsIAuthPrompt2)) { > + return this; > + } > + let provider = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider); > + if (provider) { No need for this. `getInterface` will throw if the interface isn't available. ::: toolkit/modules/addons/WebRequest.jsm:449 (Diff revision 1) > + if (!prompt) { > + throw Cr.NS_ERROR_NO_INTERFACE; > + } Not necessary. `getInterface` will throw if it doesn't provide this interface. ::: toolkit/modules/addons/WebRequest.jsm:472 (Diff revision 1) > + > + let channelData = getData(channel); > + // In the case that no listener provides credentials, we fallback to the > + // previously set callback class for authentication. > + channelData.authPromptForward = () => { > + let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL; Please move this to just before the `getAuthPrompt` call. ::: toolkit/modules/addons/WebRequest.jsm:475 (Diff revision 1) > + // previously set callback class for authentication. > + channelData.authPromptForward = () => { > + let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL; > + let provider = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider); > + let prompt; > + if (provider) { `getInterface` will throw if the interface isn't supported, so this needs to be a try-catch rather than an if. ::: toolkit/modules/addons/WebRequest.jsm:495 (Diff revision 1) > + authInfo.password = authCredentials.password; > + > + callback.onAuthAvailable(context, authInfo); > + // At least one addon has responded, so we wont forward to the regular > + // prompt handlers. > + channelData.authPromptForward = null; We also probably need to null out `authPromptCallback` here. ::: toolkit/modules/addons/WebRequest.jsm:505 (Diff revision 1) > + this.httpObserver.runChannelListener(channel, loadContext, "authRequired", data); > + > + return { > + QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]), > + cancel() { > + callback.onAuthCancelled(this.context, false); s/this.// ::: toolkit/modules/addons/WebRequest.jsm:505 (Diff revision 1) > + this.httpObserver.runChannelListener(channel, loadContext, "authRequired", data); > + > + return { > + QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]), > + cancel() { > + callback.onAuthCancelled(this.context, false); We also probably need to null things out here. ::: toolkit/modules/addons/WebRequest.jsm:935 (Diff revision 1) > + if (listener.size < 1) { > + return false; > + } This is probably an unnecessary optimization. If you keep it, though, please use `listener.size == 0`

Comment on attachment 8821372 [details] Bug 1190689 implement onAuthRequired for WebRequest, https://reviewboard.mozilla.org/r/100680/#review104758 > I think we're still missing the case where there are two blocking listeners, where the first doesn't return any credentials and the second does. Also, ideally separately for cases where it returns an empty object and where it returns undefined. > > We should also probably have listeners that return no credentials that are called after, and two listeners that both return credentials for the same request. The first modification is supposed to win, so subsequent listeners can only cancel a request.

(In reply to Shane Caraveo (:mixedpuppy) from comment #16) > The first modification is supposed to win, so subsequent listeners can only > cancel a request. Right, the first modification is supposed to win, but we still need to test multiple handlers trying to return auth data for the same request behave sanely. If at some point, we wind up making a change that causes the auth callback to be called multiple times in that case, for instance, and that causes the browser to crash, that's something I'd like to know before it makes it to production.

Comment on attachment 8821372 [details] Bug 1190689 implement onAuthRequired for WebRequest, https://reviewboard.mozilla.org/r/100680/#review106412 Looks good so far. Leaving r? set until you finish addressing the issues from the last review. ::: toolkit/components/extensions/test/mochitest/head_webrequest.js:167 (Diff revisions 1 - 2) > - function getListener(name) { > - return details => { > + let listeners = { > + onBeforeRequest(expected, details, result) { In the future, can you try to put as much as these kinds of refactorings as possible into a separate patches? ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:178 (Diff revisions 1 - 2) > - }; > + yield testXHR(`${baseUrl}?realm=test_webRequest_auth_cancelled&user=${authCredentials.username}&pass=${authCredentials.password}`) > + .then(() => { > + ok(false, "xhr succeeded"); > + }) > + .catch(() => { > + ok(true, "caught rejected xhr"); > + }); yield Assert.rejects(testXHR(...), ...); ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:235 (Diff revisions 1 - 2) > - function background() { > - browser.webRequest.onAuthRequired.addListener((details) => { > - browser.test.succeed(`handlingExt onAuthRequired called with ${details.requestId} ${details.url}`); > - browser.test.sendMessage("done"); > - }, {urls: ["<all_urls>"]}, ["blocking"]); > - browser.test.sendMessage("started"); > + let ext = getAuthHandler(); > + yield ext.startup(); > + let canceler = getAuthHandler({cancel: true}); > + yield canceler.startup(); > + > + yield testXHR(`${baseUrl}?realm=auth_blocking_noreturn&user=auth_blocking_noreturn&pass=auth_blocking_noreturn`) yield Assert.rejects(testXHR(...), ...); ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:267 (Diff revisions 1 - 2) > const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > let observer = channel => { > - if (channel instanceof Ci.nsIHttpChannel && channel.URI.host === "mochi.test") { > + if (!(channel instanceof Ci.nsIHttpChannel && channel.URI.host === "mochi.test")) { I think I'd prefer `!(channel instanceof nsIHttpChannel) || host !== "mochi.test"`, but I don't have that strong an opinion on it. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:313 (Diff revisions 1 - 2) > - }); > - > yield handlingExt.startup(); > - yield handlingExt.awaitMessage("started"); > > - yield testXHR(`${baseUrl}?realm=auth_nonblocking_forwardAuth&user=auth_nonblocking_forwardAuth&pass=auth_nonblocking_forwardAuth`); > + yield testXHR(`${baseUrl}?realm=auth_nonblocking_forwardAuth&user=auth_nonblocking_forwardAuth&pass=auth_nonblocking_forwardAuth`) yield Assert.rejects(textXHR(...), ...); ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:402 (Diff revisions 1 - 2) > + .then(() => { > + ok(true, "xhr succeeded"); > + }) > + .catch(() => { > + ok(false, "caught rejected xhr"); > + }); If we need this handler, it should be a single `.then()` handler with success and error callbacks. But I don't think we need it. The rejected promise on its own should cause the test to fail. Same for the other instances. ::: toolkit/modules/addons/WebRequest.jsm:468 (Diff revisions 1 - 2) > channelData.authPromptForward = () => { > - let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL; > - let provider = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider); > let prompt; > - if (provider) { > - prompt = provider.getAuthPrompt(reason, Ci.nsIAuthPrompt2); > - } else { > + try { > + let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL; > + prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider) > + .getAuthPrompt(reason, Ci.nsIAuthPrompt2); > + } catch (e) { > prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPrompt2); > } > prompt.asyncPromptAuth(channel, callback, context, level, authInfo); > }; We should probably put a try-catch around this entire body, in case the second `getInterface` or `asyncPromptAuth` throw. ::: toolkit/modules/addons/WebRequest.jsm:489 (Diff revisions 1 - 2) > typeof authCredentials.username === "string" && > typeof authCredentials.password === "string") { > authInfo.username = authCredentials.username; > authInfo.password = authCredentials.password; > > callback.onAuthAvailable(context, authInfo); We should also probably put a try-catch around this.

(In reply to Kris Maglione [:kmag] from comment #18) > (In reply to Shane Caraveo (:mixedpuppy) from comment #16) > > The first modification is supposed to win, so subsequent listeners can only > > cancel a request. > > Right, the first modification is supposed to win, but we still need to test > multiple handlers trying to return auth data for the same request behave > sanely. If at some point, we wind up making a change that causes the auth > callback to be called multiple times in that case, for instance, and that > causes the browser to crash, that's something I'd like to know before it > makes it to production. Yep, that last update added tests for several scenarios including this.

Comment on attachment 8821372 [details] Bug 1190689 implement onAuthRequired for WebRequest, https://reviewboard.mozilla.org/r/100680/#review107150 r=me with issues addressed. Thanks! ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:17 (Diff revisions 2 - 3) > + return promise.then(() => { > + ok(false, msg); > + }).catch(() => { > + ok(true, msg); > + }); One `.then` clause with success and failure handlers, please. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:260 (Diff revisions 2 - 3) > QueryInterface(iid) { > if (iid.equals(Ci.nsISupports) || > iid.equals(Ci.nsIInterfaceRequestor) || > iid.equals(Ci.nsIAuthPromptProvider) || > iid.equals(Ci.nsIAuthPrompt2)) { > return this; > } > - return callbacks.QueryInterface(iid); > + throw Cr.NS_ERROR_NO_INTERFACE; > }, `XPCOMUtils.generateQI`, please. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:269 (Diff revisions 2 - 3) > getInterface(iid) { > if (iid.equals(Ci.nsIAuthPromptProvider) || iid.equals(Ci.nsIAuthPrompt2)) { > return this; > } > - return callbacks.getInterface(iid); > + throw Cr.NS_ERROR_NO_INTERFACE; > }, This can also use `generateQI`. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:330 (Diff revisions 2 - 3) > + QueryInterface(iid) { > + if (iid.equals(Ci.nsISupports) || > + iid.equals(Ci.nsIInterfaceRequestor) || > + iid.equals(Ci.nsIAuthPrompt2)) { > + return this; > + } > + throw Cr.NS_ERROR_NO_INTERFACE; > + }, `XPCOMUtils.generateQI` please. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:351 (Diff revisions 2 - 3) > + }, > + asyncPromptAuth(channel, callback, context, level, authInfo) { > + // We just cancel here, we're only ensuring that non-webrequest > + // notificationcallbacks get called if webrequest doesn't handle it. > + Promise.resolve().then(() => { > + channel.cancel(Cr.NS_BINDING_ABORTED); I'm pretty sure we still need to call the auth prompt callback here, even if we cancel the request. ::: toolkit/modules/addons/WebRequest.jsm:406 (Diff revisions 2 - 3) > - this.notificationCallbacks = notificationCallbacks; > + this.notificationCallbacks = channel.notificationCallbacks; > + this.loadGroupCallbacks = channel.loadGroup && channel.loadGroup.notificationCallbacks; We should null both of these out when we're done, just to be safe. ::: toolkit/modules/addons/WebRequest.jsm:440 (Diff revisions 2 - 3) > getInterface(iid) { > if (iid.equals(Ci.nsIAuthPromptProvider) || iid.equals(Ci.nsIAuthPrompt2)) { > return this; > } > - if (this.notificationCallbacks) { > - return this.notificationCallbacks.getInterface(iid); > + try { > + return this._getForwardedInterface(iid); I think we only want to forward to the channel `notificationCallbacks` here, not the load group. Same for QI. ::: toolkit/modules/addons/WebRequest.jsm:480 (Diff revisions 2 - 3) > - let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL; > + let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL; > - prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider) > + prompt = this._getForwardedInterface(Ci.nsIAuthPromptProvider).getAuthPrompt(reason, Ci.nsIAuthPrompt2); > - .getAuthPrompt(reason, Ci.nsIAuthPrompt2); > - } catch (e) { > + } catch (e) { > - prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPrompt2); > + prompt = this._getForwardedInterface(Ci.nsIAuthPrompt2); > - } > + } This is slightly wrong. We need to check the channel's notifcationCallbacks for nsIAuthPromptProvider and nsIAuthPrompt2 before we fall back to checking the loadGroup for either. ::: toolkit/modules/addons/WebRequest.jsm:486 (Diff revisions 2 - 3) > + } catch (e) { > + Cu.reportError(`webRequest asyncPromptAuth failure ${e}`); > + } If we fail to get a prompt above, we need to call onAuthCanceled ourselves. And I think we also need to null out the `authPromptForward` and `authPromptCallback` callbacks here. Channels aren't cycle collected, so if these aren't cleared, the channel will leak.

Comment on attachment 8821372 [details] Bug 1190689 implement onAuthRequired for WebRequest, https://reviewboard.mozilla.org/r/100680/#review107150 > We should null both of these out when we're done, just to be safe. Discussed on irc. There's no good way to do this and it shouldn't be an issue, but we'll revisit if it is.

An additional try run with a webrequest addon that is loaded for all tests. Doing this to verify this wouldn't break other existing tests for http auth. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f5f06f3d8de5d6ee895811f230fdd6dcee3efa

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8b0fc745a65e implement onAuthRequired for WebRequest, r=kmag

I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onAuthRequired to be promise-y and remove references to asyncBlocking, and updated the compat data. Please let me know if we need anything else!

Docs LGTM