Navigation throttle for the Safe Browsing Subresource Filter.

components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc

Index: components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc
diff --git a/components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc b/components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8c6040e74e15c7cf14fbfe0e3da867a8fef53066
--- /dev/null
+++ b/components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc
@@ -0,0 +1,202 @@
+// Copyright 2016 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.
+
+#include "components/subresource_filter/content/browser/subresource_filter_navigation_throttle.h"
+
+#include <memory>
+
+#include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
+#include "components/safe_browsing_db/util.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_driver.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
+#include "components/subresource_filter/core/browser/subresource_filter_features.h"
+#include "components/subresource_filter/core/browser/subresource_filter_features_test_support.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/test_renderer_host.h"
+#include "testing/gmock/include/gmock/gmock.h"

[engedy, 2016/06/23 22:34:18]

nit: Not used.

[melandory, 2016/06/25 01:46:09]

Done.

+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::NavigationThrottle;
+
+namespace subresource_filter {
+
+class TestContentSubresourceFilterDriver
+    : public ContentSubresourceFilterDriver {
+ public:
+  TestContentSubresourceFilterDriver(
+      content::RenderFrameHost* render_frame_host)
+      : ContentSubresourceFilterDriver(render_frame_host) {
+    activation_state_ = ActivationState::DISABLED;
+  }
+  ~TestContentSubresourceFilterDriver() override {}
+
+  void ActivateForProvisionalLoad(
+      ActivationState new_activation_state) override {
+    activation_state_ = new_activation_state;
+  }
+
+  ActivationState activation_state() { return activation_state_; }
+
+ private:
+  ActivationState activation_state_;
+  DISALLOW_COPY_AND_ASSIGN(TestContentSubresourceFilterDriver);
+};
+
+class SubresourceFilterNavigationThrottleTest
+    : public content::RenderViewHostTestHarness {
+ public:
+  SubresourceFilterNavigationThrottleTest() {}
+
+  // content::RenderViewHostTestHarness:
+  void SetUp() override {
+    RenderViewHostTestHarness::SetUp();
+    ContentSubresourceFilterDriverFactory::CreateForWebContents(web_contents());
+
+    driver_ = new TestContentSubresourceFilterDriver(main_rfh());
+    factory()->TestingSetDriverForFrame(main_rfh(), base::WrapUnique(driver_));
+  }
+
+  void TearDown() override {
+    handle_.reset();
+    RenderViewHostTestHarness::TearDown();
+  }
+
+  void SetUpNavigationHandleForURL(const GURL& url) {
+    handle_ = content::NavigationHandle::CreateNavigationHandleForTesting(
+        url, main_rfh());
+    handle_->RegisterThrottleForTesting(
+        SubresourceFilterNavigationThrottle::Create(handle_.get()));
+  }
+
+  content::NavigationHandle* handle() { return handle_.get(); }
+
+  ContentSubresourceFilterDriverFactory* factory() {
+    return ContentSubresourceFilterDriverFactory::FromWebContents(
+        web_contents());
+  }
+
+  TestContentSubresourceFilterDriver* driver() { return driver_; }
+
+  NavigationThrottle::ThrottleCheckResult SimulateWillStart() {
+    return handle()->CallWillStartRequestForTesting(

[engedy, 2016/06/23 22:34:18]

nit: Could you please add /* param_name */ after each boolean argument so it's
clear what all the trues and falses do here?

[melandory, 2016/06/25 01:46:09]

Done.

+        false, content::Referrer(), false, ui::PAGE_TRANSITION_LINK, false);
+  }
+
+  NavigationThrottle::ThrottleCheckResult SimulateRedirects(
+      const GURL& redirect) {
+    handle()->CallWillStartRequestForTesting(true, content::Referrer(), false,

[engedy, 2016/06/23 22:34:18]

I find it strange that we need to call this here. Is this really needed?

[melandory, 2016/06/25 01:46:09]

Done.

+                                             ui::PAGE_TRANSITION_LINK, false);
+    return handle()->CallWillRedirectRequestForTesting(redirect, false, GURL(),
+                                                       false);
+  }
+
+  void SimulateWillProcessResponse() {
+    handle()->CallWillProcessResponseForTesting(main_rfh());
+  }
+
+ private:
+  TestContentSubresourceFilterDriver* driver_;
+  std::unique_ptr<content::NavigationHandle> handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(SubresourceFilterNavigationThrottleTest);
+};
+
+TEST_F(SubresourceFilterNavigationThrottleTest, RequestWithoutRedirects) {
+  base::FieldTrialList field_trial_list(nullptr);

[engedy, 2016/06/23 22:34:18]

nit: Consider moving the base::FieldTrialList instance into the test fixture to
avoid duplication.

+  testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
+      base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationStateEnabled);
+
+  const GURL url("https://example.com");

[engedy, 2016/06/23 22:34:18]

nit: Given that this literal appears multiple times within a given test, it's
running the risk of typos across different occurrences potentially masking test
failures. I'd suggest defining a constant for this literal in an unnamed
namespace at the top of the file, so that the compiler can check typos.

[melandory, 2016/06/25 01:46:09]

Done.

+  SetUpNavigationHandleForURL(url);
+  factory()->OnMainResourceMatchedSafeBrowsingBlacklist(
+      url, std::vector<GURL>(),
+      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS);
+
+  EXPECT_EQ(ActivationState::DISABLED, driver()->activation_state());
+  SimulateWillStart();
+  SimulateWillProcessResponse();
+
+  EXPECT_EQ(1U, factory()->activation_set().size());
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example.com")));
+  EXPECT_EQ(GetMaximumActivationState(), driver()->activation_state());
+}
+
+TEST_F(SubresourceFilterNavigationThrottleTest,
+       RequestWithoutRedirectsNoActivation) {
+  const GURL url("https://example.com");
+
+  factory()->OnMainResourceMatchedSafeBrowsingBlacklist(
+      url, std::vector<GURL>(),
+      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS);
+
+  EXPECT_EQ(ActivationState::DISABLED, driver()->activation_state());
+  SetUpNavigationHandleForURL(GURL("https://test.com"));
+  SimulateWillStart();
+  SimulateWillProcessResponse();
+
+  EXPECT_EQ(1U, factory()->activation_set().size());
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example.com")));
+  EXPECT_EQ(ActivationState::DISABLED, driver()->activation_state());
+}
+
+TEST_F(SubresourceFilterNavigationThrottleTest,
+       AddRedirectFromNavThrottleToServiceEnptyInitRedirects) {
+  base::FieldTrialList field_trial_list(nullptr);
+  testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
+      base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationStateEnabled);
+
+  const GURL url("https://example.com");
+  const GURL redirect("https://example1.com");
+
+  SetUpNavigationHandleForURL(url);
+  factory()->OnMainResourceMatchedSafeBrowsingBlacklist(
+      url, std::vector<GURL>(),
+      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS);
+
+  EXPECT_EQ(ActivationState::DISABLED, driver()->activation_state());
+  SimulateRedirects(redirect);
+  SimulateWillProcessResponse();
+
+  EXPECT_EQ(2U, factory()->activation_set().size());
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example.com")));
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example1.com")));
+  EXPECT_EQ(GetMaximumActivationState(), driver()->activation_state());
+}
+
+TEST_F(SubresourceFilterNavigationThrottleTest,
+       AddRedirectFromNavThrottleToServiceNonEmptyInitRedirects) {
+  base::FieldTrialList field_trial_list(nullptr);
+  testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
+      base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationStateEnabled);
+
+  const GURL url("https://example.com");
+  const GURL nav_throttle_redirect("https://example4.com");
+  SetUpNavigationHandleForURL(url);
+
+  std::vector<GURL> redirects;
+
+  redirects.push_back(GURL("https://example1.com"));

