Implement RestartWithAuth for NewFtpTransaction.

Implement RestartWithAuth for NewFtpTransaction.

TEST=Covered by net_unittests.
http://crbug.com/20112

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

[Paweł Hajdan Jr., 2009-08-24 16:26:18]

This patch still doesn't use FtpAuthCache, but I plan to do that in a later
patch. I think that a good test will require some changes to the testserver.

[Paweł Hajdan Jr., 2009-08-25 15:27:11]

ping

[wtc, 2009-08-25 20:52:50]

LGTM.

I compared your new code with the equivalent code in
URLRequestFtpJob/URLRequestInetJob and URLRequestHttpJob
carefully.

http://codereview.chromium.org/173270/diff/1007/16
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/173270/diff/1007/16#newcode60
Line 60: username_ = UTF8ToWide(request_->url.username());
Please ask Darin and Brett whether the embedded username
and password in URLs are ASCII or UTF-8, and whether they
need to be unescaped.

http://codereview.chromium.org/173270/diff/1007/16#newcode63
Line 63: else
Do we need the "else" here?  password_ is initialized
to an empty string.

http://codereview.chromium.org/173270/diff/1007/16#newcode527
Line 527: std::string command = "USER " + WideToUTF8(username_);
Can you add a comment to the header to explain why we need
to convert the username from to a wide string and back to
a narrow string?  Is std::wstring required by the
FtpAuthCache?

http://codereview.chromium.org/173270/diff/1007/17
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/173270/diff/1007/17#newcode193
Line 193: std::wstring password_;
Nit: add a blank line to separate these from retr_failed_
(unless these are related to retr_failed_).

http://codereview.chromium.org/173270/diff/1007/18
File net/ftp/ftp_response_info.h (left):

http://codereview.chromium.org/173270/diff/1007/18#oldcode18
Line 18: scoped_refptr<AuthChallengeInfo> auth_challenge;
Is a bool (auth_needed) all we need for FTP?
I guess AuthChallengeInfo is designed for HTTP auth, and
is an overkill for FTP?

http://codereview.chromium.org/173270/diff/1007/18
File net/ftp/ftp_response_info.h (right):

http://codereview.chromium.org/173270/diff/1007/18#newcode12
Line 12: struct FtpResponseInfo {
Please change this back to 'class'.

We use 'class' deliberately so that when people write
forward declarations for the types in the net module,
they can just write 'class XXX', without having to look
up whether the type is a class or struct.

http://codereview.chromium.org/173270/diff/1007/18#newcode19
Line 19: bool auth_needed;
Nit: rename this member needs_auth to match the
NeedsAuth method of URLRequestJob.

http://codereview.chromium.org/173270/diff/1007/20
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/173270/diff/1007/20#newcode114
Line 114: return server_auth_ && server_auth_->state ==
net::AUTH_STATE_NEED_AUTH;
Just FYI: URLRequestHttpJob::NeedsAuth sets
the auth state to AUTH_STATE_NEED_AUTH in this method.
I remember that part of the HTTP code is not perfect, so we
don't need to imitate it blindly.

http://codereview.chromium.org/173270/diff/1007/20#newcode132
Line 132: DCHECK(server_auth_ && server_auth_->state ==
net::AUTH_STATE_NEED_AUTH);
This conditional expression is equivalent to NeedsAuth().

http://codereview.chromium.org/173270/diff/1007/20#newcode142
Line 142: MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
URLRequestHttpJob just call RestartTransactionWithAuth
synchronously here.  So maybe it's now OK to do that.

http://codereview.chromium.org/173270/diff/1007/20#newcode147
Line 147: DCHECK(server_auth_ && server_auth_->state ==
net::AUTH_STATE_NEED_AUTH);
This conditional expression is equivalent to NeedsAuth().

http://codereview.chromium.org/173270/diff/1007/20#newcode153
Line 153: MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
URLRequestHttpJob::CancelAuth posts a task for
OnStartCompleted(net::OK), which is equivalent to
NotifyHeadersComplete).

It would be nice to make the two classes use the same
code.

http://codereview.chromium.org/173270/diff/1007/20#newcode301
Line 301: } else {
Use "else if" here.

http://codereview.chromium.org/173270/diff/1007/20#newcode330
Line 330: void URLRequestNewFtpJob::RestartWithAuth() {
Perhaps rename this method RestartTransactionWithAuth to
be consistent with URLRequestHttpJob?

http://codereview.chromium.org/173270/diff/1007/20#newcode332
Line 332: 
URLRequestHttpJob::RestartTransactionWithAuth also sets
response_info_ to NULL.

http://codereview.chromium.org/173270/diff/1007/21
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/173270/diff/1007/21#newcode35
Line 35: virtual void
GetAuthChallengeInfo(scoped_refptr<net::AuthChallengeInfo>*);
Let's provide the parameter name (auth_info) here.
I remember cpplint.py warns about this.

http://codereview.chromium.org/173270/diff/1007/22
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/173270/diff/1007/22#newcode1890
Line 1890: TEST_F(URLRequestTest, FTPCheckWrongPasswordRestart) {
Can you add comments to explain why calling
  d.set_username(L"chrome");
  d.set_password(L"chrome");
cause the test to restart and succeed?  The FTPTestServer
is created with ("chrome", "wrong_password"), so I don't
know why L"chrome" is the right password.  (I am not
familiar with this file, so this is a stupid question.)

[eroman, 2009-09-01 07:12:37]

http://codereview.chromium.org/173270/diff/1007/16
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/173270/diff/1007/16#newcode60
Line 60: username_ = UTF8ToWide(request_->url.username());
On 2009/08/25 20:52:50, wtc wrote:
> Please ask Darin and Brett whether the embedded username
> and password in URLs are ASCII or UTF-8, and whether they
> need to be unescaped.

They definitely do need to be unescaped.

See HttpNetworkTransaction::GetIdentifyFromUrl(), where we do the unescaping for
use by HTTP auth.

I suggest moving that method and its associated unittest to net_util, and
re-using it in FTP code.

As a side-note, I mis-spelled that function. It should read GetIdentityFromURL()
and not GetIdentifyFromUrl()!

http://codereview.chromium.org/173270/diff/1007/16#newcode61
Line 61: if (request_->url.has_password())
Don't actually need the guard, will just be empty string if there is none.

http://codereview.chromium.org/173270/diff/1007/16#newcode556
Line 556: std::string command = "PASS " + WideToUTF8(password_);
PLEASE READ: This looks dangerous!

|username_| and |password_| must be sanitized after extracting them from the
URL, being very careful to strip away CR and LF... since those could be used to
escape the current command and issue other things, like deleting files.

ALSO, note that the RFC for FTP makes no mention of UTF8.
The username and password are expected to be ASCII, so you should do a lossy
conversion here from wide to ASCII :(

http://codereview.chromium.org/173270/diff/1007/16#newcode579
Line 579: response_.auth_needed = true;
Hm, I dont know about this... will have to look more closely. My first reaction
is there are other error codes not covered by this which relate to login (530,
331, 332).