Create HttpAuthController.

Create HttpAuthController. (again)

This packages up the auth state into a single class to enable a HttpProxyClientSocket class (which is needed for SSLClientSocketPool).
Fix memory leak.

BUG=30357
TEST=existing unit tests

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

[vandebo (ex-Chrome), 2010-06-22 02:06:19]

Chris, let me know what you think of this - It mostly just moves code.  I still
need to add documentation comments to fields/functions in the new class, and I'm
not sure about the new class name.

[cbentzel, 2010-06-22 13:50:55]

Steve,

Thanks for doing this. I've been doing a lot of restructuring/refactoring of
the auth code to help make a "connectivity" class work and this is another
step in the right direction.

My very quick reaction is whether the behavior should move into
HttpAuthHandler itself. The HttpAuthHandler used to be a shared object, but
now it only includes state for a particular auth challenge.

I'd also like to make sure that this could be used by SocketStream, as
there's a lot of shared code between that and HttpNetworkTransaction, and
we'd actually like to support more proxy authentication schemes for the
SocketStream.

I'll take a more thorough look at the code this afternoon, focusing on the
validity of the approach rather than nits.

Chris

On Mon, Jun 21, 2010 at 10:06 PM, <vandebo@chromium.org> wrote:

> Reviewers: cbentzel,
>
> Message:
> Chris, let me know what you think of this - It mostly just moves code.  I
> still
> need to add documentation comments to fields/functions in the new class,
> and I'm
> not sure about the new class name.
>
> Description:
> Create HttpNetworkTransactionAuthHandler.
>
> This packages up the auth state into a single class to enable a
> HttpProxyClientSocket class (which is needed for SSLClientSocketPool).
>
> BUG=30357
> TEST=existing unit tests
>
> Please review this at http://codereview.chromium.org/2808020/show
>
> Affected files:
>  M net/base/net_error_list.h
>  M net/http/http_auth_handler.cc
>  M net/http/http_network_transaction.h
>  M net/http/http_network_transaction.cc
>  A net/http/http_network_transaction_auth_handler.h
>  A net/http/http_network_transaction_auth_handler.cc
>  M net/net.gyp
>
>
>

[vandebo (ex-Chrome), 2010-06-22 17:03:47]

On 2010/06/22 13:50:55, cbentzel wrote:
> My very quick reaction is whether the behavior should move into
> HttpAuthHandler itself. The HttpAuthHandler used to be a shared object, but
> now it only includes state for a particular auth challenge.

That may be the right final place for it, but pushing this code into there
requires a deeper understanding of the auth code than I have.  I did consider
puttting it in the HttpAuth class. 


> I'd also like to make sure that this could be used by SocketStream, as
> there's a lot of shared code between that and HttpNetworkTransaction, and
> we'd actually like to support more proxy authentication schemes for the
> SocketStream.

I wasn't thinking about SocketStream when I did this, but it seems like a good
fit to be used there as well.  I haven't touched the auth code aside from this
superficial refactor though, so it's out of my scope to unfork the auth code in
SocketStream.  It's unfortunate that HttpNetworkTransaction has been copied and
pasted to support other things instead of extracting the relevant portions into
other classes.

> I'll take a more thorough look at the code this afternoon, focusing on the
> validity of the approach rather than nits.

Considering your initial feedback, I'm going to run with this CL.  Do you want
me to bring in another reviewer?

--
Steve

[cbentzel, 2010-06-22 17:24:57]

> Considering your initial feedback, I'm going to run with this CL.  Do you want
> me to bring in another reviewer?

ahendrickson should take a look.

I'll take a more in-depth look now.

[vandebo (ex-Chrome), 2010-06-22 17:37:57]

On 2010/06/22 17:24:57, cbentzel wrote:
> ahendrickson should take a look.
> I'll take a more in-depth look now.

Great.  Added and ready for review now.

[cbentzel, 2010-06-22 18:26:58]

The two issues with passing in a NULL request_info_ prevent me from giving an
LGTM - the rest are nits.

