Invalidate credentials if the server rejects them.

Check and invalidate cached credentials if they were used for preemptive authentication and were rejected by the server.

BUG=72589
TEST=net_unittests --gtest_filter=HttpAuthHandler*.HandleAnotherChallenge

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

[asanka, 2011-02-16 15:39:13]

For review.

[cbentzel, 2011-02-16 16:13:05]

http://codereview.chromium.org/6525035/diff/1014/net/http/http_auth_controlle...
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/6525035/diff/1014/net/http/http_auth_controlle...
net/http/http_auth_controller.cc:273: case
HttpAuth::AUTHORIZATION_RESULT_ACCEPT:
Would it be cleaner if the new realm was done in HandleChallengeResponse, and
return a new AUTHORIZATION_RESULT_DIFFERENT_REALM which simply does
handler_.reset() and identity_ = HttpAuth::Identity() without rejecting from
cache?

One downside is that it does mean the following sequence would result in cache
credentials for realm=foo to be stored.

-> GET /resource
<- 401, WWW-Authenticate:Basic realm=foo

-> GET /resource, Authorization: Basic XUHHFC
<- 401, WWW-Authenticate:Basic realm=bar

Should you write a simple server and see how other browsers handle the nested
realm case?

[cbentzel, 2011-02-16 16:19:37]

On Wed, Feb 16, 2011 at 11:13 AM, <cbentzel@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6525035/diff/1014/net/http/http_auth_controlle...
> File net/http/http_auth_controller.cc (right):
>
>
>
http://codereview.chromium.org/6525035/diff/1014/net/http/http_auth_controlle...
> net/http/http_auth_controller.cc:273: case
> HttpAuth::AUTHORIZATION_RESULT_ACCEPT:
> Would it be cleaner if the new realm was done in
> HandleChallengeResponse, and return a new
> AUTHORIZATION_RESULT_DIFFERENT_REALM which simply does handler_.reset()
> and identity_ = HttpAuth::Identity() without rejecting from cache?
>

In this case, you would only need to add the realm logic to Basic + possible
Digest. NTLM and Negotiate have no notion of realms, despite the RFC.

[asanka, 2011-02-16 16:33:35]

On 2011/02/16 16:13:05, cbentzel wrote:
>
http://codereview.chromium.org/6525035/diff/1014/net/http/http_auth_controlle...
> net/http/http_auth_controller.cc:273: case
> HttpAuth::AUTHORIZATION_RESULT_ACCEPT:
> Would it be cleaner if the new realm was done in HandleChallengeResponse, and
> return a new AUTHORIZATION_RESULT_DIFFERENT_REALM which simply does
> handler_.reset() and identity_ = HttpAuth::Identity() without rejecting from
> cache?

Yeah.  I think that would be cleaner.  The abstraction I was going for was to
let each handler subclass have the option of determining whether to evict cached
credentials.  But I think in reality we are only going to deal with the
Basic/Digest case where we evict credentials based on the realm.

> One downside is that it does mean the following sequence would result in cache
> credentials for realm=foo to be stored.
> 
> -> GET /resource
> <- 401, WWW-Authenticate:Basic realm=foo
> 
> -> GET /resource, Authorization: Basic XUHHFC
> <- 401, WWW-Authenticate:Basic realm=bar
>
> Should you write a simple server and see how other browsers handle the nested
> realm case?

I'll test other browsers to see how they handle that case.

We don't seem to be dealing well with the nested realm case in other ways.  For
example, HttpAuthCache::LookupByPath() returns the first cache entry found to
enclose the target path, instead of the tightest enclosing path.

[cbentzel, 2011-02-16 16:36:15]

On Wed, Feb 16, 2011 at 11:33 AM, <asanka@chromium.org> wrote:

