Implement embargo in PermissionDecisionAutoBlocker

chrome/browser/permissions/permission_decision_auto_blocker.h

Index: chrome/browser/permissions/permission_decision_auto_blocker.h
diff --git a/chrome/browser/permissions/permission_decision_auto_blocker.h b/chrome/browser/permissions/permission_decision_auto_blocker.h
index 89ecfc7f77e2b920d393b9493026a214277a7408..9da52ad4bba84fc5aea7b9946395f81cd3e10409 100644
--- a/chrome/browser/permissions/permission_decision_auto_blocker.h
+++ b/chrome/browser/permissions/permission_decision_auto_blocker.h
@@ -5,14 +5,37 @@
 #ifndef CHROME_BROWSER_PERMISSIONS_PERMISSION_DECISION_AUTO_BLOCKER_H_
 #define CHROME_BROWSER_PERMISSIONS_PERMISSION_DECISION_AUTO_BLOCKER_H_
 
-#include "base/callback_forward.h"
+#include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "content/public/browser/permission_type.h"
 #include "url/gurl.h"
 
 class GURL;
 class Profile;
 
+namespace content {
+class WebContents;
+}
+
+namespace safe_browsing {
+class SafeBrowsingDatabaseManager;
+}
+
+namespace base {
+class Time;
+}
+
+class HostContentSettingsMap;
+
+// The PermissionDecisionAutoBlocker decides whether or not a given origin
+// should be automatically blocked from requesting a permission. When an origin
+// is blocked, it is placed under an "embargo". Until the embargo expires, any
+// requests made by the origin are automatically blocked. Once the embargo is
+// lifted, the origin will be permitted to request a permission again, which may
+// result in it being placed under embargo again. Currently, an origin is only
+// embargoed if it appears on Safe Browsing's API blacklist.

[raymes, 2017/01/12 00:53:15]

Hmm, this comment only talks about the embargo part of this class, it doesn't
say anything about the dismissals part which is what this class was originally
about.

It would be nice to have a sketch of what the API of the class will look like
once there is a common way of checking embargo or dismissal state and to
understand more details of how they will fit together. At present it's hard to
see why we should put the embargo logic into this class, because they aren't
currently related at all. I know that the plan is to have them merged together
in a cohesive way, but it's hard to say whether that is a good design without
knowing what it will look like.

[meredithl, 2017/01/12 05:59:19]

I have that as a TODO just below, which is to implement embargoing rather than
blocking for repeated dismissals. I can add something more descriptive if you'd
like?

+// TODO(meredithl): Incorporate embargoing into blocking on repeated dismissals.
 class PermissionDecisionAutoBlocker {
  public:
   // Removes any recorded counts for urls which match |filter| under |profile|.
@@ -50,12 +73,51 @@ class PermissionDecisionAutoBlocker {
   // Updates the threshold to start blocking prompts from the field trial.
   static void UpdateFromVariations();
 
+  // Checks if the |request_origin| is under embargo for the requested
+  // |permission|. Internally, this will make a call to IsUnderEmbargo to check
+  // the content setting first, but may also make a call to Safe Browsing to
+  // check if the |request_origin| is blacklisted for |permission|, which is
+  // performed asynchronously.
+  static void ShouldAutomaticallyBlock(
+      scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+      content::PermissionType permission,

[raymes, 2017/01/12 00:53:15]

If we make this a keyed service we can avoid passing in the database manager and
the current time and profile (in fact we can avoid passing the profile in all
the functions). PermissionContextBase won't have to know about the db_manager
and timeout as well, as they can be setup on the keyed service in tests. 

[meredithl, 2017/01/12 05:59:19]

Everyone agrees this sounds like an excellent idea. That will involve a fair bit
of surgery however, so I'll do this as a follow up CL.

+      const GURL& request_origin,
+      content::WebContents* web_contents,
+      int timeout,
+      Profile* profile,
+      base::Time current_time,
+      base::Callback<void(bool)> callback);
+
+  // Checks the status of the content setting to determine if |request_origin|
+  // is under embargo for the |permission|. This check is done synchronously.
+  static bool IsUnderEmbargo(content::PermissionType permission,
+                             Profile* profile,
+                             const GURL& request_origin,
+                             base::Time current_time);
+
  private:
   friend class PermissionContextBaseTests;
+  friend class PermissionDecisionAutoBlockerUnitTest;
+
+  // Get the result of the Safe Browsing check, if |should_be_embargoed| is true
+  // then |request_origin| will be placed under embargo for that |permission|.
+  static void CheckSafeBrowsingResult(content::PermissionType permission,
+                                      Profile* profile,
+                                      const GURL& request_origin,
+                                      base::Callback<void(bool)> callback,
+                                      bool should_be_embargoed);
+
+  // Sets the embargo status of the |request_origin| inside the |permission|
+  // dictionary.
+  static void PlaceUnderEmbargo(content::PermissionType permission,
+                                const GURL& request_origin,
+                                HostContentSettingsMap* map,
+                                base::Time current_time);
 
   // Keys used for storing count data in a website setting.
   static const char kPromptDismissCountKey[];
   static const char kPromptIgnoreCountKey[];
+  static const char kPermissionOriginEmbargoKey[];
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(PermissionDecisionAutoBlocker);
 };