DNR: Implement indexing of multiple static rulesets.

This CL:
  1. changes the installation code to index all static rulesets provided
  by an extension.
  2. Adds unittests to ensure 1. above works as intended.

Remaining work:
  1. Currently rule limits for static rulesets are only evaluated per
  ruleset. Implement them for the set of enabled rulesets once we
  start supporting the 'enabled' property as part of rulesets specified
  in manifest.
  2. We have limits on the no. of install warnings per ruleset. Also,
  add a limit on the total number of warnings across all rulesets.
  3. Load multiple static rulesets on extension load.
  4. Provide ability to dynamically toggle the set of enabled static
  rulesets.

BUG=754526

Change-Id: I92202ae69724f41ce1645af72dbc88e04fdc9f3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2107239
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754716}

chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc

@@ -390,7 +390,7 @@ class DeclarativeNetRequestBrowserTest
         "Extensions.DeclarativeNetRequest.LoadRulesetResult",
         RulesetMatcher::kLoadSuccess /*sample*/, 1 /*count*/);
 
-    EXPECT_TRUE(HasValidIndexedRuleset(*extension, profile()));
+    EXPECT_TRUE(AreAllIndexedStaticRulesetsValid(*extension, profile()));
 
     // Wait for the background page to load if needed.
     if (flags_ & kConfig_HasBackgroundScript)
@@ -1877,7 +1877,9 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
 
   const Extension* extension = extension_registry()->GetExtensionById(
       extension_id, ExtensionRegistry::ENABLED);
-  RulesetSource static_source = RulesetSource::CreateStatic(*extension);
+  std::vector<RulesetSource> static_sources =
+      RulesetSource::CreateStatic(*extension);
+  ASSERT_EQ(1u, static_sources.size());
   RulesetSource dynamic_source =
       RulesetSource::CreateDynamic(profile(), *extension);
 
@@ -1921,7 +1923,7 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   // Test static ruleset re-indexing.
   {
     SCOPED_TRACE("Static ruleset corruption");
-    corrupt_file_for_checksum_mismatch(static_source.indexed_path());
+    corrupt_file_for_checksum_mismatch(static_sources[0].indexed_path());
 
     base::HistogramTester tester;
     test_extension_works_after_reload();
@@ -1968,7 +1970,7 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   {
     SCOPED_TRACE("Static and dynamic ruleset corruption");
     corrupt_file_for_checksum_mismatch(dynamic_source.indexed_path());
-    corrupt_file_for_checksum_mismatch(static_source.indexed_path());
+    corrupt_file_for_checksum_mismatch(static_sources[0].indexed_path());
 
     base::HistogramTester tester;
     test_extension_works_after_reload();
@@ -2005,12 +2007,14 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest,
       last_loaded_extension_id(), ExtensionRegistry::ENABLED);
   ASSERT_TRUE(extension);
 
-  RulesetSource source = RulesetSource::CreateStatic(*extension);
+  std::vector<RulesetSource> sources = RulesetSource::CreateStatic(*extension);
+  ASSERT_EQ(1u, sources.size());
+
   // Mimic extension prefs corruption by overwriting the indexed ruleset
   // checksum.
   const int kInvalidRulesetChecksum = -1;
   ExtensionPrefs::Get(profile())->SetDNRStaticRulesetChecksum(
-      extension_id, source.id(), kInvalidRulesetChecksum);
+      extension_id, sources[0].id(), kInvalidRulesetChecksum);
 
   TestExtensionRegistryObserver registry_observer(
       ExtensionRegistry::Get(profile()), extension_id);

chrome/browser/extensions/api/declarative_net_request/ruleset_manager_unittest.cc

@@ -82,17 +82,20 @@ class RulesetManagerTest : public DNRTestBase {
     ExtensionRegistry::Get(browser_context())
         ->AddEnabled(last_loaded_extension_);
 
-    RulesetSource source = RulesetSource::CreateStatic(*last_loaded_extension_);
+    std::vector<RulesetSource> sources =
+        RulesetSource::CreateStatic(*last_loaded_extension_);
+    ASSERT_EQ(1u, sources.size());
+
     int expected_checksum;
     EXPECT_TRUE(ExtensionPrefs::Get(browser_context())
                     ->GetDNRStaticRulesetChecksum(last_loaded_extension_->id(),
-                                                  source.id(),
+                                                  sources[0].id(),
                                                   &expected_checksum));
 
     std::vector<std::unique_ptr<RulesetMatcher>> matchers(1);
     EXPECT_EQ(RulesetMatcher::kLoadSuccess,
               RulesetMatcher::CreateVerifiedMatcher(
-                  std::move(source), expected_checksum, &matchers[0]));
+                  std::move(sources[0]), expected_checksum, &matchers[0]));
 
     *matcher = std::make_unique<CompositeMatcher>(std::move(matchers));
   }

