DLP: Fix url matching in DlpRulesManager

- Added a mapping from the url matching conditions
IDs of sources and destinations to Rules IDs so URL
matching will work properly when multiple patterns
are configured in one rule.

Bug: 1132804
Change-Id: Idfb04bbf1887fbbdf14804e48363212a41d215e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2435643
Commit-Queue: Aya Elsayed <ayaelattar@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Nikita Podguzov <nikitapodguzov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811951}

chrome/browser/chromeos/policy/dlp/dlp_rules_manager.cc

@@ -18,7 +18,6 @@
 #include "components/policy/core/common/policy_pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
-#include "components/url_matcher/url_matcher.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -102,6 +101,36 @@ DlpRulesManager::Level GetMinLevel(const DlpRulesManager::Level& level_1,
                                                                    : level_2;
 }
 
+// Inserts a mapping between URLs conditions IDs range to `rule_id` in `map`.
+void InsertUrlsRulesMapping(
+    DlpRulesManager::UrlConditionId url_condition_id_start,
+    DlpRulesManager::UrlConditionId url_condition_id_end,
+    DlpRulesManager::RuleId rule_id,
+    std::map<DlpRulesManager::UrlConditionId, DlpRulesManager::RuleId>& map) {
+  for (auto url_condition_id = url_condition_id_start;
+       url_condition_id <= url_condition_id_end; ++url_condition_id) {
+    map[url_condition_id] = rule_id;
+  }
+}
+
+// Matches `url` against `url_matcher` patterns and returns the rules IDs
+// configured with the matched patterns.
+std::set<DlpRulesManager::RuleId> MatchUrlAndGetRulesMapping(
+    const GURL& url,
+    const url_matcher::URLMatcher* url_matcher,
+    const std::map<DlpRulesManager::UrlConditionId, DlpRulesManager::RuleId>&
+        rules_map) {
+  DCHECK(url_matcher);
+  const std::set<DlpRulesManager::UrlConditionId> url_conditions_ids =
+      url_matcher->MatchURL(url);
+
+  std::set<DlpRulesManager::RuleId> rule_ids;
+  for (const auto& id : url_conditions_ids) {
+    rule_ids.insert(rules_map.at(id));
+  }
+  return rule_ids;
+}
+
 // A singleton instance of DlpRulesManager. Set from DlpRulesManager::Init().
 static DlpRulesManager* g_dlp_rules_manager = nullptr;
 
@@ -137,8 +166,8 @@ DlpRulesManager::Level DlpRulesManager::IsRestricted(
          restriction == Restriction::kPrivacyScreen ||
          restriction == Restriction::kScreenshot);
 
-  const std::set<url_matcher::URLMatcherConditionSet::ID> source_rules_ids =
-      src_url_matcher_->MatchURL(source);
+  const std::set<RuleId> source_rules_ids = MatchUrlAndGetRulesMapping(
+      source, src_url_matcher_.get(), src_url_rules_mapping_);
 
   return GetMaxJoinRestrictionLevel(restriction, source_rules_ids);
 }
@@ -156,11 +185,11 @@ DlpRulesManager::Level DlpRulesManager::IsRestrictedDestination(
           url::Origin::Create(destination)))
     return Level::kAllow;
 
-  std::set<url_matcher::URLMatcherConditionSet::ID> source_rules_ids =
-      src_url_matcher_->MatchURL(source);
+  const std::set<RuleId> source_rules_ids = MatchUrlAndGetRulesMapping(
+      source, src_url_matcher_.get(), src_url_rules_mapping_);
 
-  std::set<url_matcher::URLMatcherConditionSet::ID> destination_rules_ids =
-      dst_url_matcher_->MatchURL(destination);
+  const std::set<RuleId> destination_rules_ids = MatchUrlAndGetRulesMapping(
+      destination, dst_url_matcher_.get(), dst_url_rules_mapping_);
 
   return GetMaxJoinRestrictionLevel(restriction, source_rules_ids,
                                     destination_rules_ids);
