Pick the closest enclosing path match when looking up HTTP auth cache entries by path

If we have two cache entries :

  < 'example.com', userA, realmA, paths=[ '/' ] > and
  < 'example.com', userB, realmB, paths=[ '/foo/' ] >

Then a LookupByPath() for '/foo/bar/baz' should return the cache entry for userB rather than userA.

BUG=73294
TEST=net_unittests --gtest_filter=HttpAuthCacheTest.Basic

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

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

3 files changed, 50 insertions, 17 deletions

diff --git a/net/http/http_auth_cache.cc b/net/http/http_auth_cache.cc
index 95d0d69..ab8be9e 100644
--- a/net/http/http_auth_cache.cc
+++ b/net/http/http_auth_cache.cc
@@ -85,6 +85,8 @@ HttpAuthCache::Entry* HttpAuthCache::Lookup(const GURL& origin,
 // kept small because AddPath() only keeps the shallowest entry.
 HttpAuthCache::Entry* HttpAuthCache::LookupByPath(const GURL& origin,
                                                   const std::string& path) {
+  HttpAuthCache::Entry* best_match = NULL;
+  size_t best_match_length = 0;
   CheckOriginIsValid(origin);
   CheckPathIsValid(path);
 
@@ -96,10 +98,14 @@ HttpAuthCache::Entry* HttpAuthCache::LookupByPath(const GURL& origin,
 
   // Linear scan through the realm entries.
   for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) {
-    if (it->origin() == origin && it->HasEnclosingPath(parent_dir))
-      return &(*it);
+    size_t len = 0;
+    if (it->origin() == origin && it->HasEnclosingPath(parent_dir, &len) &&
+        (!best_match || len > best_match_length)) {
+      best_match_length = len;
+      best_match = &(*it);
+    }
   }
-  return NULL;  // No entry found.
+  return best_match;
 }
 
 HttpAuthCache::Entry* HttpAuthCache::Add(const GURL& origin,
@@ -156,7 +162,7 @@ HttpAuthCache::Entry::Entry()
 
 void HttpAuthCache::Entry::AddPath(const std::string& path) {
   std::string parent_dir = GetParentDirectory(path);
-  if (!HasEnclosingPath(parent_dir)) {
+  if (!HasEnclosingPath(parent_dir, NULL)) {
     // Remove any entries that have been subsumed by the new entry.
     paths_.remove_if(IsEnclosedBy(parent_dir));
 
@@ -172,12 +178,20 @@ void HttpAuthCache::Entry::AddPath(const std::string& path) {
   }
 }
 
-bool HttpAuthCache::Entry::HasEnclosingPath(const std::string& dir) {
+bool HttpAuthCache::Entry::HasEnclosingPath(const std::string& dir,
+                                            size_t* path_len) {
   DCHECK(GetParentDirectory(dir) == dir);
   for (PathList::const_iterator it = paths_.begin(); it != paths_.end();
        ++it) {
-    if (IsEnclosingPath(*it, dir))
+    if (IsEnclosingPath(*it, dir)) {
+      // No element of paths_ may enclose any other element.
+      // Therefore this path is the tightest bound.  Important because
+      // the length returned is used to determine the cache entry that
+      // has the closest enclosing path in LookupByPath().
+      if (path_len)
+        *path_len = it->length();
       return true;
+    }
   }
   return false;
 }
diff --git a/net/http/http_auth_cache.h b/net/http/http_auth_cache.h
index a130956..2895401 100644
--- a/net/http/http_auth_cache.h
+++ b/net/http/http_auth_cache.h
@@ -160,8 +160,11 @@ class HttpAuthCache::Entry {
   // already contained in the protection space, is a no-op.
   void AddPath(const std::string& path);
 
-  // Returns true if |dir| is contained within the realm's protection space.
-  bool HasEnclosingPath(const std::string& dir);
+  // Returns true if |dir| is contained within the realm's protection
+  // space.  |*path_len| is set to the length of the enclosing path if
+  // such a path exists and |path_len| is non-NULL.  If no enclosing
+  // path is found, |path_len| is left unmodified.
+  bool HasEnclosingPath(const std::string& dir, size_t* path_len);
 
   // |origin_| contains the {protocol, host, port} of the server.
   GURL origin_;
diff --git a/net/http/http_auth_cache_unittest.cc b/net/http/http_auth_cache_unittest.cc
index e940527..d988f3c 100644
--- a/net/http/http_auth_cache_unittest.cc
+++ b/net/http/http_auth_cache_unittest.cc
@@ -58,6 +58,7 @@ const char* kRealm1 = "Realm1";
 const char* kRealm2 = "Realm2";
 const char* kRealm3 = "Realm3";
 const char* kRealm4 = "Realm4";
+const char* kRealm5 = "Realm5";
 const string16 k123(ASCIIToUTF16("123"));
 const string16 k1234(ASCIIToUTF16("1234"));
 const string16 kAdmin(ASCIIToUTF16("admin"));
@@ -76,7 +77,8 @@ TEST(HttpAuthCacheTest, Basic) {
   HttpAuthCache cache;
   HttpAuthCache::Entry* entry;
 
-  // Add cache entries for 3 realms: "Realm1", "Realm2", "Realm3"
+  // Add cache entries for 4 realms: "Realm1", "Realm2", "Realm3" and
+  // "Realm4"
 
   scoped_ptr<HttpAuthHandler> realm1_handler(
       new MockAuthHandler(HttpAuth::AUTH_SCHEME_BASIC,
@@ -112,8 +114,17 @@ TEST(HttpAuthCacheTest, Basic) {
             ASCIIToUTF16("realm3-digest-user"),
             ASCIIToUTF16("realm3-digest-password"), "/baz/index.html");
 
-  // There is no Realm4
-  entry = cache.Lookup(origin, kRealm4, HttpAuth::AUTH_SCHEME_BASIC);
+  scoped_ptr<HttpAuthHandler> realm4_basic_handler(
+      new MockAuthHandler(HttpAuth::AUTH_SCHEME_BASIC,
+                          kRealm4,
+                          HttpAuth::AUTH_SERVER));
+  cache.Add(origin, realm4_basic_handler->realm(),
+            realm4_basic_handler->auth_scheme(), "Basic realm=Realm4",
+            ASCIIToUTF16("realm4-basic-user"),
+            ASCIIToUTF16("realm4-basic-password"), "/");
+
+  // There is no Realm5
+  entry = cache.Lookup(origin, kRealm5, HttpAuth::AUTH_SCHEME_BASIC);
   EXPECT_TRUE(NULL == entry);
 
   // While Realm3 does exist, the origin scheme is wrong.
@@ -159,7 +170,12 @@ TEST(HttpAuthCacheTest, Basic) {
   // Check that subpaths are recognized.
   HttpAuthCache::Entry* realm2_entry = cache.Lookup(
       origin, kRealm2, HttpAuth::AUTH_SCHEME_BASIC);
+  HttpAuthCache::Entry* realm4_entry = cache.Lookup(
+      origin, kRealm4, HttpAuth::AUTH_SCHEME_BASIC);
   EXPECT_FALSE(NULL == realm2_entry);
+  EXPECT_FALSE(NULL == realm4_entry);
+  // Realm4 applies to '/' and Realm2 applies to '/foo2/'.
+  // LookupByPath() should return the closest enclosing path.
   // Positive tests:
   entry = cache.LookupByPath(origin, "/foo2/index.html");
   EXPECT_TRUE(realm2_entry == entry);
@@ -169,16 +185,16 @@ TEST(HttpAuthCacheTest, Basic) {
   EXPECT_TRUE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "/foo2/");
   EXPECT_TRUE(realm2_entry == entry);
+  entry = cache.LookupByPath(origin, "/foo2");
+  EXPECT_TRUE(realm4_entry == entry);
+  entry = cache.LookupByPath(origin, "/");
+  EXPECT_TRUE(realm4_entry == entry);
 
   // Negative tests:
-  entry = cache.LookupByPath(origin, "/foo2");
-  EXPECT_FALSE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "/foo3/index.html");
   EXPECT_FALSE(realm2_entry == entry);
   entry = cache.LookupByPath(origin, "");
   EXPECT_FALSE(realm2_entry == entry);
-  entry = cache.LookupByPath(origin, "/");
-  EXPECT_FALSE(realm2_entry == entry);
 
   // Confirm we find the same realm, different auth scheme by path lookup
   HttpAuthCache::Entry* realm3_digest_entry =
@@ -303,9 +319,9 @@ TEST(HttpAuthCacheTest, Remove) {
             realm3_digest_handler->auth_scheme(), "digest realm=Realm3",
             kRoot, kWileCoyote, "/");
 
-  // Fails, because there is no realm "Realm4".
+  // Fails, because there is no realm "Realm5".
   EXPECT_FALSE(cache.Remove(
-      origin, kRealm4, HttpAuth::AUTH_SCHEME_BASIC, kAlice, k123));
+      origin, kRealm5, HttpAuth::AUTH_SCHEME_BASIC, kAlice, k123));
 
   // Fails because the origin is wrong.
   EXPECT_FALSE(cache.Remove(GURL("http://foobar2.com:100"),