Added authentication scheme as key to HttpAuthCache.

Added authentication scheme as key to HttpAuthCache.

Behavioral changes are small; this is mostly a syntactic sugar change.  But
there are a few behavioral changes:
* If a web site replies with different schemes for the same realm, we'll have two entries in the cache.
* There will not be a log entry in HttpNetworkTransaction::SelectNextAuthIdentityToTry when we have the wrong authentication scheme (we don't see that entry any more)
* We will no longer return ERR_TUNNEL_CONNECTION_FAILED from SocketStream::HandleAuthChallenge when there's an entry in the cache with a non-basic authentication scheme (we won't know it's there).

Contributed by rdsmith@chromium.org

BUG=33433
TEST=HttpAuthCacheTest.* (as modified in this commit), HttpNetworkTransactionTest.*, SocketStreamTest.*, only on Linux.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47149

[cbentzel, 2010-05-05 16:15:42]

Overall looks good. I didn't spend much time going over with a fine-toothed comb
picking out nits - I'll do that after you address the issues below.

http://codereview.chromium.org/1949004/diff/1/3
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/1949004/diff/1/3#newcode22
net/http/http_auth_cache.h:22: //   - the realm name
The Entry also holds the authentication scheme.

http://codereview.chromium.org/1949004/diff/1/3#newcode23
net/http/http_auth_cache.h:23: //   - the origin server {scheme, host, port}
Change comment to indicate that the scheme is the network protocol scheme (like
HTTP) rather than the authentication scheme to minimize ambiguity.

http://codereview.chromium.org/1949004/diff/1/3#newcode106
net/http/http_auth_cache.h:106: const std::string scheme() const {
So, I think I'm going to move the handler_ out of the Entry as it's mostly not
needed and adds a lot of complexity. For this CL it's fine.

http://codereview.chromium.org/1949004/diff/1/4
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/1949004/diff/1/4#newcode72
net/http/http_auth_cache_unittest.cc:72: scoped_refptr<HttpAuthHandler>
realm3_handler =
name realm3_basic_handler and realm3_digest_handler instead of realm3a_handler
(which is confusing).

http://codereview.chromium.org/1949004/diff/1/4#newcode138
net/http/http_auth_cache_unittest.cc:138: // Lookup using empty path (may be
used for proxy).
Can you add a test to make sure that LookupByPath returns the digest version
when accessing a resource under /baz?

http://codereview.chromium.org/1949004/diff/1/4#newcode223
net/http/http_auth_cache_unittest.cc:223: // Fails, because there is no realm
"Realm4".
This unit test doesn't do lookups after Remove. It would be nice to test that
removing the basic handler for realm3 still handles Lookup correctly for the
digest handler for realm3.

[Randy Smith (Not in Mondays), 2010-05-05 18:04:09]

Comments incorporated, tests re-run, new patch uploaded.  One more round before
expanding the code review?

http://codereview.chromium.org/1949004/diff/1/3
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/1949004/diff/1/3#newcode22
net/http/http_auth_cache.h:22: //   - the realm name
On 2010/05/05 16:15:45, cbentzel wrote:
> The Entry also holds the authentication scheme.

The realm and authentication scheme are actually held within the handler.  I
hadn't noticed that it claims the realm is contained within the Entry.  I've
modified the comment to indicate that the realm & auth scheme are contained
within the handler.

http://codereview.chromium.org/1949004/diff/1/3#newcode23
net/http/http_auth_cache.h:23: //   - the origin server {scheme, host, port}
On 2010/05/05 16:15:45, cbentzel wrote:
> Change comment to indicate that the scheme is the network protocol scheme
(like
> HTTP) rather than the authentication scheme to minimize ambiguity.

Done.

http://codereview.chromium.org/1949004/diff/1/3#newcode106
net/http/http_auth_cache.h:106: const std::string scheme() const {
On 2010/05/05 16:15:45, cbentzel wrote:
> So, I think I'm going to move the handler_ out of the Entry as it's mostly not
> needed and adds a lot of complexity. For this CL it's fine.

Ok (i.e. understood but believe no action on my part is needed.)

http://codereview.chromium.org/1949004/diff/1/4
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/1949004/diff/1/4#newcode72
net/http/http_auth_cache_unittest.cc:72: scoped_refptr<HttpAuthHandler>
realm3_handler =
On 2010/05/05 16:15:45, cbentzel wrote:
> name realm3_basic_handler and realm3_digest_handler instead of realm3a_handler
> (which is confusing).

Done.

http://codereview.chromium.org/1949004/diff/1/4#newcode138
net/http/http_auth_cache_unittest.cc:138: // Lookup using empty path (may be
used for proxy).
On 2010/05/05 16:15:45, cbentzel wrote:
> Can you add a test to make sure that LookupByPath returns the digest version
> when accessing a resource under /baz?

Done.

http://codereview.chromium.org/1949004/diff/1/4#newcode223
net/http/http_auth_cache_unittest.cc:223: // Fails, because there is no realm
"Realm4".
On 2010/05/05 16:15:45, cbentzel wrote:
> This unit test doesn't do lookups after Remove. It would be nice to test that
> removing the basic handler for realm3 still handles Lookup correctly for the
> digest handler for realm3.

Done.

[cbentzel, 2010-05-06 01:11:40]

You should open this to a wider audience now.

http://codereview.chromium.org/1949004/diff/5001/6002
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/1949004/diff/5001/6002#newcode1
net/http/http_auth_cache.h:1: // Copyright (c) 2008 The Chromium Authors. All
rights reserved.
Nit: Update the date to 2010

http://codereview.chromium.org/1949004/diff/5001/6002#newcode35
net/http/http_auth_cache.h:35: //   |scheme| - the authentication scheme
Nit: end comment with a period.

http://codereview.chromium.org/1949004/diff/5001/6003
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/1949004/diff/5001/6003#newcode76
net/http/http_auth_cache_unittest.cc:76: scoped_refptr<HttpAuthHandler>
realm3_digest_handler =
Nit: please fix the 80-char line limit issues.

http://codereview.chromium.org/1949004/diff/5001/6003#newcode140
net/http/http_auth_cache_unittest.cc:140: HttpAuthCache::Entry*
realm3DigestEntry = cache.Lookup(origin, "Realm3", "digest");
Nit: CamelCaps aren't used for variable names, underscore_lowercase are.

[Randy Smith (Not in Mondays), 2010-05-06 18:10:41]

Chris and I have gone round a couple of times on this CL to (I hope!) get rid of
all the obvious problems, and I'd now like to open it up for larger review. 
Eric, I think it would be useful if you could look through the whole thing. 
Ukai: I'd specifically like you to look at the code that you gave an opinion on
in the issue (33433) in socket_stream.cc.  You're welcome to look at the rest of
the CL if you'd like, but my expectation is that Chris and Eric's eyes are
plenty :-}.

