Return MANUAL reason for host patterns that match default behavior.

This change enable parents to override SafeSearch with host patterns,
like "*.sex.*".
Currently "allow" patterns are not considered for the "try to block
mature sites" setting, so the URL is allowed by DEFAULT but blocked
by the SafeSearch check. By returning a MANUAL reason, instead, then it
will be honored.

This may introduce a conflict when different patterns match the same URL,
but have different defaults, say, "www.google.*" and "*.google.com" both
match "www.google.com". In this case, we prefer to honor a blacklist over
a whitelist.

parent settings.

Bug: 880909
Test: Built a Chrome APK and verified different combinations for
Change-Id: Ia8a2e8c9261c9ea0a539c32a2ca8fe5e3e576fae
Reviewed-on: https://chromium-review.googlesource.com/1207573
Commit-Queue: Bruno Kim Medeiros Cesar <brunokim@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590297}

chrome/browser/supervised_user/supervised_user_url_filter.cc

@@ -333,25 +333,11 @@ SupervisedUserURLFilter::GetFilteringBehaviorForURL(
   if (base::ContainsKey(*kWhitelistedOrigins, effective_url.GetOrigin()))
     return ALLOW;
 
-  // Check manual overrides for the exact URL.
-  auto url_it = url_map_.find(policy::url_util::Normalize(effective_url));
-  if (url_it != url_map_.end())
-    return url_it->second ? ALLOW : BLOCK;
-
-  // Check manual overrides for the hostname.
-  const std::string host = effective_url.host();
-  auto host_it = host_map_.find(host);
-  if (host_it != host_map_.end())
-    return host_it->second ? ALLOW : BLOCK;
-
-  // Look for patterns matching the hostname, with a value that is different
-  // from the default (a value of true in the map meaning allowed).
-  for (const auto& host_entry : host_map_) {
-    if ((host_entry.second == (default_behavior_ == BLOCK)) &&
-        HostMatchesPattern(host, host_entry.first)) {
-      return host_entry.second ? ALLOW : BLOCK;
-    }
-  }
+  // Check manual blacklists and whitelists.
+  FilteringBehavior manual_result =
+      GetManualFilteringBehaviorForURL(effective_url);
+  if (manual_result != INVALID)
+    return manual_result;
 
   // Check the list of URL patterns.
   std::set<URLMatcherConditionSet::ID> matching_ids =
@@ -363,7 +349,7 @@ SupervisedUserURLFilter::GetFilteringBehaviorForURL(
   }
 
   // Check the list of hostname hashes.
-  if (contents_->hostname_hashes.count(HostnameHash(host))) {
+  if (contents_->hostname_hashes.count(HostnameHash(effective_url.host()))) {
     *reason = supervised_user_error_page::WHITELIST;
     return ALLOW;
   }
@@ -380,6 +366,46 @@ SupervisedUserURLFilter::GetFilteringBehaviorForURL(
   return default_behavior_;
 }
 
+// There may be conflicting patterns, say, "allow *.google.com" and "block
+// www.google.*". To break the tie, we prefer blacklists over whitelists, by
+// returning early if there is a BLOCK and evaluating all manual overrides
+// before returning an ALLOW. If there are no applicable manual overrides,
+// return INVALID.
+SupervisedUserURLFilter::FilteringBehavior
+SupervisedUserURLFilter::GetManualFilteringBehaviorForURL(
+    const GURL& url) const {
+  FilteringBehavior result = INVALID;
+
+  // Check manual overrides for the exact URL.
+  auto url_it = url_map_.find(policy::url_util::Normalize(url));
+  if (url_it != url_map_.end()) {
+    if (!url_it->second)
+      return BLOCK;
+    result = ALLOW;
+  }
+
+  // Check manual overrides for the hostname.
+  const std::string host = url.host();
+  auto host_it = host_map_.find(host);
+  if (host_it != host_map_.end()) {
+    if (!host_it->second)
+      return BLOCK;
+    result = ALLOW;
+  }
+
+  // Look for patterns matching the hostname, with a value that is different
+  // from the default (a value of true in the map meaning allowed).
+  for (const auto& host_entry : host_map_) {
+    if (HostMatchesPattern(host, host_entry.first)) {
+      if (!host_entry.second)
+        return BLOCK;
+      result = ALLOW;
+    }
+  }
+
+  return result;
+}
+
 bool SupervisedUserURLFilter::GetFilteringBehaviorForURLWithAsyncChecks(
     const GURL& url,
     FilteringBehaviorCallback callback) const {

chrome/browser/supervised_user/supervised_user_url_filter.h

@@ -180,6 +180,7 @@ class SupervisedUserURLFilter {
       const GURL& url,
       bool manual_only,
       supervised_user_error_page::FilteringBehaviorReason* reason) const;
+  FilteringBehavior GetManualFilteringBehaviorForURL(const GURL& url) const;
 
   void CheckCallback(FilteringBehaviorCallback callback,
                      const GURL& url,

chrome/browser/supervised_user/supervised_user_url_filter_unittest.cc

@@ -29,6 +29,13 @@ class SupervisedUserURLFilterTest : public ::testing::Test,
 
   // SupervisedUserURLFilter::Observer:
   void OnSiteListUpdated() override { run_loop_.Quit(); }
+  void OnURLChecked(const GURL& url,
+                    SupervisedUserURLFilter::FilteringBehavior behavior,
+                    supervised_user_error_page::FilteringBehaviorReason reason,
+                    bool uncertain) override {
+    behavior_ = behavior;
+    reason_ = reason;
+  }
 
  protected:
   bool IsURLWhitelisted(const std::string& url) {
@@ -36,9 +43,45 @@ class SupervisedUserURLFilterTest : public ::testing::Test,
            SupervisedUserURLFilter::ALLOW;
   }
 
+  void ExpectURLInDefaultWhitelist(const std::string& url) {
+    ExpectURLCheckMatches(url, SupervisedUserURLFilter::ALLOW,
+                          supervised_user_error_page::DEFAULT);
+  }
+
+  void ExpectURLInDefaultBlacklist(const std::string& url) {
+    ExpectURLCheckMatches(url, SupervisedUserURLFilter::BLOCK,
+                          supervised_user_error_page::DEFAULT);
+  }
+
+  void ExpectURLInManualWhitelist(const std::string& url) {
+    ExpectURLCheckMatches(url, SupervisedUserURLFilter::ALLOW,
+                          supervised_user_error_page::MANUAL);
+  }
+
+  void ExpectURLInManualBlacklist(const std::string& url) {
+    ExpectURLCheckMatches(url, SupervisedUserURLFilter::BLOCK,
+                          supervised_user_error_page::MANUAL);
+  }
+
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   base::RunLoop run_loop_;
   SupervisedUserURLFilter filter_;
+  SupervisedUserURLFilter::FilteringBehavior behavior_;
+  supervised_user_error_page::FilteringBehaviorReason reason_;
+
+ private:
+  void ExpectURLCheckMatches(
+      const std::string& url,
+      SupervisedUserURLFilter::FilteringBehavior expected_behavior,
+      supervised_user_error_page::FilteringBehaviorReason expected_reason) {
+    bool called_synchronously =
+        filter_.GetFilteringBehaviorForURLWithAsyncChecks(GURL(url),
+                                                          base::DoNothing());
+    ASSERT_TRUE(called_synchronously);
+
+    EXPECT_EQ(behavior_, expected_behavior);
+    EXPECT_EQ(reason_, expected_reason);
+  }
 };
 
 TEST_F(SupervisedUserURLFilterTest, Basic) {
@@ -399,30 +442,85 @@ TEST_F(SupervisedUserURLFilterTest, HostMatchesPattern) {
                                                   "www.*.google.com"));
 }
 
-TEST_F(SupervisedUserURLFilterTest, Patterns) {
+TEST_F(SupervisedUserURLFilterTest, PatternsWithoutConflicts) {
   std::map<std::string, bool> hosts;
 
-  // Initally, the second rule is ignored because has the same value as the
-  // default (block). When we change the default to allow, the first rule is
-  // ignored instead.
+  // The third rule is redundant with the first, but it's not a conflict
+  // since they have the same value (allow).
   hosts["*.google.com"] = true;
-  hosts["www.google.*"] = false;
-
   hosts["accounts.google.com"] = false;
   hosts["mail.google.com"] = true;
+
   filter_.SetManualHosts(std::move(hosts));
+  filter_.SetDefaultFilteringBehavior(SupervisedUserURLFilter::BLOCK);
 
-  // Initially, the default filtering behavior is BLOCK.
   EXPECT_TRUE(IsURLWhitelisted("http://www.google.com/foo/"));
   EXPECT_FALSE(IsURLWhitelisted("http://accounts.google.com/bar/"));
-  EXPECT_FALSE(IsURLWhitelisted("http://www.google.co.uk/blurp/"));
   EXPECT_TRUE(IsURLWhitelisted("http://mail.google.com/moose/"));
+  EXPECT_FALSE(IsURLWhitelisted("http://www.google.co.uk/blurp/"));
 
   filter_.SetDefaultFilteringBehavior(SupervisedUserURLFilter::ALLOW);
+
+  EXPECT_TRUE(IsURLWhitelisted("http://www.google.com/foo/"));
+  EXPECT_FALSE(IsURLWhitelisted("http://accounts.google.com/bar/"));
+  EXPECT_TRUE(IsURLWhitelisted("http://mail.google.com/moose/"));
+  EXPECT_TRUE(IsURLWhitelisted("http://www.google.co.uk/blurp/"));
+}
+
+TEST_F(SupervisedUserURLFilterTest, PatternsWithConflicts) {
+  std::map<std::string, bool> hosts;
+
+  // The fourth rule conflicts with the first for "www.google.com" host.
+  // Blocking then takes precedence.
+  hosts["*.google.com"] = true;
+  hosts["accounts.google.com"] = false;
+  hosts["mail.google.com"] = true;
+  hosts["www.google.*"] = false;
+
+  filter_.SetManualHosts(std::move(hosts));
+  filter_.SetDefaultFilteringBehavior(SupervisedUserURLFilter::BLOCK);
+
   EXPECT_FALSE(IsURLWhitelisted("http://www.google.com/foo/"));
   EXPECT_FALSE(IsURLWhitelisted("http://accounts.google.com/bar/"));
+  EXPECT_TRUE(IsURLWhitelisted("http://mail.google.com/moose/"));
   EXPECT_FALSE(IsURLWhitelisted("http://www.google.co.uk/blurp/"));
+
+  filter_.SetDefaultFilteringBehavior(SupervisedUserURLFilter::ALLOW);
+
+  EXPECT_FALSE(IsURLWhitelisted("http://www.google.com/foo/"));
+  EXPECT_FALSE(IsURLWhitelisted("http://accounts.google.com/bar/"));
   EXPECT_TRUE(IsURLWhitelisted("http://mail.google.com/moose/"));
+  EXPECT_FALSE(IsURLWhitelisted("http://www.google.co.uk/blurp/"));
+}
+
+TEST_F(SupervisedUserURLFilterTest, Reason) {
+  std::map<std::string, bool> hosts;
+  std::map<GURL, bool> urls;
+  hosts["youtube.com"] = true;
+  hosts["*.google.*"] = true;
+  urls[GURL("https://youtube.com/robots.txt")] = false;
+  urls[GURL("https://google.co.uk/robots.txt")] = false;
+
+  filter_.SetManualHosts(std::move(hosts));
+  filter_.SetManualURLs(std::move(urls));
+
+  filter_.SetDefaultFilteringBehavior(SupervisedUserURLFilter::BLOCK);
+
+  ExpectURLInDefaultBlacklist("https://m.youtube.com/feed/trending");
+  ExpectURLInDefaultBlacklist("https://com.google");
+  ExpectURLInManualWhitelist("https://youtube.com/feed/trending");
+  ExpectURLInManualWhitelist("https://google.com/humans.txt");
+  ExpectURLInManualBlacklist("https://youtube.com/robots.txt");
+  ExpectURLInManualBlacklist("https://google.co.uk/robots.txt");
+
+  filter_.SetDefaultFilteringBehavior(SupervisedUserURLFilter::ALLOW);
+
+  ExpectURLInDefaultWhitelist("https://m.youtube.com/feed/trending");
+  ExpectURLInDefaultWhitelist("https://com.google");
+  ExpectURLInManualWhitelist("https://youtube.com/feed/trending");
+  ExpectURLInManualWhitelist("https://google.com/humans.txt");
+  ExpectURLInManualBlacklist("https://youtube.com/robots.txt");
+  ExpectURLInManualBlacklist("https://google.co.uk/robots.txt");
 }
 
 TEST_F(SupervisedUserURLFilterTest, WhitelistsPatterns) {