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..f3a5929cddee35e464f48a7004cae41fe68f55b3 100644
--- a/chrome/browser/permissions/permission_context_base.cc
+++ b/chrome/browser/permissions/permission_context_base.cc
@@ -5,13 +5,19 @@
 #include "chrome/browser/permissions/permission_context_base.h"
 
 #include <stddef.h>
+
+#include <set>
+#include <string>
 #include <utility>
 
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"

[raymes, 2016/12/19 00:07:41]

nit: this one is included in the header so not required here

[meredithl, 2016/12/20 07:38:26]

Done.

 #include "base/strings/stringprintf.h"
+#include "base/timer/timer.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,14 +27,18 @@
 #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"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/origin_util.h"
 #include "url/gurl.h"
 
@@ -42,6 +52,96 @@ const char PermissionContextBase::kPermissionsKillSwitchFieldStudy[] =
 // static
 const char PermissionContextBase::kPermissionsKillSwitchBlockedValue[] =
     "blocked";
+// Maximum time in milliseconds to wait for safe browsing service to check a
+// url for blacklisting. After this amount of time, the check will be aborted
+// and the url will be treated as not blacklisted.
+const int kCheckUrlTimeoutMs = 1000;

[Nathan Parker, 2016/12/15 18:15:12]

This might be a bit short.  Make sure there's (eventually) an UMA metric to show
often this is hit.

[meredithl, 2016/12/16 00:10:39]

Acknowledged.

[kcarattini, 2016/12/19 06:37:45]

Just wondering how you arrived at 1000ms? What about a bit longer, say 3s or 5s
(I think this was the standard v3 timeout)?

Please also add this case to the bug you create for UMA metrics.

[meredithl, 2016/12/20 07:38:26]

This was just a value that Dominick decided on, the idea being that having to
wait 1sec or more to see a permission prompt if the requesting origin isn't
blacklisted could be confusing.

+
+// 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. Associated with a particular
+// WebContents, so it can still delete itself when the WebContents is destroyed.
+// A weak pointer factory is used to generate 'this' pointers for callbacks.
+class PermissionsBlacklistSBClientImpl
+    : public safe_browsing::SafeBrowsingDatabaseManager::Client,
+      public content::WebContentsObserver {
+ public:
+  static void CheckSafeBrowsingBlacklist(
+      content::PermissionType permission_type,
+      const GURL& request_origin,
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      content::WebContents* web_contents,
+      base::Callback<void(bool)> callback) {
+    new PermissionsBlacklistSBClientImpl(permission_type, request_origin,
+                                         db_manager, web_contents, callback);
+  }
+
+ private:
+  PermissionsBlacklistSBClientImpl(
+      content::PermissionType permission_type,
+      const GURL& request_origin,
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      content::WebContents* web_contents,
+      base::Callback<void(bool)> callback)
+      : content::WebContentsObserver(web_contents),
+        permission_type_(permission_type),
+        callback_(callback),
+        weak_ptr_factory_(this) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&PermissionsBlacklistSBClientImpl::StartCheck,
+                   weak_ptr_factory_.GetWeakPtr(), db_manager, request_origin));
+  }
+
+  ~PermissionsBlacklistSBClientImpl() override {}
+
+  void StartCheck(
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      const GURL& request_origin) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    // Start the timer to interrupt into the client callback method with an
+    // empty response if Safe Browsing times out.
+    safe_browsing::ThreatMetadata empty_metadata;
+    timer_.Start(
+        FROM_HERE, base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs),
+        base::Bind(
+            &PermissionsBlacklistSBClientImpl::OnCheckApiBlacklistUrlResult,
+            weak_ptr_factory_.GetWeakPtr(), request_origin, empty_metadata));
+    db_manager->CheckApiBlacklistUrl(request_origin, this);
+  }
+
+  // SafeBrowsingDatabaseManager::Client implementation.
+  void OnCheckApiBlacklistUrlResult(
+      const GURL& url,
+      const safe_browsing::ThreatMetadata& metadata) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    weak_ptr_factory_.InvalidateWeakPtrs();
+    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));