> On 2011/02/16 16:13:05, cbentzel wrote:
>
>
>
http://codereview.chromium.org/6525035/diff/1014/net/http/http_auth_controlle...
>
>> net/http/http_auth_controller.cc:273: case
>> HttpAuth::AUTHORIZATION_RESULT_ACCEPT:
>> Would it be cleaner if the new realm was done in HandleChallengeResponse,
>> and
>> return a new AUTHORIZATION_RESULT_DIFFERENT_REALM which simply does
>> handler_.reset() and identity_ = HttpAuth::Identity() without rejecting
>> from
>> cache?
>>
>
> Yeah.  I think that would be cleaner.  The abstraction I was going for was
> to
> let each handler subclass have the option of determining whether to evict
> cached
> credentials.  But I think in reality we are only going to deal with the
> Basic/Digest case where we evict credentials based on the realm.
>
>
>  One downside is that it does mean the following sequence would result in
>> cache
>> credentials for realm=foo to be stored.
>>
>
>  -> GET /resource
>> <- 401, WWW-Authenticate:Basic realm=foo
>>
>
>  -> GET /resource, Authorization: Basic XUHHFC
>> <- 401, WWW-Authenticate:Basic realm=bar
>>
>
>  Should you write a simple server and see how other browsers handle the
>> nested
>> realm case?
>>
>
> I'll test other browsers to see how they handle that case.
>
> We don't seem to be dealing well with the nested realm case in other ways.
>  For
> example, HttpAuthCache::LookupByPath() returns the first cache entry found
> to
> enclose the target path, instead of the tightest enclosing path.


OK.

As you mentioned, I'm not even sure it's a legitimate case. Let's see how
other browsers handle nested realms.

[asanka, 2011-02-16 18:52:14]

On 2011/02/16 16:36:15, cbentzel wrote:
> As you mentioned, I'm not even sure it's a legitimate case. Let's see how
> other browsers handle nested realms.

I tested Firefox 3.6 on Linux and IE 8 on Windows.  The test
setup was a server with this configuration:

  http://example.com/realm/a  : Requires basic auth with realm A
  http://example.com/realm/a/b: Requires basic auth with realm B


On Firefox, the results differ based on whether resource 'a' or
'a/b' is visited first.  If 'a/b' is visited first and then 'a',
then the browser behaves correctly on subsequent visits to both
directories presenting correct pre-emptive authentication.  If
'a' is visited first and then 'a/b', subsequent visits to 'a/b'
preemptively present credentials for realm A, receive a 401, and
then present credentials for B.

On IE, if 'a' is visited before 'a/b', the results are correct.
If 'a/b' is visited before 'a', then credentials for 'a/b' are
dropped altogether.  Each subsequent visit to 'a/b' require user
interaction.

Chrome doesn't lose cached credentials in this case since the
rejected authentication attempt is preemptive.  But since
credentials selection isn't based on closest enclosing path,
Chrome's behavior is similar to Firefox.

[asanka, 2011-02-16 19:50:33]

Simplified patch by handling the realm test in HandleAnotherChallenge().  We do
end up polluting the auth cache in the case Chris pointed out.  But this case
indicates misbehavior on part of the server and requires user interaction for
each new realm.  Also we don't allow unbounded growth of the auth cache.  These
together, I think, provides enough mitigation.

[cbentzel, 2011-02-16 20:51:39]

New code looks good, just some test comments.

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:71: void
LoginPromptBrowserTest::SetUpCommandLine(CommandLine* command_line) {
Should this be in another CL?

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:431: // If a 401 response is
received after we present cached credentials,
Do you want to add a test for different realms?

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:435: const char* kTestUrlPre
=
I've been increasingly seeing const char* const for these.

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_controller.h
File net/http/http_auth_controller.h (right):

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_controlle...
net/http/http_auth_controller.h:87: void InvalidateCurrentHandler(bool
invalidate_credentials);
I prefer enum over boolean so call sites are clearer - since private, I'm not
going to force it though.

i.e.

InvalidateCurrentHandler(INVALIDATE_HANDLER)
InvalidateCurrentHandler(INVALIDATE_HANDLER_AND_CACHED_CREDENTIALS);

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_handler_d...
File net/http/http_auth_handler_digest.cc (right):

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.cc:125: } else if
(LowerCaseEqualsASCII(parameters.name(), "realm"))
Nit: retain the braces in if/else

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.cc:128: return (realm_ != realm)?
Nit: There's usually a space between the two. I

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.cc:128: return (realm_ != realm)?
You should add a test case for this in
HttpAuthHandlerDigestTest.HandleAnotherChallenge (in
http_auth_handler_digest_unittest.cc)