http://codereview.chromium.org/2808020/diff/1/2
File net/base/net_error_list.h (right):

http://codereview.chromium.org/2808020/diff/1/2#newcode159
net/base/net_error_list.h:159: // The HTTP auth handler requires a DNS lookup to
find the canonical name.
I don't like the use of the error code here (because it's actually the common
case for Kerberos authentication). 

However, I'm rewriting the Negotiate handler to be a state machine which hides
the NEEDS_CANONICAL_NAME state from the caller, so this will go away shortly.

http://codereview.chromium.org/2808020/diff/3002/5005
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2808020/diff/3002/5005#newcode971
net/http/http_network_transaction.cc:971: NULL, &io_callback_);
You can't pass in a NULL request here. Some authentication types (such as
Digest) need the request to construct the auth token in both server and proxy
cases.

http://codereview.chromium.org/2808020/diff/3002/5005#newcode1182
net/http/http_network_transaction.cc:1182: return
auth_handlers_[HttpAuth::AUTH_PROXY]->MaybeGenerateAuthToken(
Again, you need to pass in an HttpRequestInfo here.

http://codereview.chromium.org/2808020/diff/3002/5007
File net/http/http_network_transaction_auth_handler.cc (right):

http://codereview.chromium.org/2808020/diff/3002/5007#newcode92
net/http/http_network_transaction_auth_handler.cc:92: AuthOrigin(), AuthPath());
Nit: AuthOrigin() and AuthPath() could be calculated and cached at construction
time.

http://codereview.chromium.org/2808020/diff/3002/5007#newcode120
net/http/http_network_transaction_auth_handler.cc:120:
HttpAuth::GetAuthorizationHeaderName(target_), auth_token_);
Nit: The authorization header name could be calculated and cached at
construction time.

http://codereview.chromium.org/2808020/diff/3002/5007#newcode337
net/http/http_network_transaction_auth_handler.cc:337: // TODO(e_roman): decode
realm according to RFC 2047.
TODO(eroman)

http://codereview.chromium.org/2808020/diff/3002/5008
File net/http/http_network_transaction_auth_handler.h (right):

http://codereview.chromium.org/2808020/diff/3002/5008#newcode47
net/http/http_network_transaction_auth_handler.h:47: // HandleAuthChallenge()
returns OK on success, ERR_AUTH_NEEDS_CANONICAL_NAME
Nit: include |HandleAuthChallenge()| in pipes.

http://codereview.chromium.org/2808020/diff/3002/5008#newcode49
net/http/http_network_transaction_auth_handler.h:49: // It may also populate
auth_info.
Nit: |auth_info_|

http://codereview.chromium.org/2808020/diff/3002/5008#newcode53
net/http/http_network_transaction_auth_handler.h:53: int
ResolveCanonicalName(CompletionCallback* callback);
Nit: You _may_ want to document this, but it will likely go away this week.

http://codereview.chromium.org/2808020/diff/3002/5008#newcode56
net/http/http_network_transaction_auth_handler.h:56: void
AddExternalIdentity(const std::wstring& username,
The name for this method does not match the comment (which does match the code).

Maybe name this ResetAuth()?

http://codereview.chromium.org/2808020/diff/3002/5008#newcode112
net/http/http_network_transaction_auth_handler.h:112: HttpAuth::Identity
identity_;
HttpAuth::Identity could _probably_ be moved out of HttpAuth and into this
class, but I'll do that in a separate refactor.

http://codereview.chromium.org/2808020/diff/3002/5008#newcode119
net/http/http_network_transaction_auth_handler.h:119:
scoped_refptr<AuthChallengeInfo> auth_info_;
Why does this need to be a ref counted pointer? Why isn't scoped_ptr sufficient?

[cbentzel, 2010-06-22 18:28:12]

Also, I wish I could come up with a better name for the new class. 

HttpAuthChallenger?

[vandebo (ex-Chrome), 2010-06-22 22:21:56]

http://codereview.chromium.org/2808020/diff/3002/5005
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2808020/diff/3002/5005#newcode971
net/http/http_network_transaction.cc:971: NULL, &io_callback_);
On 2010/06/22 18:26:59, cbentzel wrote:
> You can't pass in a NULL request here. Some authentication types (such as
> Digest) need the request to construct the auth token in both server and proxy
> cases.

