[subresource_filter] Implement the "Smart" UI on Android

chrome/browser/subresource_filter/subresource_filter_browsertest.cc

Index: chrome/browser/subresource_filter/subresource_filter_browsertest.cc
diff --git a/chrome/browser/subresource_filter/subresource_filter_browsertest.cc b/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
index 9aa2ddb77d7944964cc57c1930c8df379610699c..57e87a1a93b1b669a420a8a21c571b382638d29b 100644
--- a/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
+++ b/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
@@ -18,6 +18,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/simple_test_clock.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/metrics/subprocess_metrics_provider.h"
@@ -25,6 +26,8 @@
 #include "chrome/browser/safe_browsing/test_safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/v4_test_utils.h"
 #include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
+#include "chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h"
+#include "chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.h"
 #include "chrome/browser/subresource_filter/test_ruleset_publisher.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
@@ -273,6 +276,13 @@ class SubresourceFilterBrowserTestImpl : public InProcessBrowserTest {
     host_resolver()->AddRule("*", "127.0.0.1");
     content::SetupCrossSiteRedirector(embedded_test_server());
     ASSERT_TRUE(embedded_test_server()->Start());
+
+    settings_manager_ =
+        SubresourceFilterContentSettingsManagerFactory::EnsureForProfile(
+            browser()->profile());
+#if defined(OS_ANDROID)
+    EXPECT_TRUE(settings_manager->should_use_smart_ui());
+#endif
   }
 
   virtual void SetUpActivationFeature() {
@@ -311,6 +321,10 @@ class SubresourceFilterBrowserTestImpl : public InProcessBrowserTest {
     return browser()->tab_strip_model()->GetActiveWebContents();
   }
 
+  SubresourceFilterContentSettingsManager* settings_manager() {
+    return settings_manager_;
+  }
+
   content::RenderFrameHost* FindFrameByName(const std::string& name) {
     for (content::RenderFrameHost* frame : web_contents()->GetAllFrames()) {
       if (frame->GetFrameName() == name)
@@ -400,6 +414,9 @@ class SubresourceFilterBrowserTestImpl : public InProcessBrowserTest {
   // Owned by the V4GetHashProtocolManager.
   safe_browsing::TestV4GetHashProtocolManagerFactory* v4_get_hash_factory_;
 
+  // Owned by the profile.
+  SubresourceFilterContentSettingsManager* settings_manager_;
+
   DISALLOW_COPY_AND_ASSIGN(SubresourceFilterBrowserTestImpl);
 };
 
@@ -841,6 +858,8 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   ASSERT_NO_FATAL_FAILURE(
       SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
   GURL url(GetTestUrl(kTestFrameSetPath));
+  GURL a_url(embedded_test_server()->GetURL(
+      "a.com", "/subresource_filter/frame_with_included_script.html"));
   ConfigureAsPhishingURL(url);
   base::HistogramTester tester;
   ui_test_utils::NavigateToURL(browser(), url);
@@ -850,8 +869,10 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   EXPECT_FALSE(IsDynamicScriptElementLoaded(FindFrameByName("five")));
   tester.ExpectBucketCount(kSubresourceFilterActionsHistogram, kActionUIShown,
                            1);
-  // Check that bubble is shown for new navigation.
-  ui_test_utils::NavigateToURL(browser(), url);
+  // Check that bubble is shown for new navigation. Must be cross site to avoid
+  // triggering smart UI on Android.
+  ConfigureAsPhishingURL(a_url);
+  ui_test_utils::NavigateToURL(browser(), a_url);
   tester.ExpectBucketCount(kSubresourceFilterActionsHistogram, kActionUIShown,
                            2);
 }
@@ -982,6 +1003,110 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
   EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
 }
 
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+                       SiteDeactivated_ContentSettingCleared) {
+  HostContentSettingsMap* settings_map =
+      HostContentSettingsMapFactory::GetForProfile(browser()->profile());
+  ContentSettingsForOneType host_settings;
+  settings_map->GetSettingsForOneType(
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::string(), &host_settings);
+
+  // Originally should just have a single default rule: * -> Allowed.
+  EXPECT_EQ(1u, host_settings.size());
+
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
+  GURL url(GetTestUrl("subresource_filter/frame_with_included_script.html"));
+  ConfigureAsPhishingURL(url);
+
+  EXPECT_EQ(CONTENT_SETTING_ALLOW, settings_manager()->GetContentSetting(url));
+  ui_test_utils::NavigateToURL(browser(), url);
+  EXPECT_EQ(CONTENT_SETTING_ALLOW, settings_manager()->GetContentSetting(url));
+
+  // Should have two rules now, one explicitly setting the origin of |url| to

[msramek, 2017/04/13 16:48:32]

This looks really wrong to me. I have one setting, I navigate somewhere, now I
have two settings.

There are two issues:

1. Privacy. There is no precedent to content setting exceptions being added
automatically. It's always manually through the settings UI or through accepting
a permission prompt. This is an important guarantee to keep, because content
settings reveal your browsing behavior yet are not deleted together with
browsing history.

2. As was explained to me, you're explicitly setting the exception to the same
value as default in order to force it to appear in the page info dialog. That's
a fragile dependency on an implementation detail of that dialog.

Basically, the permissions team decided back in the day that showing all
settings in page info is too much, and that a reasonable design would be to show
only non-default settings. If their design doesn't fit your usecase, please tell
them! Instead of working around it.

And in short-term, please try to special-case your content setting in page info.
I don't own that code, but I vaguely remember that it does a bunch of
special-casing already, so I'm sure that's allowed.

+  // Allow.
+  settings_map->GetSettingsForOneType(
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::string(), &host_settings);
+  EXPECT_EQ(2u, host_settings.size());
+
+  // Configure a different URL as the phishing URL, which resets |url|.
+  ConfigureAsPhishingURL(GURL("https://example.other/"));
+
+  // Re-navigating to |url| should reset the content setting.
+  ui_test_utils::NavigateToURL(browser(), url);
+  settings_map->GetSettingsForOneType(
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::string(), &host_settings);
+  EXPECT_EQ(1u, host_settings.size());
+  EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
+}
+
+// Test the "smart" UI, aka the logic to hide the UI on subsequent same-domain
+// navigations, until a certain time threshold has been reached. This is an
+// android-only feature.
+IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
+                       DoNotShowUIUntilThresholdReached) {
+  ASSERT_NO_FATAL_FAILURE(
+      SetRulesetToDisallowURLsWithPathSuffix("included_script.js"));
+  GURL a_url(embedded_test_server()->GetURL(
+      "a.com", "/subresource_filter/frame_with_included_script.html"));
+  GURL b_url(embedded_test_server()->GetURL(
+      "b.com", "/subresource_filter/frame_with_included_script.html"));
+  // Test utils only support one blacklisted site at a time.
+  // TODO(csharrison): Add support for more than one URL.
+  ConfigureAsPhishingURL(a_url);
+
+  // Cast is safe because this is the only type of client in non-unittest code.
+  ChromeSubresourceFilterClient* client =
+      static_cast<ChromeSubresourceFilterClient*>(
+          ContentSubresourceFilterDriverFactory::FromWebContents(web_contents())
+              ->client());
+  auto test_clock = base::MakeUnique<base::SimpleTestClock>();
+  base::SimpleTestClock* raw_clock = test_clock.get();
+  settings_manager()->set_clock_for_testing(std::move(test_clock));
+
+  base::HistogramTester histogram_tester;
+
+  // First load should trigger the UI.
+  ui_test_utils::NavigateToURL(browser(), a_url);
+  EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
+  EXPECT_TRUE(client->shown_for_navigation());
+
+  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
+                                     kActionUISuppressed, 0);
+
+  // Second load should not trigger the UI, but should still filter content.
+  ui_test_utils::NavigateToURL(browser(), a_url);
+  EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
+
+  bool use_smart_ui = settings_manager()->should_use_smart_ui();
+  EXPECT_EQ(client->shown_for_navigation(), !use_smart_ui);
+
+  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
+                                     kActionUISuppressed, use_smart_ui ? 1 : 0);
+
+  ConfigureAsPhishingURL(b_url);
+
+  // Load to another domain should trigger the UI.
+  ui_test_utils::NavigateToURL(browser(), b_url);
+  EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
+  EXPECT_TRUE(client->shown_for_navigation());
+
+  ConfigureAsPhishingURL(a_url);
+
+  // Fast forward the clock, and a_url should trigger the UI again.
+  raw_clock->Advance(
+      SubresourceFilterContentSettingsManager::kDelayBeforeShowingInfobarAgain);
+  ui_test_utils::NavigateToURL(browser(), a_url);
+  EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));
+  EXPECT_TRUE(client->shown_for_navigation());
+
+  histogram_tester.ExpectBucketCount(kSubresourceFilterActionsHistogram,
+                                     kActionUISuppressed, use_smart_ui ? 1 : 0);
+}
+
 IN_PROC_BROWSER_TEST_P(SubresourceFilterWebSocketBrowserTest, BlockWebSocket) {
   GURL url(GetTestUrl(
       base::StringPrintf("subresource_filter/page_with_websocket.html?%s",