Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(90)

Side by Side Diff: chrome/browser/permissions/permission_decision_auto_blocker.cc

Issue 2622983003: Implement embargo in PermissionDecisionAutoBlocker (Closed)
Patch Set: Create separate keys for different embargo types. Created 3 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
OLDNEW
1 // Copyright 2016 The Chromium Authors. All rights reserved. 1 // Copyright 2016 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 #include "chrome/browser/permissions/permission_decision_auto_blocker.h" 5 #include "chrome/browser/permissions/permission_decision_auto_blocker.h"
6 6
7 #include <memory> 7 #include <memory>
8 8
9 #include "base/feature_list.h" 9 #include "base/feature_list.h"
10 #include "base/logging.h" 10 #include "base/logging.h"
11 #include "base/memory/ptr_util.h" 11 #include "base/memory/ptr_util.h"
12 #include "base/strings/string_number_conversions.h" 12 #include "base/strings/string_number_conversions.h"
13 #include "base/time/time.h"
13 #include "base/values.h" 14 #include "base/values.h"
14 #include "chrome/browser/content_settings/host_content_settings_map_factory.h" 15 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
16 #include "chrome/browser/permissions/permission_blacklist_client.h"
15 #include "chrome/browser/permissions/permission_util.h" 17 #include "chrome/browser/permissions/permission_util.h"
16 #include "chrome/common/chrome_features.h" 18 #include "chrome/common/chrome_features.h"
17 #include "components/content_settings/core/browser/host_content_settings_map.h" 19 #include "components/content_settings/core/browser/host_content_settings_map.h"
18 #include "components/variations/variations_associated_data.h" 20 #include "components/variations/variations_associated_data.h"
19 #include "content/public/browser/permission_type.h" 21 #include "content/public/browser/permission_type.h"
22 #include "content/public/browser/web_contents.h"
20 #include "url/gurl.h" 23 #include "url/gurl.h"
21 24
22 namespace { 25 namespace {
23 26
24 // The number of times that users may explicitly dismiss a permission prompt 27 // The number of times that users may explicitly dismiss a permission prompt
25 // from an origin before it is automatically blocked. 28 // from an origin before it is automatically blocked.
26 int g_prompt_dismissals_before_block = 3; 29 int g_prompt_dismissals_before_block = 3;
27 30
31 // The number of days that an origin will stay under embargo for a requested
32 // permission due to blacklisting.
33 int g_blacklist_embargo_days = 7;
34
35 // The number of days that an origin will stay under embargo for a requested
36 // permission due to repeated dismissals.
37 int g_dismissal_embargo_days = 7;
38
28 std::unique_ptr<base::DictionaryValue> GetOriginDict( 39 std::unique_ptr<base::DictionaryValue> GetOriginDict(
29 HostContentSettingsMap* settings, 40 HostContentSettingsMap* settings,
30 const GURL& origin_url) { 41 const GURL& origin_url) {
31 std::unique_ptr<base::DictionaryValue> dict = 42 std::unique_ptr<base::DictionaryValue> dict =
32 base::DictionaryValue::From(settings->GetWebsiteSetting( 43 base::DictionaryValue::From(settings->GetWebsiteSetting(
33 origin_url, origin_url, 44 origin_url, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
34 CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, std::string(), 45 std::string(), nullptr));
35 nullptr));
36 if (!dict) 46 if (!dict)
37 return base::MakeUnique<base::DictionaryValue>(); 47 return base::MakeUnique<base::DictionaryValue>();
38 48
39 return dict; 49 return dict;
40 } 50 }
41 51
42 base::DictionaryValue* GetOrCreatePermissionDict( 52 base::DictionaryValue* GetOrCreatePermissionDict(
43 base::DictionaryValue* origin_dict, 53 base::DictionaryValue* origin_dict,
44 const std::string& permission) { 54 const std::string& permission) {
45 base::DictionaryValue* permission_dict = nullptr; 55 base::DictionaryValue* permission_dict = nullptr;
(...skipping 29 matching lines...) Expand all
75 return current_count; 85 return current_count;
76 } 86 }
77 87
78 int GetActionCount(const GURL& url, 88 int GetActionCount(const GURL& url,
79 content::PermissionType permission, 89 content::PermissionType permission,
80 const char* key, 90 const char* key,
81 Profile* profile) { 91 Profile* profile) {
82 HostContentSettingsMap* map = 92 HostContentSettingsMap* map =
83 HostContentSettingsMapFactory::GetForProfile(profile); 93 HostContentSettingsMapFactory::GetForProfile(profile);
84 std::unique_ptr<base::DictionaryValue> dict = GetOriginDict(map, url); 94 std::unique_ptr<base::DictionaryValue> dict = GetOriginDict(map, url);
85
86 base::DictionaryValue* permission_dict = GetOrCreatePermissionDict( 95 base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
87 dict.get(), PermissionUtil::GetPermissionString(permission)); 96 dict.get(), PermissionUtil::GetPermissionString(permission));
88 97
89 int current_count = 0; 98 int current_count = 0;
90 permission_dict->GetInteger(key, &current_count); 99 permission_dict->GetInteger(key, &current_count);
91 return current_count; 100 return current_count;
92 } 101 }
93 102
103 void PlaceUnderEmbargo(content::PermissionType permission,
104 const GURL& request_origin,
105 HostContentSettingsMap* map,
106 base::Time current_time,
107 const char* key) {
108 std::unique_ptr<base::DictionaryValue> dict =
109 GetOriginDict(map, request_origin);
110 base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
111 dict.get(), PermissionUtil::GetPermissionString(permission));
112 permission_dict->SetDouble(key, current_time.ToInternalValue());
113 map->SetWebsiteSettingDefaultScope(
114 request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
115 std::string(), std::move(dict));
116 }
117
118 void CheckSafeBrowsingResult(content::PermissionType permission,
119 Profile* profile,
120 const GURL& request_origin,
121 base::Time current_time,
122 base::Callback<void(bool)> callback,
123 const char* key,
124 bool should_be_embargoed) {
125 if (should_be_embargoed) {
126 // Requesting site is blacklisted for this permission, update the content
127 // setting to place it under embargo.
128 PlaceUnderEmbargo(permission, request_origin,
129 HostContentSettingsMapFactory::GetForProfile(profile),
130 current_time, key);
131 }
132 callback.Run(should_be_embargoed /* permission blocked */);
133 }
134
135 bool RemoveExpiredEmbargo(content::PermissionType permission,
136 Profile* profile,
137 const GURL& request_origin,
138 const char* key) {
139 HostContentSettingsMap* map =
140 HostContentSettingsMapFactory::GetForProfile(profile);
141 std::unique_ptr<base::DictionaryValue> dict =
142 GetOriginDict(map, request_origin);
143 base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
144 dict.get(), PermissionUtil::GetPermissionString(permission));
145 bool embargo_removed = permission_dict->Remove(key, nullptr);
146 map->SetWebsiteSettingDefaultScope(
147 request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
148 std::string(), std::move(dict));
149 return embargo_removed;
150 }
151
94 } // namespace 152 } // namespace
95 153
96 // static 154 // static
97 const char PermissionDecisionAutoBlocker::kPromptDismissCountKey[] = 155 const char PermissionDecisionAutoBlocker::kPromptDismissCountKey[] =
98 "dismiss_count"; 156 "dismiss_count";
99 157
100 // static 158 // static
101 const char PermissionDecisionAutoBlocker::kPromptIgnoreCountKey[] = 159 const char PermissionDecisionAutoBlocker::kPromptIgnoreCountKey[] =
102 "ignore_count"; 160 "ignore_count";
103 161
104 // static 162 // static
163 const char PermissionDecisionAutoBlocker::kPermissionBlacklistEmbargoKey[] =
164 "blacklisting_embargo_days";
165
166 // static
167 const char PermissionDecisionAutoBlocker::kPermissionDismissalEmbargoKey[] =
168 "dismissal_embargo_days";
169
170 // static
105 void PermissionDecisionAutoBlocker::RemoveCountsByUrl( 171 void PermissionDecisionAutoBlocker::RemoveCountsByUrl(
106 Profile* profile, 172 Profile* profile,
107 base::Callback<bool(const GURL& url)> filter) { 173 base::Callback<bool(const GURL& url)> filter) {
108 HostContentSettingsMap* map = 174 HostContentSettingsMap* map =
109 HostContentSettingsMapFactory::GetForProfile(profile); 175 HostContentSettingsMapFactory::GetForProfile(profile);
110 176
111 std::unique_ptr<ContentSettingsForOneType> settings( 177 std::unique_ptr<ContentSettingsForOneType> settings(
112 new ContentSettingsForOneType); 178 new ContentSettingsForOneType);
113 map->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, 179 map->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT,
114 std::string(), settings.get()); 180 std::string(), settings.get());
(...skipping 37 matching lines...) Expand 10 before | Expand all | Expand 10 after
152 // static 218 // static
153 int PermissionDecisionAutoBlocker::RecordIgnore( 219 int PermissionDecisionAutoBlocker::RecordIgnore(
154 const GURL& url, 220 const GURL& url,
155 content::PermissionType permission, 221 content::PermissionType permission,
156 Profile* profile) { 222 Profile* profile) {
157 return RecordActionInWebsiteSettings(url, permission, kPromptIgnoreCountKey, 223 return RecordActionInWebsiteSettings(url, permission, kPromptIgnoreCountKey,
158 profile); 224 profile);
159 } 225 }
160 226
161 // static 227 // static
162 bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock( 228 bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock(
raymes 2017/01/18 03:15:20 Does this function need to be removed now? We shou
meredithl 2017/01/18 08:28:16 Yes it does. It looks like there are a few gnarly
raymes 2017/01/18 23:35:00 Let's remove it in a followup CL. We should make s
meredithl 2017/01/19 02:11:43 Acknowledged.
163 const GURL& url, 229 const GURL& url,
164 content::PermissionType permission, 230 content::PermissionType permission,
165 Profile* profile) { 231 Profile* profile) {
166 int current_dismissal_count = RecordDismiss(url, permission, profile); 232 int current_dismissal_count = RecordDismiss(url, permission, profile);
167 233
168 if (!base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften)) 234 if (!base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften))
169 return false; 235 return false;
170 236
171 return current_dismissal_count >= g_prompt_dismissals_before_block; 237 return current_dismissal_count >= g_prompt_dismissals_before_block;
172 } 238 }
173 239
174 // static 240 // static
175 void PermissionDecisionAutoBlocker::UpdateFromVariations() { 241 void PermissionDecisionAutoBlocker::UpdateFromVariations() {
176 int prompt_dismissals = -1; 242 int prompt_dismissals = -1;
177 std::string value = variations::GetVariationParamValueByFeature( 243 int blacklist_embargo_days = -1;
244 int dismissal_embargo_days = -1;
245 std::string dismissals_value = variations::GetVariationParamValueByFeature(
178 features::kBlockPromptsIfDismissedOften, kPromptDismissCountKey); 246 features::kBlockPromptsIfDismissedOften, kPromptDismissCountKey);
247 std::string blacklist_embargo_value =
248 variations::GetVariationParamValueByFeature(
249 features::kPermissionsBlacklist, kPermissionBlacklistEmbargoKey);
250 std::string dismissal_embargo_value =
251 variations::GetVariationParamValueByFeature(
252 features::kBlockPromptsIfDismissedOften,
253 kPermissionDismissalEmbargoKey);
254 // If converting the value fails, stick with the current value.
255 if (base::StringToInt(dismissals_value, &prompt_dismissals) &&
256 prompt_dismissals > 0)
257 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.
258 if (base::StringToInt(blacklist_embargo_value, &blacklist_embargo_days) &&
259 blacklist_embargo_days > 0)
260 g_blacklist_embargo_days = blacklist_embargo_days;
261 if (base::StringToInt(dismissal_embargo_value, &dismissal_embargo_days) &&
262 dismissal_embargo_days > 0)
263 g_dismissal_embargo_days = dismissal_embargo_days;
264 }
179 265
180 // If converting the value fails, stick with the current value. 266 // static
181 if (base::StringToInt(value, &prompt_dismissals) && prompt_dismissals > 0) 267 // TODO(meredithl): Have PermissionDecisionAutoBlocker handle the database
182 g_prompt_dismissals_before_block = prompt_dismissals; 268 // manager, rather than passing it in.
269 void PermissionDecisionAutoBlocker::UpdateEmbargoedStatus(
270 scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> db_manager,
271 content::PermissionType permission,
272 const GURL& request_origin,
273 content::WebContents* web_contents,
274 int timeout,
275 Profile* profile,
276 base::Time current_time,
277 base::Callback<void(bool)> callback) {
278 // Check if origin is currently under embargo for the requested permission.
279 // Note: This may be inconsistent if the site was placed under embargo and the
280 // feature/s that can cause an embargo status has been turned off. The window
281 // 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,
282 if (IsUnderEmbargo(permission, profile, request_origin, current_time)) {
283 callback.Run(true /* permission_blocked */);
284 return;
285 }
286
287 // 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.
288 // never been set. RemoveExpiredEmbargo will return true if an embargo state
289 // was stored, and give a free pass to let a request through for repeated
290 // 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.
291 bool embargo_removed = RemoveExpiredEmbargo(
292 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.
293 int current_dismissal_count =
294 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.
295 if (base::FeatureList::IsEnabled(features::kBlockPromptsIfDismissedOften)) {
296 if (current_dismissal_count >= g_prompt_dismissals_before_block) {
297 // If site was previously under embargo, then let this request pass the
298 // repeated dismissals check to give the user a chance to make a decision.
299 // If the request is dismissed again, on the next request it will
300 // automatically block. Otherwise, this is a fresh embargo and just run
301 // the callback with the block flag.
302 if (!embargo_removed) {
303 HostContentSettingsMap* map =
304 HostContentSettingsMapFactory::GetForProfile(profile);
305 PlaceUnderEmbargo(permission, request_origin, map, current_time,
306 kPermissionDismissalEmbargoKey);
307 callback.Run(true /* permission_blocked */);
308 return;
309 }
310 }
311 }
raymes 2017/01/18 03:15:20 I have an alternative suggestion which I think mig
meredithl 2017/01/18 08:28:16 Done.
312
313 // Embargoing due to blacklisting happens last to overrides letting a request
314 // 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
315 if (base::FeatureList::IsEnabled(features::kPermissionsBlacklist) &&
316 db_manager) {
317 PermissionBlacklistClient::CheckSafeBrowsingBlacklist(
318 db_manager, permission, request_origin, web_contents, timeout,
319 base::Bind(&CheckSafeBrowsingResult, permission, profile,
320 request_origin, current_time, callback,
321 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
322 }
323
324 callback.Run(false /* permission blocked */);
183 } 325 }
326
327 // static
328 bool PermissionDecisionAutoBlocker::IsUnderEmbargo(
329 content::PermissionType permission,
330 Profile* profile,
331 const GURL& request_origin,
332 base::Time current_time) {
333 HostContentSettingsMap* map =
334 HostContentSettingsMapFactory::GetForProfile(profile);
335 std::unique_ptr<base::DictionaryValue> dict =
336 GetOriginDict(map, request_origin);
337 base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
338 dict.get(), PermissionUtil::GetPermissionString(permission));
339 double embargo_date = -1;
340 if ((permission_dict->GetDouble(kPermissionBlacklistEmbargoKey,
341 &embargo_date))) {
342 if (current_time < base::Time::FromInternalValue(embargo_date) +
343 base::TimeDelta::FromDays(g_blacklist_embargo_days))
344 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.
345 } else if ((permission_dict->GetDouble(kPermissionDismissalEmbargoKey,
346 &embargo_date))) {
347 if (current_time < base::Time::FromInternalValue(embargo_date) +
348 base::TimeDelta::FromDays(g_dismissal_embargo_days))
349 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
350 }
351 return false;
352 }
353
354 // static
355 void PermissionDecisionAutoBlocker::PlaceUnderEmbargoForTest(
356 content::PermissionType permission,
357 const GURL& request_origin,
358 HostContentSettingsMap* map,
359 base::Time current_time) {
360 PlaceUnderEmbargo(permission, request_origin, map, current_time,
361 kPermissionBlacklistEmbargoKey);
362 }
363
364 // static
365 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
366 content::PermissionType permission,
367 const GURL& request_origin,
368 HostContentSettingsMap* map) {
369 std::unique_ptr<base::DictionaryValue> dict =
370 GetOriginDict(map, request_origin);
371 base::DictionaryValue* permission_dict = GetOrCreatePermissionDict(
372 dict.get(), PermissionUtil::GetPermissionString(permission));
373 return permission_dict->GetDouble(kPermissionBlacklistEmbargoKey, nullptr);
374 }
OLDNEW

Powered by Google App Engine
This is Rietveld 408576698