Implement origin specific Permissions Blacklisting.

chrome/browser/permissions/permission_context_base_unittest.cc

Index: chrome/browser/permissions/permission_context_base_unittest.cc
diff --git a/chrome/browser/permissions/permission_context_base_unittest.cc b/chrome/browser/permissions/permission_context_base_unittest.cc
index fb2e8221c2d28111f3cb215d42a1e54fa665fff5..91c96d503aab468e2933f9fe964ae1800e0e238e 100644
--- a/chrome/browser/permissions/permission_context_base_unittest.cc
+++ b/chrome/browser/permissions/permission_context_base_unittest.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/permissions/permission_context_base.h"
 
 #include <map>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -14,6 +15,7 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
+#include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/mock_entropy_provider.h"
 #include "base/test/scoped_feature_list.h"
@@ -31,7 +33,10 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_types.h"
+#include "components/safe_browsing_db/database_manager.h"
+#include "components/safe_browsing_db/test_database_manager.h"
 #include "components/variations/variations_associated_data.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -50,6 +55,34 @@ const char kPermissionsKillSwitchTestGroup[] = "TestGroup";
 const char* const kPromptGroupName = kPermissionsKillSwitchTestGroup;
 const char kPromptTrialName[] = "PermissionPromptsUX";
 
+class MockSafeBrowsingDatabaseManager
+    : public safe_browsing::TestSafeBrowsingDatabaseManager {
+ public:
+  MockSafeBrowsingDatabaseManager() {}
+
+  bool CheckApiBlacklistUrl(const GURL& url, Client* client) override {
+    safe_browsing::ThreatMetadata metadata;
+    if (permissions_blacklist_.find(url) != permissions_blacklist_.end())
+      metadata.api_permissions = permissions_blacklist_.find(url)->second;

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

nit: can we just store the result of permissions_blacklist_.find() above and
reuse it here?

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

I think you might have missed this comments from a previous patchset

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

Done.

+
+    client->OnCheckApiBlacklistUrlResult(url, metadata);
+    return true;
+  }
+
+  void BlacklistUrlPermissions(const GURL& url,
+                               const std::set<std::string> permissions) {
+    permissions_blacklist_[url] = permissions;
+  }
+
+ protected:
+  ~MockSafeBrowsingDatabaseManager() override {}
+
+ private:
+  std::map<GURL, std::set<std::string>> permissions_blacklist_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingDatabaseManager);
+};
+
 class TestPermissionContext : public PermissionContextBase {
  public:
   TestPermissionContext(Profile* profile,
@@ -70,8 +103,12 @@ class TestPermissionContext : public PermissionContextBase {
 
   bool tab_context_updated() const { return tab_context_updated_; }
 
+  // Once a decision for the requested permission has been made, run the
+  // callback.
   void TrackPermissionDecision(ContentSetting content_setting) {
     decisions_.push_back(content_setting);
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                     quit_closure_);

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

Do we need this line? Could we instead just call quit_closure_.Run() after
respond_permission_.Run() below? I might be missing something though, I know
this part was tricky :)

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

ditto

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

Sorry Raymes, I did miss these. DecidePermission isn't called for every
permission request, such as in the case of TestPermissionBlacklistingBlocked and
TestRequestPermissionInvalidUrl. These automatically block the request, and run
the callback, which for tests is TrackPermissionDecided. This meant that the run
loop was never exited and tests would hang, so a simple solution was to post the
quit_closure in TrackPermissionDecided to make sure the run loop exits for all
tests. Is there a more straightforward way of doing this do you think?

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

Ah that makes sense, thanks for explaining.

   }
 
   ContentSetting GetContentSettingFromMap(const GURL& url_a,
@@ -81,6 +118,39 @@ class TestPermissionContext : public PermissionContextBase {
                                   content_settings_type(), std::string());
   }
 
+  void RequestPermission(content::WebContents* web_contents,
+                         const PermissionRequestID& id,
+                         const GURL& requesting_frame,
+                         bool user_gesture,
+                         const BrowserPermissionCallback& callback) override {
+    base::RunLoop run_loop;
+    quit_closure_ = run_loop.QuitClosure();
+    PermissionContextBase::RequestPermission(web_contents, id, requesting_frame,
+                                             true /* user_gesture */, callback);
+    run_loop.Run();
+  }
+
+  void DecidePermission(content::WebContents* web_contents,
+                        const PermissionRequestID& id,
+                        const GURL& requesting_origin,
+                        const GURL& embedding_origin,
+                        bool user_gesture,
+                        const BrowserPermissionCallback& callback) override {
+    PermissionContextBase::DecidePermission(web_contents, id, requesting_origin,
+                                            embedding_origin, user_gesture,
+                                            callback);
+    if (respond_permission_)
+      respond_permission_.Run();
+    else
+      quit_closure_.Run();

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

It's probably good to clear the quit_closure_ after it's been run to ensure that
it's not run twice by mistake.

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

ditto

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

By "clear" do you mean set to null?

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

Sorry - yep exactly, you can call .Reset()

+  }
+
+  // Permission request will need to be responded to, so pass a callback to be
+  // run once the request has completed and the decision has been made.
+  void SetRespondPermissionCallback(base::Closure callback) {
+    respond_permission_ = callback;
+  }
+
  protected:
   void UpdateTabContext(const PermissionRequestID& id,
                         const GURL& requesting_origin,
@@ -95,7 +165,10 @@ class TestPermissionContext : public PermissionContextBase {
  private:
   std::vector<ContentSetting> decisions_;
   bool tab_context_updated_;
-
+  base::Closure quit_closure_;
+  // Callback for responding to a permission once the request has been
+  // completed (valid URL, killswitch disabled, not blacklisted)
+  base::Closure respond_permission_;
   DISALLOW_COPY_AND_ASSIGN(TestPermissionContext);
 };
 
@@ -124,6 +197,25 @@ class TestKillSwitchPermissionContext : public TestPermissionContext {
   DISALLOW_COPY_AND_ASSIGN(TestKillSwitchPermissionContext);
 };
 
+class TestPermissionsBlacklistingContext : public TestPermissionContext {

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

nit: Rather than adding a new class here, I don't think it would be problematic
to put this in TestPermissionContext with a function which enables the
blacklisting for that test

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

You're right, but since we decided to go for the SetXForTests over virtual
functions, that actually rendered this class entirely obsolete so I've removed
it :)

+ public:
+  TestPermissionsBlacklistingContext(
+      Profile* profile,
+      const content::PermissionType permission_type,
+      const ContentSettingsType content_settings_type,
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager)
+      : TestPermissionContext(profile, permission_type, content_settings_type),
+        db_manager_(db_manager) {}
+
+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+  GetSafeBrowsingDatabaseManager() override {
+    return db_manager_;
+  }
+
+ private:
+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager_;
+};
+
 class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
  protected:
   PermissionContextBaseTests() {}
@@ -180,13 +272,15 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
         web_contents()->GetRenderProcessHost()->GetID(),
         web_contents()->GetMainFrame()->GetRoutingID(),
         -1);