http://codereview.chromium.org/1949004/diff/5001/6002
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/1949004/diff/5001/6002#newcode1
net/http/http_auth_cache.h:1: // Copyright (c) 2008 The Chromium Authors. All
rights reserved.
On 2010/05/06 01:11:40, cbentzel wrote:
> Nit: Update the date to 2010

Done (here and in http_auth_cache.cc; the other files were already 2010).

I've seen copyrights that list many different years, but based on your response
I just updated the date to 2010 (which matches the other files); please let me
know if that's incorrect.

http://codereview.chromium.org/1949004/diff/5001/6002#newcode35
net/http/http_auth_cache.h:35: //   |scheme| - the authentication scheme
On 2010/05/06 01:11:40, cbentzel wrote:
> Nit: end comment with a period.

Done.

http://codereview.chromium.org/1949004/diff/5001/6003
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/1949004/diff/5001/6003#newcode76
net/http/http_auth_cache_unittest.cc:76: scoped_refptr<HttpAuthHandler>
realm3_digest_handler =
On 2010/05/06 01:11:40, cbentzel wrote:
> Nit: please fix the 80-char line limit issues.

Done.  Ooops.

http://codereview.chromium.org/1949004/diff/5001/6003#newcode140
net/http/http_auth_cache_unittest.cc:140: HttpAuthCache::Entry*
realm3DigestEntry = cache.Lookup(origin, "Realm3", "digest");
On 2010/05/06 01:11:40, cbentzel wrote:
> Nit: CamelCaps aren't used for variable names, underscore_lowercase are. 

Good point, though I was following the example about (realm2Entry).  I've
changed them both and scanned for other gotchas like this (in this file only
:-}).

[cbentzel, 2010-05-06 21:33:57]

LGTM

