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..7fc5fadf07f7e52440b22250081b0156d4e017de 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,21 @@ 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 due to blacklisting.
+int g_blacklist_embargo_days = 7;
+
+// The number of days that an origin will stay under embargo for a requested
+// permission due to repeated dismissals.
+int g_dismissal_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 +92,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));
 
@@ -91,6 +100,55 @@ int GetActionCount(const GURL& url,
   return current_count;
 }
 
+void PlaceUnderEmbargo(content::PermissionType permission,
+                       const GURL& request_origin,
+                       HostContentSettingsMap* map,
+                       base::Time current_time,
+                       const char* key) {
+  std::unique_ptr<base::DictionaryValue> dict =
+      GetOriginDict(map, request_origin);
+  base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
+      dict.get(), PermissionUtil::GetPermissionString(permission));
+  permission_dict->SetDouble(key, current_time.ToInternalValue());
+  map->SetWebsiteSettingDefaultScope(
+      request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
+      std::string(), std::move(dict));
+}
+
+void CheckSafeBrowsingResult(content::PermissionType permission,
+                             Profile* profile,
+                             const GURL& request_origin,
+                             base::Time current_time,
+                             base::Callback<void(bool)> callback,
+                             const char* key,
+                             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),
+                      current_time, key);
+  }
+  callback.Run(should_be_embargoed /* permission blocked */);
+}
+
+bool RemoveExpiredEmbargo(content::PermissionType permission,
+                          Profile* profile,
+                          const GURL& request_origin,
+                          const char* key) {
+  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));
+  bool embargo_removed = permission_dict->Remove(key, nullptr);
+  map->SetWebsiteSettingDefaultScope(
+      request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
+      std::string(), std::move(dict));
+  return embargo_removed;
+}
+
 }  // namespace
 
 // static
@@ -102,6 +160,14 @@ const char PermissionDecisionAutoBlocker::kPromptIgnoreCountKey[] =
     "ignore_count";
 
 // static