chrome/browser/extensions/unpacked_installer.cc

@@ -273,20 +273,28 @@ bool UnpackedInstaller::IndexAndPersistRulesIfNeeded(std::string* error) {
     return true;
   }
 
+  using RulesetSource = declarative_net_request::RulesetSource;
+
   // TODO(crbug.com/761107): Change this so that we don't need to parse JSON
   // in the browser process.
-  auto ruleset_source =
-      declarative_net_request::RulesetSource::CreateStatic(*extension());
-  declarative_net_request::IndexAndPersistJSONRulesetResult result =
-      ruleset_source.IndexAndPersistJSONRulesetUnsafe();
-  if (!result.success) {
-    *error = std::move(result.error);
-    return false;
-  }
+  // TODO(crbug.com/754526): Impose a limit on the total number of rules across
+  // all the rulesets for an extension. Also, limit the number of install
+  // warnings across all rulesets.
+  std::vector<RulesetSource> sources =
+      RulesetSource::CreateStatic(*extension());
+
+  for (const RulesetSource& source : sources) {
+    declarative_net_request::IndexAndPersistJSONRulesetResult result =
+        source.IndexAndPersistJSONRulesetUnsafe();
+    if (!result.success) {
+      *error = std::move(result.error);
+      return false;
+    }
 
-  ruleset_checksums_.emplace_back(result.ruleset_id, result.ruleset_checksum);
-  if (!result.warnings.empty())
-    extension_->AddInstallWarnings(std::move(result.warnings));
+    ruleset_checksums_.emplace_back(result.ruleset_id, result.ruleset_checksum);
+    if (!result.warnings.empty())
+      extension_->AddInstallWarnings(std::move(result.warnings));
+  }
 
   return true;
 }

chrome/browser/extensions/unpacked_installer.h

@@ -125,10 +125,10 @@ class UnpackedInstaller
                      int flags,
                      std::string* error);
 
-  // Reads the Declarative Net Request JSON ruleset for the extension, if it
-  // provided one, and persists the indexed ruleset. Returns false and populates
-  // |error| in case of an error. Should be called on a sequence where file IO
-  // is allowed.
+  // Reads the Declarative Net Request JSON rulesets for the extension, if it
+  // provided any, and persists the indexed rulesets. Returns false and
+  // populates |error| in case of an error. Should be called on a sequence where
+  // file IO is allowed.
   bool IndexAndPersistRulesIfNeeded(std::string* error);
 
   const Extension* extension() { return extension_.get(); }

extensions/browser/api/declarative_net_request/BUILD.gn