[raymes, 2016/12/19 00:07:41]

I think there might be a few threading issues here that we need to take care of:
-It recently came up that we need to be careful of how weak ptrs are used across
threads. I think the current behavior may be racey because deletion of this
class could happen on a different thread to the one that weak ptrs are
dereferenced on. 
-We're calling delete from 2 different threads, which could be racey (e.g.
delete could get called twice)
-Similarly, the callback could get posted to the UI thread after the WebContents
has been destroyed, in which case it would get run but with a deleted
WebContents

I think we should probably come back to the UI thread inside the class and check
whether the WebContents is destroyed prior to running |callback_| and deleting
this class. That will also help ensure this class is always created/deleted on
the same thread.

I don't think that would solve the WeakPtr issue though. I see 2 ways forward
with that:
1) Use RefCountedThreadsafe for this class, which would ensure it lives until
its outstanding callbacks are finished. This might be slightly simpler.
2) We don't really need to pass weak ptrs to the timer or the sb client, because
they both allow cancelling outstanding requests. So we can ensure that they're
not holding references to this class when it gets deleted. 

In both cases, if WebContentsDestroyed is called, I think we may want to go back
to the IO thread and cancel the outstanding requests so they're not left
hanging.

[meredithl, 2016/12/20 07:38:27]

Done. 
Removed weak pointers and instead using RefCountedThreadSafe.
The client now returns to the UI thread to check if the web contents is still
active, and if so it will run the callback.
I'm not too sure of the part about returning to the IO thread to cancel
outstanding requests though? Can you elaborate?

[raymes, 2016/12/20 23:58:56]

I don't think the last bit is too important. It would just be an optimization to
cancel the existing requests if the WebContents gets destroyed. But it's
probably fine to rely on the timer finishing.

[meredithl, 2016/12/29 06:23:35]

Ohh I see. So if WebContents goes away, call the CancelApiCheck function for
this client. In that case a reference to the db manager would need to be stored
in the client class, at the moment I just pass it in when the check is actually
started. Is that okay?

+    // The result has been received, so the object can now delete itself.
+    delete this;
+  }
+
+  // WebContentsObserver implementation. WebContents should outlive the
+  // permission request, however if it doesn't, this will ensure the object
+  // is still freed properly.

[raymes, 2016/12/19 00:07:41]

Hmm what does it mean that the WebContents should outlive the permission
request? We might want to clarify a bit. My understanding here is that if the
WebContents is destroyed, we cancel the permission requests and don't run
|callback_|. The request is no longer valid at that point. Is that your
understanding also? 

[meredithl, 2016/12/20 07:38:27]

Just a bad comment I think! Fixed.

+  void WebContentsDestroyed() override { delete this; }
+
+  content::PermissionType permission_type_;
+  base::Callback<void(bool)> callback_;
+  // Timer to abort the Safe Browsing check if it takes too long.
+  base::OneShotTimer timer_;

[raymes, 2016/12/19 00:07:41]

It can be helpful to document which threads different members are used on in
multithreaded classes to help defend against racey access.

[meredithl, 2016/12/20 07:38:26]

Done.