@@ -173,8 +202,8 @@ DlpRulesManager::Level DlpRulesManager::IsRestrictedComponent(
   DCHECK(src_url_matcher_);
   DCHECK(restriction == Restriction::kClipboard);
 
-  const std::set<url_matcher::URLMatcherConditionSet::ID> source_rules_ids =
-      src_url_matcher_->MatchURL(source);
+  const std::set<RuleId> source_rules_ids = MatchUrlAndGetRulesMapping(
+      source, src_url_matcher_.get(), src_url_rules_mapping_);
 
   auto it = components_rules_.find(destination);
   if (it == components_rules_.end())
@@ -210,20 +239,24 @@ DlpRulesManager::DlpRulesManager() {
 DlpRulesManager::~DlpRulesManager() = default;
 
 void DlpRulesManager::OnPolicyUpdate() {
-  RuleId rules_counter = 0;
+  components_rules_.clear();
+  restrictions_map_.clear();
+  src_url_rules_mapping_.clear();
+  dst_url_rules_mapping_.clear();
+  src_url_matcher_ = std::make_unique<url_matcher::URLMatcher>();
+  dst_url_matcher_ = std::make_unique<url_matcher::URLMatcher>();
 
   const base::ListValue* rules_list =
       g_browser_process->local_state()->GetList(policy_prefs::kDlpRulesList);
 
-  src_url_matcher_ = std::make_unique<url_matcher::URLMatcher>();
-  dst_url_matcher_ = std::make_unique<url_matcher::URLMatcher>();
-  components_rules_.clear();
-  restrictions_map_.clear();
-
   if (!rules_list) {
     return;
   }
 
+  RuleId rules_counter = 0;
+  UrlConditionId src_url_condition_id = 0;
+  UrlConditionId dst_url_condition_id = 0;
+
   for (const base::Value& rule : *rules_list) {
     DCHECK(rule.is_dict());
     const auto* sources = rule.FindDictKey("sources");
@@ -232,18 +265,24 @@ void DlpRulesManager::OnPolicyUpdate() {
     DCHECK(sources_urls);  // This DCHECK should be removed when other types are
                            // supported as sources.
 
+    UrlConditionId prev_src_url_condition_id = src_url_condition_id;
     url_util::AddFilters(src_url_matcher_.get(), /* allowed= */ true,
-                         &rules_counter,
+                         &src_url_condition_id,
                          &base::Value::AsListValue(*sources_urls));
+    InsertUrlsRulesMapping(prev_src_url_condition_id + 1, src_url_condition_id,
+                           rules_counter, src_url_rules_mapping_);
 
     const auto* destinations = rule.FindDictKey("destinations");
     if (destinations) {
       const auto* destinations_urls = destinations->FindListKey("urls");
       if (destinations_urls) {
-        int destination_rule_id = rules_counter - 1;
+        UrlConditionId prev_dst_url_condition_id = dst_url_condition_id;
         url_util::AddFilters(dst_url_matcher_.get(), /* allowed= */ true,
-                             &destination_rule_id,
+                             &dst_url_condition_id,
                              &base::Value::AsListValue(*destinations_urls));
+        InsertUrlsRulesMapping(prev_dst_url_condition_id + 1,
+                               dst_url_condition_id, rules_counter,
+                               dst_url_rules_mapping_);
       }
       const auto* destinations_components =
           destinations->FindListKey("components");
@@ -274,6 +313,7 @@ void DlpRulesManager::OnPolicyUpdate() {
 
       restrictions_map_[rule_restriction].emplace(rules_counter, rule_level);
     }
+    ++rules_counter;
   }
 }
 
@@ -304,7 +344,7 @@ DlpRulesManager::Level DlpRulesManager::GetMaxJoinRestrictionLevel(
     const Restriction restriction,
     const std::set<RuleId>& source_rules,
     const std::set<RuleId>& destination_rules) const {
-  std::set<url_matcher::URLMatcherConditionSet::ID> intersection;
+  std::set<UrlConditionId> intersection;
   std::set_intersection(source_rules.begin(), source_rules.end(),
                         destination_rules.begin(), destination_rules.end(),
                         std::inserter(intersection, intersection.begin()));

chrome/browser/chromeos/policy/dlp/dlp_rules_manager.h

@@ -10,10 +10,7 @@
 #include <set>
 
 #include "components/prefs/pref_change_registrar.h"
-
-namespace url_matcher {
-class URLMatcher;
-}
+#include "components/url_matcher/url_matcher.h"
 
 class GURL;
 class PrefRegistrySimple;
@@ -74,6 +71,7 @@ class DlpRulesManager {
   };
 
   using RuleId = int;
+  using UrlConditionId = url_matcher::URLMatcherConditionSet::ID;
 
   // Creates a singleton instance of the class.
   static void Init();
@@ -153,6 +151,14 @@ class DlpRulesManager {
 
   // Map from the restrictions to their configured rules IDs and levels.
   std::map<Restriction, std::map<RuleId, Level>> restrictions_map_;
+
+  // Map from the URL matching conditions IDs of the sources to their configured
+  // rules IDs.
+  std::map<UrlConditionId, RuleId> src_url_rules_mapping_;
+
+  // Map from the URL matching conditions IDs of the destinations to their
+  // configured rules IDs.
+  std::map<UrlConditionId, RuleId> dst_url_rules_mapping_;
 };
 
 }  // namespace policy

chrome/browser/chromeos/policy/dlp/dlp_rules_manager_unittest.cc

@@ -7,6 +7,7 @@
 #include <string>
 #include <vector>
 
+#include "base/strings/strcat.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/policy/dlp/dlp_rules_manager_test_utils.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
@@ -26,6 +27,14 @@ constexpr char kUrlStr2[] = "https://wwww.google.com";
 constexpr char kUrlStr3[] = "*";
 constexpr char kUrlStr4[] = "https://www.gmail.com";
 
+constexpr char kHttpsPrefix[] = "https://www.";
+
+constexpr char kUrlPattern1[] = "chat.google.com";
+constexpr char kUrlPattern2[] = "salesforce.com";
+constexpr char kUrlPattern3[] = "docs.google.com";
+constexpr char kUrlPattern4[] = "drive.google.com";
+constexpr char kUrlPattern5[] = "*.company.com";
+
 }  // namespace
 
 class DlpRulesManagerTest : public testing::Test {
@@ -84,7 +93,8 @@ TEST_F(DlpRulesManagerTest, IsRestricted_LevelPrecedence) {
 
   rules.Append(dlp_test_util::CreateRule(
       "rule #1", "Block", std::move(src_urls_1), std::move(dst_urls_1),
-      base::Value(base::Value::Type::LIST), std::move(restrictions_1)));
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions_1)));
 
   // Second Rule
   base::Value src_urls_2(base::Value::Type::LIST);
@@ -99,7 +109,8 @@ TEST_F(DlpRulesManagerTest, IsRestricted_LevelPrecedence) {
 
   rules.Append(dlp_test_util::CreateRule(
       "rule #2", "exceptional allow", std::move(src_urls_2),
-      std::move(dst_urls_2), base::Value(base::Value::Type::LIST),
+      std::move(dst_urls_2),
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
       std::move(restrictions_2)));
   UpdatePolicyPref(std::move(rules));
 
@@ -148,8 +159,9 @@ TEST_F(DlpRulesManagerTest, UpdatePref) {
 
   rules_1.Append(dlp_test_util::CreateRule(
       "rule #1", "Block", std::move(src_urls_1),
-      base::Value(base::Value::Type::LIST),
-      base::Value(base::Value::Type::LIST), std::move(restrictions_1)));
+      /*dst_urls=*/base::Value(base::Value::Type::LIST),
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions_1)));
   UpdatePolicyPref(std::move(rules_1));
 
   EXPECT_EQ(DlpRulesManager::Level::kBlock,
@@ -168,8 +180,9 @@ TEST_F(DlpRulesManagerTest, UpdatePref) {
 
   rules_2.Append(dlp_test_util::CreateRule(
       "rule #2", "exceptional allow", std::move(src_urls_2),
-      base::Value(base::Value::Type::LIST),
-      base::Value(base::Value::Type::LIST), std::move(restrictions_2)));
+      /*dst_urls=*/base::Value(base::Value::Type::LIST),
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions_2)));
   UpdatePolicyPref(std::move(rules_2));
 
   EXPECT_EQ(DlpRulesManager::Level::kAllow,
@@ -195,8 +208,8 @@ TEST_F(DlpRulesManagerTest, IsRestrictedComponent_Clipboard) {
 
   rules.Append(dlp_test_util::CreateRule(
       "rule #1", "Block", std::move(src_urls),
-      base::Value(base::Value::Type::LIST), std::move(dst_components),
-      std::move(restrictions)));
+      /*dst_urls=*/base::Value(base::Value::Type::LIST),
+      std::move(dst_components), std::move(restrictions)));
   UpdatePolicyPref(std::move(rules));
 
   EXPECT_EQ(DlpRulesManager::Level::kBlock,
@@ -225,7 +238,8 @@ TEST_F(DlpRulesManagerTest, SameSrcDst_Clipboard) {
 
   rules.Append(dlp_test_util::CreateRule(
       "rule #1", "Block", std::move(src_urls), std::move(dst_urls),
-      base::Value(base::Value::Type::LIST), std::move(restrictions)));
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions)));
 
   UpdatePolicyPref(std::move(rules));
 
