DNR: Move ruleset matcher unittests to the extensions layer.

RulesetMatcher is a class in the extensions layer but it is unit-tested at the
chrome layer. Fix this by moving the tests to the extensions layer by removing
dependencies to chrome's extension loading code. This also means we don't
check the extension loading in these tests, but that is taken care of in the
browser tests.

BUG=930961

Change-Id: Ic971bce981a65cda8928a8e3d9e3b49f960b4aef
Reviewed-on: https://chromium-review.googlesource.com/c/1496314Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636663}

chrome/test/BUILD.gn

@@ -3775,7 +3775,6 @@ test("unit_tests") {
       "../browser/extensions/api/declarative_net_request/dnr_test_base.h",
       "../browser/extensions/api/declarative_net_request/rule_indexing_unittest.cc",
       "../browser/extensions/api/declarative_net_request/ruleset_manager_unittest.cc",
-      "../browser/extensions/api/declarative_net_request/ruleset_matcher_unittest.cc",
       "../browser/extensions/api/declarative_webrequest/webrequest_action_unittest.cc",
       "../browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc",
       "../browser/extensions/api/developer_private/developer_private_api_unittest.cc",

extensions/browser/BUILD.gn

@@ -560,6 +560,7 @@ source_set("unit_tests") {
     "api/declarative_net_request/flat_ruleset_indexer_unittest.cc",
     "api/declarative_net_request/indexed_rule_unittest.cc",
     "api/declarative_net_request/indexed_ruleset_format_version_unittest.cc",
+    "api/declarative_net_request/ruleset_matcher_unittest.cc",
     "api/declarative_webrequest/webrequest_condition_attribute_unittest.cc",
     "api/declarative_webrequest/webrequest_condition_unittest.cc",
     "api/document_scan/document_scan_api_unittest.cc",
@@ -667,6 +668,7 @@ source_set("unit_tests") {
     "//extensions:test_support",
     "//extensions/buildflags",
     "//extensions/common",
+    "//extensions/common:test_support",
     "//extensions/common/api",
     "//extensions/strings",
     "//ipc:test_support",

chrome/browser/extensions/api/declarative_net_request/ruleset_matcher_unittest.cc → extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc

@@ -4,22 +4,21 @@
 
 #include "extensions/browser/api/declarative_net_request/ruleset_matcher.h"
 
+#include <utility>
 #include <vector>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
-#include "chrome/browser/extensions/api/declarative_net_request/dnr_test_base.h"
-#include "chrome/browser/extensions/chrome_test_extension_loader.h"
-#include "chrome/browser/profiles/profile.h"
 #include "components/url_pattern_index/flat/url_pattern_index_generated.h"
-#include "extensions/browser/api/declarative_net_request/test_utils.h"
+#include "components/version_info/version_info.h"
+#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 #include "extensions/browser/api/declarative_net_request/utils.h"
-#include "extensions/browser/extension_prefs.h"
 #include "extensions/common/api/declarative_net_request/constants.h"
 #include "extensions/common/api/declarative_net_request/test_utils.h"
-#include "extensions/common/file_util.h"
-#include "extensions/common/url_pattern.h"
+#include "extensions/common/features/feature_channel.h"
+#include "extensions/common/value_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 #include "url/origin.h"
@@ -28,53 +27,61 @@ namespace extensions {
 namespace declarative_net_request {
 namespace {
 
-constexpr char kJSONRulesFilename[] = "rules_file.json";
-const base::FilePath::CharType kJSONRulesetFilepath[] =
-    FILE_PATH_LITERAL("rules_file.json");
-
-class RulesetMatcherTest : public DNRTestBase {
+class RulesetMatcherTest : public ::testing::Test {
  public:
-  RulesetMatcherTest() {}
+  RulesetMatcherTest() : channel_(::version_info::Channel::UNKNOWN) {}
 
  protected:
-  const Extension* extension() const { return extension_.get(); }
-
-  void LoadExtensionWithRules(const std::vector<TestRule>& rules) {
-    base::FilePath extension_dir =
-        temp_dir().GetPath().Append(FILE_PATH_LITERAL("test_extension"));
-
-    // Create extension directory.
-    ASSERT_TRUE(base::CreateDirectory(extension_dir));
-    WriteManifestAndRuleset(extension_dir, kJSONRulesetFilepath,
-                            kJSONRulesFilename, rules, {} /* hosts */);
+  // Helper to create a verified ruleset matcher. Populates |matcher| and
+  // |expected_checksum|.
+  void CreateVerifiedMatcher(const std::vector<TestRule>& rules,
+                             const RulesetSource& source,
+                             std::unique_ptr<RulesetMatcher>* matcher,
+                             int* expected_checksum = nullptr) {
+    // Serialize |rules|.
+    ListBuilder builder;
+    for (const auto& rule : rules)
+      builder.Append(rule.ToValue());
+    JSONFileValueSerializer(source.json_path).Serialize(*builder.Build());
+
+    // Index ruleset.
+    IndexAndPersistRulesResult result = IndexAndPersistRulesUnsafe(source);
+    ASSERT_TRUE(result.success);
+    ASSERT_TRUE(result.error.empty());
+
+    if (expected_checksum)
+      *expected_checksum = result.ruleset_checksum;
+
+    // Create verified matcher.
+    RulesetMatcher::LoadRulesetResult load_result =
+        RulesetMatcher::CreateVerifiedMatcher(source.indexed_path,
+                                              result.ruleset_checksum, matcher);
+    ASSERT_EQ(RulesetMatcher::kLoadSuccess, load_result);
+  }
 
-    extension_ = CreateExtensionLoader()->LoadExtension(extension_dir);
-    ASSERT_TRUE(extension_);
+  RulesetSource CreateTemporarySource() {
+    base::FilePath json_path;
+    base::FilePath indexed_path;
+    CHECK(base::CreateTemporaryFile(&json_path));
+    CHECK(base::CreateTemporaryFile(&indexed_path));
+    return RulesetSource(std::move(json_path), std::move(indexed_path));
   }
 
  private:
-  scoped_refptr<const Extension> extension_;
+  // Run this on the trunk channel to ensure the API is available.
+  ScopedCurrentChannel channel_;
 
   DISALLOW_COPY_AND_ASSIGN(RulesetMatcherTest);
 };
 
 // Tests a simple blocking rule.
-TEST_P(RulesetMatcherTest, ShouldBlockRequest) {
+TEST_F(RulesetMatcherTest, ShouldBlockRequest) {
   TestRule rule = CreateGenericRule();
   rule.condition->url_filter = std::string("google.com");
 
-  ASSERT_NO_FATAL_FAILURE(LoadExtensionWithRules({rule}));
-
-  int expected_checksum;
-  ASSERT_TRUE(
-      ExtensionPrefs::Get(browser_context())
-          ->GetDNRRulesetChecksum(extension()->id(), &expected_checksum));
-
   std::unique_ptr<RulesetMatcher> matcher;
-  EXPECT_EQ(RulesetMatcher::kLoadSuccess,
-            RulesetMatcher::CreateVerifiedMatcher(
-                file_util::GetIndexedRulesetPath(extension()->path()),
-                expected_checksum, &matcher));
+  ASSERT_NO_FATAL_FAILURE(
+      CreateVerifiedMatcher({rule}, CreateTemporarySource(), &matcher));
 
   EXPECT_TRUE(matcher->ShouldBlockRequest(
       GURL("http://google.com"), url::Origin(),
@@ -85,25 +92,16 @@ TEST_P(RulesetMatcherTest, ShouldBlockRequest) {
 }
 
 // Tests a simple redirect rule.
-TEST_P(RulesetMatcherTest, ShouldRedirectRequest) {
+TEST_F(RulesetMatcherTest, ShouldRedirectRequest) {
   TestRule rule = CreateGenericRule();
   rule.condition->url_filter = std::string("google.com");
   rule.priority = kMinValidPriority;
   rule.action->type = std::string("redirect");
   rule.action->redirect_url = std::string("http://yahoo.com");
 
-  ASSERT_NO_FATAL_FAILURE(LoadExtensionWithRules({rule}));
-
-  int expected_checksum;
-  ASSERT_TRUE(
-      ExtensionPrefs::Get(browser_context())
-          ->GetDNRRulesetChecksum(extension()->id(), &expected_checksum));
-
   std::unique_ptr<RulesetMatcher> matcher;
-  EXPECT_EQ(RulesetMatcher::kLoadSuccess,
-            RulesetMatcher::CreateVerifiedMatcher(
-                file_util::GetIndexedRulesetPath(extension()->path()),
-                expected_checksum, &matcher));
+  ASSERT_NO_FATAL_FAILURE(
+      CreateVerifiedMatcher({rule}, CreateTemporarySource(), &matcher));
 
   GURL redirect_url;
   EXPECT_TRUE(matcher->ShouldRedirectRequest(
@@ -117,26 +115,20 @@ TEST_P(RulesetMatcherTest, ShouldRedirectRequest) {
 }
 
 // Tests that a modified ruleset file fails verification.
-TEST_P(RulesetMatcherTest, FailedVerification) {
-  ASSERT_NO_FATAL_FAILURE(LoadExtensionWithRules({CreateGenericRule()}));
-
-  base::FilePath indexed_ruleset_path =
-      file_util::GetIndexedRulesetPath(extension()->path());
+TEST_F(RulesetMatcherTest, FailedVerification) {
+  RulesetSource source = CreateTemporarySource();
+  std::unique_ptr<RulesetMatcher> matcher;
+  int expected_checksum;
+  ASSERT_NO_FATAL_FAILURE(
+      CreateVerifiedMatcher({}, source, &matcher, &expected_checksum));
 
   // Persist invalid data to the ruleset file and ensure that a version mismatch
   // occurs.
   std::string data = "invalid data";
   ASSERT_EQ(static_cast<int>(data.size()),
-            base::WriteFile(indexed_ruleset_path, data.c_str(), data.size()));
-
-  int expected_checksum;
-  ASSERT_TRUE(
-      ExtensionPrefs::Get(browser_context())
-          ->GetDNRRulesetChecksum(extension()->id(), &expected_checksum));
-
-  std::unique_ptr<RulesetMatcher> matcher;
+            base::WriteFile(source.indexed_path, data.c_str(), data.size()));
   EXPECT_EQ(RulesetMatcher::kLoadErrorVersionMismatch,
-            RulesetMatcher::CreateVerifiedMatcher(indexed_ruleset_path,
+            RulesetMatcher::CreateVerifiedMatcher(source.indexed_path,
                                                   expected_checksum, &matcher));
 
   // Now, persist invalid data to the ruleset file, while maintaining the
@@ -144,17 +136,12 @@ TEST_P(RulesetMatcherTest, FailedVerification) {
   // mismatch.
   data = GetVersionHeaderForTesting() + "invalid data";
   ASSERT_EQ(static_cast<int>(data.size()),
-            base::WriteFile(indexed_ruleset_path, data.c_str(), data.size()));
+            base::WriteFile(source.indexed_path, data.c_str(), data.size()));
   EXPECT_EQ(RulesetMatcher::kLoadErrorChecksumMismatch,
-            RulesetMatcher::CreateVerifiedMatcher(indexed_ruleset_path,
+            RulesetMatcher::CreateVerifiedMatcher(source.indexed_path,
                                                   expected_checksum, &matcher));
 }
 
-INSTANTIATE_TEST_SUITE_P(,
-                         RulesetMatcherTest,
-                         ::testing::Values(ExtensionLoadType::PACKED,
-                                           ExtensionLoadType::UNPACKED));
-
 }  // namespace
 }  // namespace declarative_net_request
 }  // namespace extensions

extensions/browser/api/declarative_net_request/ruleset_source.cc

@@ -4,6 +4,8 @@
 
 #include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 
+#include <utility>
+
 #include "extensions/common/api/declarative_net_request/dnr_manifest_data.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_resource.h"
@@ -14,25 +16,22 @@ namespace declarative_net_request {
 
 // static
 RulesetSource RulesetSource::Create(const Extension& extension) {
-  RulesetSource source;
-  source.json_path =
-      declarative_net_request::DNRManifestData::GetRulesetPath(extension);
-  source.indexed_path = file_util::GetIndexedRulesetPath(extension.path());
-  return source;
+  return RulesetSource(
+      declarative_net_request::DNRManifestData::GetRulesetPath(extension),
+      file_util::GetIndexedRulesetPath(extension.path()));
 }
 
+RulesetSource::RulesetSource(base::FilePath json_path,
+                             base::FilePath indexed_path)
+    : json_path(std::move(json_path)), indexed_path(std::move(indexed_path)) {}
+
 RulesetSource::~RulesetSource() = default;
 RulesetSource::RulesetSource(RulesetSource&&) = default;
 RulesetSource& RulesetSource::operator=(RulesetSource&&) = default;
 
 RulesetSource RulesetSource::Clone() const {
-  RulesetSource clone;
-  clone.json_path = json_path;
-  clone.indexed_path = indexed_path;
-  return clone;
+  return RulesetSource(json_path, indexed_path);
 }
 
-RulesetSource::RulesetSource() = default;
-
 }  // namespace declarative_net_request
 }  // namespace extensions

extensions/browser/api/declarative_net_request/ruleset_source.h

@@ -18,6 +18,8 @@ struct RulesetSource {
   // ruleset.
   static RulesetSource Create(const Extension& extension);
 
+  RulesetSource(base::FilePath json_path, base::FilePath indexed_path);
+
   ~RulesetSource();
   RulesetSource(RulesetSource&&);
   RulesetSource& operator=(RulesetSource&&);
@@ -31,8 +33,6 @@ struct RulesetSource {
   base::FilePath indexed_path;
 
  private:
-  RulesetSource();
-
   DISALLOW_COPY_AND_ASSIGN(RulesetSource);
 };