Implement embargo in PermissionDecisionAutoBlocker

chrome/browser/permissions/permission_decision_auto_blocker.cc

Index: chrome/browser/permissions/permission_decision_auto_blocker.cc
diff --git a/chrome/browser/permissions/permission_decision_auto_blocker.cc b/chrome/browser/permissions/permission_decision_auto_blocker.cc
index 309f1230eb131912b562c3ba1fad4d67559403ca..f9fa41470702cbf1ccecc9dd3921975986a72849 100644
--- a/chrome/browser/permissions/permission_decision_auto_blocker.cc
+++ b/chrome/browser/permissions/permission_decision_auto_blocker.cc
@@ -10,13 +10,16 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
+#include "chrome/browser/permissions/permission_blacklist_client.h"
 #include "chrome/browser/permissions/permission_util.h"
 #include "chrome/common/chrome_features.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/permission_type.h"
+#include "content/public/browser/web_contents.h"
 #include "url/gurl.h"
 
 namespace {
@@ -25,14 +28,17 @@ namespace {
 // from an origin before it is automatically blocked.
 int g_prompt_dismissals_before_block = 3;
 
+// The number of days that an origin will stay under embargo for a requested
+// permission.
+int g_embargo_days = 7;
+
 std::unique_ptr<base::DictionaryValue> GetOriginDict(
     HostContentSettingsMap* settings,
     const GURL& origin_url) {
   std::unique_ptr<base::DictionaryValue> dict =
       base::DictionaryValue::From(settings->GetWebsiteSetting(
-          origin_url, origin_url,
-          CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, std::string(),
-          nullptr));
+          origin_url, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
+          std::string(), nullptr));
   if (!dict)
     return base::MakeUnique<base::DictionaryValue>();
 
@@ -82,7 +88,6 @@ int GetActionCount(const GURL& url,
   HostContentSettingsMap* map =
       HostContentSettingsMapFactory::GetForProfile(profile);
   std::unique_ptr<base::DictionaryValue> dict = GetOriginDict(map, url);
-
   base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
       dict.get(), PermissionUtil::GetPermissionString(permission));
 
@@ -102,6 +107,10 @@ const char PermissionDecisionAutoBlocker::kPromptIgnoreCountKey[] =
     "ignore_count";
 
 // static
+const char PermissionDecisionAutoBlocker::kPermissionOriginEmbargoKey[] =
+    "embargo_days";
+
+// static
 void PermissionDecisionAutoBlocker::RemoveCountsByUrl(
     Profile* profile,
     base::Callback<bool(const GURL& url)> filter) {
@@ -174,10 +183,104 @@ bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock(
 // static
 void PermissionDecisionAutoBlocker::UpdateFromVariations() {
   int prompt_dismissals = -1;
-  std::string value = variations::GetVariationParamValueByFeature(
+  int embargo_days = -1;
+  std::string dismissals_value = variations::GetVariationParamValueByFeature(
       features::kBlockPromptsIfDismissedOften, kPromptDismissCountKey);
-
+  std::string embargo_value = variations::GetVariationParamValueByFeature(
+      features::kPermissionsBlacklist, kPermissionOriginEmbargoKey);
   // If converting the value fails, stick with the current value.
-  if (base::StringToInt(value, &prompt_dismissals) && prompt_dismissals > 0)
+  if (base::StringToInt(dismissals_value, &prompt_dismissals) &&
+      prompt_dismissals > 0)
     g_prompt_dismissals_before_block = prompt_dismissals;
+  if (base::StringToInt(embargo_value, &embargo_days) && embargo_days > 0)
+    g_embargo_days = embargo_days;
+}
+
+// static
+// TODO(meredithl): Have PermissionDecisionAutoBlocker handle the database
+// manager, rather than passing it in.
+void PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock(

[raymes, 2017/01/12 02:34:10]

A slightly alternative design would be to have this as UpdateEmbargoStatus for a
particular origin. The callback would run to signify the status being updated
but not return any result. Then just query IsUnderEmbargo as a separate step.

The UpdateEmbargoStatus function could then be reused for the periodic updating
later and it could incorporate any backoff by just falling out early.

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

I'm not sure I understand what you mean. PermissionContextBase would call
UpdateEmbargoStatus when it wants to make a permission request, giving it the
callback to run when the status has been updated, which can call IsEmbargoed to
determine whether the permission should be automatically blocked. Is that right?

[raymes, 2017/01/16 02:23:42]

Yep - that's what I meant. I don't necessarily think we have to do that now
though. I do think it would be good to clarify how this relates to the
IsUnderEmbargo function though. What if we tried to find a name which associated
it with "embargoing", e.g. call it "UpdateEmbargoedStatus" or
"EmbargoIfRequired"?

[meredithl, 2017/01/16 04:28:19]

Agreed, the name is bad. I called it this to tie in with the
ShouldBlockOnRepeatedDismissals, which I've deleted anyway. Done.

+    scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
+    content::PermissionType permission,
+    const GURL& request_origin,
+    content::WebContents* web_contents,
+    int timeout,
+    Profile* profile,
+    base::Time current_time,
+    base::Callback<void(bool)> callback) {
+  if (!base::FeatureList::IsEnabled(features::kPermissionsBlacklist)) {
+    callback.Run(false /* permission blocked */);
+    return;
+  } else if (IsUnderEmbargo(permission, profile, request_origin,
+                            current_time)) {
+    callback.Run(true /* permission blocked */);
+    return;
+  } else if (!db_manager) {

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

Can this be incorporated into the feature enabled check above?

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

The only reason I didn't was if blacklisting is enabled, but the db is null, we
still want to check the content setting to see if it has been recorded as under
embargo. If it is, automatically block. If it's not, the db null check will run
the callback and allow the permission prompt through as safe browsing can't be
checked. I'm not really sure what cases would lead to the db manager being null
though, so whether this is completely necessary I'm not sure.

[raymes, 2017/01/16 02:23:42]

That makes sense, it might be worth adding a comment to this effect.

[meredithl, 2017/01/16 04:28:19]

This has since changed. Dominick suggested that I refactor it to check if its
embargoed first, if so block. Then go on to check if blocking on repeated
dismissals is enabled, then go on to check if permissions blacklisting is
enabled. This made the code a lot neater/more readable, but does introduce a
window of inconsistency where the site might be embargoed, and then the feature
is disabled. The argument for is that this is a pretty rare edge case, and
shouldn't result in too much confusion. I'm interested to hear your thoughts
though!

+    callback.Run(false /* permission blocked */);
+    return;

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

Rather than all the return statements, the below statement could just be put
into an else {} which might be a bit clearer

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

Done.

+  }
+
+  // Request origin is not yet under embargo, so we check it with Safe Browsing.
+  // TODO(meredithl): Implement incremental back off for checking blacklist.
+  PermissionBlacklistClient::CheckSafeBrowsingBlacklist(
+      db_manager, permission, request_origin, web_contents, timeout,
+      base::Bind(&PermissionDecisionAutoBlocker::CheckSafeBrowsingResult,
+                 permission, profile, request_origin, callback));
+}
+
+// static
+void PermissionDecisionAutoBlocker::CheckSafeBrowsingResult(
+    content::PermissionType permission,
+    Profile* profile,
+    const GURL& request_origin,
+    base::Callback<void(bool)> callback,
+    bool should_be_embargoed) {
+  if (should_be_embargoed) {
+    // Requesting site is blacklisted for this permission, update the content
+    // setting to place it under embargo.
+    PlaceUnderEmbargo(permission, request_origin,
+                      HostContentSettingsMapFactory::GetForProfile(profile),
+                      base::Time::Now());
+  }
+  callback.Run(should_be_embargoed /* permission blocked */);
+}
+
+// static
+bool PermissionDecisionAutoBlocker::IsUnderEmbargo(
+    content::PermissionType permission,
+    Profile* profile,
+    const GURL& request_origin,
+    base::Time current_time) {
+  HostContentSettingsMap* map =
+      HostContentSettingsMapFactory::GetForProfile(profile);
+  std::unique_ptr<base::DictionaryValue> dict =
+      GetOriginDict(map, request_origin);
+  base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
+      dict.get(), PermissionUtil::GetPermissionString(permission));
+  double embargo_days = -1;

[raymes, 2017/01/12 02:34:10]

nit: would this be better named embargo_time? 

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

Dom had me change this from embargo_time to embargo_days just this morning!
Justification being that its more descriptive of the actual value being stored.
If the length of time for the embargo has to be updated, the variable name
should be changed as well.

[raymes, 2017/01/16 02:23:42]

I see you've changed it to embargo_date which makes much more sense to me :)

[meredithl, 2017/01/16 04:28:19]

Yes, as per your other suggestion to store the date of embargoing, rather than
the date of the embargo being lifted :)

+  if ((permission_dict->GetDouble(kPermissionOriginEmbargoKey,
+                                  &embargo_days))) {
+    if (current_time < base::Time::FromInternalValue(embargo_days))

[raymes, 2017/01/12 02:34:10]

Another question is how the embargo period will relate to the periodic updating.
What I mean is that right now it's possible for the origin to become
"unembargoed", even if it hasn't been removed from the blacklist on the server.
Is that behavior we want? 

I think it's important because it raises the question in my mind of whether we
want to store time stamps at all? We could instead just store whether we last
noticed that the thing was on the blacklist and periodically update that stored
value. 

[raymes, 2017/01/12 03:19:30]

Dom, Ben and I chatted about this and concluded that the current design is ok
for now. It would be good to have details of this in the design doc,
specifically in the section on scanning and revocation.

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

Sgtm, I'll make sure to work it into the design doc to make it clearer. If
there's anywhere else you'd like me to add more information into the code
comments, let me know.

+      return true;
+  }
+  return false;
+}
+
+// static
+void PermissionDecisionAutoBlocker::PlaceUnderEmbargo(
+    content::PermissionType permission,
+    const GURL& request_origin,
+    HostContentSettingsMap* map,
+    base::Time current_time) {
+  std::unique_ptr<base::DictionaryValue> dict =
+      GetOriginDict(map, request_origin);
+  base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
+      dict.get(), PermissionUtil::GetPermissionString(permission));
+  base::Time embargo_end =
+      current_time + base::TimeDelta::FromDays(g_embargo_days);

[raymes, 2017/01/12 03:19:30]

Would it be better to just store the current timestamp here and add the embargo
days in IsUnderEmbargo? That would allow us to vary the embargo days parameter
later and affect all existing embargos.

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

Done.

+  permission_dict->SetDouble(kPermissionOriginEmbargoKey,
+                             embargo_end.ToInternalValue());
+  map->SetWebsiteSettingDefaultScope(
+      request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,

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

I don't quite understand what the end state of the pref will look like once
dismissal blocking is incorporated. Could you help me understand how those 2
things will fit together?

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

The idea that Dom and I are going with at the moment is that the permission
context base doesn't really need to know why something was automatically
blocked, just that it was. Once dismissal blocking is incorporated, rather than
changing the current permission request to blocked on the nth recorded
dismissal, it will place it under embargo, meaning the next time it is requested
it will be automatically blocked. The pref I think should be storing the embargo
date if one exists, and the count of dismiss/ignores. Not sure if this answers
your question...

[raymes, 2017/01/16 02:23:42]

Ok - I still have some questions about how the dismissal blocking will change in
behavior and how we want to store the prefs. Maybe we can chat about this
tomorrow morning?

[meredithl, 2017/01/16 04:28:19]

Sgtm!

+      std::string(), std::move(dict));
 }