+    permission_context.SetRespondPermissionCallback(
+        base::Bind(&PermissionContextBaseTests::RespondToPermission,
+                   base::Unretained(this), &permission_context, id, url,
+                   persist, decision));
     permission_context.RequestPermission(
         web_contents(),
         id, url, true /* user_gesture */,
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
-
-    RespondToPermission(&permission_context, id, url, persist, decision);
     ASSERT_EQ(1u, permission_context.decisions().size());
     EXPECT_EQ(decision, permission_context.decisions()[0]);
     EXPECT_TRUE(permission_context.tab_context_updated());
@@ -237,13 +331,16 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
       const PermissionRequestID id(
           web_contents()->GetRenderProcessHost()->GetID(),
           web_contents()->GetMainFrame()->GetRoutingID(), i);
+
+      permission_context.SetRespondPermissionCallback(
+          base::Bind(&PermissionContextBaseTests::RespondToPermission,
+                     base::Unretained(this), &permission_context, id, url,
+                     false, CONTENT_SETTING_ASK));
+
       permission_context.RequestPermission(
           web_contents(), id, url, true /* user_gesture */,
           base::Bind(&TestPermissionContext::TrackPermissionDecision,
                     base::Unretained(&permission_context)));
-
-      RespondToPermission(&permission_context, id, url, false, /* persist */
-                          CONTENT_SETTING_ASK);
       histograms.ExpectTotalCount(
           "Permissions.Prompt.Dismissed.PriorDismissCount." +
               PermissionUtil::GetPermissionString(permission_type),
@@ -281,13 +378,15 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
       const PermissionRequestID id(
           web_contents()->GetRenderProcessHost()->GetID(),
           web_contents()->GetMainFrame()->GetRoutingID(), i);
+
+      permission_context.SetRespondPermissionCallback(
+          base::Bind(&PermissionContextBaseTests::RespondToPermission,
+                     base::Unretained(this), &permission_context, id, url,
+                     false, CONTENT_SETTING_ASK));
       permission_context.RequestPermission(
           web_contents(), id, url, true /* user_gesture */,
           base::Bind(&TestPermissionContext::TrackPermissionDecision,
                     base::Unretained(&permission_context)));
-
-      RespondToPermission(&permission_context, id, url, false, /* persist */
-                          CONTENT_SETTING_ASK);
       histograms.ExpectTotalCount(
           "Permissions.Prompt.Dismissed.PriorDismissCount.Geolocation",
           i + 1);
@@ -365,13 +464,15 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
       const PermissionRequestID id(
           web_contents()->GetRenderProcessHost()->GetID(),
           web_contents()->GetMainFrame()->GetRoutingID(), i);
+      permission_context.SetRespondPermissionCallback(
+          base::Bind(&PermissionContextBaseTests::RespondToPermission,
+                     base::Unretained(this), &permission_context, id, url,
+                     false, CONTENT_SETTING_ASK));
       permission_context.RequestPermission(
           web_contents(), id, url, true /* user_gesture */,
           base::Bind(&TestPermissionContext::TrackPermissionDecision,
-                    base::Unretained(&permission_context)));
+                     base::Unretained(&permission_context)));
 
