OOR-CORS: OriginAccessList cleanups

This patch changes the data structure to have the allow/block lists.
Also, modifies the interfaces not to have list specific clear methods,
but just have them to clear both lists together.

Still |Patterns| isn't the best structure to have multiple patterns
for the same origin with priorities, but since matching rule is not
simple, and will be changed to count port number, it is not updated
in this patch.

Bug: 908756
Change-Id: Ie148b5f35c66968fb2a1c8a9332a1442b6091586
TBR: tsepez@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614380
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660779}

services/network/public/cpp/cors/origin_access_list.cc

@@ -16,7 +16,7 @@ OriginAccessList::~OriginAccessList() = default;
 void OriginAccessList::SetAllowListForOrigin(
     const url::Origin& source_origin,
     const std::vector<CorsOriginPatternPtr>& patterns) {
-  SetForOrigin(source_origin, patterns, &allow_list_);
+  SetForOrigin(source_origin, patterns, &map_, MapType::kAllowPatterns);
 }
 
 void OriginAccessList::AddAllowListEntryForOrigin(
@@ -27,23 +27,13 @@ void OriginAccessList::AddAllowListEntryForOrigin(
     const mojom::CorsOriginAccessMatchPriority priority) {
   AddForOrigin(source_origin,
                mojom::CorsOriginPattern::New(protocol, domain, mode, priority),
-               &allow_list_);
-}
-
-void OriginAccessList::ClearAllowListForOrigin(
-    const url::Origin& source_origin) {
-  SetForOrigin(source_origin, std::vector<mojom::CorsOriginPatternPtr>(),
-               &allow_list_);
-}
-
-void OriginAccessList::ClearAllowList() {
-  allow_list_.clear();
+               &map_, MapType::kAllowPatterns);
 }
 
 void OriginAccessList::SetBlockListForOrigin(
     const url::Origin& source_origin,
     const std::vector<CorsOriginPatternPtr>& patterns) {
-  SetForOrigin(source_origin, patterns, &block_list_);
+  SetForOrigin(source_origin, patterns, &map_, MapType::kBlockPatterns);
 }
 
 void OriginAccessList::AddBlockListEntryForOrigin(
@@ -54,17 +44,16 @@ void OriginAccessList::AddBlockListEntryForOrigin(
     const mojom::CorsOriginAccessMatchPriority priority) {
   AddForOrigin(source_origin,
                mojom::CorsOriginPattern::New(protocol, domain, mode, priority),
-               &block_list_);
+               &map_, MapType::kBlockPatterns);
 }
 
