Use AuthCredentials throughout the network stack instead of username/password.

Use AuthCredentials throughout the network stack instead of username/password.

This is a refactor only - no behavior change should happen.

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

[asanka, 2011-10-27 20:01:20]

http://codereview.chromium.org/8340026/diff/1081/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/8340026/diff/1081/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt.cc:363:
request_->SetAuth(net::AuthCredentials(username, password));
Perhaps create a net::AuthCredentials object before the PostTask?

http://codereview.chromium.org/8340026/diff/1081/net/base/auth.cc
File net/base/auth.cc (right):

http://codereview.chromium.org/8340026/diff/1081/net/base/auth.cc#newcode54
net/base/auth.cc:54: password_ = password;
Zap password_ before assignment?

Also consider assignment of AuthCredentials objects.  I.e. a construct like
'credentials = AuthCredentials()' should cause credentials.password_ to be
zapped.

http://codereview.chromium.org/8340026/diff/1081/net/http/http_auth_cache_uni...
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/8340026/diff/1081/net/http/http_auth_cache_uni...
net/http/http_auth_cache_unittest.cc:73: return
AuthCredentials(ASCIIToUTF16(username), ASCIIToUTF16(password));
This pattern seems to occur often.  Perhaps a AuthCredentials::FromASCII()?

http://codereview.chromium.org/8340026/diff/1081/net/socket_stream/socket_str...
File net/socket_stream/socket_stream_unittest.cc (right):

http://codereview.chromium.org/8340026/diff/1081/net/socket_stream/socket_str...
net/socket_stream/socket_stream_unittest.cc:147: VLOG(1) << "RestartWithAuth
username=" << credentials_.username()
DVLOG

[cbentzel, 2011-10-28 02:18:40]

http://codereview.chromium.org/8340026/diff/1081/chrome/browser/ui/login/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

http://codereview.chromium.org/8340026/diff/1081/chrome/browser/ui/login/logi...
chrome/browser/ui/login/login_prompt.cc:363:
request_->SetAuth(net::AuthCredentials(username, password));
On 2011/10/27 20:01:20, asanka wrote:
> Perhaps create a net::AuthCredentials object before the PostTask?

I did that initially, but then it ended up propagating further into the chrome
code. This was already big enough, so I wanted to restrict to the net stack
only, and the few callers of the net code.

http://codereview.chromium.org/8340026/diff/1081/net/base/auth.cc
File net/base/auth.cc (right):

http://codereview.chromium.org/8340026/diff/1081/net/base/auth.cc#newcode54
net/base/auth.cc:54: password_ = password;
On 2011/10/27 20:01:20, asanka wrote:
> Zap password_ before assignment?
> 
> Also consider assignment of AuthCredentials objects.  I.e. a construct like
> 'credentials = AuthCredentials()' should cause credentials.password_ to be
> zapped.

As we discussed, I'll change behavior in another CL. This is intended to be a
refactor only.

http://codereview.chromium.org/8340026/diff/1081/net/http/http_auth_cache_uni...
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/8340026/diff/1081/net/http/http_auth_cache_uni...
net/http/http_auth_cache_unittest.cc:73: return
AuthCredentials(ASCIIToUTF16(username), ASCIIToUTF16(password));
On 2011/10/27 20:01:20, asanka wrote:
> This pattern seems to occur often.  Perhaps a AuthCredentials::FromASCII()?

I was worried about adding it there since it's only used by tests. I was
actually considering a new constructor.

http://codereview.chromium.org/8340026/diff/1081/net/socket_stream/socket_str...
File net/socket_stream/socket_stream_unittest.cc (right):

http://codereview.chromium.org/8340026/diff/1081/net/socket_stream/socket_str...
net/socket_stream/socket_stream_unittest.cc:147: VLOG(1) << "RestartWithAuth
username=" << credentials_.username()
On 2011/10/27 20:01:20, asanka wrote:
> DVLOG

Not in this CL.

[cbentzel, 2011-10-28 14:03:47]

Oh - and the new patch adds a Zap method to AuthCredentials and only calls it in
the NTLM portable code so the behavior is the same as before.

[asanka, 2011-10-28 14:39:40]

LGTM with some nitty cleanup for string16.h.

http://codereview.chromium.org/8340026/diff/1081/net/http/http_auth_cache_uni...
File net/http/http_auth_cache_unittest.cc (right):

http://codereview.chromium.org/8340026/diff/1081/net/http/http_auth_cache_uni...
net/http/http_auth_cache_unittest.cc:73: return
AuthCredentials(ASCIIToUTF16(username), ASCIIToUTF16(password));
On 2011/10/28 02:18:40, cbentzel wrote:
> I was
> actually considering a new constructor.