@@ -251,7 +265,8 @@ TEST_F(DlpRulesManagerTest, EmptyUrl_Clipboard) {
 
   rules.Append(dlp_test_util::CreateRule(
       "rule #1", "Block *", std::move(src_urls_1), std::move(dst_urls_1),
-      base::Value(base::Value::Type::LIST), std::move(restrictions_1)));
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions_1)));
 
   // First Rule
   base::Value src_urls_2(base::Value::Type::LIST);
@@ -266,7 +281,8 @@ TEST_F(DlpRulesManagerTest, EmptyUrl_Clipboard) {
 
   rules.Append(dlp_test_util::CreateRule(
       "rule #2", "Block", std::move(src_urls_2), std::move(dst_urls_2),
-      base::Value(base::Value::Type::LIST), std::move(restrictions_2)));
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions_2)));
 
   UpdatePolicyPref(std::move(rules));
 
@@ -287,8 +303,8 @@ TEST_F(DlpRulesManagerTest, IsRestrictedAnyOfComponents_Clipboard) {
   base::Value src_urls(base::Value::Type::LIST);
   src_urls.Append(kUrlStr1);
 
-  base::Value dst_urls(base::Value::Type::LIST);
-  dst_urls.Append(dlp::kPluginVm);
+  base::Value dst_components(base::Value::Type::LIST);
+  dst_components.Append(dlp::kPluginVm);
 
   base::Value restrictions(base::Value::Type::LIST);
   restrictions.Append(dlp_test_util::CreateRestrictionWithLevel(
@@ -296,8 +312,8 @@ TEST_F(DlpRulesManagerTest, IsRestrictedAnyOfComponents_Clipboard) {
 
   rules.Append(dlp_test_util::CreateRule(
       "rule #1", "Block PluginVM", std::move(src_urls),
-      base::Value(base::Value::Type::LIST), std::move(dst_urls),
-      std::move(restrictions)));
+      /*dst_urls=*/base::Value(base::Value::Type::LIST),
+      std::move(dst_components), std::move(restrictions)));
 
   UpdatePolicyPref(std::move(rules));
 
@@ -317,4 +333,82 @@ TEST_F(DlpRulesManagerTest, IsRestrictedAnyOfComponents_Clipboard) {
                 DlpRulesManager::Restriction::kClipboard));
 }
 