+const char PermissionDecisionAutoBlocker::kPermissionBlacklistEmbargoKey[] =
+    "blacklisting_embargo_days";
+
+// static
+const char PermissionDecisionAutoBlocker::kPermissionDismissalEmbargoKey[] =
+    "dismissal_embargo_days";
+
+// static
 void PermissionDecisionAutoBlocker::RemoveCountsByUrl(
     Profile* profile,
     base::Callback<bool(const GURL& url)> filter) {
@@ -174,10 +240,135 @@ bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock(
 // static
 void PermissionDecisionAutoBlocker::UpdateFromVariations() {
   int prompt_dismissals = -1;
-  std::string value = variations::GetVariationParamValueByFeature(
+  int blacklist_embargo_days = -1;
+  int dismissal_embargo_days = -1;
+  std::string dismissals_value = variations::GetVariationParamValueByFeature(
       features::kBlockPromptsIfDismissedOften, kPromptDismissCountKey);
-
+  std::string blacklist_embargo_value =
+      variations::GetVariationParamValueByFeature(
+          features::kPermissionsBlacklist, kPermissionBlacklistEmbargoKey);
+  std::string dismissal_embargo_value =
+      variations::GetVariationParamValueByFeature(
+          features::kBlockPromptsIfDismissedOften,
+          kPermissionDismissalEmbargoKey);
   // 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;

[raymes, 2017/01/18 03:15:20]

nit: (here and below) always use  {} around multi-line if statements

[meredithl, 2017/01/18 08:28:16]

Done.

+  if (base::StringToInt(blacklist_embargo_value, &blacklist_embargo_days) &&
+      blacklist_embargo_days > 0)
+    g_blacklist_embargo_days = blacklist_embargo_days;
+  if (base::StringToInt(dismissal_embargo_value, &dismissal_embargo_days) &&
+      dismissal_embargo_days > 0)
+    g_dismissal_embargo_days = dismissal_embargo_days;
+}
+
+// static
+// TODO(meredithl): Have PermissionDecisionAutoBlocker handle the database
+// manager, rather than passing it in.
+void PermissionDecisionAutoBlocker::UpdateEmbargoedStatus(
+    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) {
+  // Check if origin is currently under embargo for the requested permission.
+  // Note: This may be inconsistent if the site was placed under embargo and the
+  // feature/s that can cause an embargo status has been turned off. The window
+  // of inconsistency will at most be g_embargo_days.

[raymes, 2017/01/18 03:15:20]

I do think it's important (from a release engineering perspective) to be able to
fully disable a feature if we need to. What if we just check whether the
features are enabled in IsUnderEmbargo as well - then we could remove this note?

[meredithl, 2017/01/18 08:28:16]

I've moved the feature check into IsUnderEmbargo, good suggestion, thanks!

+  if (IsUnderEmbargo(permission, profile, request_origin, current_time)) {
+    callback.Run(true /* permission_blocked */);
+    return;
+  }
+
+  // If the site is not currently under embargo, then it is either expired or

[raymes, 2017/01/18 03:15:20]

nit: clarify with "under dismissal embargo"
nit: "expired or has"

[meredithl, 2017/01/18 08:28:15]

Done.

+  // never been set. RemoveExpiredEmbargo will return true if an embargo state
+  // was stored, and give a free pass to let a request through for repeated
+  // dismissals.

[raymes, 2017/01/18 03:15:20]

nit: merge this comment with the one right above the if (!embargo_removed) check
(IIUC they're both talking about the same thing. It might be worth a comment
here which just says that this block handles embargoing due to dismissals.

[meredithl, 2017/01/18 08:28:16]

Done.

+  bool embargo_removed = RemoveExpiredEmbargo(
+      permission, profile, request_origin, kPermissionDismissalEmbargoKey);

[raymes, 2017/01/18 03:15:20]

nit: move this down right before it is needed

[meredithl, 2017/01/18 08:28:16]

Done.

+  int current_dismissal_count =
+      GetDismissCount(request_origin, permission, profile);

[raymes, 2017/01/18 03:15:20]

nit: move this down right before it is needed

[meredithl, 2017/01/18 08:28:15]

Done.

+  if (base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften)) {
+    if (current_dismissal_count >= g_prompt_dismissals_before_block) {
+      // If site was previously under embargo, then let this request pass the
+      // repeated dismissals check to give the user a chance to make a decision.
+      // If the request is dismissed again, on the next request it will
+      // automatically block. Otherwise, this is a fresh embargo and just run
+      // the callback with the block flag.
+      if (!embargo_removed) {
+        HostContentSettingsMap* map =
+            HostContentSettingsMapFactory::GetForProfile(profile);
+        PlaceUnderEmbargo(permission, request_origin, map, current_time,
+                          kPermissionDismissalEmbargoKey);
+        callback.Run(true /* permission_blocked */);
+        return;
+      }
+    }
+  }

[raymes, 2017/01/18 03:15:20]

I have an alternative suggestion which I think might be simpler and slightly
more consistent. Could we just call PlaceUnderEmbargo in RecordDismiss if the
number of dismissals is greater than the threshhold? That way it would get
placed under embargo at the right time. If the embargo expired, this function
would return true (matching IsUnderEmbargo) until the user clicked dismiss
again, at which point it would be put under embargo again. Does that work?

We could get rid of the entire RemoveExpiredEmbargo function in that case.

[meredithl, 2017/01/18 08:28:16]

Done.

+
+  // Embargoing due to blacklisting happens last to overrides letting a request
+  // through to allow for another dismiss.

[raymes, 2017/01/18 03:15:20]

Is the part that says "happens last to overrides letting a request through to
allow for another dismiss." really true? It seems like if we did the blacklist
check first, it would still return the right result? 

[meredithl, 2017/01/18 08:28:16]

It was just poorly worded, and also now irrelevant as the dismissals embargo is
set on the 3rd permission dismissal, so i've removed it.

+  if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
+      db_manager) {
+    PermissionBlacklistClient::CheckSafeBrowsingBlacklist(
+        db_manager, permission, request_origin, web_contents, timeout,
+        base::Bind(&CheckSafeBrowsingResult, permission, profile,
+                   request_origin, current_time, callback,
+                   kPermissionBlacklistEmbargoKey));

[raymes, 2017/01/18 03:15:20]

is it necessary to pass the kPermissionBlacklistEmbargoKey? Could we just use it
directly in CheckSafeBrowsingResult?

[meredithl, 2017/01/18 08:28:15]

The key is a private member of PermissionDecisionAutoBlocker, and
CheckSafeBrowsing is defined in the anonymous namespace. I've moved
CheckSafeBrowsing to a private method to avoid passing in the key.

+  }
+
+  callback.Run(false /* 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_date = -1;
+  if ((permission_dict->GetDouble(kPermissionBlacklistEmbargoKey,
+                                  &embargo_date))) {
+    if (current_time < base::Time::FromInternalValue(embargo_date) +
+                           base::TimeDelta::FromDays(g_blacklist_embargo_days))
+      return true;

[raymes, 2017/01/18 03:15:20]

nit: always use {} for multi-line if-statements - here and below

[meredithl, 2017/01/18 08:28:16]

Done.

+  } else if ((permission_dict->GetDouble(kPermissionDismissalEmbargoKey,
+                                         &embargo_date))) {
+    if (current_time < base::Time::FromInternalValue(embargo_date) +
+                           base::TimeDelta::FromDays(g_dismissal_embargo_days))
+      return true;

[raymes, 2017/01/18 03:15:20]

Hmm - I think this logic is going to return the wrong value in some cases.
Specifically - what if there is an expired blacklist embargo date. We will enter
the top if-statement, but the nested if will fail and we won't enter the
else-if, even though there might be a dismissal embargo. 

I would suggest pulling out both the blacklisting embargo date and the dismissal
embargo date at the top and then checking both at once.

Please also add a test case that covers this case so we make sure we don't
introduce a regression later.

[meredithl, 2017/01/18 08:28:15]

I've made the suggested changes and added a test which (i think) covers it.

+  }
+  return false;
+}
+
+// static
+void PermissionDecisionAutoBlocker::PlaceUnderEmbargoForTest(
+    content::PermissionType permission,
+    const GURL& request_origin,
+    HostContentSettingsMap* map,
+    base::Time current_time) {
+  PlaceUnderEmbargo(permission, request_origin, map, current_time,
+                    kPermissionBlacklistEmbargoKey);
+}
+
+// static
+bool PermissionDecisionAutoBlocker::GetEmbargoStatusForTest(

[raymes, 2017/01/18 03:15:20]

It's better to test the public API to classes if possible. Can we just use
IsUnderEmbargo instead of this function? Similarly, with PlaceUnderEmbargo for
test, it would be better to simulate going into embargo via the public API. I
understand that would require passing a mock safe browing service but it would
be good to get coverage of that code here anyway.

[meredithl, 2017/01/18 08:28:16]

At the moment, there is test coverage in PermissionContextBaseUnitTest for the
embargoing on blacklisting, and then the unit tests for the autoblocker that
handle repeated dismissals. I have a CL for converting the autoblocker into a
KeyedService. Is it better to add the tests you're after to that CL rather than
this one, as the autoblocker will be handling the database manager etc from then
onwards anyway?

[raymes, 2017/01/18 23:35:00]

I'm ok if we add them after it becomes a keyed service, but I do think we should
add them. There is some code coverage that we miss in this class because we
don't have them.

A couple of suggestions for this CL. Can we make PlaceUnderEmbargo a private
function of this class and then we avoid needing to have the ForTest function
(the test can access it directly). And I think using IsUnderEmbargo rather than
this function is sufficient enough for the tests so that would remove the need
for these 2 functions.

[meredithl, 2017/01/19 02:11:43]

1) Acknowledged, and I completely agree that they need to be there. I've added a
TODO in the unit tests to make sure that gets addressed in a follow up CL, which
I've already started, I may ping you to weigh in to make sure it ticks the boxes
you're after before sending it for review.
2) Done. I've created a method that will place it under embargo for
blacklisting, as dismissal embargo can be achieved by calling
RecordDismissAndEmbargo N times (which there is a test for).

+    content::PermissionType permission,
+    const GURL& request_origin,
+    HostContentSettingsMap* map) {
+  std::unique_ptr<base::DictionaryValue> dict =
+      GetOriginDict(map, request_origin);
+  base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
+      dict.get(), PermissionUtil::GetPermissionString(permission));
+  return permission_dict->GetDouble(kPermissionBlacklistEmbargoKey, nullptr);
 }