I was suggesting a factory method so that it would be explicit at the call site
what the encoding of the parameters are (ASCII vs UTF-8).

http://codereview.chromium.org/8340026/diff/7002/net/ftp/ftp_transaction.h
File net/ftp/ftp_transaction.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/ftp/ftp_transaction.h#ne...
net/ftp/ftp_transaction.h:9: #include "base/string16.h"
Not needed.

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_cache.h
File net/http/http_auth_cache.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_cache.h#n...
net/http/http_auth_cache.h:14: #include "base/string16.h"
Not needed?

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_controller.h
File net/http/http_auth_controller.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_controlle...
net/http/http_auth_controller.h:15: #include "base/string16.h"
Not needed?

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_gssapi_po...
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_gssapi_po...
net/http/http_auth_gssapi_posix.h:15: #include "base/string16.h"
Not needed?

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler.h
File net/http/http_auth_handler.h (left):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler.h...
net/http/http_auth_handler.h:164: std::string* auth_token) = 0;
It's probably worthwhile putting in OVERRIDEs for the HttpAuthHandler derived
classes.

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler.h
File net/http/http_auth_handler.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler.h...
net/http/http_auth_handler.h:11: #include "base/string16.h"
Not needed?

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_b...
File net/http/http_auth_handler_basic.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_b...
net/http/http_auth_handler_basic.h:11: #include "base/string16.h"
Here as well.

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_d...
File net/http/http_auth_handler_digest.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.h:14: #include "base/string16.h"
Is base/string16.h needed anymore?

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_m...
File net/http/http_auth_handler_mock.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_m...
net/http/http_auth_handler_mock.h:13: #include "base/string16.h"
Is base/string16.h needed anymore?

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_n...
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler_n...
net/http/http_auth_handler_negotiate.h:11: #include "base/string16.h"
base/string16.h not needed anymore.

http://codereview.chromium.org/8340026/diff/7002/net/http/http_cache_transact...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_cache_transact...
net/http/http_cache_transaction.h:14: #include "base/string16.h"
base/string16.h not needed?

http://codereview.chromium.org/8340026/diff/7002/net/socket_stream/socket_str...
File net/socket_stream/socket_stream.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/socket_stream/socket_str...
net/socket_stream/socket_stream.h:16: #include "base/string16.h"
Not needed?

http://codereview.chromium.org/8340026/diff/7002/net/socket_stream/socket_str...
File net/socket_stream/socket_stream_job.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/socket_stream/socket_str...
net/socket_stream/socket_stream_job.h:12: #include "base/string16.h"
Not used?

http://codereview.chromium.org/8340026/diff/7002/net/url_request/url_request_...
File net/url_request/url_request_ftp_job.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/url_request/url_request_...
net/url_request/url_request_ftp_job.h:11: #include "base/string16.h"
Not needed?

http://codereview.chromium.org/8340026/diff/7002/net/url_request/url_request_...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/url_request/url_request_...
net/url_request/url_request_http_job.h:15: #include "base/string16.h"
Not needed?

http://codereview.chromium.org/8340026/diff/7002/net/url_request/url_request_...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/url_request/url_request_...
net/url_request/url_request_job.h:14: #include "base/string16.h"
Not needed.

http://codereview.chromium.org/8340026/diff/7002/net/websockets/websocket_job.h
File net/websockets/websocket_job.h (right):

http://codereview.chromium.org/8340026/diff/7002/net/websockets/websocket_job...
net/websockets/websocket_job.h:13: #include "base/string16.h"
Not needed.

[cbentzel, 2011-10-28 15:58:06]

Thanks for the review.

The latest patchset removes the string16 includes, as well as fixes a few
comments that were referring to username/password instead of credentials.

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler.h
File net/http/http_auth_handler.h (left):

http://codereview.chromium.org/8340026/diff/7002/net/http/http_auth_handler.h...
net/http/http_auth_handler.h:164: std::string* auth_token) = 0;
On 2011/10/28 14:39:40, asanka wrote:
> It's probably worthwhile putting in OVERRIDEs for the HttpAuthHandler derived
> classes.

I agree to an extent - it's not as critical for overriding =0 since the compiler
will complain about not being able to create a class with pure virtual methods.
But I'll do it in a separate CL.

[commit-bot: I haz the power, 2011-10-28 17:37:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/8340026/4085

[commit-bot: I haz the power, 2011-10-28 18:45:01]

Change committed as 107766