+TEST_F(DlpRulesManagerTest, IsRestricted_MultipleURLs) {
+  base::Value rules(base::Value::Type::LIST);
+
+  base::Value src_urls_1(base::Value::Type::LIST);
+  src_urls_1.Append(kUrlPattern1);
+  src_urls_1.Append(kUrlPattern2);
+  src_urls_1.Append(kUrlPattern3);
+  src_urls_1.Append(kUrlPattern4);
+  src_urls_1.Append(kUrlPattern5);
+
+  base::Value dst_urls_1 = src_urls_1.Clone();
+  base::Value src_urls_2 = src_urls_1.Clone();
+
+  base::Value restrictions_1(base::Value::Type::LIST);
+  restrictions_1.Append(dlp_test_util::CreateRestrictionWithLevel(
+      dlp::kClipboardRestriction, dlp::kAllowLevel));
+
+  rules.Append(dlp_test_util::CreateRule(
+      "Support agent work flows", "Allow copy and paste for work purposes",
+      std::move(src_urls_1), std::move(dst_urls_1),
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions_1)));
+
+  base::Value dst_urls_2(base::Value::Type::LIST);
+  dst_urls_2.Append(kUrlStr3);
+
+  base::Value restrictions_2(base::Value::Type::LIST);
+  restrictions_2.Append(dlp_test_util::CreateRestrictionWithLevel(
+      dlp::kClipboardRestriction, dlp::kBlockLevel));
+
+  rules.Append(dlp_test_util::CreateRule(
+      "Block non-agent work flows",
+      "Disallow copy and paste for non-work purposes", std::move(src_urls_2),
+      std::move(dst_urls_2),
+      /*dst_components=*/base::Value(base::Value::Type::LIST),
+      std::move(restrictions_2)));
+
+  UpdatePolicyPref(std::move(rules));
+
+  EXPECT_EQ(DlpRulesManager::Level::kAllow,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern1})),
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern2})),
+                DlpRulesManager::Restriction::kClipboard));
+  EXPECT_EQ(DlpRulesManager::Level::kAllow,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern3})),
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern4})),
+                DlpRulesManager::Restriction::kClipboard));
+  EXPECT_EQ(DlpRulesManager::Level::kAllow,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern5})),
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern2})),
+                DlpRulesManager::Restriction::kClipboard));
+  EXPECT_EQ(DlpRulesManager::Level::kAllow,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern2})),
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern3})),
+                DlpRulesManager::Restriction::kClipboard));
+
+  EXPECT_EQ(DlpRulesManager::Level::kBlock,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern1})),
+                GURL(kUrlStr2), DlpRulesManager::Restriction::kClipboard));
+  EXPECT_EQ(DlpRulesManager::Level::kBlock,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern2})),
+                GURL(kUrlStr1), DlpRulesManager::Restriction::kClipboard));
+  EXPECT_EQ(DlpRulesManager::Level::kBlock,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern3})),
+                GURL(kUrlStr2), DlpRulesManager::Restriction::kClipboard));
+  EXPECT_EQ(DlpRulesManager::Level::kBlock,
+            dlp_rules_manager_->IsRestrictedDestination(
+                GURL(base::StrCat({kHttpsPrefix, kUrlPattern4})),
+                GURL(kUrlStr1), DlpRulesManager::Restriction::kClipboard));
+}
+
 }  // namespace policy