PasswordProtectionService verdict cache management

components/safe_browsing/password_protection/password_protection_service_unittest.cc

Index: components/safe_browsing/password_protection/password_protection_service_unittest.cc
diff --git a/components/safe_browsing/password_protection/password_protection_service_unittest.cc b/components/safe_browsing/password_protection/password_protection_service_unittest.cc
index 4901dc6ce2c12241f21412cda8530e4b15a9ea0c..7aa9b40798120a87c0cc57657ca4978d160096f5 100644
--- a/components/safe_browsing/password_protection/password_protection_service_unittest.cc
+++ b/components/safe_browsing/password_protection/password_protection_service_unittest.cc
@@ -5,8 +5,10 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/test/histogram_tester.h"
 #include "components/safe_browsing_db/test_database_manager.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -39,10 +41,28 @@ class PasswordProtectionServiceTest : public testing::Test {
  public:
   PasswordProtectionServiceTest(){};
 
+  ~PasswordProtectionServiceTest() override {
+    content_setting_map_->ShutdownOnUIThread();
+  }
+
+  LoginReputationClientResponse CreateVerdictProto(
+      LoginReputationClientResponse::VerdictType verdict,
+      int cache_duration_sec,
+      const std::string& cache_expression) {
+    LoginReputationClientResponse verdict_proto;
+    verdict_proto.set_verdict_type(verdict);
+    verdict_proto.set_cache_duration_sec(cache_duration_sec);
+    verdict_proto.set_cache_expression(cache_expression);
+    return verdict_proto;
+  }
+
   void SetUp() override {
     database_manager_ = new MockSafeBrowsingDatabaseManager();
     password_protection_service_ =
         base::MakeUnique<PasswordProtectionService>(database_manager_);
+    HostContentSettingsMap::RegisterProfilePrefs(test_pref_service_.registry());
+    content_setting_map_ = new HostContentSettingsMap(
+        &test_pref_service_, false /* incognito */, false /* guest_profile */);
   }
 
  protected:
@@ -51,6 +71,8 @@ class PasswordProtectionServiceTest : public testing::Test {
   content::TestBrowserThreadBundle thread_bundle_;
   scoped_refptr<MockSafeBrowsingDatabaseManager> database_manager_;
   std::unique_ptr<PasswordProtectionService> password_protection_service_;
+  sync_preferences::TestingPrefServiceSyncable test_pref_service_;
+  scoped_refptr<HostContentSettingsMap> content_setting_map_;
 };
 
 TEST_F(PasswordProtectionServiceTest,
@@ -85,4 +107,161 @@ TEST_F(PasswordProtectionServiceTest,
       testing::ElementsAre(base::Bucket(0, 1), base::Bucket(1, 1)));
 }
 
+TEST_F(PasswordProtectionServiceTest, TestParseVerdictEntry) {

[Nathan Parker, 2017/03/16 21:27:11]

nit: split valid/invalid cases into two tests, since they don't share data.

[Jialiu Lin, 2017/03/18 23:46:32]

Done.

+  int now_in_sec = static_cast<int>(base::Time::Now().ToDoubleT());
+  std::unique_ptr<base::DictionaryValue> valid_verdict_entry =
+      base::MakeUnique<base::DictionaryValue>();
+  valid_verdict_entry->SetInteger("cache_ttl", 10 * 60);
+  valid_verdict_entry->SetInteger("cache_creation_time", now_in_sec);
+  valid_verdict_entry->SetInteger("verdict", 1 /* SAFE */);
+
+  std::unique_ptr<base::DictionaryValue> invalid_verdict_entry =
+      base::MakeUnique<base::DictionaryValue>();
+  invalid_verdict_entry->SetString("cache_ttl", "invalid_ttl");
+
+  int cache_creation_time, cache_ttl, verdict_type_value;
+  // ParseVerdictEntry fails if input is empty.
+  EXPECT_FALSE(PasswordProtectionService::ParseVerdictEntry(
+      nullptr, &cache_creation_time, &cache_ttl, &verdict_type_value));
+
+  // ParseVerdictEntry fails if the input dict value is invalid.
+  EXPECT_FALSE(PasswordProtectionService::ParseVerdictEntry(
+      invalid_verdict_entry.get(), &cache_creation_time, &cache_ttl,
+      &verdict_type_value));
+
+  // ParseVerdictEntry success case.
+  ASSERT_TRUE(PasswordProtectionService::ParseVerdictEntry(
+      valid_verdict_entry.get(), &cache_creation_time, &cache_ttl,
+      &verdict_type_value));
+  EXPECT_EQ(600, cache_ttl);
+  EXPECT_EQ(now_in_sec, cache_creation_time);
+  EXPECT_EQ(LoginReputationClientResponse::SAFE,
+            static_cast<LoginReputationClientResponse::VerdictType>(
+                verdict_type_value));
+}
+
+TEST_F(PasswordProtectionServiceTest, TestUrlMatchCacheExpression) {
+  // Cache expression without path.
+  std::string cache_expression("google.com");
+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("https://www.google.com"), cache_expression));

