|
|
Created:
9 years, 3 months ago by cbentzel Modified:
9 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Erik does not do reviews, mihaip+watch_chromium.org, Aaron Boodman, darin-cc_chromium.org, Paweł Hajdan Jr., pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionwebRequest.onAuthRequired listeners can provide authentication credentials.
onAuthRequired listeners that specify "blocking" in the extraInfoSpec can return authentication credentials [username and password] in the BlockingResponse. If these are provided, Chrome will use these credentials rather than showing a login prompt for the user.
If "blocking" is not specified, or an authCredentials object is not present in the BlockingResponse, then a login prompt will be displayed.
Warning: If the authentication credentials are invalid, the extension may still present credentials for subsequent challenges. This could lead to infinite loops of bad credentials being entered without user intervention.
BUG=32056
TEST=Write an extension which does a blocking onAuthRequired and provides correct credentials, validate that it works.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104896
Patch Set 1 #Patch Set 2 : Handle multiple blocking onAuthRequired #
Total comments: 1
Patch Set 3 : Rebase #
Total comments: 8
Patch Set 4 : Lots of revisions #Patch Set 5 : Merge with trunk #Patch Set 6 : Browser tests #Patch Set 7 : API change, comment improvements #Patch Set 8 : Merge with head #Patch Set 9 : Fix tests #Patch Set 10 : Small fixes #Patch Set 11 : Fix tests #Patch Set 12 : Fix indent #
Total comments: 18
Patch Set 13 : Small fixes from battre #Patch Set 14 : Update description #Patch Set 15 : Merge #Messages
Total messages: 36 (0 generated)
I need to add tests, but otherwise I think this approach is OK. mpcomplete: Please review, particularly anything in chrome/*/extensions/ directories. battre: More of an FYI, not sure if there have been other people reviewing the NetworkDelegate changes. asanka: Please look at this due to your auth expertise. http://codereview.chromium.org/8015004/diff/2001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_webrequest_api_constants.h (right): http://codereview.chromium.org/8015004/diff/2001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_webrequest_api_constants.h:38: extern const char kAuthCredentialsKey[]; It didn't look like there was any sorting going on for these keys - please let me know if this is wrong.
thanks. willchan has reviewed most of my network related changes of the webrequest api. http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request.... net/url_request/url_request.cc:776: // OnBeforeSendHeaders. i think this comment becomes obsolete now. Could you please check whether all authentication can be handled with the new signal and, if yes, delete this. http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request.... net/url_request/url_request.cc:785: SetBlockedOnDelegate(); the corresponding SetUnblockedOnDelegate(); is missing http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request.... net/url_request/url_request.cc:804: // be equivalent to canceling authentication, probably yes http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request_... File net/url_request/url_request_test_util.cc (right): http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:372: kStageBeforeRedirect | please add a kStageAuthRequired here and edit TestNetworkDelegate::OnAuthRequired analogously. http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request_... net/url_request/url_request_test_util.cc:378: next_states_[req_id] |= kStageBeforeSendHeaders; please delete this (if it is resolved)
Thanks for the reviews. I'm going to split this into 2 CLs: One which adds the NetworkDelegate interface change and the URLRequest changes, the other which focuses on the extension implementation. http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request.... net/url_request/url_request.cc:785: SetBlockedOnDelegate(); On 2011/09/24 13:17:55, battre wrote: > the corresponding SetUnblockedOnDelegate(); is missing Done. http://codereview.chromium.org/8015004/diff/4001/net/url_request/url_request.... net/url_request/url_request.cc:804: // be equivalent to canceling authentication, On 2011/09/24 13:17:55, battre wrote: > probably yes Done.
LGTM http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_webrequest_api.h (right): http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_webrequest_api.h:348: // registered listener that supplies authentication credentials in a response, This comment is inaccurate. Since the |deltas| are sorted by install time from newest to oldest, the method actually accepts the most recently installed extension that has a response. (The code is correct, the comment is wrong.)
Chris, couple of questions after eyeballing: - what happens on browser startup, when extension might not be active yet? 407 dialogs pop up? How can the extension dismiss those once it starts? - auth realm is still weakly authenticating the server, no indication of https vs. http? - if extension and server disagree about creds, a 407 loop ensues? The extension writers need handle that by tracking some state, I presume? On 2011/09/26 21:19:45, Matt Perry wrote: > LGTM > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_webrequest_api.h (right): > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_webrequest_api.h:348: // registered listener > that supplies authentication credentials in a response, > This comment is inaccurate. Since the |deltas| are sorted by install time from > newest to oldest, the method actually accepts the most recently installed > extension that has a response. > > (The code is correct, the comment is wrong.)
On 2011/09/27 17:59:36, Marius wrote: > Chris, > couple of questions after eyeballing: > - what happens on browser startup, when extension might not be active yet? 407 > dialogs pop up? How can the extension dismiss those once it starts? This is a general problem for the webRequest API. We plan on delaying requests at startup until the necessary extensions are ready.
On Tue, Sep 27, 2011 at 2:11 PM, <mpcomplete@chromium.org> wrote: > On 2011/09/27 17:59:36, Marius wrote: >> >> Chris, >> couple of questions after eyeballing: >> - what happens on browser startup, when extension might not be active yet? >> 407 >> dialogs pop up? How can the extension dismiss those once it starts? > > This is a general problem for the webRequest API. We plan on delaying > requests > at startup until the necessary extensions are ready. For things in browser context this won't generally be an issue as they are retrieved via URLFetcher - which will just cancel the request rather than present auth prompt to the user. > > http://codereview.chromium.org/8015004/ >
On Tue, Sep 27, 2011 at 1:59 PM, <mschilder@google.com> wrote: > Chris, > couple of questions after eyeballing: > - what happens on browser startup, when extension might not be active yet? > 407 > dialogs pop up? How can the extension dismiss those once it starts? > - auth realm is still weakly authenticating the server, no indication of > https > vs. http? Good point. > - if extension and server disagree about creds, a 407 loop ensues? The > extension > writers need handle that by tracking some state, I presume? We could potentially short circuit the extension or the NetworkDelegate in this case and present the user with login prompts. You could imagine an extension which tries a number of possible credentials, so we may not want to bypass the extension after the first failure. > > On 2011/09/26 21:19:45, Matt Perry wrote: >> >> LGTM > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... >> >> File chrome/browser/extensions/extension_webrequest_api.h (right): > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... >> >> chrome/browser/extensions/extension_webrequest_api.h:348: // registered > > listener >> >> that supplies authentication credentials in a response, >> This comment is inaccurate. Since the |deltas| are sorted by install time >> from >> newest to oldest, the method actually accepts the most recently installed >> extension that has a response. > >> (The code is correct, the comment is wrong.) > > > > http://codereview.chromium.org/8015004/ >
The extension's part of handling the auth request appears fairly synchronous. What if the extension needs to do something (xhr, other page interaction, user interaction, ..) to get the creds? How can the extension suppress the dialog boxes until further notice? (probably until one of those in their realm does get creds?) On 2011/09/27 18:18:47, cbentzel wrote: > On Tue, Sep 27, 2011 at 1:59 PM, <mailto:mschilder@google.com> wrote: > > Chris, > > couple of questions after eyeballing: > > - what happens on browser startup, when extension might not be active yet? > > 407 > > dialogs pop up? How can the extension dismiss those once it starts? > > - auth realm is still weakly authenticating the server, no indication of > > https > > vs. http? > > Good point. > > > - if extension and server disagree about creds, a 407 loop ensues? The > > extension > > writers need handle that by tracking some state, I presume? > > We could potentially short circuit the extension or the > NetworkDelegate in this case and present the user with login prompts. > You could imagine an extension which tries a number of possible > credentials, so we may not want to bypass the extension after the > first failure. > > > > > On 2011/09/26 21:19:45, Matt Perry wrote: > >> > >> LGTM > > > > > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... > >> > >> File chrome/browser/extensions/extension_webrequest_api.h (right): > > > > > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... > >> > >> chrome/browser/extensions/extension_webrequest_api.h:348: // registered > > > > listener > >> > >> that supplies authentication credentials in a response, > >> This comment is inaccurate. Since the |deltas| are sorted by install time > >> from > >> newest to oldest, the method actually accepts the most recently installed > >> extension that has a response. > > > >> (The code is correct, the comment is wrong.) > > > > > > > > http://codereview.chromium.org/8015004/ > >
On Tue, Sep 27, 2011 at 3:56 PM, <mschilder@google.com> wrote: > The extension's part of handling the auth request appears fairly > synchronous. > What if the extension needs to do something (xhr, other page interaction, > user > interaction, ..) to get the creds? For now, this is intentional. We discussed passing in a callback which is invoked when the extension is ready, rather than using BlockedRequest. This is primarily to be consistent with the rest of the WebRequest extension API. However, in those cases the extension hooks need to run very quickly, as they may are in the critical path of potentially all network requests. This will be used only on a small set of network requests - and if the extension doesn't handle it, a human will which will eat up a lot of time. So, it may make sense to break consistency in this case. The callback can do synchronous XHR if needed here. > How can the extension suppress the dialog boxes until further notice? It needs to block. > (probably > until one of those in their realm does get creds?) > > On 2011/09/27 18:18:47, cbentzel wrote: >> >> On Tue, Sep 27, 2011 at 1:59 PM, <mailto:mschilder@google.com> wrote: >> > Chris, >> > couple of questions after eyeballing: >> > - what happens on browser startup, when extension might not be active >> > yet? >> > 407 >> > dialogs pop up? How can the extension dismiss those once it starts? >> > - auth realm is still weakly authenticating the server, no indication of >> > https >> > vs. http? > >> Good point. > >> > - if extension and server disagree about creds, a 407 loop ensues? The >> > extension >> > writers need handle that by tracking some state, I presume? > >> We could potentially short circuit the extension or the >> NetworkDelegate in this case and present the user with login prompts. >> You could imagine an extension which tries a number of possible >> credentials, so we may not want to bypass the extension after the >> first failure. > >> > >> > On 2011/09/26 21:19:45, Matt Perry wrote: >> >> >> >> LGTM >> > >> > >> > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... >> >> >> >> >> File chrome/browser/extensions/extension_webrequest_api.h (right): >> > >> > >> > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... >> >> >> >> >> chrome/browser/extensions/extension_webrequest_api.h:348: // registered >> > >> > listener >> >> >> >> that supplies authentication credentials in a response, >> >> This comment is inaccurate. Since the |deltas| are sorted by install >> >> time >> >> from >> >> newest to oldest, the method actually accepts the most recently >> >> installed >> >> extension that has a response. >> > >> >> (The code is correct, the comment is wrong.) >> > >> > >> > >> > http://codereview.chromium.org/8015004/ >> > > > > > http://codereview.chromium.org/8015004/ >
On Tue, Sep 27, 2011 at 4:03 PM, Chris Bentzel <cbentzel@chromium.org> wrote: > On Tue, Sep 27, 2011 at 3:56 PM, <mschilder@google.com> wrote: >> The extension's part of handling the auth request appears fairly >> synchronous. >> What if the extension needs to do something (xhr, other page interaction, >> user >> interaction, ..) to get the creds? > The callback can do synchronous XHR if needed here. Chatted with asanka, and this will clearly be a very bad thing if the URLRequest triggered by that XHR re-enters the extension API which is currently blocked. Perhaps this should move to a callback model to prevent things like that from happening. However, that introduces the possibility of a cascading chain of unsatisfied requests on a bad extension [first attempt through a proxy hits onAuthRequired, which triggers an XHR that goes through same proxy, which calls onAuthRequired again, ...] > >> How can the extension suppress the dialog boxes until further notice? > > It needs to block. >> (probably >> until one of those in their realm does get creds?) >> >> On 2011/09/27 18:18:47, cbentzel wrote: >>> >>> On Tue, Sep 27, 2011 at 1:59 PM, <mailto:mschilder@google.com> wrote: >>> > Chris, >>> > couple of questions after eyeballing: >>> > - what happens on browser startup, when extension might not be active >>> > yet? >>> > 407 >>> > dialogs pop up? How can the extension dismiss those once it starts? >>> > - auth realm is still weakly authenticating the server, no indication of >>> > https >>> > vs. http? >> >>> Good point. >> >>> > - if extension and server disagree about creds, a 407 loop ensues? The >>> > extension >>> > writers need handle that by tracking some state, I presume? >> >>> We could potentially short circuit the extension or the >>> NetworkDelegate in this case and present the user with login prompts. >>> You could imagine an extension which tries a number of possible >>> credentials, so we may not want to bypass the extension after the >>> first failure. >> >>> > >>> > On 2011/09/26 21:19:45, Matt Perry wrote: >>> >> >>> >> LGTM >>> > >>> > >>> > >> >> http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... >>> >>> >> >>> >> File chrome/browser/extensions/extension_webrequest_api.h (right): >>> > >>> > >>> > >> >> http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... >>> >>> >> >>> >> chrome/browser/extensions/extension_webrequest_api.h:348: // registered >>> > >>> > listener >>> >> >>> >> that supplies authentication credentials in a response, >>> >> This comment is inaccurate. Since the |deltas| are sorted by install >>> >> time >>> >> from >>> >> newest to oldest, the method actually accepts the most recently >>> >> installed >>> >> extension that has a response. >>> > >>> >> (The code is correct, the comment is wrong.) >>> > >>> > >>> > >>> > http://codereview.chromium.org/8015004/ >>> > >> >> >> >> http://codereview.chromium.org/8015004/ >> >
On 2011/09/27 20:22:55, cbentzel wrote: > On Tue, Sep 27, 2011 at 4:03 PM, Chris Bentzel <mailto:cbentzel@chromium.org> wrote: > > On Tue, Sep 27, 2011 at 3:56 PM, mailto: <mschilder@google.com> wrote: > >> The extension's part of handling the auth request appears fairly > >> synchronous. > >> What if the extension needs to do something (xhr, other page interaction, > >> user > >> interaction, ..) to get the creds? > > > The callback can do synchronous XHR if needed here. > > Chatted with asanka, and this will clearly be a very bad thing if the > URLRequest triggered by that XHR re-enters the extension API which is > currently blocked. We want to disable synchronous XHR from within webRequest event handlers. I think for a v1, we should go with the current approach, and only add async responses if extensions need them.
On Tue, Sep 27, 2011 at 1:03 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > On Tue, Sep 27, 2011 at 3:56 PM, <mschilder@google.com> wrote: > > The extension's part of handling the auth request appears fairly > > synchronous. > > What if the extension needs to do something (xhr, other page interaction, > > user > > interaction, ..) to get the creds? > > For now, this is intentional. > > We discussed passing in a callback which is invoked when the extension > is ready, rather than using BlockedRequest. This is primarily to be > consistent with the rest of the WebRequest extension API. > > However, in those cases the extension hooks need to run very quickly, > as they may are in the critical path of potentially all network > requests. This will be used only on a small set of network requests - > and if the extension doesn't handle it, a human will which will eat up > a lot of time. So, it may make sense to break consistency in this > case. > > The callback can do synchronous XHR if needed here. > > > How can the extension suppress the dialog boxes until further notice? > > It needs to block. > Here's my use case: extension hears about 407. extension looks in cookie jar for 'creds'. If not found, extension opens tab for user to login with real user creds, which in return cause a cookie to show up. Once cookie shows up, extension can populate the 407 dialog. How can this work with the current api? The only way i can see how is having the extension continuously provide bogus creds to the 407 dialog, causing network traffic etc (basically busy-wait). The extension cannot opt to block, since it needs to hear about the cookie events etc. > (probably > > until one of those in their realm does get creds?) > > > > On 2011/09/27 18:18:47, cbentzel wrote: > >> > >> On Tue, Sep 27, 2011 at 1:59 PM, <mailto:mschilder@google.com> wrote: > >> > Chris, > >> > couple of questions after eyeballing: > >> > - what happens on browser startup, when extension might not be active > >> > yet? > >> > 407 > >> > dialogs pop up? How can the extension dismiss those once it starts? > >> > - auth realm is still weakly authenticating the server, no indication > of > >> > https > >> > vs. http? > > > >> Good point. > > > >> > - if extension and server disagree about creds, a 407 loop ensues? The > >> > extension > >> > writers need handle that by tracking some state, I presume? > > > >> We could potentially short circuit the extension or the > >> NetworkDelegate in this case and present the user with login prompts. > >> You could imagine an extension which tries a number of possible > >> credentials, so we may not want to bypass the extension after the > >> first failure. > > > >> > > >> > On 2011/09/26 21:19:45, Matt Perry wrote: > >> >> > >> >> LGTM > >> > > >> > > >> > > > > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... > >> > >> >> > >> >> File chrome/browser/extensions/extension_webrequest_api.h (right): > >> > > >> > > >> > > > > > > http://codereview.chromium.org/8015004/diff/4001/chrome/browser/extensions/ex... > >> > >> >> > >> >> chrome/browser/extensions/extension_webrequest_api.h:348: // > registered > >> > > >> > listener > >> >> > >> >> that supplies authentication credentials in a response, > >> >> This comment is inaccurate. Since the |deltas| are sorted by install > >> >> time > >> >> from > >> >> newest to oldest, the method actually accepts the most recently > >> >> installed > >> >> extension that has a response. > >> > > >> >> (The code is correct, the comment is wrong.) > >> > > >> > > >> > > >> > http://codereview.chromium.org/8015004/ > >> > > > > > > > > > http://codereview.chromium.org/8015004/ > > >
On 2011/09/27 22:21:14, Marius wrote: > On Tue, Sep 27, 2011 at 1:03 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > > > On Tue, Sep 27, 2011 at 3:56 PM, <mailto:mschilder@google.com> wrote: > > > The extension's part of handling the auth request appears fairly > > > synchronous. > > > What if the extension needs to do something (xhr, other page interaction, > > > user > > > interaction, ..) to get the creds? > > > > For now, this is intentional. > > > > We discussed passing in a callback which is invoked when the extension > > is ready, rather than using BlockedRequest. This is primarily to be > > consistent with the rest of the WebRequest extension API. > > > > However, in those cases the extension hooks need to run very quickly, > > as they may are in the critical path of potentially all network > > requests. This will be used only on a small set of network requests - > > and if the extension doesn't handle it, a human will which will eat up > > a lot of time. So, it may make sense to break consistency in this > > case. > > > > The callback can do synchronous XHR if needed here. > > > > > How can the extension suppress the dialog boxes until further notice? > > > > It needs to block. > > > > Here's my use case: > > extension hears about 407. extension looks in cookie jar for 'creds'. If not > found, extension opens tab for user to login with real user creds, which in > return cause a cookie to show up. Once cookie shows up, extension can > populate the 407 dialog. > > How can this work with the current api? The only way i can see how is having > the extension continuously provide bogus creds to the 407 dialog, causing > network traffic etc (basically busy-wait). The extension cannot opt to > block, since it needs to hear about the cookie events etc. I picture it going like this: 1. extension gets onAuthRequired 2. extension doesn't have creds, so it returns an empty response 3. user logs in through normal dialog 4. extension is notified of auth login, with new creds. it saves them for next time Step 4 might require another piece of data provided in the API, but I think that's preferable to allowing indefinite blocking by extensions.
On Tue, Sep 27, 2011 at 3:29 PM, <mpcomplete@chromium.org> wrote: > On 2011/09/27 22:21:14, Marius wrote: > >> On Tue, Sep 27, 2011 at 1:03 PM, Chris Bentzel <cbentzel@chromium.org >> >wrote: >> > > > On Tue, Sep 27, 2011 at 3:56 PM, <mailto:mschilder@google.com> wrote: >> > > The extension's part of handling the auth request appears fairly >> > > synchronous. >> > > What if the extension needs to do something (xhr, other page >> interaction, >> > > user >> > > interaction, ..) to get the creds? >> > >> > For now, this is intentional. >> > >> > We discussed passing in a callback which is invoked when the extension >> > is ready, rather than using BlockedRequest. This is primarily to be >> > consistent with the rest of the WebRequest extension API. >> > >> > However, in those cases the extension hooks need to run very quickly, >> > as they may are in the critical path of potentially all network >> > requests. This will be used only on a small set of network requests - >> > and if the extension doesn't handle it, a human will which will eat up >> > a lot of time. So, it may make sense to break consistency in this >> > case. >> > >> > The callback can do synchronous XHR if needed here. >> > >> > > How can the extension suppress the dialog boxes until further notice? >> > >> > It needs to block. >> > >> > > Here's my use case: >> > > extension hears about 407. extension looks in cookie jar for 'creds'. If >> not >> found, extension opens tab for user to login with real user creds, which >> in >> return cause a cookie to show up. Once cookie shows up, extension can >> populate the 407 dialog. >> > > How can this work with the current api? The only way i can see how is >> having >> the extension continuously provide bogus creds to the 407 dialog, causing >> network traffic etc (basically busy-wait). The extension cannot opt to >> block, since it needs to hear about the cookie events etc. >> > > I picture it going like this: > 1. extension gets onAuthRequired > 2. extension doesn't have creds, so it returns an empty response > 3. user logs in through normal dialog > Define 'normal dialog'? In our use case, no way could the user come up with a proper password to put in the 407 dialog. It is machine generated, by another machine (a login / credential server). We explicitly do not want the user to be tempted to type their 'normal' username and password in these 407 dialogs. Hence better to hide them. 4. extension is notified of auth login, with new creds. it saves them for > next > time > > Step 4 might require another piece of data provided in the API, but I think > that's preferable to allowing indefinite blocking by extensions. > > > http://codereview.chromium.**org/8015004/<http://codereview.chromium.org/8015... >
On 2011/09/27 22:33:59, Marius wrote: > On Tue, Sep 27, 2011 at 3:29 PM, <mailto:mpcomplete@chromium.org> wrote: > > > On 2011/09/27 22:21:14, Marius wrote: > > > >> On Tue, Sep 27, 2011 at 1:03 PM, Chris Bentzel <mailto:cbentzel@chromium.org > >> >wrote: > >> > > > > > On Tue, Sep 27, 2011 at 3:56 PM, <mailto:mschilder@google.com> wrote: > >> > > The extension's part of handling the auth request appears fairly > >> > > synchronous. > >> > > What if the extension needs to do something (xhr, other page > >> interaction, > >> > > user > >> > > interaction, ..) to get the creds? > >> > > >> > For now, this is intentional. > >> > > >> > We discussed passing in a callback which is invoked when the extension > >> > is ready, rather than using BlockedRequest. This is primarily to be > >> > consistent with the rest of the WebRequest extension API. > >> > > >> > However, in those cases the extension hooks need to run very quickly, > >> > as they may are in the critical path of potentially all network > >> > requests. This will be used only on a small set of network requests - > >> > and if the extension doesn't handle it, a human will which will eat up > >> > a lot of time. So, it may make sense to break consistency in this > >> > case. > >> > > >> > The callback can do synchronous XHR if needed here. > >> > > >> > > How can the extension suppress the dialog boxes until further notice? > >> > > >> > It needs to block. > >> > > >> > > > > Here's my use case: > >> > > > > extension hears about 407. extension looks in cookie jar for 'creds'. If > >> not > >> found, extension opens tab for user to login with real user creds, which > >> in > >> return cause a cookie to show up. Once cookie shows up, extension can > >> populate the 407 dialog. > >> > > > > How can this work with the current api? The only way i can see how is > >> having > >> the extension continuously provide bogus creds to the 407 dialog, causing > >> network traffic etc (basically busy-wait). The extension cannot opt to > >> block, since it needs to hear about the cookie events etc. > >> > > > > I picture it going like this: > > 1. extension gets onAuthRequired > > 2. extension doesn't have creds, so it returns an empty response > > 3. user logs in through normal dialog > > > > Define 'normal dialog'? > > In our use case, no way could the user come up with a proper password to put > in the 407 dialog. > It is machine generated, by another machine (a login / credential server). > > We explicitly do not want the user to be tempted to type their 'normal' > username and password in these 407 dialogs. Hence better to hide them. OK, that's a good argument for having a callback. Chris, what do you think about Dominic's earlier proposal: adding another extraInfoSpec option "async_blocking" that gives you a callback to call later?
On 2011/09/28 00:02:30, Matt Perry wrote: > On 2011/09/27 22:33:59, Marius wrote: > > On Tue, Sep 27, 2011 at 3:29 PM, <mailto:mpcomplete@chromium.org> wrote: > > > > > On 2011/09/27 22:21:14, Marius wrote: > > > > > >> On Tue, Sep 27, 2011 at 1:03 PM, Chris Bentzel > <mailto:cbentzel@chromium.org > > >> >wrote: > > >> > > > > > > > On Tue, Sep 27, 2011 at 3:56 PM, <mailto:mschilder@google.com> wrote: > > >> > > The extension's part of handling the auth request appears fairly > > >> > > synchronous. > > >> > > What if the extension needs to do something (xhr, other page > > >> interaction, > > >> > > user > > >> > > interaction, ..) to get the creds? > > >> > > > >> > For now, this is intentional. > > >> > > > >> > We discussed passing in a callback which is invoked when the extension > > >> > is ready, rather than using BlockedRequest. This is primarily to be > > >> > consistent with the rest of the WebRequest extension API. > > >> > > > >> > However, in those cases the extension hooks need to run very quickly, > > >> > as they may are in the critical path of potentially all network > > >> > requests. This will be used only on a small set of network requests - > > >> > and if the extension doesn't handle it, a human will which will eat up > > >> > a lot of time. So, it may make sense to break consistency in this > > >> > case. > > >> > > > >> > The callback can do synchronous XHR if needed here. > > >> > > > >> > > How can the extension suppress the dialog boxes until further notice? > > >> > > > >> > It needs to block. > > >> > > > >> > > > > > > Here's my use case: > > >> > > > > > > extension hears about 407. extension looks in cookie jar for 'creds'. If > > >> not > > >> found, extension opens tab for user to login with real user creds, which > > >> in > > >> return cause a cookie to show up. Once cookie shows up, extension can > > >> populate the 407 dialog. > > >> > > > > > > How can this work with the current api? The only way i can see how is > > >> having > > >> the extension continuously provide bogus creds to the 407 dialog, causing > > >> network traffic etc (basically busy-wait). The extension cannot opt to > > >> block, since it needs to hear about the cookie events etc. > > >> > > > > > > I picture it going like this: > > > 1. extension gets onAuthRequired > > > 2. extension doesn't have creds, so it returns an empty response > > > 3. user logs in through normal dialog > > > > > > > Define 'normal dialog'? > > > > In our use case, no way could the user come up with a proper password to put > > in the 407 dialog. > > It is machine generated, by another machine (a login / credential server). > > > > We explicitly do not want the user to be tempted to type their 'normal' > > username and password in these 407 dialogs. Hence better to hide them. > > OK, that's a good argument for having a callback. > > Chris, what do you think about Dominic's earlier proposal: adding another > extraInfoSpec option "async_blocking" that gives you a callback to call later? (to be clear, I'm suggesting doing this in addition to the current approach)
On Tue, Sep 27, 2011 at 8:02 PM, <mpcomplete@chromium.org> wrote: > Chris, what do you think about Dominic's earlier proposal: adding another > extraInfoSpec option "async_blocking" that gives you a callback to call > later? Would it make more sense to just remove the blocking option, and always pass in a callback to be invoked? Although this is less consistent with the rest of the webRequest API, it's more consistent with other APIs. It would also reduce the amount of options we would need to support.
On 2011/09/28 00:50:36, cbentzel wrote: > On Tue, Sep 27, 2011 at 8:02 PM, <mailto:mpcomplete@chromium.org> wrote: > > > Chris, what do you think about Dominic's earlier proposal: adding another > > extraInfoSpec option "async_blocking" that gives you a callback to call > > later? > > Would it make more sense to just remove the blocking option, and > always pass in a callback to be invoked? > > Although this is less consistent with the rest of the webRequest API, > it's more consistent with other APIs. It would also reduce the amount > of options we would need to support. What if an extension just wanted to log the event, and not intervene? How would we know not to wait for the callback to run?
On Wed, Sep 28, 2011 at 2:45 PM, <mpcomplete@chromium.org> wrote: > On 2011/09/28 00:50:36, cbentzel wrote: >> >> On Tue, Sep 27, 2011 at 8:02 PM, <mailto:mpcomplete@chromium.org> wrote: > >> > Chris, what do you think about Dominic's earlier proposal: adding >> > another >> > extraInfoSpec option "async_blocking" that gives you a callback to call >> > later? > >> Would it make more sense to just remove the blocking option, and >> always pass in a callback to be invoked? > >> Although this is less consistent with the rest of the webRequest API, >> it's more consistent with other APIs. It would also reduce the amount >> of options we would need to support. > > What if an extension just wanted to log the event, and not intervene? How > would > we know not to wait for the callback to run? We couldn't require the callback to be invoked? If that can't be a requirement, then I think the async_blocking extraInfoSpec will be needed. > > http://codereview.chromium.org/8015004/ >
On 2011/09/28 19:03:36, cbentzel wrote: > On Wed, Sep 28, 2011 at 2:45 PM, <mailto:mpcomplete@chromium.org> wrote: > > On 2011/09/28 00:50:36, cbentzel wrote: > >> > >> On Tue, Sep 27, 2011 at 8:02 PM, <mailto:mpcomplete@chromium.org> wrote: > > > >> > Chris, what do you think about Dominic's earlier proposal: adding > >> > another > >> > extraInfoSpec option "async_blocking" that gives you a callback to call > >> > later? > > > >> Would it make more sense to just remove the blocking option, and > >> always pass in a callback to be invoked? > > > >> Although this is less consistent with the rest of the webRequest API, > >> it's more consistent with other APIs. It would also reduce the amount > >> of options we would need to support. > > > > What if an extension just wanted to log the event, and not intervene? How > > would > > we know not to wait for the callback to run? > > We couldn't require the callback to be invoked? If that can't be a > requirement, then I think the async_blocking extraInfoSpec will be > needed. I don't know of any way to require it. I think the async_blocking option is the way to go.
On 2011/09/28 19:19:25, Matt Perry wrote: > On 2011/09/28 19:03:36, cbentzel wrote: > > On Wed, Sep 28, 2011 at 2:45 PM, <mailto:mpcomplete@chromium.org> wrote: > > > On 2011/09/28 00:50:36, cbentzel wrote: > > >> > > >> On Tue, Sep 27, 2011 at 8:02 PM, <mailto:mpcomplete@chromium.org> > wrote: > > > > > >> > Chris, what do you think about Dominic's earlier proposal: adding > > >> > another > > >> > extraInfoSpec option "async_blocking" that gives you a callback to call > > >> > later? > > > > > >> Would it make more sense to just remove the blocking option, and > > >> always pass in a callback to be invoked? > > > > > >> Although this is less consistent with the rest of the webRequest API, > > >> it's more consistent with other APIs. It would also reduce the amount > > >> of options we would need to support. > > > > > > What if an extension just wanted to log the event, and not intervene? How > > > would > > > we know not to wait for the callback to run? > > > > We couldn't require the callback to be invoked? If that can't be a > > requirement, then I think the async_blocking extraInfoSpec will be > > needed. > > I don't know of any way to require it. I think the async_blocking option is the > way to go. Hi Guys, Any update here? Cheers, Ryan
No update on async_blocking. Hoping to land blocking change soon.
Please review again. A previous CL split the URLRequest/NetworkDelegate half of onAuthRequired out, and has landed. This focuses on the webRequest extension implementation. It also adds extension_webrequest_apitest tests.
http://codereview.chromium.org/8015004/diff/47020/chrome/common/extensions/do... File chrome/common/extensions/docs/experimental.devtools.console.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/common/extensions/do... chrome/common/extensions/docs/experimental.devtools.console.html:19: <title>chrome.experimental.devtools.console I'm not sure why these line breaks were introduced - this was done using the document building tools and I did not edit manually.
http://codereview.chromium.org/8015004/diff/47020/chrome/browser/extensions/e... File chrome/browser/extensions/extension_webrequest_api.cc (right): http://codereview.chromium.org/8015004/diff/47020/chrome/browser/extensions/e... chrome/browser/extensions/extension_webrequest_api.cc:1471: if (canceled) { This is a change the semantics of "cancel". The meaning in other signal handlers is "cancel the request, don't send any more messages to the server". http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_api.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_api.html:21: function (details) {}); nit: same format everywhere? http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_auth_required.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:1: <script src="framework.js"> as some tests are blocking, there is a certain chance that this will be flaky (test_blocking.html has been disabled for that reason). http://crbug.com/91715 http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:157: {}, ["responseHeaders"]); forgot "blocking"? http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:206: retval: {cancel: true} in all other handlers, {cancel: true} leads to "onErrorOccurred". I am not sure whether we want to stick to this alternate meaning of "cancel". http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:288: retval: {authCredentials: {username: "foo", password: "secret"}} we could add a test case where we specify the wrong credentials. Are we asked again? What was the conclusion about the question how often an extension may try to submit credentials?
Thanks for the fast review. http://codereview.chromium.org/8015004/diff/47020/chrome/browser/extensions/e... File chrome/browser/extensions/extension_webrequest_api.cc (right): http://codereview.chromium.org/8015004/diff/47020/chrome/browser/extensions/e... chrome/browser/extensions/extension_webrequest_api.cc:1471: if (canceled) { On 2011/10/10 13:47:03, battre wrote: > This is a change the semantics of "cancel". The meaning in other signal handlers > is "cancel the request, don't send any more messages to the server". This is actually the case as well here - it just displays whatever page was sent with the 401/407 response. In this case, we don't resend the request with auth credentials, nor do we allow the URLRequest::Delegate handle it. http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_api.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_api.html:21: function (details) {}); On 2011/10/10 13:47:03, battre wrote: > nit: same format everywhere? Done. http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_auth_required.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:157: {}, ["responseHeaders"]); On 2011/10/10 13:47:03, battre wrote: > forgot "blocking"? Oops! Thanks. http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:206: retval: {cancel: true} On 2011/10/10 13:47:03, battre wrote: > in all other handlers, {cancel: true} leads to "onErrorOccurred". I am not sure > whether we want to stick to this alternate meaning of "cancel". Good question. I mentioned overloading "cancel" on an email thread and mpcomplete sounded OK with it, but I'm happy to change this. http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:288: retval: {authCredentials: {username: "foo", password: "secret"}} On 2011/10/10 13:47:03, battre wrote: > we could add a test case where we specify the wrong credentials. Are we asked > again? Yes, we're asked again. I'll add a test case. > > What was the conclusion about the question how often an extension may try to > submit credentials? I don't think there was consensus on what should happen in that case. One problem if we don't ask a second time is that the extension gets no signal that the provided credentials are bad. We could potentially add an onAuthFailed() handler to deal with this case, but that's actually a bit tricky since the authDetails are not guaranteed to be the same each round.
http://codereview.chromium.org/8015004/diff/47020/chrome/browser/extensions/e... File chrome/browser/extensions/extension_webrequest_api.cc (right): http://codereview.chromium.org/8015004/diff/47020/chrome/browser/extensions/e... chrome/browser/extensions/extension_webrequest_api.cc:1471: if (canceled) { On 2011/10/10 13:59:54, cbentzel wrote: > On 2011/10/10 13:47:03, battre wrote: > > This is a change the semantics of "cancel". The meaning in other signal > handlers > > is "cancel the request, don't send any more messages to the server". > > This is actually the case as well here - it just displays whatever page was sent > with the 401/407 response. In this case, we don't resend the request with auth > credentials, nor do we allow the URLRequest::Delegate handle it. SGTM http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_auth_required.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:206: retval: {cancel: true} On 2011/10/10 13:59:54, cbentzel wrote: > On 2011/10/10 13:47:03, battre wrote: > > in all other handlers, {cancel: true} leads to "onErrorOccurred". I am not > sure > > whether we want to stick to this alternate meaning of "cancel". > > Good question. I mentioned overloading "cancel" on an email thread and > mpcomplete sounded OK with it, but I'm happy to change this. I guess I am OK with either approach. http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:288: retval: {authCredentials: {username: "foo", password: "secret"}} On 2011/10/10 13:59:54, cbentzel wrote: > On 2011/10/10 13:47:03, battre wrote: > > we could add a test case where we specify the wrong credentials. Are we asked > > again? > > Yes, we're asked again. I'll add a test case. > > > > > What was the conclusion about the question how often an extension may try to > > submit credentials? > > I don't think there was consensus on what should happen in that case. > > One problem if we don't ask a second time is that the extension gets no signal > that the provided credentials are bad. We could potentially add an > onAuthFailed() handler to deal with this case, but that's actually a bit tricky > since the authDetails are not guaranteed to be the same each round. How about adding a counter "authorizationAttempt"? The extension author could use this to stop trying to log in after a number of attempts that are appropriate in his opinion. The author could track this number himself but this way, it might be more explicit that he is encouraged to think about the number of failures.
The test for using an invalid password will require some changes to framework.js, since the event matching finds two onAuthRequired events which have matching details. http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_auth_required.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:288: retval: {authCredentials: {username: "foo", password: "secret"}} On 2011/10/10 14:32:13, battre wrote: > On 2011/10/10 13:59:54, cbentzel wrote: > > On 2011/10/10 13:47:03, battre wrote: > > > we could add a test case where we specify the wrong credentials. Are we > asked > > > again? > > > > Yes, we're asked again. I'll add a test case. > > > > > > > > What was the conclusion about the question how often an extension may try to > > > submit credentials? > > > > I don't think there was consensus on what should happen in that case. > > > > One problem if we don't ask a second time is that the extension gets no signal > > that the provided credentials are bad. We could potentially add an > > onAuthFailed() handler to deal with this case, but that's actually a bit > tricky > > since the authDetails are not guaranteed to be the same each round. > > How about adding a counter "authorizationAttempt"? The extension author could > use this to stop trying to log in after a number of attempts that are > appropriate in his opinion. > > The author could track this number himself but this way, it might be more > explicit that he is encouraged to think about the number of failures. Arguably the extension author could maintain that count as well, using the details as a dictionary into a map. I'm always hesitant to add additional parameters to the API if not needed.
I think the test for a second onAuthRequired events is not essential. LGTM from my side. http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_auth_required.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:288: retval: {authCredentials: {username: "foo", password: "secret"}} On 2011/10/10 14:38:06, cbentzel wrote: > On 2011/10/10 14:32:13, battre wrote: > > On 2011/10/10 13:59:54, cbentzel wrote: > > > On 2011/10/10 13:47:03, battre wrote: > > > > we could add a test case where we specify the wrong credentials. Are we > > asked > > > > again? > > > > > > Yes, we're asked again. I'll add a test case. > > > > > > > > > > > What was the conclusion about the question how often an extension may try > to > > > > submit credentials? > > > > > > I don't think there was consensus on what should happen in that case. > > > > > > One problem if we don't ask a second time is that the extension gets no > signal > > > that the provided credentials are bad. We could potentially add an > > > onAuthFailed() handler to deal with this case, but that's actually a bit > > tricky > > > since the authDetails are not guaranteed to be the same each round. > > > > How about adding a counter "authorizationAttempt"? The extension author could > > use this to stop trying to log in after a number of attempts that are > > appropriate in his opinion. > > > > The author could track this number himself but this way, it might be more > > explicit that he is encouraged to think about the number of failures. > > Arguably the extension author could maintain that count as well, using the > details as a dictionary into a map. I'm always hesitant to add additional > parameters to the API if not needed. Ok, but then we should highlight this problem in the API documentation.
I'm still working on the bad-credentials test case, but will do it in a separate CL since it requires changes to framework.js http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/webrequest/test_auth_required.html (right): http://codereview.chromium.org/8015004/diff/47020/chrome/test/data/extensions... chrome/test/data/extensions/api_test/webrequest/test_auth_required.html:288: retval: {authCredentials: {username: "foo", password: "secret"}} On 2011/10/10 14:47:10, battre wrote: > On 2011/10/10 14:38:06, cbentzel wrote: > > On 2011/10/10 14:32:13, battre wrote: > > > On 2011/10/10 13:59:54, cbentzel wrote: > > > > On 2011/10/10 13:47:03, battre wrote: > > > > > we could add a test case where we specify the wrong credentials. Are we > > > asked > > > > > again? > > > > > > > > Yes, we're asked again. I'll add a test case. > > > > > > > > > > > > > > What was the conclusion about the question how often an extension may > try > > to > > > > > submit credentials? > > > > > > > > I don't think there was consensus on what should happen in that case. > > > > > > > > One problem if we don't ask a second time is that the extension gets no > > signal > > > > that the provided credentials are bad. We could potentially add an > > > > onAuthFailed() handler to deal with this case, but that's actually a bit > > > tricky > > > > since the authDetails are not guaranteed to be the same each round. > > > > > > How about adding a counter "authorizationAttempt"? The extension author > could > > > use this to stop trying to log in after a number of attempts that are > > > appropriate in his opinion. > > > > > > The author could track this number himself but this way, it might be more > > > explicit that he is encouraged to think about the number of failures. > > > > Arguably the extension author could maintain that count as well, using the > > details as a dictionary into a map. I'm always hesitant to add additional > > parameters to the API if not needed. > > Ok, but then we should highlight this problem in the API documentation. SGTM.
mpcomplete: Do you want to do another pass over this?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbentzel@chromium.org/8015004/51003
Can't apply patch for file chrome/common/extensions/docs/experimental.devtools.console.html. While running patch -p1 --forward --force; patching file chrome/common/extensions/docs/experimental.devtools.console.html Hunk #1 FAILED at 16. 1 out of 1 hunk FAILED -- saving rejects to file chrome/common/extensions/docs/experimental.devtools.console.html.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbentzel@chromium.org/8015004/55002
Change committed as 104896 |