Added authentication scheme as key to HttpAuthCache.

Behavioral changes are small; this is mostly a syntactic sugar change.  But
there are a few behavioral changes:
* If a web site replies with different schemes for the same realm, we'll have two entries in the cache.
* There will not be a log entry in HttpNetworkTransaction::SelectNextAuthIdentityToTry when we have the wrong authentication scheme (we don't see that entry any more)
* We will no longer return ERR_TUNNEL_CONNECTION_FAILED from SocketStream::HandleAuthChallenge when there's an entry in the cache with a non-basic authentication scheme (we won't know it's there).

Contributed by rdsmith@chromium.org

BUG=33433
TEST=HttpAuthCacheTest.* (as modified in this commit), HttpNetworkTransactionTest.*, SocketStreamTest.*, only on Linux.

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

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

5 files changed, 131 insertions, 84 deletions

diff --git a/net/http/http_auth_cache.cc b/net/http/http_auth_cache.cc
index a7e2309..2be2198 100644
--- a/net/http/http_auth_cache.cc
+++ b/net/http/http_auth_cache.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -60,13 +60,15 @@ struct IsEnclosedBy {
 namespace net {
 
 // Performance: O(n), where n is the number of realm entries.
-HttpAuthCache::Entry* HttpAuthCache::LookupByRealm(const GURL& origin,
-                                                   const std::string& realm) {
+HttpAuthCache::Entry* HttpAuthCache::Lookup(const GURL& origin,
+                                            const std::string& realm,
+                                            const std::string& scheme) {
   CheckOriginIsValid(origin);
 
   // Linear scan through the realm entries.
   for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) {
-    if (it->origin() == origin && it->realm() == realm)
+    if (it->origin() == origin && it->realm() == realm &&
+        it->scheme() == scheme)
       return &(*it);
   }
   return NULL; // No realm entry found.
@@ -103,7 +105,8 @@ HttpAuthCache::Entry* HttpAuthCache::Add(const GURL& origin,
   CheckPathIsValid(path);
 
   // Check for existing entry (we will re-use it if present).
-  HttpAuthCache::Entry* entry = LookupByRealm(origin, handler->realm());
+  HttpAuthCache::Entry* entry = Lookup(origin, handler->realm(),
+                                       handler->scheme());
 
   if (!entry) {
     // Failsafe to prevent unbounded memory growth of the cache.
@@ -155,10 +158,12 @@ bool HttpAuthCache::Entry::HasEnclosingPath(const std::string& dir) {
 
 bool HttpAuthCache::Remove(const GURL& origin,
                            const std::string& realm,
+                           const std::string& scheme,
                            const std::wstring& username,
                            const std::wstring& password) {
   for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) {
-    if (it->origin() == origin && it->realm() == realm) {
+    if (it->origin() == origin && it->realm() == realm &&
+        it->scheme() == scheme) {
       if (username == it->username() && password == it->password()) {
         entries_.erase(it);
         return true;
diff --git a/net/http/http_auth_cache.h b/net/http/http_auth_cache.h
index 62c09e9..7062c76 100644
--- a/net/http/http_auth_cache.h
+++ b/net/http/http_auth_cache.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -16,28 +16,28 @@
 
 namespace net {
 
-// TODO(eroman): Can we change the key from (origin, realm) to
-// (origin, realm, auth_scheme)?
-
 // HttpAuthCache stores HTTP authentication identities and challenge info.
-// For each realm the cache stores a HttpAuthCache::Entry, which holds:
-//   - the realm name
-//   - the origin server {scheme, host, port}
+// For each (origin, realm, scheme) triple the cache stores a
+// HttpAuthCache::Entry, which holds:
+//   - the origin server {protocol scheme, host, port}
 //   - the last identity used (username/password)
-//   - the last auth handler used
+//   - the last auth handler used (contains realm and authentication scheme)
 //   - the list of paths which used this realm
-// Entries can be looked up by either (origin, realm) or (origin, path).
+// Entries can be looked up by either (origin, realm, scheme) or (origin, path).
 class HttpAuthCache {
  public:
   class Entry;
 
-  // Find the realm entry on server |origin| for realm |realm|.
+  // Find the realm entry on server |origin| for realm |realm| and
+  // scheme |scheme|.
   //   |origin| - the {scheme, host, port} of the server.
   //   |realm|  - case sensitive realm string.
+  //   |scheme| - case sensitive authentication scheme, should be lower-case.
   //   returns  - the matched entry or NULL.
-  Entry* LookupByRealm(const GURL& origin, const std::string& realm);
+  Entry* Lookup(const GURL& origin, const std::string& realm,
+                const std::string& scheme);
 
-  // Find the realm entry on server |origin| whose protection space includes
+  // Find the entry on server |origin| whose protection space includes
   // |path|. This uses the assumption in RFC 2617 section 2 that deeper
   // paths lie in the same protection space.
   //   |origin| - the {scheme, host, port} of the server.
@@ -46,9 +46,10 @@ class HttpAuthCache {
   //   returns  - the matched entry or NULL.
   Entry* LookupByPath(const GURL& origin, const std::string& path);
 
-  // Add a realm entry on server |origin| for realm |handler->realm()|, If an
-  // entry for this realm already exists, update it rather than replace it --
-  // this  preserves the realm's paths list.
+  // Add an entry on server |origin| for realm |handler->realm()| and
+  // scheme |handler->scheme()|.  If an entry for this (realm,scheme)
+  // already exists, update it rather than replace it -- this  preserves the
+  // paths list.
   //   |origin|   - the {scheme, host, port} of the server.
   //   |handler|  - handler for the challenge.
   //   |username| - login information for the realm.
@@ -62,15 +63,17 @@ class HttpAuthCache {
              const std::wstring& password,
              const std::string& path);
 
-  // Remove realm entry on server |origin| for realm |realm| if one exists
-  // AND if the cached identity matches (|username|, |password|).
+  // Remove entry on server |origin| for realm |realm| and scheme |scheme|
+  // if one exists AND if the cached identity matches (|username|, |password|).
   //   |origin|   - the {scheme, host, port} of the server.
   //   |realm|    - case sensitive realm string.
+  //   |scheme|   - authentication scheme
   //   |username| - condition to match.
   //   |password| - condition to match.
   //   returns    - true if an entry was removed.
   bool Remove(const GURL& origin,
               const std::string& realm,
+              const std::string& scheme,
               const std::wstring& username,
               const std::wstring& password);
 
@@ -98,6 +101,11 @@ class HttpAuthCache::Entry {
     return handler_->realm();
   }
 
+  // The authentication scheme string of the challenge
+  const std::string scheme() const {
+    return handler_->scheme();
+  }
+
   // The handler for the challenge.
   HttpAuthHandler* handler() const {
     return handler_.get();
diff --git a/net/http/http_auth_cache_unittest.cc b/net/http/http_auth_cache_unittest.cc
index c233f01..28cb138 100644
--- a/net/http/http_auth_cache_unittest.cc
+++ b/net/http/http_auth_cache_unittest.cc
@@ -69,58 +69,89 @@ TEST(HttpAuthCacheTest, Basic) {
   cache.Add(origin, realm2_handler, L"realm2-user", L"realm2-password",
             "/foo2/index.html");
 
-  scoped_refptr<HttpAuthHandler> realm3_handler =
+  scoped_refptr<HttpAuthHandler> realm3_basic_handler =
       new MockAuthHandler("basic", "Realm3", HttpAuth::AUTH_PROXY);
-  cache.Add(origin, realm3_handler, L"realm3-user", L"realm3-password", "");
+  cache.Add(origin, realm3_basic_handler, L"realm3-basic-user",
+            L"realm3-basic-password", "");
+
+  scoped_refptr<HttpAuthHandler> realm3_digest_handler =
+      new MockAuthHandler("digest", "Realm3", HttpAuth::AUTH_PROXY);
+  cache.Add(origin, realm3_digest_handler, L"realm3-digest-user",
+            L"realm3-digest-password", "/baz/index.html");
 
   // There is no Realm4
-  entry = cache.LookupByRealm(origin, "Realm4");
+  entry = cache.Lookup(origin, "Realm4", "basic");
   EXPECT_TRUE(NULL == entry);
 
   // While Realm3 does exist, the origin scheme is wrong.
-  entry = cache.LookupByRealm(GURL("https://www.google.com"), "Realm3");
+  entry = cache.Lookup(GURL("https://www.google.com"), "Realm3",
+                       "basic");
   EXPECT_TRUE(NULL == entry);
 
-  // Valid lookup by realm.
-  entry = cache.LookupByRealm(GURL("http://www.google.com:80"), "Realm3");
+  // Realm, origin scheme ok, authentication scheme wrong
+  entry = cache.Lookup(GURL("http://www.google.com"), "Realm1", "digest");
+  EXPECT_TRUE(NULL == entry);
+
+  // Valid lookup by origin, realm, scheme.
+  entry = cache.Lookup(GURL("http://www.google.com:80"), "Realm3", "basic");
   EXPECT_FALSE(NULL == entry);
-  EXPECT_TRUE(entry->handler() == realm3_handler.get());
-  EXPECT_EQ(L"realm3-user", entry->username());
-  EXPECT_EQ(L"realm3-password", entry->password());
+  EXPECT_TRUE(entry->handler() == realm3_basic_handler.get());
+  EXPECT_EQ(L"realm3-basic-user", entry->username());
+  EXPECT_EQ(L"realm3-basic-password", entry->password());
+
+  // Valid lookup by origin, realm, scheme when there's a duplicate
+  // origin, realm in the cache
+  entry = cache.Lookup(GURL("http://www.google.com:80"), "Realm3", "digest");
+  EXPECT_FALSE(NULL == entry);
+  EXPECT_TRUE(entry->handler() == realm3_digest_handler.get());
+  EXPECT_EQ(L"realm3-digest-user", entry->username());
+  EXPECT_EQ(L"realm3-digest-password", entry->password());
 
   // Valid lookup by realm.
-  entry = cache.LookupByRealm(origin, "Realm2");
+  entry = cache.Lookup(origin, "Realm2", "basic");
   EXPECT_FALSE(NULL == entry);
   EXPECT_TRUE(entry->handler() == realm2_handler.get());
   EXPECT_EQ(L"realm2-user", entry->username());
   EXPECT_EQ(L"realm2-password", entry->password());
 
   // Check that subpaths are recognized.
-  HttpAuthCache::Entry* realm2Entry = cache.LookupByRealm(origin, "Realm2");
-  EXPECT_FALSE(NULL == realm2Entry);
+  HttpAuthCache::Entry* realm2_entry = cache.Lookup(origin, "Realm2", "basic");
+  EXPECT_FALSE(NULL == realm2_entry);
   // Positive tests:
   entry = cache.LookupByPath(origin, "/foo2/index.html");
-  EXPECT_TRUE(realm2Entry == entry);
+  EXPECT_TRUE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "/foo2/foobar.html");
-  EXPECT_TRUE(realm2Entry == entry);
+  EXPECT_TRUE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "/foo2/bar/index.html");
-  EXPECT_TRUE(realm2Entry == entry);
+  EXPECT_TRUE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "/foo2/");
-  EXPECT_TRUE(realm2Entry == entry);
+  EXPECT_TRUE(realm2_entry == entry);
+
   // Negative tests:
   entry = cache.LookupByPath(origin, "/foo2");
-  EXPECT_FALSE(realm2Entry == entry);
+  EXPECT_FALSE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "/foo3/index.html");
-  EXPECT_FALSE(realm2Entry == entry);
+  EXPECT_FALSE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "");
-  EXPECT_FALSE(realm2Entry == entry);
+  EXPECT_FALSE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "/");
-  EXPECT_FALSE(realm2Entry == entry);
+  EXPECT_FALSE(realm2_entry == entry);
+
+  // Confirm we find the same realm, different auth scheme by path lookup
+  HttpAuthCache::Entry* realm3_digest_entry =
+      cache.Lookup(origin, "Realm3", "digest");
+  EXPECT_FALSE(NULL == realm3_digest_entry);
+  entry = cache.LookupByPath(origin, "/baz/index.html");
+  EXPECT_TRUE(realm3_digest_entry == entry);
+  entry = cache.LookupByPath(origin, "/baz/");
+  EXPECT_TRUE(realm3_digest_entry == entry);
+  entry = cache.LookupByPath(origin, "/baz");
+  EXPECT_FALSE(realm3_digest_entry == entry);
 
   // Lookup using empty path (may be used for proxy).
   entry = cache.LookupByPath(origin, "");
   EXPECT_FALSE(NULL == entry);
-  EXPECT_TRUE(entry->handler() == realm3_handler.get());
+  EXPECT_TRUE(entry->handler() == realm3_basic_handler.get());
   EXPECT_EQ("Realm3", entry->realm());
 }
 
@@ -170,7 +201,7 @@ TEST(HttpAuthCacheTest, AddToExistingEntry) {
   cache.Add(origin, handler, L"user2", L"password2", "/z/y/x/");
   cache.Add(origin, handler, L"user3", L"password3", "/z/y");
 
-  HttpAuthCache::Entry* entry = cache.LookupByRealm(origin, "MyRealm");
+  HttpAuthCache::Entry* entry = cache.Lookup(origin, "MyRealm", "basic");
 
   EXPECT_TRUE(entry == orig_entry);
   EXPECT_EQ(L"user3", entry->username());
@@ -190,32 +221,51 @@ TEST(HttpAuthCacheTest, Remove) {
   scoped_refptr<HttpAuthHandler> realm2_handler =
       new MockAuthHandler("basic", "Realm2", HttpAuth::AUTH_SERVER);
 
-  scoped_refptr<HttpAuthHandler> realm3_handler =
+  scoped_refptr<HttpAuthHandler> realm3_basic_handler =
       new MockAuthHandler("basic", "Realm3", HttpAuth::AUTH_SERVER);
 
+  scoped_refptr<HttpAuthHandler> realm3_digest_handler =
+      new MockAuthHandler("digest", "Realm3", HttpAuth::AUTH_SERVER);
+
   HttpAuthCache cache;
   cache.Add(origin, realm1_handler, L"alice", L"123", "/");
   cache.Add(origin, realm2_handler, L"bob", L"princess", "/");
-  cache.Add(origin, realm3_handler, L"admin", L"password", "/");
+  cache.Add(origin, realm3_basic_handler, L"admin", L"password", "/");
+  cache.Add(origin, realm3_digest_handler, L"root", L"wilecoyote", "/");
 
   // Fails, because there is no realm "Realm4".
-  EXPECT_FALSE(cache.Remove(origin, "Realm4", L"alice", L"123"));
+  EXPECT_FALSE(cache.Remove(origin, "Realm4", "basic", L"alice", L"123"));
 
   // Fails because the origin is wrong.
   EXPECT_FALSE(cache.Remove(
-      GURL("http://foobar2.com:100"), "Realm1", L"alice", L"123"));
+      GURL("http://foobar2.com:100"), "Realm1", "basic", L"alice", L"123"));
 
   // Fails because the username is wrong.
-  EXPECT_FALSE(cache.Remove(origin, "Realm1", L"alice2", L"123"));
+  EXPECT_FALSE(cache.Remove(origin, "Realm1", "basic", L"alice2", L"123"));
 
   // Fails because the password is wrong.
-  EXPECT_FALSE(cache.Remove(origin, "Realm1", L"alice", L"1234"));
+  EXPECT_FALSE(cache.Remove(origin, "Realm1", "basic", L"alice", L"1234"));
+
+  // Fails because the authentication type is wrong.
+  EXPECT_FALSE(cache.Remove(origin, "Realm1", "digest", L"alice", L"123"));
 
   // Succeeds.
-  EXPECT_TRUE(cache.Remove(origin, "Realm1", L"alice", L"123"));
+  EXPECT_TRUE(cache.Remove(origin, "Realm1", "basic", L"alice", L"123"));
 
   // Fails because we just deleted the entry!
-  EXPECT_FALSE(cache.Remove(origin, "Realm1", L"alice", L"123"));
+  EXPECT_FALSE(cache.Remove(origin, "Realm1", "basic", L"alice", L"123"));
+
+  // Succeed when there are two authentication types for the same origin,realm.
+  EXPECT_TRUE(cache.Remove(origin, "Realm3", "digest", L"root", L"wilecoyote"));
+
+  // Succeed as above, but when entries were added in opposite order
+  cache.Add(origin, realm3_digest_handler, L"root", L"wilecoyote", "/");
+  EXPECT_TRUE(cache.Remove(origin, "Realm3", "basic", L"admin", L"password"));
+
+  // Make sure that removing one entry still leaves the other available
+  // for lookup
+  HttpAuthCache::Entry* entry = cache.Lookup(origin, "Realm3", "digest");
+  EXPECT_FALSE(NULL == entry);
 }
 
 // Test fixture class for eviction tests (contains helpers for bulk
@@ -246,7 +296,7 @@ class HttpAuthCacheEvictionTest : public testing::Test {
 
   void CheckRealmExistence(int realm_i, bool exists) {
     const HttpAuthCache::Entry* entry =
-        cache_.LookupByRealm(origin_, GenerateRealm(realm_i));
+      cache_.Lookup(origin_, GenerateRealm(realm_i), "basic");
     if (exists) {
       EXPECT_FALSE(entry == NULL);
       EXPECT_EQ(GenerateRealm(realm_i), entry->realm());
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 6ac135e..eeb9941 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -1877,6 +1877,7 @@ void HttpNetworkTransaction::InvalidateRejectedAuthFromCache(
   // since the entry in the cache may be newer than what we used last time.
   session_->auth_cache()->Remove(auth_origin,
                                  auth_handler_[target]->realm(),
+                                 auth_handler_[target]->scheme(),
                                  auth_identity_[target].username,
                                  auth_identity_[target].password);
 }
@@ -1937,31 +1938,16 @@ bool HttpNetworkTransaction::SelectNextAuthIdentityToTry(
   }
 
   // Check the auth cache for a realm entry.
-  HttpAuthCache::Entry* entry = session_->auth_cache()->LookupByRealm(
-      auth_origin, auth_handler_[target]->realm());
+  HttpAuthCache::Entry* entry =
+    session_->auth_cache()->Lookup(auth_origin, auth_handler_[target]->realm(),
+                                   auth_handler_[target]->scheme());
 
   if (entry) {
-    // Disallow re-using of identity if the scheme of the originating challenge
-    // does not match. This protects against the following situation:
-    // 1. Browser prompts user to sign into DIGEST realm="Foo".
-    // 2. Since the auth-scheme is not BASIC, the user is reasured that it
-    //    will not be sent over the wire in clear text. So they use their
-    //    most trusted password.
-    // 3. Next, the browser receives a challenge for BASIC realm="Foo". This
-    //    is the same realm that we have a cached identity for. However if
-    //    we use that identity, it would get sent over the wire in
-    //    clear text (which isn't what the user agreed to when entering it).
-    if (entry->handler()->scheme() == auth_handler_[target]->scheme()) {
-      auth_identity_[target].source = HttpAuth::IDENT_SRC_REALM_LOOKUP;
-      auth_identity_[target].invalid = false;
-      auth_identity_[target].username = entry->username();
-      auth_identity_[target].password = entry->password();
-      return true;
-    }
-    LOG(WARNING) << "The scheme of realm " << auth_handler_[target]->realm()
-                 << " has changed from " << entry->handler()->scheme()
-                 << " to " << auth_handler_[target]->scheme();
-    // Fall through.
+    auth_identity_[target].source = HttpAuth::IDENT_SRC_REALM_LOOKUP;
+    auth_identity_[target].invalid = false;
+    auth_identity_[target].username = entry->username();
+    auth_identity_[target].password = entry->password();
+    return true;
   }
 
   // Use default credentials (single sign on) if this is the first attempt
diff --git a/net/socket_stream/socket_stream.cc b/net/socket_stream/socket_stream.cc
index b957d8a..68fbbf3 100644
--- a/net/socket_stream/socket_stream.cc
+++ b/net/socket_stream/socket_stream.cc
@@ -862,6 +862,7 @@ int SocketStream::HandleAuthChallenge(const HttpResponseHeaders* headers) {
     if (auth_identity_.source != HttpAuth::IDENT_SRC_PATH_LOOKUP)
       auth_cache_.Remove(auth_origin,
                          auth_handler_->realm(),
+                         auth_handler_->scheme(),
                          auth_identity_.username,
                          auth_identity_.password);
     auth_handler_ = NULL;
@@ -877,14 +878,11 @@ int SocketStream::HandleAuthChallenge(const HttpResponseHeaders* headers) {
     return ERR_TUNNEL_CONNECTION_FAILED;
   }
   if (auth_handler_->NeedsIdentity()) {
-    HttpAuthCache::Entry* entry = auth_cache_.LookupByRealm(
-        auth_origin, auth_handler_->realm());
+    // We only support basic authentication scheme now.
+    // TODO(ukai): Support other authentication scheme.
+    HttpAuthCache::Entry* entry =
+        auth_cache_.Lookup(auth_origin, auth_handler_->realm(), "basic");
     if (entry) {
-      if (entry->handler()->scheme() != "basic") {
-        // We only support basic authentication scheme now.
-        // TODO(ukai): Support other authentication scheme.
-        return ERR_TUNNEL_CONNECTION_FAILED;
-      }
       auth_identity_.source = HttpAuth::IDENT_SRC_REALM_LOOKUP;
       auth_identity_.invalid = false;
       auth_identity_.username = entry->username();