Cache login identity for NewFTP transactions.

Cache login identity for NewFTP transactions.

TEST=net_unittests
BUG=21184
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26815

[Paweł Hajdan Jr., 2009-09-10 19:51:25]

eroman, you suggested earlier that the transaction should succeed when auth
fails. I can make this change here. What do you think?

[Paweł Hajdan Jr., 2009-09-11 22:05:27]

ping

[eroman, 2009-09-11 23:02:52]

http://codereview.chromium.org/201083/diff/1/2
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/201083/diff/1/2#newcode372
Line 372: request_->context()->ftp_auth_cache()->Remove(origin);
This could end up invalidating an auth entry which hasn't failed yet -- if you
have concurrent requests, using different auths, since the entry is added to the
cache first, this could remove the wrong one.

In HttpNetworkTransaction we handle this by making the Remove() function take
the identity (and it only removes if the username/password in cache match).

[Paweł Hajdan Jr., 2009-09-14 16:23:44]

Good catch, fixed (with a test).

[wtc, 2009-09-15 02:05:45]

LGTM.

http://codereview.chromium.org/201083/diff/2002/4002
File net/ftp/ftp_auth_cache.h (right):

http://codereview.chromium.org/201083/diff/2002/4002#newcode38
Line 38: void Add(const GURL& origin, AuthData* auth_data);
The second parameter should be 'const'.

You may want to replace the second parameter by
             const std::wstring& username,
             const std::wstring& password,
to be consistent with HttpAuthCache::Add().

http://codereview.chromium.org/201083/diff/2002/4002#newcode41
Line 41: // passed |auth_data|.
Nit: add "the" before "passed", or just remove "passed".

http://codereview.chromium.org/201083/diff/2002/4002#newcode42
Line 42: void Remove(const GURL& origin, const AuthData* auth_data);
Can we use a const reference for the second parameter?
  const AuthData& auth_data

Why don't we use
  const std::wstring& username,
  const std::wstring& password
instead, to be consistent with HttpAuthCache::Remove()?

http://codereview.chromium.org/201083/diff/2002/4004
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/201083/diff/2002/4004#newcode371
Line 371: // Cached credentials are wrong.
This comment ("cached credentials") is misleading.  The
credentials in server_auth_ may not be from the FTP auth
cache.

http://codereview.chromium.org/201083/diff/2002/4004#newcode374
Line 374: server_auth_ = new net::AuthData();
Nit: it is wasteful to throw away the old server_auth_ if
it exists.  (I know you copied this code from
URLRequestFtpJob::OnIOComplete.)

http://codereview.chromium.org/201083/diff/2002/4004#newcode386
Line 386: SetStatus(URLRequestStatus());
Remove this (already done on line 364 above).

[eroman, 2009-09-15 21:39:06]

This is inconsistent with how embedded URLs are treated -- they too should be
added to the ftp auth cache.

This is particularly important since there are no "logout" semantics for
FTP/HTTP auth.

So users expect to be able to use a username@password embedded in the URL to
force the cache.