[Nathan Parker, 2017/03/16 21:27:11]

How about w/ and w/o the trailing slash? Is there something there we should
test?

[Jialiu Lin, 2017/03/18 23:46:32]

Sure.

+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("http://google.com"), cache_expression));
+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("https://maps.google.com"), cache_expression));
+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("https://www.google.com/maps/local/"), cache_expression));
+
+  // Cache expression with sub-domain.
+  cache_expression = "maps.google.com";
+  EXPECT_FALSE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("https://google.com"), cache_expression));
+  EXPECT_FALSE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("https://www.google.com"), cache_expression));
+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("https://maps.google.com"), cache_expression));
+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("https://maps.google.com/local/"), cache_expression));
+
+  // Cache expression with path.
+  cache_expression = std::string("evil.com/bad");
+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("http://evil.com/bad/index.html"), cache_expression));
+  EXPECT_FALSE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("http://evil.com/worse/index.html"), cache_expression));
+  EXPECT_TRUE(PasswordProtectionService::UrlMatchCacheExpression(
+      GURL("http://phishing.malware.evil.com/bad/index.html"),
+      cache_expression));
+}
+
+TEST_F(PasswordProtectionServiceTest, TestSetGetAndClearCachedVerdict) {
+  // Assume each verdict has a TTL of 10 minutes.
+  LoginReputationClientResponse verdict_proto_1(CreateVerdictProto(
+      LoginReputationClientResponse::SAFE, 10 * 60, "test.com/foo"));
+  LoginReputationClientResponse verdict_proto_2(CreateVerdictProto(
+      LoginReputationClientResponse::LOW_REPUTATION, 10 * 60, "test.com/bar"));
+  LoginReputationClientResponse verdict_proto_3(CreateVerdictProto(
+      LoginReputationClientResponse::PHISHING, 10 * 60, "evil.com"));
+
+  base::Time now = base::Time::Now();
+  base::Time yesterday =
+      base::Time::FromDoubleT(now.ToDoubleT() - 24.0 * 60.0 * 60.0);
+  base::Time one_minute_ago = base::Time::FromDoubleT(now.ToDoubleT() - 60.0);
+  password_protection_service_->CacheVerdict(

[Nathan Parker, 2017/03/16 21:27:11]

nit: I find these a bit hard to reason about.  Would be more clear if you
grouped the verdict with the CacheVerdict() call, with blank lines between them?
And can we do without the LoginReputationClientResponse:: prefix on verdict
types?  This implies it's in the safe_browsing namespace:

https://cs.chromium.org/chromium/src/out/Debug/gen/components/safe_browsing/c...

[Jialiu Lin, 2017/03/18 23:46:32]

Regrouped test case.
There are multiple SAFE enum values defined in csd.proto, I have to keep the
LoginReputationClientResponse prefix.. 

+      content_setting_map_.get(), GURL("http://www.test.com/foo/index.html"),
+      &verdict_proto_1, now);
+  password_protection_service_->CacheVerdict(
+      content_setting_map_.get(), GURL("http://www.test.com/bar/index.html"),
+      &verdict_proto_2, yesterday);
+  password_protection_service_->CacheVerdict(
+      content_setting_map_.get(),
+      GURL("http://phishing.evil.com/local/login.html"), &verdict_proto_3,
+      one_minute_ago);
+
+  // Return VERDICT_TYPE_UNSPECIFIED if content setting map has nothing of this
+  // origin.
+  EXPECT_EQ(LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED,
+            password_protection_service_->GetCachedVerdict(
+                content_setting_map_.get(),
+                GURL("http://www.unkown.com/index.html")));

[Nathan Parker, 2017/03/16 21:27:11]

(nit: www.unknown.com)

[Jialiu Lin, 2017/03/18 23:46:32]

Good eye!

+
+  // Return VERDICT_TYPE_UNSPECIFIED if verdict is expired.
+  EXPECT_EQ(LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED,
+            password_protection_service_->GetCachedVerdict(
+                content_setting_map_.get(),
+                GURL("http://www.test.com/bar/test.html")));
+
+  // Return its corresponding verdict type if verdict is available and not
+  // expired.
+  EXPECT_EQ(LoginReputationClientResponse::SAFE,
+            password_protection_service_->GetCachedVerdict(
+                content_setting_map_.get(),
+                GURL("http://www.test.com/foo/index1.html")));
+  EXPECT_EQ(LoginReputationClientResponse::PHISHING,
+            password_protection_service_->GetCachedVerdict(
+                content_setting_map_.get(),
+                GURL("http://phishing.evil.com/phishing/")));
+
+  // Cache www.test.com/bar again, but this time it is not expired.
+  password_protection_service_->CacheVerdict(
+      content_setting_map_.get(), GURL("http://www.test.com/bar/index.html"),
+      &verdict_proto_2, now);
+  EXPECT_EQ(LoginReputationClientResponse::LOW_REPUTATION,
+            password_protection_service_->GetCachedVerdict(
+                content_setting_map_.get(),
+                GURL("http://www.test.com/bar/test2.html")));
+
+  // Now, there should be cached verdicts for 2 origins: http://www.test.com,
+  // and http://phishing.evil.com
+  ContentSettingsForOneType password_protection_settings;
+  content_setting_map_->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
+      &password_protection_settings);
+  EXPECT_EQ(2U, password_protection_settings.size());
+
+  history::URLRows deleted_urls;
+  deleted_urls.push_back(
+      history::URLRow(GURL("http://phishing.evil.com/phishing/")));
+  // We delete the history of http://phishing.evil.com/phishing.
+  password_protection_service_->RemoveContentSettingsOnURLsDeleted(
+      content_setting_map_.get(), false, deleted_urls);
+  content_setting_map_->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
+      &password_protection_settings);
+  EXPECT_EQ(1U, password_protection_settings.size());
+  EXPECT_EQ(LoginReputationClientResponse::LOW_REPUTATION,
+            password_protection_service_->GetCachedVerdict(
+                content_setting_map_.get(),
+                GURL("http://www.test.com/bar/test2.html")));
+
+  // Now we delete all history. All password protection content settings will be
+  // gone too.
+  password_protection_service_->RemoveContentSettingsOnURLsDeleted(
+      content_setting_map_.get(), true, deleted_urls);
+  content_setting_map_->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
+      &password_protection_settings);
+  EXPECT_EQ(0U, password_protection_settings.size());
+}

[Nathan Parker, 2017/03/16 21:27:11]

This is a long test testing multiple different things.  I think it would cleaner
to split it, maybe into matching origins/paths, checking TTLs, and deleting
entries.  Or some such.

[Jialiu Lin, 2017/03/18 23:46:32]

Done. Separated this one into 3 tests.

+
 }  // namespace safe_browsing