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 42ea34f91a2017d4eec893d5ff443ef25dc768e9..b0b190ca63787a022c4103fe56b96a5281f19759 100644
--- a/chrome/browser/permissions/permission_context_base.cc
+++ b/chrome/browser/permissions/permission_context_base.cc
@@ -5,13 +5,18 @@
 #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/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 +26,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 +51,128 @@ 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;

[kcarattini, 2016/12/20 19:57:10]

I'm concerned that this is too short. How about we make it longer for now and
measure the request time with an UMA metric? You can add a TODO to revisit this
once we have some data about request time.

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

Done.

+
+// 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 (since additional permission requests may be made).
+// This class must be created and destroyed on the UI thread.
+class PermissionsBlacklistingClient

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

This is a fairly substantial class now. I think it could be good to consider
moving it into a separate file in a followup CL.

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

Yep, that's in the works, which I'll be detailing in the design doc.

+    : public safe_browsing::SafeBrowsingDatabaseManager::Client,
+      public base::RefCountedThreadSafe<PermissionsBlacklistingClient>,
+      public content::WebContentsObserver {
+ public:
+  static void CheckSafeBrowsingBlacklist(
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      content::PermissionType permission_type,
+      const GURL& request_origin,
+      content::WebContents* web_contents,
+      int timeout,

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

Could we just make this a member variable of the class rather than passing it
around?

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

Done.

[raymes, 2017/01/09 06:28:17]

Can we remove this as an argument from this function now and just use timeout_?

[meredithl, 2017/01/10 02:24:57]

It gets passed from here into the constructor to be set as a member variable
there, is that sufficient? Or should timeout_ be static so it can be initialized
elsewhere?

[raymes, 2017/01/10 02:57:15]

Oh sorry, you're right, we need it here :)

[meredithl, 2017/01/10 03:40:46]

Acknowledged.

+      base::Callback<void(bool)> callback) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+    new PermissionsBlacklistingClient(db_manager, permission_type,
+                                      request_origin, web_contents, timeout,
+                                      callback);
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<PermissionsBlacklistingClient>;
+  ~PermissionsBlacklistingClient() override {}

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

nit: constructor before destructor usually

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

Done.

+
+  PermissionsBlacklistingClient(
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      content::PermissionType permission_type,
+      const GURL& request_origin,
+      content::WebContents* web_contents,
+      int timeout,
+      base::Callback<void(bool)> callback)
+      : content::WebContentsObserver(web_contents),
+        db_manager_(db_manager),
+        permission_type_(permission_type),
+        callback_(callback) {
+    // Balanced by a call to Release() in OnCheckApiBlacklistUrlResult().
+    AddRef();
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&PermissionsBlacklistingClient::StartCheck, this,
+                   request_origin, timeout));
+  }
+
+  void StartCheck(const GURL& request_origin, int timeout) {

[raymes, 2017/01/09 06:28:17]

nit: Can we remove timeout here and just use timeout_?

[meredithl, 2017/01/10 02:24:57]

Done.

+    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_ = base::MakeUnique<base::OneShotTimer>();
+    timer_->Start(
+        FROM_HERE, base::TimeDelta::FromMilliseconds(timeout),
+        base::Bind(&PermissionsBlacklistingClient::OnCheckApiBlacklistUrlResult,
+                   this, request_origin, empty_metadata));
+    db_manager_->CheckApiBlacklistUrl(request_origin, this);
+  }
+
+  // SafeBrowsingDatabaseManager::Client implementation.
+  // TODO(meredithl) : fix this for post task

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

I'm not sure what this comment means?

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

Sorry. Was an old comment for me to look at, forgot to remove it.

+  void OnCheckApiBlacklistUrlResult(
+      const GURL& url,
+      const safe_browsing::ThreatMetadata& metadata) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+    if (timer_->IsRunning())
+      timer_->Stop();
+    else
+      db_manager_->CancelApiCheck(this);
+    timer_.reset(nullptr);
+
+    // TODO(meredithl) : Implement correct permission string check.

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

Hmm I'm not sure what this one means either?

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

Safe Browsing stringify the PermissionType enum, which don't match those
returned by GetPermissionString(), so I need to do my own comparison or convert
between the two. I'll update the comment to be more descriptive.

+    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(
+            &PermissionsBlacklistingClient::EvaluateBlacklistResultOnUiThread,
+            this, permission_blocked));
+  }
+
+  void EvaluateBlacklistResultOnUiThread(bool permission_blocked) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+    if (is_active_)

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

