Implement embargo in PermissionDecisionAutoBlocker

chrome/browser/permissions/permission_decision_auto_blocker.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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|.
   static void RemoveCountsByUrl(Profile* profile,
                                 base::Callback<bool(const GURL& url)> filter);
 
   // Returns the current number of dismisses recorded for |permission| type at
   // |url|.
   static int GetDismissCount(const GURL& url,
                              content::PermissionType permission,
@@ skipping 17 matching lines @@
 
   // Records that a dismissal of a prompt for |permission| was made, and returns
   // true if this dismissal should be considered a block. False otherwise.
   static bool ShouldChangeDismissalToBlock(const GURL& url,
                                            content::PermissionType permission,
                                            Profile* profile);
 
   // 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);
 };
 
 #endif  // CHROME_BROWSER_PERMISSIONS_PERMISSION_DECISION_AUTO_BLOCKER_H_