Refactor WebContentSettingsClient to dedupe AllowXYZ methods

This change consolidates the storage related methods in the
WebContentSettingsClient interface into two new methods to prevent
use of repeated logic in implementations.

Implementations and usage sites have also been updated to use the new
methods, RequestStorageAccess and RequestStorageAccessSync.

Change-Id: If46b3d0ca7555b8db212b014580f0b5513bbe508
Bug: 1019415
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2353552
Commit-Queue: Shubham Aggarwal <shuagga@microsoft.com>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805360}

chrome/renderer/worker_content_settings_client.cc

@@ -59,24 +59,20 @@ WorkerContentSettingsClient::Clone() {
   return base::WrapUnique(new WorkerContentSettingsClient(*this));
 }
 
-bool WorkerContentSettingsClient::RequestFileSystemAccessSync() {
-  return AllowStorageAccess(content_settings::mojom::ContentSettingsManager::
-                                StorageType::FILE_SYSTEM);
-}
-
-bool WorkerContentSettingsClient::AllowIndexedDB() {
-  return AllowStorageAccess(
-      content_settings::mojom::ContentSettingsManager::StorageType::INDEXED_DB);
-}
+bool WorkerContentSettingsClient::AllowStorageAccessSync(
+    StorageType storage_type) {
+  if (is_unique_origin_)
+    return false;
 
-bool WorkerContentSettingsClient::AllowCacheStorage() {
-  return AllowStorageAccess(
-      content_settings::mojom::ContentSettingsManager::StorageType::CACHE);
-}
+  EnsureContentSettingsManager();
 
-bool WorkerContentSettingsClient::AllowWebLocks() {
-  return AllowStorageAccess(
-      content_settings::mojom::ContentSettingsManager::StorageType::WEB_LOCKS);
+  bool result = false;
+  content_settings_manager_->AllowStorageAccess(
+      render_frame_id_,
+      content_settings::ContentSettingsAgentImpl::ConvertToMojoStorageType(
+          storage_type),
+      document_origin_, site_for_cookies_, top_frame_origin_, &result);
+  return result;
 }
 
 bool WorkerContentSettingsClient::AllowRunningInsecureContent(
@@ -128,20 +124,6 @@ bool WorkerContentSettingsClient::ShouldAutoupgradeMixedContent() {
   return false;
 }
 
-bool WorkerContentSettingsClient::AllowStorageAccess(
-    content_settings::mojom::ContentSettingsManager::StorageType storage_type) {
-  if (is_unique_origin_)
-    return false;
-
-  EnsureContentSettingsManager();
-
-  bool result = false;
-  content_settings_manager_->AllowStorageAccess(
-      render_frame_id_, storage_type, document_origin_, site_for_cookies_,
-      top_frame_origin_, &result);
-  return result;
-}
-
 void WorkerContentSettingsClient::EnsureContentSettingsManager() const {
   // Lazily bind |content_settings_manager_| so it is bound on the right thread.
   if (content_settings_manager_)

chrome/renderer/worker_content_settings_client.h

@@ -29,10 +29,7 @@ class WorkerContentSettingsClient : public blink::WebContentSettingsClient {
 
   // WebContentSettingsClient overrides.
   std::unique_ptr<blink::WebContentSettingsClient> Clone() override;
-  bool RequestFileSystemAccessSync() override;
-  bool AllowIndexedDB() override;
-  bool AllowCacheStorage() override;
-  bool AllowWebLocks() override;
+  bool AllowStorageAccessSync(StorageType storage_type) override;
   bool AllowRunningInsecureContent(bool allowed_per_settings,
                                    const blink::WebURL& url) override;
   bool AllowScriptFromSource(bool enabled_per_settings,
@@ -42,9 +39,6 @@ class WorkerContentSettingsClient : public blink::WebContentSettingsClient {
  private:
   explicit WorkerContentSettingsClient(
       const WorkerContentSettingsClient& other);
-  bool AllowStorageAccess(
-      content_settings::mojom::ContentSettingsManager::StorageType
-          storage_type);
   void EnsureContentSettingsManager() const;
 
   // Loading document context for this worker.

components/content_settings/renderer/content_settings_agent_impl.cc

@@ -260,12 +260,29 @@ void ContentSettingsAgentImpl::OnContentSettingsAgentRequest(
   receivers_.Add(this, std::move(receiver));
 }
 
-bool ContentSettingsAgentImpl::AllowDatabase() {
-  return AllowStorageAccess(
-      mojom::ContentSettingsManager::StorageType::DATABASE);
+mojom::ContentSettingsManager::StorageType
+ContentSettingsAgentImpl::ConvertToMojoStorageType(StorageType storage_type) {
+  switch (storage_type) {
+    case StorageType::kDatabase: {
+      return mojom::ContentSettingsManager::StorageType::DATABASE;
+    }
+    case StorageType::kIndexedDB: {
+      return mojom::ContentSettingsManager::StorageType::INDEXED_DB;
+    }
+    case StorageType::kCacheStorage: {
+      return mojom::ContentSettingsManager::StorageType::CACHE;
+    }
+    case StorageType::kWebLocks: {
+      return mojom::ContentSettingsManager::StorageType::WEB_LOCKS;
+    }
+    case StorageType::kFileSystem: {
+      return mojom::ContentSettingsManager::StorageType::FILE_SYSTEM;
+    }
+  }
 }
 
-void ContentSettingsAgentImpl::RequestFileSystemAccessAsync(
+void ContentSettingsAgentImpl::AllowStorageAccess(
+    StorageType storage_type,
     base::OnceCallback<void(bool)> callback) {
   WebLocalFrame* frame = render_frame()->GetWebFrame();
   if (IsFrameWithOpaqueOrigin(frame)) {
@@ -274,12 +291,27 @@ void ContentSettingsAgentImpl::RequestFileSystemAccessAsync(
   }
 
   GetContentSettingsManager().AllowStorageAccess(
-      routing_id(), mojom::ContentSettingsManager::StorageType::FILE_SYSTEM,
+      routing_id(), ConvertToMojoStorageType(storage_type),
       frame->GetSecurityOrigin(),
       frame->GetDocument().SiteForCookies().RepresentativeUrl(),
       frame->GetDocument().TopFrameOrigin(), std::move(callback));
 }
 
+bool ContentSettingsAgentImpl::AllowStorageAccessSync(
+    StorageType storage_type) {
+  WebLocalFrame* frame = render_frame()->GetWebFrame();
+  if (IsFrameWithOpaqueOrigin(frame))
+    return false;
+
+  bool result = false;
+  GetContentSettingsManager().AllowStorageAccess(
+      routing_id(), ConvertToMojoStorageType(storage_type),
+      frame->GetSecurityOrigin(),
+      frame->GetDocument().SiteForCookies().RepresentativeUrl(),
+      frame->GetDocument().TopFrameOrigin(), &result);
+  return result;
+}
+
 bool ContentSettingsAgentImpl::AllowImage(bool enabled_per_settings,
                                           const WebURL& image_url) {
   bool allow = enabled_per_settings;
@@ -301,20 +333,6 @@ bool ContentSettingsAgentImpl::AllowImage(bool enabled_per_settings,
   return allow;
 }
 
-bool ContentSettingsAgentImpl::AllowIndexedDB() {
-  return AllowStorageAccess(
-      mojom::ContentSettingsManager::StorageType::INDEXED_DB);
-}
-
-bool ContentSettingsAgentImpl::AllowCacheStorage() {
-  return AllowStorageAccess(mojom::ContentSettingsManager::StorageType::CACHE);
-}
-
-bool ContentSettingsAgentImpl::AllowWebLocks() {
-  return AllowStorageAccess(
-      mojom::ContentSettingsManager::StorageType::WEB_LOCKS);
-}
-
 bool ContentSettingsAgentImpl::AllowScript(bool enabled_per_settings) {
   if (!enabled_per_settings)
     return false;
@@ -494,18 +512,4 @@ bool ContentSettingsAgentImpl::IsWhitelistedForContentSettings() const {
   return false;
 }
 
-bool ContentSettingsAgentImpl::AllowStorageAccess(
-    mojom::ContentSettingsManager::StorageType storage_type) {
-  WebLocalFrame* frame = render_frame()->GetWebFrame();
-  if (IsFrameWithOpaqueOrigin(frame))
-    return false;
-
-  bool result = false;
-  GetContentSettingsManager().AllowStorageAccess(
-      routing_id(), storage_type, frame->GetSecurityOrigin(),
-      frame->GetDocument().SiteForCookies().RepresentativeUrl(),
-      frame->GetDocument().TopFrameOrigin(), &result);
-  return result;
-}
-
 }  // namespace content_settings

components/content_settings/renderer/content_settings_agent_impl.h

@@ -78,15 +78,16 @@ class ContentSettingsAgentImpl
   // Sends an IPC notification that the specified content type was blocked.
   void DidBlockContentType(ContentSettingsType settings_type);
 
+  // Helper to convert StorageType to its Mojo counterpart.
+  static mojom::ContentSettingsManager::StorageType ConvertToMojoStorageType(
+      StorageType storage_type);
+
   // blink::WebContentSettingsClient:
-  bool AllowDatabase() override;
-  void RequestFileSystemAccessAsync(
-      base::OnceCallback<void(bool)> callback) override;
+  void AllowStorageAccess(StorageType storage_type,
+                          base::OnceCallback<void(bool)> callback) override;
+  bool AllowStorageAccessSync(StorageType type) override;
   bool AllowImage(bool enabled_per_settings,
                   const blink::WebURL& image_url) override;
-  bool AllowIndexedDB() override;
-  bool AllowCacheStorage() override;
-  bool AllowWebLocks() override;
   bool AllowScript(bool enabled_per_settings) override;
   bool AllowScriptFromSource(bool enabled_per_settings,
                              const blink::WebURL& script_url) override;
@@ -148,10 +149,6 @@ class ContentSettingsAgentImpl
   // settings.
   bool IsWhitelistedForContentSettings() const;
 
-  // Common logic for AllowIndexedDB, AllowCacheStorage, etc.
-  bool AllowStorageAccess(
-      mojom::ContentSettingsManager::StorageType storage_type);
-
   // A getter for |content_settings_manager_| that ensures it is bound.
   mojom::ContentSettingsManager& GetContentSettingsManager();
 

third_party/blink/public/platform/web_content_settings_client.h

@@ -24,32 +24,30 @@ class WebContentSettingsClient {
   // this WebContentSettingsClient so it can be used by another worker thread.
   virtual std::unique_ptr<WebContentSettingsClient> Clone() { return nullptr; }
 
-  // Controls whether access to Web Databases is allowed for this frame.
-  virtual bool AllowDatabase() { return true; }
-
-  // Controls whether access to File System is allowed for this frame.
-  virtual bool RequestFileSystemAccessSync() { return true; }
-
-  // Controls whether access to File System is allowed for this frame.
-  virtual void RequestFileSystemAccessAsync(
-      base::OnceCallback<void(bool)> callback) {
+  enum class StorageType {
+    kDatabase,
+    kCacheStorage,
+    kIndexedDB,
+    kFileSystem,
+    kWebLocks
+  };
+
+  // Controls whether access to the given StorageType is allowed for this frame.
+  // Runs asynchronously.
+  virtual void AllowStorageAccess(StorageType storage_type,
+                                  base::OnceCallback<void(bool)> callback) {
     std::move(callback).Run(true);
   }
 
+  // Controls whether access to the given StorageType is allowed for this frame.
+  // Blocks until done.
+  virtual bool AllowStorageAccessSync(StorageType storage_type) { return true; }
+
   // Controls whether images are allowed for this frame.
   virtual bool AllowImage(bool enabled_per_settings, const WebURL& image_url) {
     return enabled_per_settings;
   }
 
-  // Controls whether access to Indexed DB are allowed for this frame.
-  virtual bool AllowIndexedDB() { return true; }
-
-  // Controls whether access to CacheStorage is allowed for this frame.
-  virtual bool AllowCacheStorage() { return true; }
-
-  // Controls whether access to Web Locks is allowed for this frame.
-  virtual bool AllowWebLocks() { return true; }
-
   // Controls whether scripts are allowed to execute for this frame.
   virtual bool AllowScript(bool enabled_per_settings) {
     return enabled_per_settings;

third_party/blink/renderer/core/workers/shared_worker_content_settings_proxy.cc

@@ -15,27 +15,32 @@ SharedWorkerContentSettingsProxy::SharedWorkerContentSettingsProxy(
     : host_info_(std::move(host_info)) {}
 SharedWorkerContentSettingsProxy::~SharedWorkerContentSettingsProxy() = default;
 
-bool SharedWorkerContentSettingsProxy::AllowIndexedDB() {
+bool SharedWorkerContentSettingsProxy::AllowStorageAccessSync(
+    StorageType storage_type) {
   bool result = false;
-  GetService()->AllowIndexedDB(&result);
-  return result;
-}
-
-bool SharedWorkerContentSettingsProxy::AllowCacheStorage() {
-  bool result = false;
-  GetService()->AllowCacheStorage(&result);
-  return result;
-}
-
-bool SharedWorkerContentSettingsProxy::AllowWebLocks() {
-  bool result = false;
-  GetService()->AllowWebLocks(&result);
-  return result;
-}
+  switch (storage_type) {
+    case StorageType::kIndexedDB: {
+      GetService()->AllowIndexedDB(&result);
+      break;
+    }
+    case StorageType::kCacheStorage: {
+      GetService()->AllowCacheStorage(&result);
+      break;
+    }
+    case StorageType::kWebLocks: {
+      GetService()->AllowWebLocks(&result);
+      break;
+    }
+    case StorageType::kFileSystem: {
+      GetService()->RequestFileSystemAccessSync(&result);
+      break;
+    }
+    case StorageType::kDatabase: {
+      // TODO(shuagga@microsoft.com): Revisit this default in the future.
+      return true;
+    }
+  }
 
-bool SharedWorkerContentSettingsProxy::RequestFileSystemAccessSync() {
-  bool result = false;
-  GetService()->RequestFileSystemAccessSync(&result);
   return result;
 }
 

third_party/blink/renderer/core/workers/shared_worker_content_settings_proxy.h

@@ -24,10 +24,7 @@ class SharedWorkerContentSettingsProxy : public WebContentSettingsClient {
   ~SharedWorkerContentSettingsProxy() override;
 
   // WebContentSettingsClient overrides.
-  bool AllowIndexedDB() override;
-  bool AllowCacheStorage() override;
-  bool AllowWebLocks() override;
-  bool RequestFileSystemAccessSync() override;
+  bool AllowStorageAccessSync(StorageType storage_type) override;
 
  private:
   // To ensure the returned pointer is destructed on the same thread

third_party/blink/renderer/modules/cache_storage/cache_storage.cc

@@ -76,7 +76,10 @@ bool IsCacheStorageAllowed(ScriptState* script_state) {
     settings_client = To<WorkerGlobalScope>(context)->ContentSettingsClient();
 
   // This triggers a sync IPC.
-  return settings_client ? settings_client->AllowCacheStorage() : true;
+  return settings_client
+             ? settings_client->AllowStorageAccessSync(
+                   WebContentSettingsClient::StorageType::kCacheStorage)
+             : true;
 }
 
 }  // namespace

third_party/blink/renderer/modules/filesystem/local_file_system.cc

@@ -107,7 +107,9 @@ void LocalFileSystem::RequestFileSystemAccessInternal(
     if (!client) {
       std::move(callback).Run(true);
     } else {
-      client->RequestFileSystemAccessAsync(std::move(callback));
+      client->AllowStorageAccess(
+          WebContentSettingsClient::StorageType::kFileSystem,
+          std::move(callback));
     }
     return;
   }
@@ -116,7 +118,8 @@ void LocalFileSystem::RequestFileSystemAccessInternal(
     if (!client) {
       std::move(callback).Run(true);
     } else {
-      std::move(callback).Run(client->RequestFileSystemAccessSync());
+      std::move(callback).Run(client->AllowStorageAccessSync(
+          WebContentSettingsClient::StorageType::kFileSystem));
     }
     return;
   }

third_party/blink/renderer/modules/indexeddb/idb_factory.cc

@@ -491,7 +491,8 @@ bool IDBFactory::AllowIndexedDB(ScriptState* script_state) {
       return false;
     if (auto* settings_client = frame->GetContentSettingsClient()) {
       // This triggers a sync IPC.
-      return settings_client->AllowIndexedDB();
+      return settings_client->AllowStorageAccessSync(
+          WebContentSettingsClient::StorageType::kIndexedDB);
     }
     return true;
   }
@@ -501,7 +502,8 @@ bool IDBFactory::AllowIndexedDB(ScriptState* script_state) {
   if (!content_settings_client)
     return true;
   // This triggers a sync IPC.
-  return content_settings_client->AllowIndexedDB();
+  return content_settings_client->AllowStorageAccessSync(
+      WebContentSettingsClient::StorageType::kIndexedDB);
 }
 
 bool IDBFactory::CachedAllowIndexedDB(ScriptState* script_state) {

third_party/blink/renderer/modules/locks/lock_manager.cc

@@ -448,7 +448,8 @@ bool LockManager::AllowLocks(ScriptState* script_state) {
         cached_allowed_ = false;
       } else if (auto* settings_client = frame->GetContentSettingsClient()) {
         // This triggers a sync IPC.
-        cached_allowed_ = settings_client->AllowWebLocks();
+        cached_allowed_ = settings_client->AllowStorageAccessSync(
+            WebContentSettingsClient::StorageType::kWebLocks);
       } else {
         cached_allowed_ = true;
       }
@@ -459,7 +460,8 @@ bool LockManager::AllowLocks(ScriptState* script_state) {
         cached_allowed_ = true;
       } else {
         // This triggers a sync IPC.
-        cached_allowed_ = content_settings_client->AllowWebLocks();
+        cached_allowed_ = content_settings_client->AllowStorageAccessSync(
+            WebContentSettingsClient::StorageType::kWebLocks);
       }
     }
   }

third_party/blink/renderer/modules/service_worker/service_worker_content_settings_proxy.cc

@@ -15,15 +15,19 @@ ServiceWorkerContentSettingsProxy::ServiceWorkerContentSettingsProxy(
 ServiceWorkerContentSettingsProxy::~ServiceWorkerContentSettingsProxy() =
     default;
 
-bool ServiceWorkerContentSettingsProxy::RequestFileSystemAccessSync() {
-  NOTREACHED();
-  return false;
-}
-
-bool ServiceWorkerContentSettingsProxy::AllowIndexedDB() {
+bool ServiceWorkerContentSettingsProxy::AllowStorageAccessSync(
+    StorageType storage_type) {
   bool result = false;
-  GetService()->AllowIndexedDB(&result);
-  return result;
+  if (storage_type == StorageType::kIndexedDB) {
+    GetService()->AllowIndexedDB(&result);
+    return result;
+  } else if (storage_type == StorageType::kFileSystem) {
+    NOTREACHED();
+    return false;
+  } else {
+    // TODO(shuagga@microsoft.com): Revisit this default in the future.
+    return true;
+  }
 }
 
 // Use ThreadSpecific to ensure that |content_settings_instance_host| is

third_party/blink/renderer/modules/service_worker/service_worker_content_settings_proxy.h

@@ -29,8 +29,7 @@ class ServiceWorkerContentSettingsProxy final
   // WebContentSettingsClient overrides.
   // Asks the browser process about the settings.
   // Blocks until the response arrives.
-  bool RequestFileSystemAccessSync() override;
-  bool AllowIndexedDB() override;
+  bool AllowStorageAccessSync(StorageType storage_type) override;
 
  private:
   // To ensure the returned pointer is destructed on the same thread

third_party/blink/renderer/modules/webdatabase/database_client.cc

@@ -60,8 +60,10 @@ const char DatabaseClient::kSupplementName[] = "DatabaseClient";
 bool DatabaseClient::AllowDatabase(ExecutionContext* context) {
   DCHECK(context->IsContextThread());
   LocalDOMWindow* window = To<LocalDOMWindow>(context);
-  if (auto* client = window->GetFrame()->GetContentSettingsClient())
-    return client->AllowDatabase();
+  if (auto* client = window->GetFrame()->GetContentSettingsClient()) {
+    return client->AllowStorageAccessSync(
+        WebContentSettingsClient::StorageType::kDatabase);
+  }
   return true;
 }