+  // Note: Factory remains last member so it is destroyed and any weak
+  // pointers invalidated before other members are destroyed.
+  base::WeakPtrFactory<PermissionsBlacklistSBClientImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PermissionsBlacklistSBClientImpl);
+};
 
 PermissionContextBase::PermissionContextBase(
     Profile* profile,
@@ -101,12 +201,57 @@ 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 or timed out.

[raymes, 2016/12/19 00:07:41]

We're not allocating the object here anymore so we probably don't need that part
of the comment here. But it may be worth noting why it's safe to pass
base::Unretained(this) in here. My understanding is that the callback passed in
will not be run if the WebContents is destroyed while it's outstanding.
PermissionContext is tied to the Profile which should always outlive the
WebContents. Do you agree?

[meredithl, 2016/12/20 07:38:26]

Yep. I've updated the comment.

+    PermissionsBlacklistSBClientImpl::CheckSafeBrowsingBlacklist(
+        permission_type_, requesting_origin, database_manager, web_contents,
+        base::Bind(&PermissionContextBase::CheckPermissionsBlacklistResult,
+                   base::Unretained(this), web_contents, id, requesting_origin,
+                   embedding_origin, user_gesture, callback));
+  } else {
+    // TODO(meredithl) : add UMA metrics here.

[raymes, 2016/12/19 00:07:41]

nit: It might be better to remove this TODO (and the one below about metrics) as
I'm not sure if they give any useful information to someone reading the code. We
could track the reminder for ourselves in a bug instead.

[meredithl, 2016/12/20 07:38:26]

Okay. Should I leave them in for now until a bug gets filed (if one hasn't
already)?

+    CheckPermissionsBlacklistResult(web_contents, id, requesting_origin,
+                                    embedding_origin, user_gesture, callback,
+                                    false);

[raymes, 2016/12/19 00:07:41]

nit: We tend to document bool arguments so that it's clear to readers what
they're for, e.g.
false /* permission_blocked */);

[meredithl, 2016/12/20 07:38:26]

Done.

+  }
+}
+
+void PermissionContextBase::CheckPermissionsBlacklistResult(

[raymes, 2016/12/19 00:07:41]

very nitty: this does more than just check the permission blacklist result, so
it might be worth naming it something more general, like
ContinueRequestPermission (other ideas welcome too :). The bool parameter can be
called permission_blacklisted or something to make it clear that part is coming
from the blacklist.

[meredithl, 2016/12/20 07:38:27]

Done.

+    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);

[raymes, 2016/12/19 00:07:41]

I think we might want to call NotifyPermissionSet here so that we update the tab
context correctly.

[kcarattini, 2016/12/19 06:37:45]

Won't that result in a page action icon being shown? That's why it's not called
for the kill switch. Either way the two should be consistent.

[meredithl, 2016/12/20 07:38:27]

For now I'll leave it out for consistency. Is there any downside to it not being
run?

[raymes, 2016/12/20 23:58:56]

I think this case is different to the kill switch because we want to allow the
user to override the setting. Do we actually not want to show the page action in
that case?

[meredithl, 2016/12/29 06:23:35]

I think that makes sense. The kill switch is like the ban hammer, blacklisting
is more of a suggested setting which the user can change. Is more UI bad in this
case?

+    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,
@@ -124,7 +269,6 @@ void PermissionContextBase::RequestPermission(
 ContentSetting PermissionContextBase::GetPermissionStatus(
     const GURL& requesting_origin,
     const GURL& embedding_origin) const {
-
   // If the permission has been disabled through Finch, block all requests.
   if (IsPermissionKillSwitchOn())
     return CONTENT_SETTING_BLOCK;
@@ -139,9 +283,8 @@ ContentSetting PermissionContextBase::GetPermissionStatus(
                           content_settings_type_, std::string());
 }
 
-void PermissionContextBase::ResetPermission(
-    const GURL& requesting_origin,
-    const GURL& embedding_origin) {
+void PermissionContextBase::ResetPermission(const GURL& requesting_origin,
+                                            const GURL& embedding_origin) {
   HostContentSettingsMapFactory::GetForProfile(profile_)
       ->SetContentSettingDefaultScope(requesting_origin, embedding_origin,
                                       content_settings_type_, std::string(),
@@ -181,8 +324,8 @@ void PermissionContextBase::DecidePermission(
   if (PermissionRequestManager::IsEnabled()) {
     PermissionRequestManager* permission_request_manager =
         PermissionRequestManager::FromWebContents(web_contents);
-    // TODO(felt): sometimes |permission_request_manager| is null. This check is
-    // meant to prevent crashes. See crbug.com/457091.
+    // TODO(felt): sometimes |permission_request_manager| is null. This check
+    // is meant to prevent crashes. See crbug.com/457091.
     if (!permission_request_manager)
       return;
 
@@ -318,3 +461,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;
+}