It doesn't look like is_active_ is initialized anywhere. That would result in
undefined behavior.

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

Done.

+      callback_.Run(permission_blocked);
+    Release();
+  }
+
+  // WebContentsObserver implementation. Sets the flag so that when the database
+  // manager returns with a result, it won't attempt to run the callback with a
+  // deleted WebContents.
+  void WebContentsDestroyed() override {
+    is_active_ = false;
+    Observe(nullptr);

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

Why is this needed? Is it just so we don't accidentally touch web_contents()?

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

I believe so. Dominick suggested to put it in, and there are examples of it
scattered (albeit inconsistently) throughout other parts of the code base.
Whether or not it's actually needed here, I'm not sure. 

+  }
+
+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager_;
+  content::PermissionType permission_type_;
+
+  // PermissionContextBase callback to run on the UI thread.
+  base::Callback<void(bool)> callback_;
+
+  // Timer to abort the Safe Browsing check if it takes too long. Created and
+  // used on the IO Thread.
+  std::unique_ptr<base::OneShotTimer> timer_;
+
+  // True if |callback_| should be invoked. For instance, if web_contents() is

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

nit: Since the only reason why this would be false is if the WebContents is
destroyed, I think you can just remove the "For instance" here.

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

Done.

+  // destroyed, this is set to false.
+  bool is_active_;
+
+  DISALLOW_COPY_AND_ASSIGN(PermissionsBlacklistingClient);
+};
 
 PermissionContextBase::PermissionContextBase(
     Profile* profile,
@@ -50,6 +181,7 @@ PermissionContextBase::PermissionContextBase(
     : profile_(profile),
       permission_type_(permission_type),
       content_settings_type_(content_settings_type),
+      safe_browsing_timeout_(kCheckUrlTimeoutMs),
       weak_factory_(this) {
 #if defined(OS_ANDROID)
   permission_queue_controller_.reset(new PermissionQueueController(
@@ -101,12 +233,64 @@ void PermissionContextBase::RequestPermission(
     return;
   }
 
+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager =
+      db_manager_;
+  if (!db_manager) {
+    safe_browsing::SafeBrowsingService* sb_service =
+        g_browser_process->safe_browsing_service();
+    if (sb_service)
+      db_manager = sb_service->database_manager();

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

Hmm, we never set the instance variable here. I think we want to have
db_manager_ = ...

That should avoid needing the local variable as well, I think we could just use
db_manager_

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

Done.

+  }

[kcarattini, 2016/12/20 19:57:10]

Do you need these 8 lines here or can you move them inside the if clause below
(and change the "if" to just check whether the feature is enabled)? You aren't
using the db_manager when the feature is off so there's no need for the extra
call to SB Service, and I'd prefer to move it closer to where it's being used.

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

Good thought.

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

Ooh yep. Good catch. Done.

+
+  if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
+      db_manager) {
+    // The client contacts Safe Browsing, and runs the callback with the result.
+    PermissionsBlacklistingClient::CheckSafeBrowsingBlacklist(
+        db_manager, permission_type_, requesting_origin, web_contents,
+        safe_browsing_timeout_,
+        base::Bind(&PermissionContextBase::ContinueRequestPermission,
+                   weak_factory_.GetWeakPtr(), web_contents, id,
+                   requesting_origin, embedding_origin, user_gesture,
+                   callback));
+  } else {
+    // TODO(meredithl) : add UMA metrics here.

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

nit: here and elsewhere, no space before : in TODOs and capital letter at the
start of sentences, e.g.
TODO(raymes): The dog went to sleep.

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

Done.

+    ContinueRequestPermission(web_contents, id, requesting_origin,
+                              embedding_origin, user_gesture, callback,
+                              false /* permission blocked */);
+  }
+}
+
+void PermissionContextBase::ContinueRequestPermission(
+    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,
@@ -323,3 +507,12 @@ void PermissionContextBase::UpdateContentSetting(
                                       content_settings_type_, std::string(),
                                       content_setting);
 }
+
+void PermissionContextBase::SetSafeBrowsingDatabaseManagerForTests(
+    scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager) {
+  db_manager_ = db_manager;
+}
+
+void PermissionContextBase::SetTimeoutForSafeBrowsingClient(int timeout) {
+  safe_browsing_timeout_ = timeout;
+}