Ditto for Basic.

I'm glad you added some browser tests, but the net unittests run a lot faster,
so worth keeping in there.

[asanka, 2011-02-16 22:39:19]

Added tests and addressed Chris' comments.

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:71: void
LoginPromptBrowserTest::SetUpCommandLine(CommandLine* command_line) {
On 2011/02/16 20:51:39, cbentzel wrote:
> Should this be in another CL? 

Removed.

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:431: // If a 401 response is
received after we present cached credentials,
On 2011/02/16 20:51:39, cbentzel wrote:
> Do you want to add a test for different realms? 

Added.  Also added one for Digest.

http://codereview.chromium.org/6525035/diff/8001/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt_browsertest.cc:435: const char* kTestUrlPre
=
On 2011/02/16 20:51:39, cbentzel wrote:
> I've been increasingly seeing const char* const for these. 

Done.

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_controller.h
File net/http/http_auth_controller.h (right):

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_controlle...
net/http/http_auth_controller.h:87: void InvalidateCurrentHandler(bool
invalidate_credentials);
On 2011/02/16 20:51:39, cbentzel wrote:
> I prefer enum over boolean so call sites are clearer - since private, I'm not
> going to force it though.
> 
> i.e.
> 
> InvalidateCurrentHandler(INVALIDATE_HANDLER)
> InvalidateCurrentHandler(INVALIDATE_HANDLER_AND_CACHED_CREDENTIALS);

Done.

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_handler_d...
File net/http/http_auth_handler_digest.cc (right):

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.cc:125: } else if
(LowerCaseEqualsASCII(parameters.name(), "realm"))
On 2011/02/16 20:51:39, cbentzel wrote:
> Nit: retain the braces in if/else

Done.

http://codereview.chromium.org/6525035/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.cc:128: return (realm_ != realm)?
On 2011/02/16 20:51:39, cbentzel wrote:
> You should add a test case for this in
> HttpAuthHandlerDigestTest.HandleAnotherChallenge (in
> http_auth_handler_digest_unittest.cc)
> 
> Ditto for Basic.
> 
> I'm glad you added some browser tests, but the net unittests run a lot faster,
> so worth keeping in there. 

Done.  I wonder if there's value in moving these couple of tests into
net_unittests entirely.

[cbentzel, 2011-02-16 22:43:54]

LGTM

[cbentzel, 2011-02-17 12:05:42]

On 2011/02/16 22:43:54, cbentzel wrote:
> LGTM

As we discussed in person, it might make sense to move the browsertest and
testserver.py change into a separate CL. I'm fine with the current CL, but OK if
you do that as well.

[asanka, 2011-02-17 14:51:19]

Deferred browser tests for another CL.  Also added a check to the
HttpAuthController::HandleAuthChallenge() for invalidating cached credentials if
the server switches realms on us after a challenge.  We actually had a net
unittest for that case http_network_transaction_unittest.cc.

[cbentzel, 2011-02-17 20:53:41]

LGTM

[wtc, 2011-02-22 23:17:32]

LGTM.  I have some suggested changes and questions below.

I didn't read the discussion between you and cbentzel, so
I apologize if I asked about something that has been answered
before.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
net/http/http_auth_controller.cc:299: // header.
This comment is confusing because it seems self-contradictory.

If the server asks for credentials, that means our authorization
header is NOT preemptive.