http://codereview.chromium.org/1949004/diff/5001/6002
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/1949004/diff/5001/6002#newcode1
net/http/http_auth_cache.h:1: // Copyright (c) 2008 The Chromium Authors. All
rights reserved.
On 2010/05/06 18:10:41, rdsmith wrote:
> On 2010/05/06 01:11:40, cbentzel wrote:
> > Nit: Update the date to 2010
> 
> Done (here and in http_auth_cache.cc; the other files were already 2010).
> 
> I've seen copyrights that list many different years, but based on your
response
> I just updated the date to 2010 (which matches the other files); please let me
> know if that's incorrect.

Just a single most recent year is the correct form.

[cbentzel, 2010-05-10 16:57:24]

Also - please remove the note to me from the CL description.

[Randy Smith (Not in Mondays), 2010-05-10 17:17:20]

On 2010/05/10 16:57:24, cbentzel wrote:
> Also - please remove the note to me from the CL description.

Done.

[eroman, 2010-05-10 17:34:11]

lgtm after a couple of nits.

> * If a web site replies with different schemes for the same realm, we'll have
two entries in the cache. 

nit: two --> "multiple".

http://codereview.chromium.org/1949004/diff/14001/15002
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/1949004/diff/14001/15002#newcode35
net/http/http_auth_cache.h:35: //   |scheme| - the authentication scheme.
Please mention that this is a case sensitive string (and expected to be
lower-case).

Or do a case-insensitive comparison during lookup.

http://codereview.chromium.org/1949004/diff/14001/15003
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/1949004/diff/14001/15003#newcode88
net/http/http_auth_cache_unittest.cc:88: "basic");
nit: indentation -- line up with the paren from "Lookup".

http://codereview.chromium.org/1949004/diff/14001/15003#newcode142
net/http/http_auth_cache_unittest.cc:142: cache.Lookup(origin, "Realm3",
"digest");
indent continued lines by 4.

http://codereview.chromium.org/1949004/diff/14001/15004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1949004/diff/14001/15004#newcode1843
net/http/http_network_transaction.cc:1843: // Disallow re-using of identity if
the scheme of the originating challenge
This comment block is no longer applicable, please remove it.

http://codereview.chromium.org/1949004/diff/14001/15005
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/1949004/diff/14001/15005#newcode884
net/socket_stream/socket_stream.cc:884: auth_cache_.Lookup(auth_origin,
auth_handler_->realm(), "basic");
nit: indent by an additional 2 spaces.

[Randy Smith (Not in Mondays), 2010-05-10 20:18:51]

Fixed nits from Eric.

http://codereview.chromium.org/1949004/diff/14001/15002
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/1949004/diff/14001/15002#newcode35
net/http/http_auth_cache.h:35: //   |scheme| - the authentication scheme.
On 2010/05/10 17:34:12, eroman wrote:
> Please mention that this is a case sensitive string (and expected to be
> lower-case).
> 
> Or do a case-insensitive comparison during lookup.

Done.

http://codereview.chromium.org/1949004/diff/14001/15003
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/1949004/diff/14001/15003#newcode88
net/http/http_auth_cache_unittest.cc:88: "basic");
On 2010/05/10 17:34:12, eroman wrote:
> nit: indentation -- line up with the paren from "Lookup".

Done.

http://codereview.chromium.org/1949004/diff/14001/15003#newcode142
net/http/http_auth_cache_unittest.cc:142: cache.Lookup(origin, "Realm3",
"digest");
On 2010/05/10 17:34:12, eroman wrote:
> indent continued lines by 4.

Done.

http://codereview.chromium.org/1949004/diff/14001/15004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1949004/diff/14001/15004#newcode1843
net/http/http_network_transaction.cc:1843: // Disallow re-using of identity if
the scheme of the originating challenge
On 2010/05/10 17:34:12, eroman wrote:
> This comment block is no longer applicable, please remove it.

Done.

http://codereview.chromium.org/1949004/diff/14001/15005
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/1949004/diff/14001/15005#newcode884
net/socket_stream/socket_stream.cc:884: auth_cache_.Lookup(auth_origin,
auth_handler_->realm(), "basic");
On 2010/05/10 17:34:12, eroman wrote:
> nit: indent by an additional 2 spaces.

Done.

[eroman, 2010-05-11 06:29:51]

LGTM

[ukai, 2010-05-12 06:16:51]

LGTM