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-
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 engineer
meredithl
2017/01/18 08:28:16
I've moved the feature check into IsUnderEmbargo,
|
+ 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: "
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 t
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 mig
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 l
meredithl
2017/01/18 08:28:16
It was just poorly worded, and also now irrelevant
|
+ 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 kPermissionBlacklistEm
meredithl
2017/01/18 08:28:15
The key is a private member of PermissionDecisionA
|
+ } |
+ |
+ 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 -
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 wr
meredithl
2017/01/18 08:28:15
I've made the suggested changes and added a test w
|
+ } |
+ 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 p
meredithl
2017/01/18 08:28:16
At the moment, there is test coverage in Permissio
raymes
2017/01/18 23:35:00
I'm ok if we add them after it becomes a keyed ser
meredithl
2017/01/19 02:11:43
1) Acknowledged, and I completely agree that they
|
+ 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); |
} |