-      RespondToPermission(&permission_context, id, url, false, /* persist */
-                          CONTENT_SETTING_ASK);
       EXPECT_EQ(1u, permission_context.decisions().size());
       ASSERT_EQ(expected, permission_context.decisions()[0]);
       EXPECT_TRUE(permission_context.tab_context_updated());
@@ -431,14 +532,17 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
         web_contents()->GetRenderProcessHost()->GetID(),
         web_contents()->GetMainFrame()->GetRoutingID(),
         -1);
+    permission_context.SetRespondPermissionCallback(
+        base::Bind(&PermissionContextBaseTests::RespondToPermission,
+                   base::Unretained(this), &permission_context, id, url, true,
+                   CONTENT_SETTING_ALLOW));
+
     permission_context.RequestPermission(
         web_contents(),
         id, url, true /* user_gesture */,
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
 
-    RespondToPermission(&permission_context, id, url, true, /* persist */
-                        CONTENT_SETTING_ALLOW);
     ASSERT_EQ(1u, permission_context.decisions().size());
     EXPECT_EQ(CONTENT_SETTING_ALLOW, permission_context.decisions()[0]);
     EXPECT_TRUE(permission_context.tab_context_updated());
@@ -490,21 +594,27 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
         web_contents()->GetRenderProcessHost()->GetID(),
         web_contents()->GetMainFrame()->GetRoutingID(), 1);
 