@@ -67,6 +67,8 @@ source_set("core") {
     "constants.h",
     "flat_ruleset_indexer.cc",
     "flat_ruleset_indexer.h",
+    "index_helper.cc",
+    "index_helper.h",
     "indexed_rule.cc",
     "indexed_rule.h",
     "parse_info.cc",

extensions/browser/api/declarative_net_request/index_helper.cc

+// Copyright 2020 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/index_helper.h"
+
+#include <utility>
+
+#include "base/barrier_closure.h"
+
+namespace extensions {
+namespace declarative_net_request {
+
+// static
+void IndexHelper::Start(std::vector<RulesetSource> sources,
+                        IndexCallback callback) {
+  // Note we use ref-counting instead of manual memory management since there
+  // are some subtle cases:
+  //  - Zero rulesets to index.
+  //  - All individual callbacks return synchronously.
+  // In these cases there's a potential for a use-after-free with manual memory
+  // management.
+  auto index_helper = base::WrapRefCounted(
+      new IndexHelper(std::move(sources), std::move(callback)));
+  index_helper->Start();
+}
+
+IndexHelper::IndexHelper(std::vector<RulesetSource> sources,
+                         IndexCallback callback)
+    : sources_(std::move(sources)), callback_(std::move(callback)) {}
+
+IndexHelper::~IndexHelper() = default;
+
+void IndexHelper::Start() {
+  // |all_done_closure| will be invoked once |barrier_closure| is run
+  // |sources_.size()| times.
+  base::OnceClosure all_done_closure =
+      base::BindOnce(&IndexHelper::OnAllRulesetsIndexed, this);
+  base::RepeatingClosure barrier_closure =
+      base::BarrierClosure(sources_.size(), std::move(all_done_closure));
+
+  for (const RulesetSource& source : sources_) {
+    auto callback =
+        base::BindOnce(&IndexHelper::OnRulesetIndexed, this, barrier_closure);
+    source.IndexAndPersistJSONRuleset(&decoder_, std::move(callback));
+  }
+}
+
+void IndexHelper::OnAllRulesetsIndexed() {
+  DCHECK_EQ(sources_.size(), results_.size());
+
+  // Our job is done.
+  std::move(callback_).Run(std::move(results_));
+}
+
+// Callback invoked when indexing of a single ruleset is completed.
+void IndexHelper::OnRulesetIndexed(base::OnceClosure ruleset_done_closure,
+                                   IndexAndPersistJSONRulesetResult result) {
+  results_.push_back(std::move(result));
+  std::move(ruleset_done_closure).Run();
+}
+
+}  // namespace declarative_net_request
+}  // namespace extensions

extensions/browser/api/declarative_net_request/index_helper.h

+// Copyright 2020 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.
+
+#ifndef EXTENSIONS_BROWSER_API_DECLARATIVE_NET_REQUEST_INDEX_HELPER_H_
+#define EXTENSIONS_BROWSER_API_DECLARATIVE_NET_REQUEST_INDEX_HELPER_H_
+
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/memory/ref_counted.h"
+#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
+#include "services/data_decoder/public/cpp/data_decoder.h"
+
+namespace extensions {
+namespace declarative_net_request {
+
+// A class to help in indexing multiple rulesets.
+class IndexHelper : public base::RefCountedThreadSafe<IndexHelper> {
+ public:
+  // Starts indexing rulesets. Must be called on a sequence which supports file
+  // IO. The |callback| will be dispatched to the same sequence on which Start()
+  // is called.
+  using Results = std::vector<IndexAndPersistJSONRulesetResult>;
+  using IndexCallback = base::OnceCallback<void(Results)>;
+  static void Start(std::vector<RulesetSource> sources, IndexCallback callback);
+
+ private:
+  friend class base::RefCountedThreadSafe<IndexHelper>;
+
+  IndexHelper(std::vector<RulesetSource> sources, IndexCallback callback);
+  ~IndexHelper();
+
+  // Starts indexing the rulesets.
+  void Start();
+
+  // Callback invoked when indexing of all rulesets is completed.
+  void OnAllRulesetsIndexed();
+
+  // Callback invoked when indexing of a single ruleset is completed.
+  void OnRulesetIndexed(base::OnceClosure ruleset_done_closure,
+                        IndexAndPersistJSONRulesetResult result);
+
+  std::vector<RulesetSource> sources_;
+  IndexCallback callback_;
+  Results results_;
+
+  // We use a single shared Data Decoder service instance to process all of the
+  // rulesets for this IndexHelper.
+  data_decoder::DataDecoder decoder_;
+};
+
+}  // namespace declarative_net_request
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_API_DECLARATIVE_NET_REQUEST_INDEX_HELPER_H_

extensions/browser/api/declarative_net_request/rules_monitor_service.cc

@@ -190,7 +190,11 @@ void RulesMonitorService::OnExtensionLoaded(
 
   // Static ruleset.
   {
-    RulesetInfo static_ruleset(RulesetSource::CreateStatic(*extension));
+    std::vector<RulesetSource> static_rulesets =
+        RulesetSource::CreateStatic(*extension);
+
+    // TODO(crbug.com/754526): Load all static rulesets for the extension.
+    RulesetInfo static_ruleset(std::move(static_rulesets[0]));
     bool has_checksum = prefs_->GetDNRStaticRulesetChecksum(
         extension->id(), static_ruleset.source().id(),
         &expected_ruleset_checksum);

extensions/browser/api/declarative_net_request/ruleset_source.cc

@@ -301,16 +301,21 @@ ReadJSONRulesResult& ReadJSONRulesResult::operator=(ReadJSONRulesResult&&) =
     default;
 
 // static
-RulesetSource RulesetSource::CreateStatic(const Extension& extension) {
-  const DNRManifestData::RulesetInfo& info =
-      declarative_net_request::DNRManifestData::GetRuleset(extension);
-
-  DCHECK_GE(info.id, kMinValidStaticRulesetID);
-  return RulesetSource(extension.path().Append(info.relative_path),
-                       extension.path().Append(
-                           file_util::GetIndexedRulesetRelativePath(info.id)),
-                       info.id, dnr_api::SOURCE_TYPE_MANIFEST,
-                       dnr_api::MAX_NUMBER_OF_RULES, extension.id());
+std::vector<RulesetSource> RulesetSource::CreateStatic(
+    const Extension& extension) {
+  const std::vector<DNRManifestData::RulesetInfo>& rulesets =
+      declarative_net_request::DNRManifestData::GetRulesets(extension);
+
+  std::vector<RulesetSource> sources;
+  for (const auto& info : rulesets) {
+    sources.push_back(
+        RulesetSource(extension.path().Append(info.relative_path),
+                      extension.path().Append(
+                          file_util::GetIndexedRulesetRelativePath(info.id)),
+                      info.id, dnr_api::SOURCE_TYPE_MANIFEST,
+                      dnr_api::MAX_NUMBER_OF_RULES, extension.id()));
+  }
+  return sources;
 }
 
 // static

extensions/browser/api/declarative_net_request/ruleset_source.h

@@ -122,10 +122,10 @@ struct ReadJSONRulesResult {
 // Holds paths for an extension ruleset.
 class RulesetSource {
  public:
-  // Creates RulesetSource corresponding to the static ruleset in the extension
-  // package. This must only be called for extensions which specified a
-  // declarative ruleset.
-  static RulesetSource CreateStatic(const Extension& extension);
+  // Creates RulesetSources corresponding to the static rulesets in the
+  // extension package. This must only be called for extensions which specified
+  // a declarative ruleset.
+  static std::vector<RulesetSource> CreateStatic(const Extension& extension);
 
   // Creates RulesetSource corresponding to the dynamic rules added by the
   // extension. This must only be called for extensions which specified a

extensions/browser/api/declarative_net_request/test_utils.cc

@@ -255,20 +255,28 @@ std::ostream& operator<<(std::ostream& output, const ParseResult& result) {
   return output;
 }
 
-bool HasValidIndexedRuleset(const Extension& extension,
-                            content::BrowserContext* browser_context) {
-  RulesetSource source = RulesetSource::CreateStatic(extension);
-  int expected_checksum;
-  if (!ExtensionPrefs::Get(browser_context)
-           ->GetDNRStaticRulesetChecksum(extension.id(), source.id(),
-                                         &expected_checksum)) {
-    return false;
+bool AreAllIndexedStaticRulesetsValid(
+    const Extension& extension,
+    content::BrowserContext* browser_context) {
+  std::vector<RulesetSource> sources = RulesetSource::CreateStatic(extension);
+
+  for (RulesetSource& source : sources) {
+    int expected_checksum = -1;
+    if (!ExtensionPrefs::Get(browser_context)
+             ->GetDNRStaticRulesetChecksum(extension.id(), source.id(),
+                                           &expected_checksum)) {
+      return false;
+    }
+
+    std::unique_ptr<RulesetMatcher> matcher;
+    if (RulesetMatcher::CreateVerifiedMatcher(std::move(source),
+                                              expected_checksum, &matcher) !=
+        RulesetMatcher::kLoadSuccess) {
+      return false;
+    }
   }
 
-  std::unique_ptr<RulesetMatcher> matcher;
-  return RulesetMatcher::CreateVerifiedMatcher(std::move(source),
-                                               expected_checksum, &matcher) ==
-         RulesetMatcher::kLoadSuccess;
+  return true;
 }
 
 bool CreateVerifiedMatcher(const std::vector<TestRule>& rules,

extensions/browser/api/declarative_net_request/test_utils.h

@@ -54,10 +54,10 @@ std::ostream& operator<<(std::ostream& output, const ParseResult& result);
 std::ostream& operator<<(std::ostream& output,
                          const base::Optional<RequestAction>& action);
 
-// Returns true if the given extension has a valid indexed ruleset. Should be
-// called on a sequence where file IO is allowed.
-bool HasValidIndexedRuleset(const Extension& extension,
-                            content::BrowserContext* browser_context);
+// Returns true if the given extension's indexed static rulesets are all valid.
+// Should be called on a sequence where file IO is allowed.
+bool AreAllIndexedStaticRulesetsValid(const Extension& extension,
+                                      content::BrowserContext* browser_context);
 
 // Helper to create a verified ruleset matcher. Populates |matcher| and
 // |expected_checksum|. Returns true on success.

extensions/browser/content_verifier.cc

@@ -130,10 +130,11 @@ std::unique_ptr<ContentVerifierIOData::ExtensionData> CreateIOData(
       std::make_unique<std::set<CanonicalRelativePath>>();
   using DNRManifestData = declarative_net_request::DNRManifestData;
   if (DNRManifestData::HasRuleset(*extension)) {
-    const DNRManifestData::RulesetInfo& info =
-        DNRManifestData::GetRuleset(*extension);
-    indexed_ruleset_paths->insert(
-        canonicalize_path(file_util::GetIndexedRulesetRelativePath(info.id)));
+    for (const DNRManifestData::RulesetInfo& info :
+         DNRManifestData::GetRulesets(*extension)) {
+      indexed_ruleset_paths->insert(
+          canonicalize_path(file_util::GetIndexedRulesetRelativePath(info.id)));
+    }
   }
 
   return std::make_unique<ContentVerifierIOData::ExtensionData>(

extensions/browser/sandboxed_unpacker.cc

@@ -30,6 +30,7 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/api/declarative_net_request/constants.h"
+#include "extensions/browser/api/declarative_net_request/index_helper.h"
 #include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 #include "extensions/browser/computed_hashes.h"
 #include "extensions/browser/extension_file_task_runner.h"
@@ -652,7 +653,7 @@ void SandboxedUnpacker::MessageCatalogsSanitized(
     const std::string& error_msg) {
   DCHECK(unpacker_io_task_runner_->RunsTasksInCurrentSequence());
   if (status == JsonFileSanitizer::Status::kSuccess) {
-    IndexAndPersistJSONRulesetIfNeeded();
+    IndexAndPersistJSONRulesetsIfNeeded();
     return;
   }
 
@@ -687,7 +688,7 @@ void SandboxedUnpacker::MessageCatalogsSanitized(
   ReportFailure(failure_reason, error);
 }
 
-void SandboxedUnpacker::IndexAndPersistJSONRulesetIfNeeded() {
+void SandboxedUnpacker::IndexAndPersistJSONRulesetsIfNeeded() {
   DCHECK(unpacker_io_task_runner_->RunsTasksInCurrentSequence());
   DCHECK(extension_);
 
@@ -697,32 +698,43 @@ void SandboxedUnpacker::IndexAndPersistJSONRulesetIfNeeded() {
     return;
   }
 
-  auto ruleset_source =
-      declarative_net_request::RulesetSource::CreateStatic(*extension_);
-  ruleset_source.IndexAndPersistJSONRuleset(
-      &data_decoder_,
-      base::BindOnce(&SandboxedUnpacker::OnJSONRulesetIndexed, this));
+  declarative_net_request::IndexHelper::Start(
+      declarative_net_request::RulesetSource::CreateStatic(*extension_),
+      base::BindOnce(&SandboxedUnpacker::OnJSONRulesetsIndexed, this));
 }
 
-void SandboxedUnpacker::OnJSONRulesetIndexed(
-    declarative_net_request::IndexAndPersistJSONRulesetResult result) {
-  if (result.success) {
+void SandboxedUnpacker::OnJSONRulesetsIndexed(
+    std::vector<declarative_net_request::IndexAndPersistJSONRulesetResult>
+        results) {
+  base::TimeDelta total_index_and_persist_time;
+  size_t total_rules_count = 0;
+
+  // TODO(crbug.com/754526): Impose a limit on the total number of rules across
+  // all the rulesets for an extension. Also, limit the number of install
+  // warnings across all rulesets.
+  for (auto& result : results) {
+    if (!result.success) {
+      ReportFailure(
+          SandboxedUnpackerFailureReason::ERROR_INDEXING_DNR_RULESET,
+          l10n_util::GetStringFUTF16(IDS_EXTENSION_PACKAGE_ERROR_MESSAGE,
+                                     base::UTF8ToUTF16(result.error)));
+      return;
+    }
+
     if (!result.warnings.empty())
       extension_->AddInstallWarnings(std::move(result.warnings));
-    UMA_HISTOGRAM_COUNTS_100000(
-        declarative_net_request::kManifestRulesCountHistogram,
-        result.rules_count);
-    UMA_HISTOGRAM_TIMES(
-        declarative_net_request::kIndexAndPersistRulesTimeHistogram,
-        result.index_and_persist_time);
+
+    total_index_and_persist_time += result.index_and_persist_time;
+    total_rules_count += result.rules_count;
     ruleset_checksums_.emplace_back(result.ruleset_id, result.ruleset_checksum);
-    CheckComputeHashes();
-    return;
   }
 
-  ReportFailure(SandboxedUnpackerFailureReason::ERROR_INDEXING_DNR_RULESET,
-                l10n_util::GetStringFUTF16(IDS_EXTENSION_PACKAGE_ERROR_MESSAGE,
-                                           base::UTF8ToUTF16(result.error)));
+  UMA_HISTOGRAM_TIMES(
+      declarative_net_request::kIndexAndPersistRulesTimeHistogram,
+      total_index_and_persist_time);
+  UMA_HISTOGRAM_COUNTS_100000(
+      declarative_net_request::kManifestRulesCountHistogram, total_rules_count);
+  CheckComputeHashes();
 }
 
 void SandboxedUnpacker::CheckComputeHashes() {

extensions/browser/sandboxed_unpacker.h

@@ -218,12 +218,13 @@ class SandboxedUnpacker : public base::RefCountedThreadSafe<SandboxedUnpacker> {
   void Cleanup();
 
   // If a Declarative Net Request JSON ruleset is present, parses the JSON
-  // ruleset for the Declarative Net Request API and persists the indexed
-  // ruleset.
-  void IndexAndPersistJSONRulesetIfNeeded();
+  // rulesets for the Declarative Net Request API and persists the indexed
+  // rulesets.
+  void IndexAndPersistJSONRulesetsIfNeeded();
 
-  void OnJSONRulesetIndexed(
-      declarative_net_request::IndexAndPersistJSONRulesetResult result);
+  void OnJSONRulesetsIndexed(
+      std::vector<declarative_net_request::IndexAndPersistJSONRulesetResult>
+          results);
 
   // Computed hashes: if requested (via ShouldComputeHashes callback in
   // SandbloxedUnpackerClient), calculate hashes of all extensions' resources

extensions/common/api/declarative_net_request/dnr_manifest_data.cc

@@ -26,15 +26,13 @@ bool DNRManifestData::HasRuleset(const Extension& extension) {
 }
 
 // static
-const DNRManifestData::RulesetInfo& DNRManifestData::GetRuleset(
+const std::vector<DNRManifestData::RulesetInfo>& DNRManifestData::GetRulesets(
     const Extension& extension) {
   Extension::ManifestData* data =
       extension.GetManifestData(manifest_keys::kDeclarativeNetRequestKey);
   DCHECK(data);
 
-  // TODO(crbug.com/754526): Change this to return |rulesets|. Currently we only
-  // index the first ruleset specified by the extension in its manifest.
-  return static_cast<DNRManifestData*>(data)->rulesets[0];
+  return static_cast<DNRManifestData*>(data)->rulesets;
 }
 
 }  // namespace declarative_net_request

extensions/common/api/declarative_net_request/dnr_manifest_data.h

@@ -37,7 +37,8 @@ struct DNRManifestData : Extension::ManifestData {
 
   // Returns the RulesetInfo for the |extension|. This must be called only if
   // HasRuleset returns true for the |extension|.
-  static const RulesetInfo& GetRuleset(const Extension& extension);
+  static const std::vector<RulesetInfo>& GetRulesets(
+      const Extension& extension);
 
   std::vector<RulesetInfo> rulesets;
 

extensions/common/api/declarative_net_request/dnr_manifest_unittest.cc

@@ -21,6 +21,7 @@
 #include "extensions/common/file_util.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/value_builder.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
@@ -56,15 +57,24 @@ class DNRManifestTest : public testing::Test {
     EXPECT_EQ(expected_error, error);
   }
 
-  // Loads the extension and verifies that the JSON ruleset location is
-  // correctly set up. Returns the loaded extension.
-  scoped_refptr<Extension> LoadAndExpectSuccess() {
+  // Loads the extension and verifies that the manifest info is correctly set
+  // up.. Returns the loaded extension.
+  void LoadAndExpectSuccess(const std::vector<base::FilePath>& expected_paths) {
     std::string error;
     scoped_refptr<Extension> extension = file_util::LoadExtension(
         temp_dir_.GetPath(), Manifest::UNPACKED, Extension::NO_FLAGS, &error);
-    EXPECT_TRUE(extension) << error;
+    ASSERT_TRUE(extension) << error;
     EXPECT_TRUE(error.empty());
-    return extension;
+
+    ASSERT_TRUE(DNRManifestData::HasRuleset(*extension));
+
+    const std::vector<DNRManifestData::RulesetInfo>& rulesets =
+        DNRManifestData::GetRulesets(*extension);
+    ASSERT_EQ(expected_paths.size(), rulesets.size());
+    for (size_t i = 0; i < rulesets.size(); ++i) {
+      EXPECT_GE(rulesets[i].id, kMinValidStaticRulesetID);
+      EXPECT_EQ(rulesets[i].relative_path, expected_paths[i]);
+    }
   }
 
   void WriteManifestAndRuleset(
@@ -98,10 +108,7 @@ TEST_F(DNRManifestTest, EmptyRuleset) {
   base::FilePath ruleset_path = base::FilePath(kJSONRulesetFilepath);
   WriteManifestAndRuleset(*CreateManifest(kJSONRulesFilename), {ruleset_path});
 
-  scoped_refptr<Extension> extension = LoadAndExpectSuccess();
-  ASSERT_TRUE(DNRManifestData::HasRuleset(*extension));
-  EXPECT_EQ(ruleset_path,
-            DNRManifestData::GetRuleset(*extension).relative_path);
+  LoadAndExpectSuccess({ruleset_path});
 }
 
 TEST_F(DNRManifestTest, InvalidManifestKey) {
@@ -139,14 +146,16 @@ TEST_F(DNRManifestTest, MultipleRulesFileSuccess) {
   ruleset.path = path2.AsUTF8Unsafe();
   rule_resources.Append(ruleset.ToValue());
 
-  manifest->SetList(GetRuleResourcesKey(), rule_resources.Build());
+  base::FilePath path3(FILE_PATH_LITERAL("file3.json"));
+  ruleset.path = path3.AsUTF8Unsafe();
+  rule_resources.Append(ruleset.ToValue());
 
-  WriteManifestAndRuleset(*manifest, {path1, path2});
+  manifest->SetList(GetRuleResourcesKey(), rule_resources.Build());
 
-  scoped_refptr<Extension> extension = LoadAndExpectSuccess();
+  std::vector<base::FilePath> paths = {path1, path2, path3};
+  WriteManifestAndRuleset(*manifest, paths);
 
-  ASSERT_TRUE(DNRManifestData::HasRuleset(*extension));
-  EXPECT_EQ(path1, DNRManifestData::GetRuleset(*extension).relative_path);
+  LoadAndExpectSuccess(paths);
 }
 
 TEST_F(DNRManifestTest, MultipleRulesFileInvalidPath) {
@@ -244,10 +253,7 @@ TEST_F(DNRManifestTest, RulesFileInNestedDirectory) {
 
   WriteManifestAndRuleset(*manifest, {nested_path});
 
-  scoped_refptr<Extension> extension = LoadAndExpectSuccess();
-
-  ASSERT_TRUE(DNRManifestData::HasRuleset(*extension));
-  EXPECT_EQ(nested_path, DNRManifestData::GetRuleset(*extension).relative_path);
+  LoadAndExpectSuccess({nested_path});
 }
 
 }  // namespace

extensions/common/api/declarative_net_request/test_utils.cc

@@ -57,6 +57,56 @@ void SetValue(base::DictionaryValue* dict,
   dict->Set(key, ToValue(*value));
 }
 
+// Helper to build an extension manifest which uses the
+// kDeclarativeNetRequestKey manifest key. |hosts| specifies the host
+// permissions to grant. |flags| is a bitmask of ConfigFlag to configure the
+// extension. |ruleset_info| specifies the static rulesets for the extension.
+std::unique_ptr<base::DictionaryValue> CreateManifest(
+    const std::vector<TestRulesetInfo>& ruleset_info,
+    const std::vector<std::string>& hosts,
+    unsigned flags) {
+  std::vector<std::string> permissions = hosts;
+  permissions.push_back(kAPIPermission);
+
+  // These permissions are needed for some tests. TODO(karandeepb): Add a
+  // ConfigFlag for these.
+  permissions.push_back("webRequest");
+  permissions.push_back("webRequestBlocking");
+
+  if (flags & kConfig_HasFeedbackPermission)
+    permissions.push_back(kFeedbackAPIPermission);
+
+  if (flags & kConfig_HasActiveTab)
+    permissions.push_back("activeTab");
+
+  std::vector<std::string> background_scripts;
+  if (flags & kConfig_HasBackgroundScript)
+    background_scripts.push_back("background.js");
+
+  ListBuilder rule_resources_builder;
+  for (const TestRulesetInfo& info : ruleset_info) {
+    dnr_api::Ruleset ruleset;
+    ruleset.path = info.relative_file_path;
+    rule_resources_builder.Append(ruleset.ToValue());
+  }
+
+  return DictionaryBuilder()
+      .Set(keys::kName, "Test extension")
+      .Set(keys::kDeclarativeNetRequestKey,
+           DictionaryBuilder()
+               .Set(keys::kDeclarativeRuleResourcesKey,
+                    rule_resources_builder.Build())
+               .Build())
+      .Set(keys::kPermissions, ToListValue(permissions))
+      .Set(keys::kVersion, "1.0")
+      .Set(keys::kManifestVersion, 2)
+      .Set("background", DictionaryBuilder()
+                             .Set("scripts", ToListValue(background_scripts))
+                             .Build())
+      .Set(keys::kBrowserAction, DictionaryBuilder().Build())
+      .Build();
+}
+
 }  // namespace
 
 TestRuleCondition::TestRuleCondition() = default;
@@ -187,42 +237,9 @@ std::unique_ptr<base::DictionaryValue> CreateManifest(
     const std::string& json_rules_filename,
     const std::vector<std::string>& hosts,
     unsigned flags) {
-  std::vector<std::string> permissions = hosts;
-  permissions.push_back(kAPIPermission);
-  permissions.push_back("webRequest");
-  permissions.push_back("webRequestBlocking");
-
-  if (flags & kConfig_HasFeedbackPermission)
-    permissions.push_back(kFeedbackAPIPermission);
-
-  if (flags & kConfig_HasActiveTab)
-    permissions.push_back("activeTab");
-
-  std::vector<std::string> background_scripts;
-  if (flags & kConfig_HasBackgroundScript)
-    background_scripts.push_back("background.js");
-
-  dnr_api::Ruleset ruleset;
-  ruleset.path = json_rules_filename;
-
-  std::unique_ptr<base::ListValue> rule_resources =
-      ListBuilder().Append(ruleset.ToValue()).Build();
-
-  return DictionaryBuilder()
-      .Set(keys::kName, "Test extension")
-      .Set(keys::kDeclarativeNetRequestKey,
-           DictionaryBuilder()
-               .Set(keys::kDeclarativeRuleResourcesKey,
-                    std::move(rule_resources))
-               .Build())
-      .Set(keys::kPermissions, ToListValue(permissions))
-      .Set(keys::kVersion, "1.0")
-      .Set(keys::kManifestVersion, 2)
-      .Set("background", DictionaryBuilder()
-                             .Set("scripts", ToListValue(background_scripts))
-                             .Build())
-      .Set(keys::kBrowserAction, DictionaryBuilder().Build())
-      .Build();
+  std::vector<TestRulesetInfo> rulesets;
+  rulesets.push_back({json_rules_filename, base::ListValue()});
+  return CreateManifest(rulesets, hosts, flags);
 }
 
 std::unique_ptr<base::ListValue> ToListValue(
@@ -240,13 +257,15 @@ std::unique_ptr<base::ListValue> ToListValue(const std::vector<TestRule>& vec) {
   return builder.Build();
 }
 
-void WriteManifestAndRuleset(const base::FilePath& extension_dir,
-                             const TestRulesetInfo& info,
-                             const std::vector<std::string>& hosts,
-                             unsigned flags) {
-  // Persist JSON rules file.
-  JSONFileValueSerializer(extension_dir.AppendASCII(info.relative_file_path))
-      .Serialize(info.rules_value);
+void WriteManifestAndRulesets(const base::FilePath& extension_dir,
+                              const std::vector<TestRulesetInfo>& ruleset_info,
+                              const std::vector<std::string>& hosts,
+                              unsigned flags) {
+  // Persist JSON rules files.
+  for (const TestRulesetInfo& info : ruleset_info) {
+    JSONFileValueSerializer(extension_dir.AppendASCII(info.relative_file_path))
+        .Serialize(info.rules_value);
+  }
 
   // Persists a background script if needed.
   if (flags & ConfigFlag::kConfig_HasBackgroundScript) {
@@ -258,7 +277,16 @@ void WriteManifestAndRuleset(const base::FilePath& extension_dir,
 
   // Persist manifest file.
   JSONFileValueSerializer(extension_dir.Append(kManifestFilename))
-      .Serialize(*CreateManifest(info.relative_file_path, hosts, flags));
+      .Serialize(*CreateManifest(ruleset_info, hosts, flags));
+}
+
+void WriteManifestAndRuleset(const base::FilePath& extension_dir,
+                             const TestRulesetInfo& info,
+                             const std::vector<std::string>& hosts,
+                             unsigned flags) {
+  std::vector<TestRulesetInfo> rulesets;
+  rulesets.push_back({info.relative_file_path, info.rules_value.Clone()});
+  WriteManifestAndRulesets(extension_dir, rulesets, hosts, flags);
 }
 
 }  // namespace declarative_net_request

extensions/common/api/declarative_net_request/test_utils.h

@@ -152,10 +152,19 @@ enum ConfigFlag {
   kConfig_HasActiveTab = 1 << 2,
 };
 
+// Describes a single extension ruleset.
+struct TestRulesetInfo {
+  // File path relative to the extension directory.
+  std::string relative_file_path;
+
+  // The base::Value corresponding to the rules in the ruleset.
+  base::Value rules_value;
+};
+
 // Helper to build an extension manifest which uses the
 // kDeclarativeNetRequestKey manifest key. |hosts| specifies the host
 // permissions to grant. |flags| is a bitmask of ConfigFlag to configure the
-// extension.
+// extension. Should be used when the extension has a single static ruleset.
 std::unique_ptr<base::DictionaryValue> CreateManifest(
     const std::string& json_rules_filename,
     const std::vector<std::string>& hosts = {},
@@ -169,16 +178,17 @@ std::unique_ptr<base::ListValue> ToListValue(
 std::unique_ptr<base::ListValue> ToListValue(
     const std::vector<TestRule>& rules);
 
-// Describes a single extension ruleset.
-struct TestRulesetInfo {
-  std::string relative_file_path;
-  base::Value rules_value;
-};
+// Writes the rulesets specified in |ruleset_info| in the given |extension_dir|
+// together with the manifest file. |hosts| specifies the host permissions, the
+// extensions should have. |flags| is a bitmask of ConfigFlag to configure the
+// extension.
+void WriteManifestAndRulesets(const base::FilePath& extension_dir,
+                              const std::vector<TestRulesetInfo>& ruleset_info,
+                              const std::vector<std::string>& hosts,
+                              unsigned flags = ConfigFlag::kConfig_None);
 
-// Writes the declarative rules specified in |ruleset_info| in the given
-// |extension_dir| together with the manifest file. |hosts| specifies the host
-// permissions, the extensions should have. |flags| is a bitmask of ConfigFlag
-// to configure the extension.
+// Specialization of WriteManifestAndRulesets above for an extension with a
+// single static ruleset.
 void WriteManifestAndRuleset(const base::FilePath& extension_dir,
                              const TestRulesetInfo& ruleset_info,
                              const std::vector<std::string>& hosts,