[engedy, 2016/06/23 22:34:18]

nit: Same goes for this. Please either define const named GURL instances at the
top of this test, or const char[]s at the top of this file.

[melandory, 2016/06/25 01:46:10]

Done.

+  redirects.push_back(GURL("https://example2.com"));
+  redirects.push_back(GURL("https://example3.com"));
+  factory()->OnMainResourceMatchedSafeBrowsingBlacklist(
+      GURL("https://example.com"), redirects,
+      safe_browsing::ThreatPatternType::SOCIAL_ENGINEERING_ADS);
+
+  EXPECT_EQ(ActivationState::DISABLED, driver()->activation_state());
+  SimulateRedirects(nav_throttle_redirect);
+  SimulateWillProcessResponse();
+
+  EXPECT_EQ(5U, factory()->activation_set().size());
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example.com")));
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example1.com")));
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example2.com")));
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example3.com")));
+  EXPECT_TRUE(factory()->ShouldActivateForURL(GURL("https://example4.com")));
+  EXPECT_EQ(GetMaximumActivationState(), driver()->activation_state());
+}
+
+}  // namespace navigation_interception

[engedy, 2016/06/23 22:34:18]

Could you please add two more tests that have a subsequent navigation? One where
the first URL matches the activation_set(), and a second with A, B, C, with B
matching the activation_set()?

[melandory, 2016/06/25 01:46:09]

Isn't RequestWithoutRedirectsNoActivation is what you described?
Done.