Done.

http://codereview.chromium.org/2808020/diff/3002/5007
File net/http/http_network_transaction_auth_handler.cc (right):

http://codereview.chromium.org/2808020/diff/3002/5007#newcode92
net/http/http_network_transaction_auth_handler.cc:92: AuthOrigin(), AuthPath());
On 2010/06/22 18:26:59, cbentzel wrote:
> Nit: AuthOrigin() and AuthPath() could be calculated and cached at
construction
> time.

Done.  Though I still have to store auth_url for the username and password it
contains.

http://codereview.chromium.org/2808020/diff/3002/5007#newcode120
net/http/http_network_transaction_auth_handler.cc:120:
HttpAuth::GetAuthorizationHeaderName(target_), auth_token_);
On 2010/06/22 18:26:59, cbentzel wrote:
> Nit: The authorization header name could be calculated and cached at
> construction time.

With it off in HttpAuth, there's only one copy of each string.

http://codereview.chromium.org/2808020/diff/3002/5007#newcode337
net/http/http_network_transaction_auth_handler.cc:337: // TODO(e_roman): decode
realm according to RFC 2047.
On 2010/06/22 18:26:59, cbentzel wrote:
> TODO(eroman)

Done.

http://codereview.chromium.org/2808020/diff/3002/5008
File net/http/http_network_transaction_auth_handler.h (right):

http://codereview.chromium.org/2808020/diff/3002/5008#newcode47
net/http/http_network_transaction_auth_handler.h:47: // HandleAuthChallenge()
returns OK on success, ERR_AUTH_NEEDS_CANONICAL_NAME
On 2010/06/22 18:26:59, cbentzel wrote:
> Nit: include |HandleAuthChallenge()| in pipes.

Done.

http://codereview.chromium.org/2808020/diff/3002/5008#newcode49
net/http/http_network_transaction_auth_handler.h:49: // It may also populate
auth_info.
On 2010/06/22 18:26:59, cbentzel wrote:
> Nit: |auth_info_|

Done.

http://codereview.chromium.org/2808020/diff/3002/5008#newcode56
net/http/http_network_transaction_auth_handler.h:56: void
AddExternalIdentity(const std::wstring& username,
On 2010/06/22 18:26:59, cbentzel wrote:
> The name for this method does not match the comment (which does match the
code).
> 
> Maybe name this ResetAuth()?

Done.

http://codereview.chromium.org/2808020/diff/3002/5008#newcode119
net/http/http_network_transaction_auth_handler.h:119:
scoped_refptr<AuthChallengeInfo> auth_info_;
On 2010/06/22 18:26:59, cbentzel wrote:
> Why does this need to be a ref counted pointer? Why isn't scoped_ptr
sufficient?

AuthChallengeInfo is refcounted and therefore has a private destructor.

[cbentzel, 2010-06-23 10:49:17]

LGTM

The valgrind trybot failure seems due to ld memory exhaustion (many of the
trybots have been impacted by this lately). Do you know what the Win trybot
failure is due to?

[ahendrickson, 2010-06-23 18:49:02]

LGTM, with minor nits.

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

http://codereview.chromium.org/2808020/diff/14001/15004#newcode82
net/http/http_auth_controller.cc:82: DCHECK(!HaveAuth());
Could you add a DCHECK(identity_.invalid)?  Or else set it to true before
creating the auth handler.

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

http://codereview.chromium.org/2808020/diff/14001/15005#newcode28
net/http/http_auth_controller.h:28: // The arguments are self explanatory except
for possible auth_url, which
Nit: for possible --> possibly for.
auth_url --> |auth_url|