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/1496314
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636663}diff --git a/chrome/browser/extensions/api/declarative_net_request/ruleset_matcher_unittest.cc b/chrome/browser/extensions/api/declarative_net_request/ruleset_matcher_unittest.cc
deleted file mode 100644
index f3cdae8..0000000
--- a/chrome/browser/extensions/api/declarative_net_request/ruleset_matcher_unittest.cc
+++ /dev/null
@@ -1,160 +0,0 @@
-// Copyright 2017 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 "extensions/browser/api/declarative_net_request/ruleset_matcher.h"
-
-#include <vector>
-
-#include "base/files/file_path.h"
-#include "base/files/file_util.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 "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 "testing/gtest/include/gtest/gtest.h"
-#include "url/gurl.h"
-#include "url/origin.h"
-
-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 {
- public:
- RulesetMatcherTest() {}
-
- 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 */);
-
- extension_ = CreateExtensionLoader()->LoadExtension(extension_dir);
- ASSERT_TRUE(extension_);
- }
-
- private:
- scoped_refptr<const Extension> extension_;
-
- DISALLOW_COPY_AND_ASSIGN(RulesetMatcherTest);
-};
-
-// Tests a simple blocking rule.
-TEST_P(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));
-
- EXPECT_TRUE(matcher->ShouldBlockRequest(
- GURL("http://google.com"), url::Origin(),
- url_pattern_index::flat::ElementType_SUBDOCUMENT, true));
- EXPECT_FALSE(matcher->ShouldBlockRequest(
- GURL("http://yahoo.com"), url::Origin(),
- url_pattern_index::flat::ElementType_SUBDOCUMENT, true));
-}
-
-// Tests a simple redirect rule.
-TEST_P(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));
-
- GURL redirect_url;
- EXPECT_TRUE(matcher->ShouldRedirectRequest(
- GURL("http://google.com"), url::Origin(),
- url_pattern_index::flat::ElementType_SUBDOCUMENT, true, &redirect_url));
- EXPECT_EQ(GURL("http://yahoo.com"), redirect_url);
-
- EXPECT_FALSE(matcher->ShouldRedirectRequest(
- GURL("http://yahoo.com"), url::Origin(),
- url_pattern_index::flat::ElementType_SUBDOCUMENT, true, &redirect_url));
-}
-
-// 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());
-
- // 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;
- EXPECT_EQ(RulesetMatcher::kLoadErrorVersionMismatch,
- RulesetMatcher::CreateVerifiedMatcher(indexed_ruleset_path,
- expected_checksum, &matcher));
-
- // Now, persist invalid data to the ruleset file, while maintaining the
- // correct version header. Ensure that it fails verification due to checksum
- // mismatch.
- data = GetVersionHeaderForTesting() + "invalid data";
- ASSERT_EQ(static_cast<int>(data.size()),
- base::WriteFile(indexed_ruleset_path, data.c_str(), data.size()));
- EXPECT_EQ(RulesetMatcher::kLoadErrorChecksumMismatch,
- RulesetMatcher::CreateVerifiedMatcher(indexed_ruleset_path,
- expected_checksum, &matcher));
-}
-
-INSTANTIATE_TEST_SUITE_P(,
- RulesetMatcherTest,
- ::testing::Values(ExtensionLoadType::PACKED,
- ExtensionLoadType::UNPACKED));
-
-} // namespace
-} // namespace declarative_net_request
-} // namespace extensions
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index 8b6c59ba..a5adc4c 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -3775,7 +3775,6 @@
"../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",
diff --git a/extensions/browser/BUILD.gn b/extensions/browser/BUILD.gn
index 05fc051..fbd4986 100644
--- a/extensions/browser/BUILD.gn
+++ b/extensions/browser/BUILD.gn
@@ -560,6 +560,7 @@
"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 @@
"//extensions:test_support",
"//extensions/buildflags",
"//extensions/common",
+ "//extensions/common:test_support",
"//extensions/common/api",
"//extensions/strings",
"//ipc:test_support",
diff --git a/extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc b/extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc
new file mode 100644
index 0000000..13b38f17
--- /dev/null
+++ b/extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc
@@ -0,0 +1,147 @@
+// Copyright 2017 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 "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 "components/url_pattern_index/flat/url_pattern_index_generated.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/common/api/declarative_net_request/constants.h"
+#include "extensions/common/api/declarative_net_request/test_utils.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"
+
+namespace extensions {
+namespace declarative_net_request {
+namespace {
+
+class RulesetMatcherTest : public ::testing::Test {
+ public:
+ RulesetMatcherTest() : channel_(::version_info::Channel::UNKNOWN) {}
+
+ protected:
+ // 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);
+ }
+
+ 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:
+ // 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_F(RulesetMatcherTest, ShouldBlockRequest) {
+ TestRule rule = CreateGenericRule();
+ rule.condition->url_filter = std::string("google.com");
+
+ std::unique_ptr<RulesetMatcher> matcher;
+ ASSERT_NO_FATAL_FAILURE(
+ CreateVerifiedMatcher({rule}, CreateTemporarySource(), &matcher));
+
+ EXPECT_TRUE(matcher->ShouldBlockRequest(
+ GURL("http://google.com"), url::Origin(),
+ url_pattern_index::flat::ElementType_SUBDOCUMENT, true));
+ EXPECT_FALSE(matcher->ShouldBlockRequest(
+ GURL("http://yahoo.com"), url::Origin(),
+ url_pattern_index::flat::ElementType_SUBDOCUMENT, true));
+}
+
+// Tests a simple redirect rule.
+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");
+
+ std::unique_ptr<RulesetMatcher> matcher;
+ ASSERT_NO_FATAL_FAILURE(
+ CreateVerifiedMatcher({rule}, CreateTemporarySource(), &matcher));
+
+ GURL redirect_url;
+ EXPECT_TRUE(matcher->ShouldRedirectRequest(
+ GURL("http://google.com"), url::Origin(),
+ url_pattern_index::flat::ElementType_SUBDOCUMENT, true, &redirect_url));
+ EXPECT_EQ(GURL("http://yahoo.com"), redirect_url);
+
+ EXPECT_FALSE(matcher->ShouldRedirectRequest(
+ GURL("http://yahoo.com"), url::Origin(),
+ url_pattern_index::flat::ElementType_SUBDOCUMENT, true, &redirect_url));
+}
+
+// Tests that a modified ruleset file fails verification.
+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(source.indexed_path, data.c_str(), data.size()));
+ EXPECT_EQ(RulesetMatcher::kLoadErrorVersionMismatch,
+ RulesetMatcher::CreateVerifiedMatcher(source.indexed_path,
+ expected_checksum, &matcher));
+
+ // Now, persist invalid data to the ruleset file, while maintaining the
+ // correct version header. Ensure that it fails verification due to checksum
+ // mismatch.
+ data = GetVersionHeaderForTesting() + "invalid data";
+ ASSERT_EQ(static_cast<int>(data.size()),
+ base::WriteFile(source.indexed_path, data.c_str(), data.size()));
+ EXPECT_EQ(RulesetMatcher::kLoadErrorChecksumMismatch,
+ RulesetMatcher::CreateVerifiedMatcher(source.indexed_path,
+ expected_checksum, &matcher));
+}
+
+} // namespace
+} // namespace declarative_net_request
+} // namespace extensions
diff --git a/extensions/browser/api/declarative_net_request/ruleset_source.cc b/extensions/browser/api/declarative_net_request/ruleset_source.cc
index a454de9..323d674 100644
--- a/extensions/browser/api/declarative_net_request/ruleset_source.cc
+++ b/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 @@
// 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
diff --git a/extensions/browser/api/declarative_net_request/ruleset_source.h b/extensions/browser/api/declarative_net_request/ruleset_source.h
index e657451..f99fc53 100644
--- a/extensions/browser/api/declarative_net_request/ruleset_source.h
+++ b/extensions/browser/api/declarative_net_request/ruleset_source.h
@@ -18,6 +18,8 @@
// 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 @@
base::FilePath indexed_path;
private:
- RulesetSource();
-
DISALLOW_COPY_AND_ASSIGN(RulesetSource);
};
