Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13609002:
fix a problem that android cannot download files with basic authentication (Closed)
|
Created:
7 years, 8 months ago by qinmin Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, joi+watch-content_chromium.org, melevin, samarth+watch_chromium.org, benjhayden+dwatch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, Paweł Hajdan Jr., jam, gideonwald, sreeram, dominich, darin-cc_chromium.org, David Black, rdsmith+dwatch_chromium.org, tfarina, kmadhusu+watch_chromium.org, android-webview-reviews_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionfix a problem that android cannot download files with basic authentication
Android download manager does not support authentication headers.
This change adds a call to check if a url request contains authentication headers.
If so, we fallback to chrome download path instead of using the download manager.
BUG=159687
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193318
Patch Set 1 #
Total comments: 4
Patch Set 2 : adding a call in http_transactions to get the auth info #
Total comments: 2
Patch Set 3 : moving hasAuth to httpResponseInfo #Patch Set 4 : rebase #Patch Set 5 : nits #
Total comments: 6
Patch Set 6 : addressing comments #
Total comments: 2
Patch Set 7 : removing some redundant code #Patch Set 8 : nits #
Messages
Total messages: 23 (0 generated)
PTAL
aw/ lgtm https://codereview.chromium.org/13609002/diff/1/content/public/browser/web_co... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/13609002/diff/1/content/public/browser/web_co... content/public/browser/web_contents_delegate.h:226: bool has_auth); @jam - in line with your recent CL https://codereview.chromium.org/13409003/ --- note how the CanDownload() WCD method is not actually called from within the content module at all! It's only purpose is to let chrome/ code talk to other chrome & extension and CF code.
I think things will be much simpler if we add an android specific resource throttle which makes the decision of allowing a download or not. We can make this decision on the IO thread (looking at the request object) and cancel the request accordingly. If we add this new throttle after the existing DownloadResourceThrottle we can get the "this page is trying too many downloads" infobar for GET downloads too. What do you guys think?
You will bypass some calls on the UI thread in download_request_limiter.cc if you return early on the IO thread. On 2013/04/04 03:11:59, nilesh wrote: > I think things will be much simpler if we add an android specific resource > throttle which makes the decision of allowing a download or not. > We can make this decision on the IO thread (looking at the request object) and > cancel the request accordingly. If we add this new throttle after the existing > DownloadResourceThrottle we can get the "this page is trying too many downloads" > infobar for GET downloads too. > What do you guys think?
In the bug, comment #7 suggests that the Classic Browser supports this - it was my understanding it just passed downloads to the Download Manager too. Does it also have a special case for Authenticated downloads?
I consider Asanka a better person than I for a Downloads/Auth crossover issue.
On 2013/04/04 10:37:44, benm wrote: > In the bug, comment #7 suggests that the Classic Browser supports this - it was > my understanding it just passed downloads to the Download Manager too. Does it > also have a special case for Authenticated downloads? hmm...I just tried andrioid browser on my nexus S, and it failed to download if there is basic authentication. Either I am missing sth or the report is wrong.
https://codereview.chromium.org/13609002/diff/1/content/public/browser/web_co... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/13609002/diff/1/content/public/browser/web_co... content/public/browser/web_contents_delegate.h:226: bool has_auth); On 2013/04/04 03:10:15, joth wrote: > @jam - in line with your recent CL https://codereview.chromium.org/13409003/ --- > note how the CanDownload() WCD method is not actually called from within the > content module at all! It's only purpose is to let chrome/ code talk to other > chrome & extension and CF code. oh, in that case that's a bug. i had gone through before and removed all such WCD methods. i may have missed this one, or it may have gotten added after. either way we should remove this and do it another way.
It seems the signal you are looking for is whether any authorization headers were added to the request. The addition of authorization headers happen in HttpNetworkTransaction::BuildRequestHeaders(). So this information would have to be picked up from there. Also, the flag could be added to HttpResponseInfo instead of adding a method to URLRequest. https://chromiumcodereview.appspot.com/13609002/diff/1/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://chromiumcodereview.appspot.com/13609002/diff/1/net/url_request/url_re... net/url_request/url_request_http_job.cc:1098: server_auth_state_ == AUTH_STATE_HAVE_AUTH; These aren't set if the request used cached credentials.
On 2013/04/04 16:19:51, qinmin wrote: > On 2013/04/04 10:37:44, benm wrote: > > In the bug, comment #7 suggests that the Classic Browser supports this - it > was > > my understanding it just passed downloads to the Download Manager too. Does it > > also have a special case for Authenticated downloads? > > hmm...I just tried andrioid browser on my nexus S, and it failed to download if > there is basic authentication. Either I am missing sth or the report is wrong. I suspect the report is wrong. I don't recall special handling for this in the classic browser.
Added a HasAuth() call in HttpTransaction to check if the request has authentication credentials. This call will return true if BuildRequestHeaders() adds auth to the request. https://chromiumcodereview.appspot.com/13609002/diff/1/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://chromiumcodereview.appspot.com/13609002/diff/1/net/url_request/url_re... net/url_request/url_request_http_job.cc:1098: server_auth_state_ == AUTH_STATE_HAVE_AUTH; Use transaction_->HasAuth() to get the credentials. On 2013/04/04 17:20:31, asanka wrote: > These aren't set if the request used cached credentials.
On 2013/04/04 03:28:22, qinmin wrote: > You will bypass some calls on the UI thread in download_request_limiter.cc if > you return early on the IO thread. > > On 2013/04/04 03:11:59, nilesh wrote: > > I think things will be much simpler if we add an android specific resource > > throttle which makes the decision of allowing a download or not. > > We can make this decision on the IO thread (looking at the request object) and > > cancel the request accordingly. If we add this new throttle after the existing > > DownloadResourceThrottle we can get the "this page is trying too many > downloads" > > infobar for GET downloads too. > > What do you guys think? Please take a look at: https://codereview.chromium.org/13649009/ Using this we dont need to pass all the bits needed to make a "allow download" decision to the WebContentsDelegate
Have you considered using HttpResponseInfo to indicate whether HTTP authentication was used during the fetch? (as opposed to adding accessors). https://chromiumcodereview.appspot.com/13609002/diff/14001/net/http/http_netw... File net/http/http_network_transaction.h (right): https://chromiumcodereview.appspot.com/13609002/diff/14001/net/http/http_netw... net/http/http_network_transaction.h:55: virtual bool HasAuth() const OVERRIDE; This is confusingly similar to HaveAuth() below but has different semantics.
ok, moved the logic into HttpResponseInfo https://chromiumcodereview.appspot.com/13609002/diff/14001/net/http/http_netw... File net/http/http_network_transaction.h (right): https://chromiumcodereview.appspot.com/13609002/diff/14001/net/http/http_netw... net/http/http_network_transaction.h:55: virtual bool HasAuth() const OVERRIDE; moved this logic to httpresponseinfo and renamed it to request_has_auth On 2013/04/05 16:04:50, asanka wrote: > This is confusingly similar to HaveAuth() below but has different semantics.
rebase after https://codereview.chromium.org/13649009/, asanka, would you please take a look again？
https://codereview.chromium.org/13609002/diff/33001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/13609002/diff/33001/net/http/http_network_tra... net/http/http_network_transaction.cc:781: request_headers_.MergeFrom(request_->extra_headers); I'm sorry I didn't see this earlier. Authorization headers could also come from request_->extra_headers. In this case, the flag in response_ may be incorrect. You'll need to check for the kAuthorization and kProxyAuthorization headers after all the headers are done to check if the request is going to use HTTP auth. This is unlikely to be a problem on Android, but since this flag is going to exist on all platforms, we should make sure it's correct. https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... File net/http/http_response_info.cc (right): https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... net/http/http_response_info.cc:86: Can you add a flag here and persist the auth-used flag? If the response is cached and then later the browser tries to download the same resource, I think you'd want to know if it used HTTP auth the first time. https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... File net/http/http_response_info.h (right): https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... net/http/http_response_info.h:79: bool request_has_auth; Nit: This field indicates the state of the request prior to it being sent. It should be something like "did_use_http_auth." The comment isn't quite correct either. The network stack supports SSL client authentication, HTTP proxy authentication and HTTP server authentication. This flag means that either proxy or server authentication was used for the corresponding request. Certificates are currently only used for SSL client authentication.
https://codereview.chromium.org/13609002/diff/33001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/13609002/diff/33001/net/http/http_network_tra... net/http/http_network_transaction.cc:781: request_headers_.MergeFrom(request_->extra_headers); On 2013/04/09 17:38:51, asanka wrote: > I'm sorry I didn't see this earlier. Authorization headers could also come from > request_->extra_headers. In this case, the flag in response_ may be incorrect. > > You'll need to check for the kAuthorization and kProxyAuthorization headers > after all the headers are done to check if the request is going to use HTTP > auth. > > This is unlikely to be a problem on Android, but since this flag is going to > exist on all platforms, we should make sure it's correct. Done. https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... File net/http/http_response_info.cc (right): https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... net/http/http_response_info.cc:86: On 2013/04/09 17:38:51, asanka wrote: > Can you add a flag here and persist the auth-used flag? If the response is > cached and then later the browser tries to download the same resource, I think > you'd want to know if it used HTTP auth the first time. Done. https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... File net/http/http_response_info.h (right): https://codereview.chromium.org/13609002/diff/33001/net/http/http_response_in... net/http/http_response_info.h:79: bool request_has_auth; On 2013/04/09 17:38:51, asanka wrote: > Nit: This field indicates the state of the request prior to it being sent. It > should be something like "did_use_http_auth." The comment isn't quite correct > either. > > The network stack supports SSL client authentication, HTTP proxy authentication > and HTTP server authentication. This flag means that either proxy or server > authentication was used for the corresponding request. > > Certificates are currently only used for SSL client authentication. Done.
https://codereview.chromium.org/13609002/diff/40001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/13609002/diff/40001/net/http/http_network_tra... net/http/http_network_transaction.cc:773: has_auth = true; These are redundant with the check below. If the AddAuthorizationHeader() calls succeed, then the headers would be present when you check them again below. If they fail, then the request is technically not using authentication.
https://codereview.chromium.org/13609002/diff/40001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/13609002/diff/40001/net/http/http_network_tra... net/http/http_network_transaction.cc:773: has_auth = true; ah... ok, done On 2013/04/09 21:55:13, asanka wrote: > These are redundant with the check below. If the AddAuthorizationHeader() calls > succeed, then the headers would be present when you check them again below. If > they fail, then the request is technically not using authentication.
LGTM
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/13609002/47001
Message was sent while issue was closed.
Change committed as 193318 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