Also, the server does more than just "rejects them", right?
It should also give a different realm in the new challenge.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
File net/http/http_auth_controller.h (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
net/http/http_auth_controller.h:92: // Invalidates the current handler,
including cache.
IMPORTANT: please update the comment.  "including cache"
is now true only if the 'action' argument is
INVALIDATE_HANDLER_AND_CACHED_CREDENTIALS.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
File net/http/http_auth_handler_basic.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
net/http/http_auth_handler_basic.cc:64: break;
Nit: this while loop should match the while loop in
ParseChallenge (lines 41-45 exactly).

If multiple 'realm' parameters are present, that while loop
takes the last one whereas this while loop takes the first
one.

Your HttpAuthHandlerDigest::HandleAnotherChallenge doesn't
have a 'break' statement in the while loop either.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
File net/http/http_auth_handler_digest.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
net/http/http_auth_handler_digest.cc:124: return
HttpAuth::AUTHORIZATION_RESULT_STALE;
IMPORTANT: what if the new challenge has both stale=true
and a different realm?  Can that happen?

[cbentzel, 2011-02-23 14:49:54]

Thanks for the careful review.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
net/http/http_auth_controller.cc:299: // header.
On 2011/02/22 23:17:32, wtc wrote:
> This comment is confusing because it seems self-contradictory.
> 
> If the server asks for credentials, that means our authorization
> header is NOT preemptive.

It means that the server returns a 401 after we provided preemptive
authorization headers during the GET. 

> 
> Also, the server does more than just "rejects them", right?
> It should also give a different realm in the new challenge.

for this particular case, it means that a new realm was provided. In the case of
HttpAuth::AUTHORIZATION_RESULT_REJECT, it likely means that the challenge had
the same realm.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
File net/http/http_auth_handler_digest.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
net/http/http_auth_handler_digest.cc:124: return
HttpAuth::AUTHORIZATION_RESULT_STALE;
On 2011/02/22 23:17:32, wtc wrote:
> IMPORTANT: what if the new challenge has both stale=true
> and a different realm?  Can that happen?

It could happen, but it seems unexpected. It seems like it would indicate that
the specified realm has stale credentials - not the one that we previously
issued a challenge for. I'll see if the RFC or httpbis mentions this corner
case.

[asanka, 2011-02-23 18:06:40]

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
net/http/http_auth_controller.cc:299: // header.
On 2011/02/22 23:17:32, wtc wrote:
> This comment is confusing because it seems self-contradictory.

I'll clarify the comment.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
File net/http/http_auth_controller.h (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_controll...
net/http/http_auth_controller.h:92: // Invalidates the current handler,
including cache.
On 2011/02/22 23:17:32, wtc wrote:
> IMPORTANT: please update the comment.  "including cache"
> is now true only if the 'action' argument is
> INVALIDATE_HANDLER_AND_CACHED_CREDENTIALS.

I'll fix this in another CL, since I already landed this one.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
File net/http/http_auth_handler_basic.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
net/http/http_auth_handler_basic.cc:64: break;
On 2011/02/22 23:17:32, wtc wrote:
> Nit: this while loop should match the while loop in
> ParseChallenge (lines 41-45 exactly).
> 
> If multiple 'realm' parameters are present, that while loop
> takes the last one whereas this while loop takes the first
> one.

Will fix in a new CL.

> Your HttpAuthHandlerDigest::HandleAnotherChallenge doesn't
> have a 'break' statement in the while loop either.

In the Digest case, that was to give precedence to the "stale" directive if
there was one, but that has the side-effect of picking the last 'realm'
directive if there is more than one.

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
File net/http/http_auth_handler_digest.cc (right):

http://codereview.chromium.org/6525035/diff/14001/net/http/http_auth_handler_...
net/http/http_auth_handler_digest.cc:124: return
HttpAuth::AUTHORIZATION_RESULT_STALE;
On 2011/02/23 14:49:54, cbentzel wrote:
> On 2011/02/22 23:17:32, wtc wrote:
> > IMPORTANT: what if the new challenge has both stale=true
> > and a different realm?  Can that happen?
> 
> It could happen, but it seems unexpected. It seems like it would indicate that
> the specified realm has stale credentials - not the one that we previously
> issued a challenge for. I'll see if the RFC or httpbis mentions this corner
> case.

RFC 2617 states that the 'stale' value should only be set to 'true' if the
server "receives a request for which the nonce is invalid but with a valid
digest for that nonce (indicating that the client knows the correct
username/password)," and that the client "may wish to simply retry the request
with a new encrypted response, without reprompting the user for a new username
and password."

Based on that, I think giving precedence to the "stale=true" case and
disregarding a change in the realm is reasonable behavior.