-void OriginAccessList::ClearBlockListForOrigin(
-    const url::Origin& source_origin) {
-  SetForOrigin(source_origin, std::vector<mojom::CorsOriginPatternPtr>(),
-               &block_list_);
+void OriginAccessList::ClearForOrigin(const url::Origin& source_origin) {
+  DCHECK(!source_origin.opaque());
+  map_.erase(source_origin.Serialize());
 }
 
-void OriginAccessList::ClearBlockList() {
-  block_list_.clear();
+void OriginAccessList::Clear() {
+  map_.clear();
 }
 
 OriginAccessList::AccessState OriginAccessList::CheckAccessState(
@@ -73,56 +62,48 @@ OriginAccessList::AccessState OriginAccessList::CheckAccessState(
   if (source_origin.opaque())
     return AccessState::kBlocked;
 
-  std::string source = source_origin.Serialize();
-  url::Origin destination_origin = url::Origin::Create(destination);
-  network::mojom::CorsOriginAccessMatchPriority allow_list_priority =
-      GetHighestPriorityOfRuleForOrigin(source, destination_origin,
-                                        allow_list_);
-  network::mojom::CorsOriginAccessMatchPriority block_list_priority =
-      GetHighestPriorityOfRuleForOrigin(source, destination_origin,
-                                        block_list_);
-
-  if (allow_list_priority ==
+  const std::string source = source_origin.Serialize();
+  const url::Origin destination_origin = url::Origin::Create(destination);
+  const auto patterns_map_it = map_.find(source);
+  if (patterns_map_it == map_.end())
+    return AccessState::kNotListed;
+
+  const network::mojom::CorsOriginAccessMatchPriority allow_list_priority =
+      GetHighestPriorityOfRuleForOrigin(
+          destination_origin, patterns_map_it->second, MapType::kAllowPatterns);
+  const network::mojom::CorsOriginAccessMatchPriority block_list_priority =
+      GetHighestPriorityOfRuleForOrigin(
+          destination_origin, patterns_map_it->second, MapType::kBlockPatterns);
+  if (block_list_priority ==
       network::mojom::CorsOriginAccessMatchPriority::kNoMatchingOrigin) {
-    return block_list_priority ==
-                   network::mojom::CorsOriginAccessMatchPriority::
-                       kNoMatchingOrigin
+    return (allow_list_priority ==
+            network::mojom::CorsOriginAccessMatchPriority::kNoMatchingOrigin)
                ? AccessState::kNotListed
-               : AccessState::kBlocked;
+               : AccessState::kAllowed;
   }
 
-  if (block_list_priority ==
-      network::mojom::CorsOriginAccessMatchPriority::kNoMatchingOrigin)
-    return AccessState::kAllowed;
-
   return (allow_list_priority > block_list_priority) ? AccessState::kAllowed
                                                      : AccessState::kBlocked;
 }
 
 std::vector<mojo::StructPtr<mojom::CorsOriginAccessPatterns>>
 OriginAccessList::CreateCorsOriginAccessPatternsList() const {
-  std::set<std::string> origins;
-  for (const auto& allow_map : allow_list_)
-    origins.insert(allow_map.first);
-  for (const auto& block_map : block_list_)
-    origins.insert(block_map.first);
-
   std::vector<mojom::CorsOriginAccessPatternsPtr> access_patterns;
-  for (const auto& origin : origins) {
+  for (const auto& it : map_) {
     std::vector<mojom::CorsOriginPatternPtr> allow_patterns;
-    const auto& allow_entries = allow_list_.find(origin);
-    if (allow_entries != allow_list_.end()) {
+    const auto& allow_entries = it.second.find(MapType::kAllowPatterns);
+    if (allow_entries != it.second.end()) {
       for (const auto& pattern : allow_entries->second)
         allow_patterns.push_back(pattern.CreateCorsOriginPattern());
     }
     std::vector<mojom::CorsOriginPatternPtr> block_patterns;
-    const auto& block_entries = block_list_.find(origin);
-    if (block_entries != block_list_.end()) {
+    const auto& block_entries = it.second.find(MapType::kBlockPatterns);
+    if (block_entries != it.second.end()) {
       for (const auto& pattern : block_entries->second)
         block_patterns.push_back(pattern.CreateCorsOriginPattern());
     }
     access_patterns.push_back(mojom::CorsOriginAccessPatterns::New(
-        origin, std::move(allow_patterns), std::move(block_patterns)));
+        it.first, std::move(allow_patterns), std::move(block_patterns)));
   }
   return access_patterns;
 }
@@ -131,32 +112,36 @@ OriginAccessList::CreateCorsOriginAccessPatternsList() const {
 void OriginAccessList::SetForOrigin(
     const url::Origin& source_origin,
     const std::vector<CorsOriginPatternPtr>& patterns,
-    PatternMap* map) {
+    OriginPatternsMap* map,
+    MapType type) {
   DCHECK(map);
   DCHECK(!source_origin.opaque());
 
-  std::string source = source_origin.Serialize();
-  map->erase(source);
-  if (patterns.empty())
-    return;
-
-  Patterns& native_patterns = (*map)[source];
+  const std::string source = source_origin.Serialize();
+  PatternsMap& patterns_map = (*map)[source];
+  Patterns& patterns_for_type = patterns_map[type];
+  patterns_for_type.clear();
   for (const auto& pattern : patterns) {
-    native_patterns.push_back(OriginAccessEntry(
+    patterns_for_type.push_back(OriginAccessEntry(
         pattern->protocol, pattern->domain, pattern->mode, pattern->priority));
   }
+  if (patterns_map[MapType::kAllowPatterns].empty() &&
+      patterns_map[MapType::kBlockPatterns].empty()) {
+    (*map).erase(source);
+  }
 }
 
 // static
 void OriginAccessList::AddForOrigin(const url::Origin& source_origin,
                                     const CorsOriginPatternPtr& pattern,
-                                    PatternMap* map) {
+                                    OriginPatternsMap* map,
+                                    MapType type) {
   DCHECK(map);
   DCHECK(!source_origin.opaque());
 
-  std::string source = source_origin.Serialize();
-  (*map)[source].push_back(OriginAccessEntry(pattern->protocol, pattern->domain,
-                                             pattern->mode, pattern->priority));
+  const std::string source = source_origin.Serialize();
+  (*map)[source][type].push_back(OriginAccessEntry(
+      pattern->protocol, pattern->domain, pattern->mode, pattern->priority));
 }
 
 // static
@@ -164,15 +149,16 @@ void OriginAccessList::AddForOrigin(const url::Origin& source_origin,
 // first match which will be the top priority.
 network::mojom::CorsOriginAccessMatchPriority
 OriginAccessList::GetHighestPriorityOfRuleForOrigin(
-    const std::string& source,
     const url::Origin& destination_origin,
-    const PatternMap& map) {
+    const PatternsMap& patterns_map,
+    MapType type) {
+  const auto patterns_it = patterns_map.find(type);
+  if (patterns_it == patterns_map.end())
+    return network::mojom::CorsOriginAccessMatchPriority::kNoMatchingOrigin;
+
   network::mojom::CorsOriginAccessMatchPriority highest_priority =
       network::mojom::CorsOriginAccessMatchPriority::kNoMatchingOrigin;
-  auto patterns_for_origin_it = map.find(source);
-  if (patterns_for_origin_it == map.end())
-    return highest_priority;
-  for (const auto& entry : patterns_for_origin_it->second) {
+  for (const auto& entry : patterns_it->second) {
     if (entry.MatchesOrigin(destination_origin) !=
         OriginAccessEntry::kDoesNotMatchOrigin) {
       highest_priority = std::max(highest_priority, entry.priority());

services/network/public/cpp/cors/origin_access_list.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/component_export.h"
+#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "services/network/public/cpp/cors/origin_access_entry.h"
 #include "services/network/public/mojom/cors_origin_pattern.mojom-shared.h"
@@ -56,12 +57,6 @@ class COMPONENT_EXPORT(NETWORK_CPP) OriginAccessList {
       const mojom::CorsOriginAccessMatchMode mode,
       const mojom::CorsOriginAccessMatchPriority priority);
 
-  // Clears the old allow list for |source_origin|.
-  void ClearAllowListForOrigin(const url::Origin& source_origin);
-
-  // Clears the old allow list.
-  void ClearAllowList();
-
   // Clears the old block list for |source_origin| and set |patterns| to the
   // block list. When two or more patterns in a list match, the entry with the
   // higher |priority| takes precedence.
@@ -77,11 +72,11 @@ class COMPONENT_EXPORT(NETWORK_CPP) OriginAccessList {
       const mojom::CorsOriginAccessMatchMode mode,
       const mojom::CorsOriginAccessMatchPriority priority);
 
-  // Clears the old block list for |source_origin|.
-  void ClearBlockListForOrigin(const url::Origin& source_origin);
+  // Clears the old allow/block lists for |source_origin|.
+  void ClearForOrigin(const url::Origin& source_origin);
 
-  // Clears the old block list.
-  void ClearBlockList();
+  // Clears the old allow/block lists.
+  void Clear();
 
   // Returns |destination|'s AccessState in the list for |source_origin|.
   AccessState CheckAccessState(const url::Origin& source_origin,
@@ -93,24 +88,29 @@ class COMPONENT_EXPORT(NETWORK_CPP) OriginAccessList {
   CreateCorsOriginAccessPatternsList() const;
 
  private:
+  enum class MapType {
+    kAllowPatterns,
+    kBlockPatterns,
+  };
   using Patterns = std::vector<OriginAccessEntry>;
-  using PatternMap = std::map<std::string, Patterns>;
+  using PatternsMap = base::flat_map<MapType, Patterns>;
+  using OriginPatternsMap =
+      std::map<std::string /* source_origin */, PatternsMap>;
 
   static void SetForOrigin(const url::Origin& source_origin,
                            const std::vector<CorsOriginPatternPtr>& patterns,
-                           PatternMap* map);
+                           OriginPatternsMap* map,
+                           MapType type);
   static void AddForOrigin(const url::Origin& source_origin,
                            const CorsOriginPatternPtr& pattern,
-                           PatternMap* map);
+                           OriginPatternsMap* map,
+                           MapType type);
   static mojom::CorsOriginAccessMatchPriority GetHighestPriorityOfRuleForOrigin(
-      const std::string& source,
       const url::Origin& destination_origin,
-      const PatternMap& map);
+      const PatternsMap& patterns_map,
+      MapType type);
 
-  // TODO(toyoshim): Redesign to have an unified map to be consistent with
-  // mojom::CorsOriginAccessPatterns. See https://crbug.com/908756.
-  PatternMap allow_list_;
-  PatternMap block_list_;
+  OriginPatternsMap map_;
 
   DISALLOW_COPY_AND_ASSIGN(OriginAccessList);
 };

third_party/blink/renderer/platform/weborigin/security_policy.cc

@@ -278,15 +278,12 @@ void SecurityPolicy::AddOriginAccessBlockListEntry(
 void SecurityPolicy::ClearOriginAccessListForOrigin(
     const SecurityOrigin& source_origin) {
   MutexLocker lock(GetMutex());
-  const url::Origin origin = source_origin.ToUrlOrigin();
-  GetOriginAccessList().ClearAllowListForOrigin(origin);
-  GetOriginAccessList().ClearBlockListForOrigin(origin);
+  GetOriginAccessList().ClearForOrigin(source_origin.ToUrlOrigin());
 }
 
 void SecurityPolicy::ClearOriginAccessList() {
   MutexLocker lock(GetMutex());
-  GetOriginAccessList().ClearAllowList();
-  GetOriginAccessList().ClearBlockList();
+  GetOriginAccessList().Clear();
 }
 
 bool SecurityPolicy::ReferrerPolicyFromString(