Cache login identity for NewFTP transactions.

TEST=net_unittests
BUG=21184

Review URL: http://codereview.chromium.org/201083

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@26815 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 225 insertions, 91 deletions

diff --git a/net/ftp/ftp_auth_cache.cc b/net/ftp/ftp_auth_cache.cc
index c29146a..d3bff90 100644
--- a/net/ftp/ftp_auth_cache.cc
+++ b/net/ftp/ftp_auth_cache.cc
@@ -12,20 +12,25 @@ namespace net {
 // static
 const size_t FtpAuthCache::kMaxEntries = 10;
 
-AuthData* FtpAuthCache::Lookup(const GURL& origin) {
-  Entry* entry = LookupEntry(origin);
-  return (entry ? entry->auth_data : NULL);
+FtpAuthCache::Entry* FtpAuthCache::Lookup(const GURL& origin) {
+  for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) {
+    if (it->origin == origin)
+      return &(*it);
+  }
+  return NULL;
 }
 
-void FtpAuthCache::Add(const GURL& origin, AuthData* auth_data) {
+void FtpAuthCache::Add(const GURL& origin, const std::wstring& username,
+                       const std::wstring& password) {
   DCHECK(origin.SchemeIs("ftp"));
   DCHECK_EQ(origin.GetOrigin(), origin);
 
-  Entry* entry = LookupEntry(origin);
+  Entry* entry = Lookup(origin);
   if (entry) {
-    entry->auth_data = auth_data;
+    entry->username = username;
+    entry->password = password;
   } else {
-    entries_.push_front(Entry(origin, auth_data));
+    entries_.push_front(Entry(origin, username, password));
 
     // Prevent unbound memory growth of the cache.
     if (entries_.size() > kMaxEntries)
@@ -33,22 +38,16 @@ void FtpAuthCache::Add(const GURL& origin, AuthData* auth_data) {
   }
 }
 
-void FtpAuthCache::Remove(const GURL& origin) {
+void FtpAuthCache::Remove(const GURL& origin, const std::wstring& username,
+                          const std::wstring& password) {
   for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) {
-    if (it->origin == origin) {
+    if (it->origin == origin && it->username == username &&
+        it->password == password) {
       entries_.erase(it);
-      DCHECK(!LookupEntry(origin));
+      DCHECK(!Lookup(origin));
       return;
     }
   }
 }
 
-FtpAuthCache::Entry* FtpAuthCache::LookupEntry(const GURL& origin) {
-  for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) {
-    if (it->origin == origin)
-      return &(*it);
-  }
-  return NULL;
-}
-
 }  // namespace net
diff --git a/net/ftp/ftp_auth_cache.h b/net/ftp/ftp_auth_cache.h
index 1a5b088..83cdd63 100644
--- a/net/ftp/ftp_auth_cache.h
+++ b/net/ftp/ftp_auth_cache.h
@@ -6,9 +6,9 @@
 #define NET_FTP_FTP_AUTH_CACHE_H_
 
 #include <list>
+#include <string>
 
 #include "googleurl/src/gurl.h"
-#include "net/base/auth.h"
 
 namespace net {
 
@@ -25,35 +25,38 @@ class FtpAuthCache {
   // Maximum number of entries we allow in the cache.
   static const size_t kMaxEntries;
 
-  FtpAuthCache() {}
-  ~FtpAuthCache() {}
-
-  // Check if we have authentication data for ftp server at |origin|.
-  // Returns the address of corresponding AuthData object (if found) or NULL
-  // (if not found).
-  AuthData* Lookup(const GURL& origin);
-
-  // Add an entry for |origin| to the cache. If there is already an
-  // entry for |origin|, it will be overwritten. Both parameters are IN only.
-  void Add(const GURL& origin, AuthData* auth_data);
-
-  // Remove the entry for |origin| from the cache, if one exists.
-  void Remove(const GURL& origin);
-
- private:
   struct Entry {
-    Entry(const GURL& origin, AuthData* auth_data)
+   Entry(const GURL& origin, const std::wstring& username,
+         const std::wstring& password)
         : origin(origin),
-          auth_data(auth_data) {
+          username(username),
+          password(password) {
     }
 
     const GURL origin;
-    scoped_refptr<AuthData> auth_data;
+    std::wstring username;
+    std::wstring password;
   };
-  typedef std::list<Entry> EntryList;
+
+  FtpAuthCache() {}
+  ~FtpAuthCache() {}
 
   // Return Entry corresponding to given |origin| or NULL if not found.
-  Entry* LookupEntry(const GURL& origin);
+  Entry* Lookup(const GURL& origin);
+
+  // Add an entry for |origin| to the cache (consisting of |username| and
+  // |password|). If there is already an entry for |origin|, it will be
+  // overwritten.
+  void Add(const GURL& origin, const std::wstring& username,
+           const std::wstring& password);
+
+  // Remove the entry for |origin| from the cache, if one exists and matches
+  // |username| and |password|.
+  void Remove(const GURL& origin, const std::wstring& username,
+              const std::wstring& password);
+
+ private:
+  typedef std::list<Entry> EntryList;
 
   // Internal representation of cache, an STL list. This makes lookups O(n),
   // but we expect n to be very low.
diff --git a/net/ftp/ftp_auth_cache_unittest.cc b/net/ftp/ftp_auth_cache_unittest.cc
index f74762b..3f04d96 100644
--- a/net/ftp/ftp_auth_cache_unittest.cc
+++ b/net/ftp/ftp_auth_cache_unittest.cc
@@ -8,47 +8,51 @@
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using net::AuthData;
 using net::FtpAuthCache;
 
 TEST(FtpAuthCacheTest, LookupAddRemove) {
   FtpAuthCache cache;
 
   GURL origin1("ftp://foo1");
-  scoped_refptr<AuthData> data1(new AuthData());
-
   GURL origin2("ftp://foo2");
-  scoped_refptr<AuthData> data2(new AuthData());
-
-  GURL origin3("ftp://foo3");
-  scoped_refptr<AuthData> data3(new AuthData());
 
   // Lookup non-existent entry.
   EXPECT_EQ(NULL, cache.Lookup(origin1));
 
   // Add entry for origin1.
-  cache.Add(origin1, data1.get());
-  EXPECT_EQ(data1.get(), cache.Lookup(origin1));
+  cache.Add(origin1, L"username1", L"password1");
+  FtpAuthCache::Entry* entry1 = cache.Lookup(origin1);
+  ASSERT_TRUE(entry1);
+  EXPECT_EQ(origin1, entry1->origin);
+  EXPECT_EQ(L"username1", entry1->username);
+  EXPECT_EQ(L"password1", entry1->password);
 
   // Add an entry for origin2.
-  cache.Add(origin2, data2.get());
-  EXPECT_EQ(data1.get(), cache.Lookup(origin1));
-  EXPECT_EQ(data2.get(), cache.Lookup(origin2));
+  cache.Add(origin2, L"username2", L"password2");
+  FtpAuthCache::Entry* entry2 = cache.Lookup(origin2);
+  ASSERT_TRUE(entry2);
+  EXPECT_EQ(origin2, entry2->origin);
+  EXPECT_EQ(L"username2", entry2->username);
+  EXPECT_EQ(L"password2", entry2->password);
+
+  // The original entry1 should still be there.
+  EXPECT_EQ(entry1, cache.Lookup(origin1));
 
   // Overwrite the entry for origin1.
-  cache.Add(origin1, data3.get());
-  EXPECT_EQ(data3.get(), cache.Lookup(origin1));
-  EXPECT_EQ(data2.get(), cache.Lookup(origin2));
+  cache.Add(origin1, L"username3", L"password3");
+  FtpAuthCache::Entry* entry3 = cache.Lookup(origin1);
+  ASSERT_TRUE(entry3);
+  EXPECT_EQ(origin1, entry3->origin);
+  EXPECT_EQ(L"username3", entry3->username);
+  EXPECT_EQ(L"password3", entry3->password);
 
   // Remove entry of origin1.
-  cache.Remove(origin1);
+  cache.Remove(origin1, L"username3", L"password3");
   EXPECT_EQ(NULL, cache.Lookup(origin1));
-  EXPECT_EQ(data2.get(), cache.Lookup(origin2));
 
-  // Remove non-existent entry
-  cache.Remove(origin1);
+  // Remove non-existent entry.
+  cache.Remove(origin1, L"username3", L"password3");
   EXPECT_EQ(NULL, cache.Lookup(origin1));
-  EXPECT_EQ(data2.get(), cache.Lookup(origin2));
 }
 
 // Check that if the origin differs only by port number, it is considered
@@ -57,16 +61,12 @@ TEST(FtpAuthCacheTest, LookupWithPort) {
   FtpAuthCache cache;
 
   GURL origin1("ftp://foo:80");
-  scoped_refptr<AuthData> data1(new AuthData());
-
   GURL origin2("ftp://foo:21");
-  scoped_refptr<AuthData> data2(new AuthData());
 
-  cache.Add(origin1, data1.get());
-  cache.Add(origin2, data2.get());
+  cache.Add(origin1, L"username", L"password");
+  cache.Add(origin2, L"username", L"password");
 
-  EXPECT_EQ(data1.get(), cache.Lookup(origin1));
-  EXPECT_EQ(data2.get(), cache.Lookup(origin2));
+  EXPECT_NE(cache.Lookup(origin1), cache.Lookup(origin2));
 }
 
 TEST(FtpAuthCacheTest, NormalizedKey) {
@@ -76,48 +76,61 @@ TEST(FtpAuthCacheTest, NormalizedKey) {
 
   FtpAuthCache cache;
 
-  scoped_refptr<AuthData> data1(new AuthData());
-  scoped_refptr<AuthData> data2(new AuthData());
-
   // Add.
-  cache.Add(GURL("ftp://HoSt:21"), data1.get());
+  cache.Add(GURL("ftp://HoSt:21"), L"username", L"password");
 
   // Lookup.
-  EXPECT_EQ(data1.get(), cache.Lookup(GURL("ftp://HoSt:21")));
-  EXPECT_EQ(data1.get(), cache.Lookup(GURL("ftp://host:21")));
-  EXPECT_EQ(data1.get(), cache.Lookup(GURL("ftp://host")));
+  FtpAuthCache::Entry* entry1 = cache.Lookup(GURL("ftp://HoSt:21"));
+  ASSERT_TRUE(entry1);
+  EXPECT_EQ(entry1, cache.Lookup(GURL("ftp://host:21")));
+  EXPECT_EQ(entry1, cache.Lookup(GURL("ftp://host")));
 
   // Overwrite.
-  cache.Add(GURL("ftp://host"), data2.get());
-  EXPECT_EQ(data2.get(), cache.Lookup(GURL("ftp://HoSt:21")));
+  cache.Add(GURL("ftp://host"), L"othername", L"otherword");
+  FtpAuthCache::Entry* entry2 = cache.Lookup(GURL("ftp://HoSt:21"));
+  ASSERT_TRUE(entry2);
+  EXPECT_EQ(GURL("ftp://host"), entry2->origin);
+  EXPECT_EQ(L"othername", entry2->username);
+  EXPECT_EQ(L"otherword", entry2->password);
 
   // Remove
-  cache.Remove(GURL("ftp://HOsT"));
+  cache.Remove(GURL("ftp://HOsT"), L"othername", L"otherword");
   EXPECT_EQ(NULL, cache.Lookup(GURL("ftp://host")));
 }
 
-TEST(FtpAuthCacheTest, EvictOldEntries) {
+TEST(FtpAuthCacheTest, OnlyRemoveMatching) {
   FtpAuthCache cache;
 
-  scoped_refptr<AuthData> auth_data(new AuthData());
+  cache.Add(GURL("ftp://host"), L"username", L"password");
+  EXPECT_TRUE(cache.Lookup(GURL("ftp://host")));
+
+  // Auth data doesn't match, shouldn't remove.
+  cache.Remove(GURL("ftp://host"), L"bogus", L"bogus");
+  EXPECT_TRUE(cache.Lookup(GURL("ftp://host")));
+
+  // Auth data matches, should remove.
+  cache.Remove(GURL("ftp://host"), L"username", L"password");
+  EXPECT_EQ(NULL, cache.Lookup(GURL("ftp://host")));
+}
+
+TEST(FtpAuthCacheTest, EvictOldEntries) {
+  FtpAuthCache cache;
 
   for (size_t i = 0; i < FtpAuthCache::kMaxEntries; i++)
-    cache.Add(GURL("ftp://host" + IntToString(i)), auth_data.get());
+    cache.Add(GURL("ftp://host" + IntToString(i)), L"username", L"password");
 
   // No entries should be evicted before reaching the limit.
   for (size_t i = 0; i < FtpAuthCache::kMaxEntries; i++) {
-    EXPECT_EQ(auth_data.get(),
-              cache.Lookup(GURL("ftp://host" + IntToString(i))));
+    EXPECT_TRUE(cache.Lookup(GURL("ftp://host" + IntToString(i))));
   }
 
   // Adding one entry should cause eviction of the first entry.
-  cache.Add(GURL("ftp://last_host"), auth_data.get());
+  cache.Add(GURL("ftp://last_host"), L"username", L"password");
   EXPECT_EQ(NULL, cache.Lookup(GURL("ftp://host0")));
 
   // Remaining entries should not get evicted.
   for (size_t i = 1; i < FtpAuthCache::kMaxEntries; i++) {
-    EXPECT_EQ(auth_data.get(),
-              cache.Lookup(GURL("ftp://host" + IntToString(i))));
+    EXPECT_TRUE(cache.Lookup(GURL("ftp://host" + IntToString(i))));
   }
-  EXPECT_EQ(auth_data.get(), cache.Lookup(GURL("ftp://last_host")));
+  EXPECT_TRUE(cache.Lookup(GURL("ftp://last_host")));
 }
diff --git a/net/url_request/url_request_ftp_job.cc b/net/url_request/url_request_ftp_job.cc
index eca845a..bc7f085 100644
--- a/net/url_request/url_request_ftp_job.cc
+++ b/net/url_request/url_request_ftp_job.cc
@@ -138,7 +138,8 @@ void URLRequestFtpJob::SendRequest() {
     username = WideToUTF8(server_auth_->username);
     password = WideToUTF8(server_auth_->password);
     request_->context()->ftp_auth_cache()->Add(request_->url().GetOrigin(),
-                                               server_auth_.get());
+                                               server_auth_->username,
+                                               server_auth_->password);
   } else {
     if (request_->url().has_username()) {
       username = request_->url().username();
@@ -183,13 +184,15 @@ void URLRequestFtpJob::OnIOComplete(const AsyncResult& result) {
         GURL origin = request_->url().GetOrigin();
         if (server_auth_ != NULL &&
             server_auth_->state == net::AUTH_STATE_HAVE_AUTH) {
-          request_->context()->ftp_auth_cache()->Remove(origin);
+          request_->context()->ftp_auth_cache()->Remove(origin,
+                                                        server_auth_->username,
+                                                        server_auth_->password);
         } else {
           server_auth_ = new net::AuthData();
         }
         server_auth_->state = net::AUTH_STATE_NEED_AUTH;
 
-        scoped_refptr<net::AuthData> cached_auth =
+        net::FtpAuthCache::Entry* cached_auth =
             request_->context()->ftp_auth_cache()->Lookup(origin);
 
         if (cached_auth) {
diff --git a/net/url_request/url_request_new_ftp_job.cc b/net/url_request/url_request_new_ftp_job.cc
index 7755503..83d1a9c 100644
--- a/net/url_request/url_request_new_ftp_job.cc
+++ b/net/url_request/url_request_new_ftp_job.cc
@@ -136,6 +136,9 @@ void URLRequestNewFtpJob::SetAuth(const std::wstring& username,
   server_auth_->username = username;
   server_auth_->password = password;
 
+  request_->context()->ftp_auth_cache()->Add(request_->url().GetOrigin(),
+                                             username, password);
+
   RestartTransactionWithAuth();
 }
 
@@ -361,9 +364,26 @@ void URLRequestNewFtpJob::OnStartCompleted(int result) {
   if (result == net::OK) {
     NotifyHeadersComplete();
   } else if (transaction_->GetResponseInfo()->needs_auth) {
-    server_auth_ = new net::AuthData();
+    GURL origin = request_->url().GetOrigin();
+    if (server_auth_ && server_auth_->state == net::AUTH_STATE_HAVE_AUTH) {
+      request_->context()->ftp_auth_cache()->Remove(origin,
+                                                    server_auth_->username,
+                                                    server_auth_->password);
+    } else if (!server_auth_) {
+      server_auth_ = new net::AuthData();
+    }
     server_auth_->state = net::AUTH_STATE_NEED_AUTH;
-    NotifyHeadersComplete();
+
+    net::FtpAuthCache::Entry* cached_auth =
+        request_->context()->ftp_auth_cache()->Lookup(origin);
+
+    if (cached_auth) {
+      // Retry using cached auth data.
+      SetAuth(cached_auth->username, cached_auth->password);
+    } else {
+      // Prompt for a username/password.
+      NotifyHeadersComplete();
+    }
   } else {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
   }
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index d82e085..0926c30 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -2123,3 +2123,99 @@ TEST_F(URLRequestTestFTP, FTPCheckWrongUserRestart) {
     EXPECT_EQ(d.bytes_received(), static_cast<int>(file_size));
   }
 }
+
+TEST_F(URLRequestTestFTP, FTPCacheURLCredentials) {
+  ASSERT_TRUE(NULL != server_.get());
+  FilePath app_path;
+  PathService::Get(base::DIR_SOURCE_ROOT, &app_path);
+  app_path = app_path.AppendASCII("LICENSE");
+
+  scoped_ptr<TestDelegate> d(new TestDelegate);
+  {
+    // Pass correct login identity in the URL.
+    TestURLRequest r(server_->TestServerPage("/LICENSE",
+                                             "chrome", "chrome"),
+                     d.get());
+    r.Start();
+    EXPECT_TRUE(r.is_pending());
+
+    MessageLoop::current()->Run();
+
+    int64 file_size = 0;
+    file_util::GetFileSize(app_path, &file_size);
+
+    EXPECT_FALSE(r.is_pending());
+    EXPECT_EQ(1, d->response_started_count());
+    EXPECT_FALSE(d->received_data_before_response());
+    EXPECT_EQ(d->bytes_received(), static_cast<int>(file_size));
+  }
+
+  d.reset(new TestDelegate);
+  {
+    // This request should use cached identity from previous request.
+    TestURLRequest r(server_->TestServerPage("/LICENSE"), d.get());
+    r.Start();
+    EXPECT_TRUE(r.is_pending());
+
+    MessageLoop::current()->Run();
+
+    int64 file_size = 0;
+    file_util::GetFileSize(app_path, &file_size);
+
+    EXPECT_FALSE(r.is_pending());
+    EXPECT_EQ(1, d->response_started_count());
+    EXPECT_FALSE(d->received_data_before_response());
+    EXPECT_EQ(d->bytes_received(), static_cast<int>(file_size));
+  }
+}
+
+TEST_F(URLRequestTestFTP, FTPCacheLoginBoxCredentials) {
+  ASSERT_TRUE(NULL != server_.get());
+  FilePath app_path;
+  PathService::Get(base::DIR_SOURCE_ROOT, &app_path);
+  app_path = app_path.AppendASCII("LICENSE");
+
+  scoped_ptr<TestDelegate> d(new TestDelegate);
+  // Set correct login credentials. The delegate will be asked for them when
+  // the initial login with wrong credentials will fail.
+  d->set_username(L"chrome");
+  d->set_password(L"chrome");
+  {
+    TestURLRequest r(server_->TestServerPage("/LICENSE",
+                                             "chrome", "wrong_password"),
+                     d.get());
+    r.Start();
+    EXPECT_TRUE(r.is_pending());
+
+    MessageLoop::current()->Run();
+
+    int64 file_size = 0;
+    file_util::GetFileSize(app_path, &file_size);
+
+    EXPECT_FALSE(r.is_pending());
+    EXPECT_EQ(1, d->response_started_count());
+    EXPECT_FALSE(d->received_data_before_response());
+    EXPECT_EQ(d->bytes_received(), static_cast<int>(file_size));
+  }
+
+  // Use a new delegate without explicit credentials. The cached ones should be
+  // used.
+  d.reset(new TestDelegate);
+  {
+    // Don't pass wrong credentials in the URL, they would override valid cached
+    // ones.
+    TestURLRequest r(server_->TestServerPage("/LICENSE"), d.get());
+    r.Start();
+    EXPECT_TRUE(r.is_pending());
+
+    MessageLoop::current()->Run();
+
+    int64 file_size = 0;
+    file_util::GetFileSize(app_path, &file_size);
+
+    EXPECT_FALSE(r.is_pending());
+    EXPECT_EQ(1, d->response_started_count());
+    EXPECT_FALSE(d->received_data_before_response());
+    EXPECT_EQ(d->bytes_received(), static_cast<int>(file_size));
+  }
+}