http://codereview.chromium.org/201083/diff/2002/4004
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/201083/diff/2002/4004#newcode369
Line 369: if (server_auth_ != NULL &&
style nit: no need for the "!= NULL"

http://codereview.chromium.org/201083/diff/2002/4004#newcode385
Line 385: // The io completed fine, the error was due to invalid auth.
style nit: "io" --> "IO"

[Paweł Hajdan Jr., 2009-09-16 16:12:38]

I think I fixed the issues. I also added a test to prove that username@password
embedded in the URL will be cached too.

Please review.

[wtc, 2009-09-16 18:16:48]

LGTM.

http://codereview.chromium.org/201083/diff/7/8002
File net/ftp/ftp_auth_cache.h (right):

http://codereview.chromium.org/201083/diff/7/8002#newcode29
Line 29: Entry(const GURL& origin, const std::wstring& username,
Nit: indentation is off by one.

Our style guide suggests three ways to format the
parameter lists in function declarations and definitions:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Although I personally think the form you used in this file
should also be allowed, it's better to follow the style
guide.

[eroman, 2009-09-17 02:50:37]

> I also added a test to prove that username@password
> embedded in the URL will be cached too.

What test is that?

I still don't see how this can be working for embedded URLs.

Essentially the issue is that the auth-cache interactions are in the job, but
embedded URL logic is part of the transaction.

http://codereview.chromium.org/201083/diff/7/8001
File net/ftp/ftp_auth_cache.cc (right):

http://codereview.chromium.org/201083/diff/7/8001#newcode16
Line 16: for (EntryList::iterator it = entries_.begin(); it != entries_.end();
++it) {
[entirely option]: I recommend also adding a DCHECK of the sort:

DCHECK_EQ(origin, origin.GetOrigin());

(to make sure callers aren't accidentally passing in bad stuff which will result
in misses)

http://codereview.chromium.org/201083/diff/7/8001#newcode30
Line 30: entry->username = username;
Excellent. I am glad you have gotten rid of AuthData here.
(I am really not thrilled about the stuff in base/auth.h)

http://codereview.chromium.org/201083/diff/7/8005
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/201083/diff/7/8005#newcode368
Line 368: GURL origin = request_->url().GetOrigin();
minor style nit: can this be declared as "const GURL" ?

[Paweł Hajdan Jr., 2009-09-21 19:42:04]

Indeed, the change was not working properly for embedded identity. Fixed, and
added more reliable tests.

Someone reverted the change earlier for some bogus reason, so it's ready for a
fresh review. Please take a look.

[eroman, 2009-09-22 07:42:21]

looks good; a couple comments (which you don't have to address).

http://codereview.chromium.org/201083/diff/11002/12007
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/201083/diff/11002/12007#newcode1145
Line 1145: option_parser.add_option('--ftp-enable-anonymous',
action='store_true',
It might be more convenient to invert the direction of this flag (enable
anonymous FTP by default, require a flag to disable it)

http://codereview.chromium.org/201083/diff/11002/12009
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/201083/diff/11002/12009#newcode372
Line 372: request_->context()->ftp_auth_cache()->Add(origin, username,
password);
Ok. Note that for HTTP we only save it to the cache if a challenge was received
(we do all of the auth cache logic as part of the transaction rather than the
job).

http://codereview.chromium.org/201083/diff/11002/12009#newcode390
Line 390: SetAuth(cached_auth->username, cached_auth->password);
This is kind of awkward that we set it in the cache twice.

(i.e we have either j added it to the cache, or we have just retrieved it from
the cache).

[Paweł Hajdan Jr., 2009-09-22 16:15:58]

On 2009/09/22 07:42:21, eroman wrote:
> looks good; a couple comments (which you don't have to address).
> 
> http://codereview.chromium.org/201083/diff/11002/12007
> File net/tools/testserver/testserver.py (right):
> 
> http://codereview.chromium.org/201083/diff/11002/12007#newcode1145
> Line 1145: option_parser.add_option('--ftp-enable-anonymous',
> action='store_true',
> It might be more convenient to invert the direction of this flag (enable
> anonymous FTP by default, require a flag to disable it)

I also considered that. For now we have only one test for anonymous FTP, and 5+
for non-anonymous, which makes the current choice look better. Maybe in the
future the proportion will change.

> http://codereview.chromium.org/201083/diff/11002/12009
> File net/url_request/url_request_new_ftp_job.cc (right):
> 
> http://codereview.chromium.org/201083/diff/11002/12009#newcode372
> Line 372: request_->context()->ftp_auth_cache()->Add(origin, username,
> password);
> Ok. Note that for HTTP we only save it to the cache if a challenge was
received
> (we do all of the auth cache logic as part of the transaction rather than the
> job).

Makes sense. I think I will fix that, in the future. For now I mostly used logic
from the old ftp job.

> http://codereview.chromium.org/201083/diff/11002/12009#newcode390
> Line 390: SetAuth(cached_auth->username, cached_auth->password);
> This is kind of awkward that we set it in the cache twice.
> 
> (i.e we have either j added it to the cache, or we have just retrieved it from
> the cache).

Same as above, I'm going to study http transaction more. Thanks for pointing
these out.