Implement origin specific Permissions Blacklisting.

chrome/browser/permissions/permission_context_base.cc

Index: chrome/browser/permissions/permission_context_base.cc
diff --git a/chrome/browser/permissions/permission_context_base.cc b/chrome/browser/permissions/permission_context_base.cc
index 5c99fbd3668796176b5a1b073fa2f16d013f9c3b..a461ab34566cc6c76f4987f5061c96c7e50e8521 100644
--- a/chrome/browser/permissions/permission_context_base.cc
+++ b/chrome/browser/permissions/permission_context_base.cc
@@ -5,6 +5,9 @@
 #include "chrome/browser/permissions/permission_context_base.h"
 
 #include <stddef.h>
+
+#include <set>
+#include <string>
 #include <utility>
 
 #include "base/callback.h"
@@ -12,6 +15,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/permissions/permission_decision_auto_blocker.h"
 #include "chrome/browser/permissions/permission_request.h"
@@ -21,10 +25,13 @@
 #include "chrome/browser/permissions/permission_uma_util.h"
 #include "chrome/browser/permissions/permission_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/browser/website_settings_registry.h"
 #include "components/prefs/pref_service.h"
+#include "components/safe_browsing_db/database_manager.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
@@ -43,6 +50,54 @@ const char PermissionContextBase::kPermissionsKillSwitchFieldStudy[] =
 const char PermissionContextBase::kPermissionsKillSwitchBlockedValue[] =
     "blocked";
 
+// The client used when checking whether a permission has been blacklisted by
+// Safe Browsing. The check is done asynchronously as no state can be stored in
+// PermissionContextBase while it is in flight (since additional permission
+// requests may be made). Hence, the client is heap allocated and is responsible
+// for deleting itself when it is finished.
+class PermissionsBlacklistSBClientImpl
+    : public safe_browsing::SafeBrowsingDatabaseManager::Client {
+ public:
+  PermissionsBlacklistSBClientImpl(
+      content::PermissionType permission_type,
+      const GURL& request_origin,
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      base::Callback<void(bool)> callback)
+      : permission_type_(permission_type), callback_(callback) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&PermissionsBlacklistSBClientImpl::StartCheck,
+                   base::Unretained(this), db_manager, request_origin));
+  }
+
+ private:
+  void StartCheck(

[Nathan Parker, 2016/12/12 19:02:25]

Do this code properly handle the case where the webcontents/etc is deleted
before the call back is done?

[meredithl, 2016/12/15 00:15:44]

Done.

[meredithl, 2016/12/15 00:15:45]

Updated the client to inherit from WebContentsObserver.

+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      const GURL& request_origin) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    db_manager->CheckApiBlacklistUrl(request_origin, this);
+  }
+
+  void OnCheckApiBlacklistUrlResult(
+      const GURL& url,
+      const safe_browsing::ThreatMetadata& metadata) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    bool permission_blocked =
+        metadata.api_permissions.find(PermissionUtil::GetPermissionString(
+            permission_type_)) != metadata.api_permissions.end();
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                     base::Bind(callback_, permission_blocked));
+    // The result has been received, so the object can now delete itself.
+    delete this;

[Nathan Parker, 2016/12/12 19:02:25]

If you're going to have this obj own itself, you should more clearly define the
ownership. 

One idea: How about making the constructor private, and making a static method
to kick off one of these requests?  That way you avoid the possibility that
someone will create one of these on the stack and then have it call delete.

[meredithl, 2016/12/15 00:15:45]

Done.

+  }
+
+  ~PermissionsBlacklistSBClientImpl() override {}
+
+  content::PermissionType permission_type_;
+  base::Callback<void(bool)> callback_;
+  // add timer as member
+};
+
 PermissionContextBase::PermissionContextBase(
     Profile* profile,
     const content::PermissionType permission_type,
@@ -101,12 +156,58 @@ void PermissionContextBase::RequestPermission(
     return;
   }
 
+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> database_manager =
+      GetSafeBrowsingDatabaseManager();
+  if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
+      database_manager) {
+    // The client will contact Safe Browsing, and invoke the callback with the
+    // result. This object will be freed once Safe Browsing has returned the
+    // results.
+    // TODO(meredithl): Check if Safe Browsing Service has timed out
+    new PermissionsBlacklistSBClientImpl(

[Nathan Parker, 2016/12/12 19:02:25]

A new'd bare pointer is kind of a red flag to me here.

[kcarattini, 2016/12/13 01:49:51]

I don't disagree with you. Could PermissionContextBase just keep track of its
in-progress SB checks like SB database manager does?

[meredithl, 2016/12/15 00:15:44]

We went with the static request to instantiate a new client for the request
through a private constructor, so this has been removed.

[meredithl, 2016/12/15 00:15:45]

Dom and I decided it was better to encapsulate as much as possible inside the
client, rather than having to store anything in the PermissionContextBase.

+        permission_type_, requesting_origin, database_manager,
+        base::Bind(&PermissionContextBase::CheckPermissionsBlacklistResult,
+                   base::Unretained(this), web_contents, id, requesting_origin,
+                   embedding_origin, user_gesture, callback));
+  } else {
+    // TODO(meredithl) : add UMA metrics here.
+    CheckPermissionsBlacklistResult(web_contents, id, requesting_origin,
+                                    embedding_origin, user_gesture, callback,
+                                    false);
+  }
+}
+
+void PermissionContextBase::CheckPermissionsBlacklistResult(
+    content::WebContents* web_contents,
+    const PermissionRequestID& id,
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    bool user_gesture,
+    const BrowserPermissionCallback& callback,
+    bool permission_blocked) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (permission_blocked) {
+    // TODO(meredithl) : add UMA metrics here.
+    web_contents->GetMainFrame()->AddMessageToConsole(
+        content::CONSOLE_MESSAGE_LEVEL_LOG,
+        base::StringPrintf(
+            "%s permission has been auto-blocked.",
+            PermissionUtil::GetPermissionString(permission_type_).c_str()));
+    // Permission has been blacklisted, block the request.
+    // TODO(meredithl) : consider setting the content setting and persisting the
+    // decision to block.
+    callback.Run(CONTENT_SETTING_BLOCK);
+    return;
+  }
+
+  // Site is not blacklisted by Safe Browsing for the requested permission.
   ContentSetting content_setting =
       GetPermissionStatus(requesting_origin, embedding_origin);
   if (content_setting == CONTENT_SETTING_ALLOW) {
     HostContentSettingsMapFactory::GetForProfile(profile_)->UpdateLastUsage(
         requesting_origin, embedding_origin, content_settings_type_);
   }
+
   if (content_setting == CONTENT_SETTING_ALLOW ||
       content_setting == CONTENT_SETTING_BLOCK) {
     NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
@@ -318,3 +419,10 @@ bool PermissionContextBase::IsPermissionKillSwitchOn() const {
 
   return param == kPermissionsKillSwitchBlockedValue;
 }
+
+scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+PermissionContextBase::GetSafeBrowsingDatabaseManager() {
+  safe_browsing::SafeBrowsingService* sb_service =
+      g_browser_process->safe_browsing_service();
+  return sb_service ? sb_service->database_manager() : nullptr;
+}