Respect cookies set in a 401 responses when restarting the http transaction....

Respect cookies set in a 401 responses when restarting the http transaction.

There are two parts to this change:
(1) rebuild the request cookies before each transaction restart for
    authentication
(2) notify the URLRequestHttpJob of header completion before *each*
    transaction restart for authentication

By "each transaction" I mean the automatic restarts that don't require
user input, such as:
 - replying to the first step of NTLM
 - selecting identity embedded in URL
 - selecting identity in auth-cache

Needing to notify URLRequestHttpJob for these intermediate restarts is
a consequence of cookie store management being done outside of
HttpNetworkTransaction.

After updating the cookie store, URLRequestHttpJob now tests
|HttpTransaction::IsReadyToRestartForAuth()| to check whether the
notification was informational or an identity is actually needed.

R=wtc
BUG=6450
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12635

[eroman, 2009-03-21 23:53:41]

There is still a unit-test I want to add to this CL; will update with it
shortly.

http://codereview.chromium.org/51004/diff/1/7
File net/http/http_util.cc (right):

http://codereview.chromium.org/51004/diff/1/7#newcode221
Line 221: std::string HttpUtil::StripHeaders(const std::string& headers,
this code is unchanged from url_request.cc where I moved it from.

http://codereview.chromium.org/51004/diff/1/12
File net/url_request/url_request_http_job.cc (left):

http://codereview.chromium.org/51004/diff/1/12#oldcode58
Line 58: proxy_auth_state_(net::AUTH_STATE_DONT_NEED_AUTH),
I got rid of all this cruft, since it duplicates logic already in
HttpNetworkTransaction.

Probably a left-over from when there was winhttp vs newhttp or something.

[wtc, 2009-03-23 23:14:31]

LGTM.

The solution seems ad hoc, created specifically to address
the 401 set-cookie issue.  But I can't come up with a better
solution.

http://codereview.chromium.org/51004/diff/3001/4004
File net/http/http_auth.h (right):

http://codereview.chromium.org/51004/diff/3001/4004#newcode24
Line 24: AUTH_NONE = -1,
You should explain why AUTH_NONE can't be 0.  The
auth_handler_[2] and auth_identity_[2] members of
HttpNetworkTransaction depend on this.

http://codereview.chromium.org/51004/diff/3001/4003
File net/http/http_cache.cc (right):

http://codereview.chromium.org/51004/diff/3001/4003#newcode439
Line 439: if (!network_trans_.get())
Do we really need this null pointer check for
network_trans_?  Isn't it a bug if network_trans_ is NULL?

http://codereview.chromium.org/51004/diff/3001/4001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/51004/diff/3001/4001#newcode101
Line 101: if (auth_identity_[target].invalid) {
We should return ERR_UNEXPECTED immediately if target is
AUTH_NONE.

http://codereview.chromium.org/51004/diff/3001/4002
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/51004/diff/3001/4002#newcode159
Line 159: // HandleAuthChallenge() returns OK on success, and may update
We should keep the original comment that says this function
returns a network error code on failure.

http://codereview.chromium.org/51004/diff/3001/4002#newcode207
Line 207: // Whether this transaction is waiting for proxy auth, server auth, or
is
Nit: waiting for "the identity for" or "the credentials for"
... auth?

http://codereview.chromium.org/51004/diff/3001/4002#newcode208
Line 208: // not waiting for any auth at all. |pending_auth_target_| is popped
What do you mean by "popped"?  Cleared?

http://codereview.chromium.org/51004/diff/3001/4008
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/51004/diff/3001/4008#newcode337
Line 337: // Each time the HttpNetworkTransaction needs to restart (start a new
request),
This comment is confusing.  I believe "our callback" and
"us" refer to URLRequestHttpJob, but URLRequestHttpJob is
not used in this file.

"automatic auth restart" is also confusing because clearly
this function calls RestartWithAuth.

It seems that you're using this function to reduce the
amount of changes to the relevant unit tests.  You could
just "inline" this function in the relevant tests; it'll
be clearer, at the cost of more lines of code.

http://codereview.chromium.org/51004/diff/3001/4010
File net/http/http_transaction.h (right):

http://codereview.chromium.org/51004/diff/3001/4010#newcode62
Line 62: // chance to process the response headers from all of the intermediary
Nit: intermediary => intermediate?

http://codereview.chromium.org/51004/diff/3001/4005
File net/http/http_util_unittest.cc (right):

http://codereview.chromium.org/51004/diff/3001/4005#newcode56
Line 56: HttpUtil::StripHeaders(
Nit: align this line with the opening parenthesis ( on
the previous line.

http://codereview.chromium.org/51004/diff/3001/4011
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/51004/diff/3001/4011#newcode497
Line 497: if (transaction_->IsReadyToRestartForAuth()) {
Do you think we should move this code up, right after
the code that handles cookies?  This will allow us to
skip the sdch dictionary processing, and make it clear
that cookie processing is all we want to do.

[wtc, 2009-03-23 23:17:38]

http://codereview.chromium.org/51004/diff/1/12
File net/url_request/url_request_http_job.cc (left):

http://codereview.chromium.org/51004/diff/1/12#oldcode58
Line 58: proxy_auth_state_(net::AUTH_STATE_DONT_NEED_AUTH),
On 2009/03/21 23:53:41, eroman wrote:
> I got rid of all this cruft, since it duplicates logic already in
> HttpNetworkTransaction.
> 
> Probably a left-over from when there was winhttp vs newhttp or something.

This change is not in Patch Set 2, which I reviewed.
Should I ignore this then?

[eroman, 2009-03-24 23:29:54]

> This change is not in Patch Set 2, which I reviewed.
> Should I ignore this then?

Yes please disregard that comment.
(I need to do some more work before I can remove that, since ui tests depend on
it)

> There is still a unit-test I want to add to this CL; will update with it
> shortly.

Done.

Please see:

M      net\tools\testserver\testserver.py
M      net\url_request\url_request_unittest.cc

http://codereview.chromium.org/51004/diff/3001/4004
File net/http/http_auth.h (right):

http://codereview.chromium.org/51004/diff/3001/4004#newcode24
Line 24: AUTH_NONE = -1,
On 2009/03/23 23:14:31, wtc wrote:
> You should explain why AUTH_NONE can't be 0.  The
> auth_handler_[2] and auth_identity_[2] members of
> HttpNetworkTransaction depend on this.

Done.
Added comment:

// We depend on the valid targets (!= AUTH_NONE) being usable as indexes
// in an array, so start from 0.

http://codereview.chromium.org/51004/diff/3001/4003
File net/http/http_cache.cc (right):

http://codereview.chromium.org/51004/diff/3001/4003#newcode439
Line 439: if (!network_trans_.get())
On 2009/03/23 23:14:31, wtc wrote:
> Do we really need this null pointer check for
> network_trans_?  Isn't it a bug if network_trans_ is NULL?

network_trans_ may be NULL if it was reset after getting a 304 response.

http://codereview.chromium.org/51004/diff/3001/4001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/51004/diff/3001/4001#newcode101
Line 101: if (auth_identity_[target].invalid) {
On 2009/03/23 23:14:31, wtc wrote:
> We should return ERR_UNEXPECTED immediately if target is
> AUTH_NONE.

Done.

Replaced the DCHECK by:

if (target == HttpAuth::AUTH_NONE) {
  NOTREACHED();
  return ERR_UNEXPECTED;
}

http://codereview.chromium.org/51004/diff/3001/4002
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/51004/diff/3001/4002#newcode159
Line 159: // HandleAuthChallenge() returns OK on success, and may update
On 2009/03/23 23:14:31, wtc wrote:
> We should keep the original comment that says this function
> returns a network error code on failure.

Changed to:

// HandleAuthChallenge() returns a network error code, or OK on success.
// May update |pending_auth_target_| or |response_.auth_challenge|.

http://codereview.chromium.org/51004/diff/3001/4002#newcode207
Line 207: // Whether this transaction is waiting for proxy auth, server auth, or
is
On 2009/03/23 23:14:31, wtc wrote:
> Nit: waiting for "the identity for" or "the credentials for"
> ... auth?

I'm not sure how to phrase this. In the case of the automatic restarts, it may
not actually be waiting for an identity... it is simply waiting to continue the
restart with identity it already has.

http://codereview.chromium.org/51004/diff/3001/4002#newcode208
Line 208: // not waiting for any auth at all. |pending_auth_target_| is popped
On 2009/03/23 23:14:31, wtc wrote:
> What do you mean by "popped"?  Cleared?

Changed to:

"|pending_auth_target_| is read and cleared by RestartWithAuth()."

http://codereview.chromium.org/51004/diff/3001/4008
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/51004/diff/3001/4008#newcode337
Line 337: // Each time the HttpNetworkTransaction needs to restart (start a new
request),
On 2009/03/23 23:14:31, wtc wrote:
> This comment is confusing.  I believe "our callback" and
> "us" refer to URLRequestHttpJob, but URLRequestHttpJob is
> not used in this file.
> 
> "automatic auth restart" is also confusing because clearly
> this function calls RestartWithAuth.
> 
> It seems that you're using this function to reduce the
> amount of changes to the relevant unit tests.  You could
> just "inline" this function in the relevant tests; it'll
> be clearer, at the cost of more lines of code.

ok, inlined.

http://codereview.chromium.org/51004/diff/3001/4010
File net/http/http_transaction.h (right):

http://codereview.chromium.org/51004/diff/3001/4010#newcode62
Line 62: // chance to process the response headers from all of the intermediary
On 2009/03/23 23:14:31, wtc wrote:
> Nit: intermediary => intermediate?

Done.

http://codereview.chromium.org/51004/diff/3001/4005
File net/http/http_util_unittest.cc (right):

http://codereview.chromium.org/51004/diff/3001/4005#newcode56
Line 56: HttpUtil::StripHeaders(
On 2009/03/23 23:14:31, wtc wrote:
> Nit: align this line with the opening parenthesis ( on
> the previous line.

Done.

http://codereview.chromium.org/51004/diff/3001/4011
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/51004/diff/3001/4011#newcode497
Line 497: if (transaction_->IsReadyToRestartForAuth()) {
On 2009/03/23 23:14:31, wtc wrote:
> Do you think we should move this code up, right after
> the code that handles cookies?  This will allow us to
> skip the sdch dictionary processing, and make it clear
> that cookie processing is all we want to do.

I think I will leave it here for now, for consistency.

If I move it up then it means there are certain 401s where a Get-Dictionary is
processed, but in other cases it isn't.

[wtc, 2009-03-25 17:55:53]

LGTM.  Nice work!

http://codereview.chromium.org/51004/diff/3001/4003
File net/http/http_cache.cc (right):

http://codereview.chromium.org/51004/diff/3001/4003#newcode439
Line 439: if (!network_trans_.get())
On 2009/03/24 23:29:54, eroman wrote:
>
> network_trans_ may be NULL if it was reset after getting a 304 response.

I see.  I guess we either need this null check here, or
we need to protect the IsReadyToRestartAuth() call in
URLRequestHttpJob by checking that the response code is
401 or 407.

http://codereview.chromium.org/51004/diff/4014/3009
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/51004/diff/4014/3009#newcode1615
Line 1615: // After automatically restarting with a null identity, this is the
Nit: We should remove "automatically" from this comment, and
the same comment in the next unit test.  Now
HttpNetworkTransaction no longer automatically restarts
itself; it is restarted (automatically) by
URLRequestHttpJob.

http://codereview.chromium.org/51004/diff/4017/4032
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/51004/diff/4017/4032#newcode938
Line 938: // Check that cookie headers in 401 responses are respected.
Nit: Set-Cookie headers

http://codereview.chromium.org/51004/diff/4017/4032#newcode946
Line 946: server->TestServerPage("auth-basic?setCookieIfChallenged");
Nit: we seem to use dashes (-) instead of camelCaps in
the TestServerPage URLs.

http://codereview.chromium.org/51004/diff/4017/4032#newcode970
Line 970: // (without user intervention).
You may want to point out this is because identity is
embedded in the URL.

Why isn't the HTTP auth cache being used?  Is it because
the two URLRequests don't share the same URLRequestContext?

http://codereview.chromium.org/51004/diff/4017/4032#newcode976
Line 976: std::string username("victoria");
Is this another test to see if I am really reviewing?

[eroman, 2009-03-26 00:42:42]

http://codereview.chromium.org/51004/diff/3001/4003
File net/http/http_cache.cc (right):

http://codereview.chromium.org/51004/diff/3001/4003#newcode439
Line 439: if (!network_trans_.get())
On 2009/03/25 17:55:53, wtc wrote:
> On 2009/03/24 23:29:54, eroman wrote:
> >
> > network_trans_ may be NULL if it was reset after getting a 304 response.
> 
> I see.  I guess we either need this null check here, or
> we need to protect the IsReadyToRestartAuth() call in
> URLRequestHttpJob by checking that the response code is
> 401 or 407.
> 
> 

I will leave this as-is for now.

(trying to consolidate the 401/407 checks into just HttpNetworkTransaction).

http://codereview.chromium.org/51004/diff/4014/3009
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/51004/diff/4014/3009#newcode1615
Line 1615: // After automatically restarting with a null identity, this is the
On 2009/03/25 17:55:54, wtc wrote:
> Nit: We should remove "automatically" from this comment, and
> the same comment in the next unit test.  Now
> HttpNetworkTransaction no longer automatically restarts
> itself; it is restarted (automatically) by
> URLRequestHttpJob.

Done.
Good catch!

http://codereview.chromium.org/51004/diff/4017/4032
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/51004/diff/4017/4032#newcode938
Line 938: // Check that cookie headers in 401 responses are respected.
On 2009/03/25 17:55:54, wtc wrote:
> Nit: Set-Cookie headers

Done.

http://codereview.chromium.org/51004/diff/4017/4032#newcode946
Line 946: server->TestServerPage("auth-basic?setCookieIfChallenged");
On 2009/03/25 17:55:54, wtc wrote:
> Nit: we seem to use dashes (-) instead of camelCaps in
> the TestServerPage URLs.

Done.

http://codereview.chromium.org/51004/diff/4017/4032#newcode970
Line 970: // (without user intervention).
On 2009/03/25 17:55:54, wtc wrote:
> You may want to point out this is because identity is
> embedded in the URL.

Done.

> Why isn't the HTTP auth cache being used?  Is it because
> the two URLRequests don't share the same URLRequestContext?

Correct. I put them in their own scopes to keep the cases separate.

http://codereview.chromium.org/51004/diff/4017/4032#newcode976
Line 976: std::string username("victoria");
On 2009/03/25 17:55:54, wtc wrote:
> Is this another test to see if I am really reviewing?

what?! ... its legit :)

Ok, changed to "user2".

(Note, I want to use a different username from the earlier test just to make it
harder for the test to get messed up from earlier state and pass when it should
be failing).