+    bool persist = (response == CONTENT_SETTING_ALLOW ||
+                    response == CONTENT_SETTING_BLOCK);
+
+    // Request a permission without setting the callback to DecidePermission.
     permission_context.RequestPermission(
         web_contents(), id0, url, true /* user_gesture */,
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
+
+    EXPECT_EQ(0u, permission_context.decisions().size());
+
+    // Set the callback, and make a second permission request.
+    permission_context.SetRespondPermissionCallback(
+        base::Bind(&PermissionContextBaseTests::RespondToPermission,
+                   base::Unretained(this), &permission_context, id0, url,
+                   persist, response));
     permission_context.RequestPermission(
         web_contents(), id1, url, true /* user_gesture */,
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
 
-    EXPECT_EQ(0u, permission_context.decisions().size());
-
-    bool persist = (response == CONTENT_SETTING_ALLOW ||
-                    response == CONTENT_SETTING_BLOCK);
-    RespondToPermission(&permission_context, id0, url, persist, response);
-
     ASSERT_EQ(2u, permission_context.decisions().size());
     EXPECT_EQ(response, permission_context.decisions()[0]);
     EXPECT_EQ(response, permission_context.decisions()[1]);
@@ -513,6 +623,34 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
     EXPECT_EQ(response, permission_context.GetContentSettingFromMap(url, url));
   }
 
+  void TestPermissionsBlacklisting(
+      content::PermissionType permission_type,
+      ContentSettingsType content_settings_type,
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      const GURL& url,
+      ContentSetting response) {
+    NavigateAndCommit(url);
+    base::test::ScopedFeatureList scoped_feature_list;
+    scoped_feature_list.InitAndEnableFeature(features::kPermissionsBlacklist);
+    TestPermissionsBlacklistingContext permission_context(
+        profile(), permission_type, content_settings_type, db_manager);
+
+    const PermissionRequestID id(
+        web_contents()->GetRenderProcessHost()->GetID(),
+        web_contents()->GetMainFrame()->GetRoutingID(), -1);
+
+    permission_context.SetRespondPermissionCallback(base::Bind(
+        &PermissionContextBaseTests::RespondToPermission,
+        base::Unretained(this), &permission_context, id, url, false, response));
+    permission_context.RequestPermission(
+        web_contents(), id, url, true /* user_gesture */,
+        base::Bind(&TestPermissionContext::TrackPermissionDecision,
+                   base::Unretained(&permission_context)));
+
+    ASSERT_EQ(1u, permission_context.decisions().size());
+    EXPECT_EQ(response, permission_context.decisions()[0]);
+  }
+
  private:
   // ChromeRenderViewHostTestHarness:
   void SetUp() override {
@@ -669,3 +807,36 @@ TEST_F(PermissionContextBaseTests, TestParallelRequestsBlocked) {
 TEST_F(PermissionContextBaseTests, TestParallelRequestsDismissed) {
   TestParallelRequests(CONTENT_SETTING_ASK);
 }
+
+// Tests a blacklisted (URL, permission) pair has had its permission request
+// blocked.
+TEST_F(PermissionContextBaseTests, TestPermissionsBlacklistingBlocked) {
+  scoped_refptr<MockSafeBrowsingDatabaseManager> db_manager =
+      new MockSafeBrowsingDatabaseManager();
+  const GURL url("https://www.example.com");
+  std::set<std::string> blacklisted_permissions{
+      PermissionUtil::GetPermissionString(
+          content::PermissionType::GEOLOCATION)};
+  db_manager->BlacklistUrlPermissions(url, blacklisted_permissions);
+  TestPermissionsBlacklisting(content::PermissionType::GEOLOCATION,
+                              CONTENT_SETTINGS_TYPE_GEOLOCATION, db_manager,
+                              url, CONTENT_SETTING_BLOCK);
+}
+
+// Tests that a URL with a blacklisted permission is permitted to request a
+// non-blacklisted permission.
+TEST_F(PermissionContextBaseTests, TestPermissionsBlacklistingAllowed) {
+  scoped_refptr<MockSafeBrowsingDatabaseManager> db_manager =
+      new MockSafeBrowsingDatabaseManager();
+  const GURL url("https://www.example.com");
+  std::set<std::string> blacklisted_permissions{
+      PermissionUtil::GetPermissionString(
+          content::PermissionType::GEOLOCATION)};
+  db_manager->BlacklistUrlPermissions(url, blacklisted_permissions);
+  TestPermissionsBlacklisting(content::PermissionType::GEOLOCATION,
+                              CONTENT_SETTINGS_TYPE_GEOLOCATION, db_manager,
+                              url, CONTENT_SETTING_BLOCK);
+  TestPermissionsBlacklisting(content::PermissionType::NOTIFICATIONS,
+                              CONTENT_SETTINGS_TYPE_NOTIFICATIONS, db_manager,
+                              url, CONTENT_SETTING_ASK);
+}