Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622983003:
Implement embargo in PermissionDecisionAutoBlocker (Closed)
DescriptionImplement embargo in PermissionDecisionAutoBlocker
Creates an embargo state for a permission request for a given origin. An
origin will remain under embargo for a certain amount of time, where any
requests it makes for the permission it is under embargo for will be
automatically blocked. After the embargo state is lifted, it will either
be allowed to request permissions again, or be placed under embargo
again if it remains blacklisted by Safe Browsing. In future, there may
be more cases for assigning the embargo state, but for now it is just
for Permissions Blacklisting.
The embargo state is stored as a date for when the embargo was placed,
which is kept inside the permission dictionary for the given
origin and profile.
BUG=679877
Review-Url: https://codereview.chromium.org/2622983003
Cr-Commit-Position: refs/heads/master@{#444616}
Committed: https://chromium.googlesource.com/chromium/src/+/cda94daf85133a29be61558d556d1fdc3412d088
Patch Set 1 #
Total comments: 29
Patch Set 2 : Nits + review #
Total comments: 31
Patch Set 3 : Review, comments, formatting. #
Total comments: 2
Patch Set 4 : Refactor autoblocker to fix tests, hide methods. #Patch Set 5 : Fix flaky test. #Patch Set 6 : Create separate keys for different embargo types. #
Total comments: 44
Patch Set 7 : Block on nth dismissal and tests #
Total comments: 10
Patch Set 8 : Testing clean up and nits #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 42 (19 generated)
meredithl@google.com changed reviewers: + dominickn@chromium.org, kcarattini@chromium.org, raymes@chromium.org
PTAL, thanks!
Looks pretty good! https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:16: #include "base/time/time.h" This include isn't used https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:84: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Nit: probably can leave this new line. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:99: GURL embedding_origin = web_contents->GetLastCommittedURL().GetOrigin(); Nit: probably can leave this new line. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:115: // Check the content setting first before autoblocking. Remove "before autoblocking" - it's a bit misleading. Instead, write it out in more detail: 1. Synchronously check the content setting to see if the user has made a decision, or if the origin is under embargo. If so, respect that decision. 2. Asynchronously check whether the origin should be blocked from making a request. It may be on the Safe Browsing API blacklist, or it may have been dismissed too many times in a row. If the origin is allowed to request, that request will be made in ContinueRequestPermission https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:137: PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock( db_manager_ could still be null here (e.g. if sb_service is null). I guess ShouldAutomaticallyBlock does null-check it before using it, and your TODO covers moving this from here into the auto blocker (i.e. changing all the tests) https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:190: if (content_setting == CONTENT_SETTING_ASK) { Nit: combine the two if statements using && https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:40: origin_url, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, Do you want to rename the content setting in this CL too? raymes is the relevant OWNER for all of that code as well (aside from webui which you can TBR) https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:186: int embargo_time = -1; Nit: call this embargo_days? https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:266: if (current_time < base::Time::FromInternalValue(embargo_time)) { Nit: remove braces from this if https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:283: double embargo_end = Minor nit: I'd find this clearer as: base::Time embargo_end = current_time + base::TimeDelta::FromDays(g_embargo_days); permission_dict->SetDouble(kPermissionOriginEmbargoKey, embargo_end.ToInternalValue()) https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:9: #include "base/callback_forward.h" Remove callback_forward.h https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:31: Add a class comment here explaining what embargo is. "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." etc. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:95: // Get the result of the Safe Browsing check, if |should_be_embargoed| then "if |should_be_embargoed| is true then". https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:127: EXPECT_TRUE(PermissionDecisionAutoBlocker::IsUnderEmbargo( Perhaps add a check that a different permission is not under embargo at this point?
Thanks! https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:16: #include "base/time/time.h" On 2017/01/11 07:52:13, dominickn wrote: > This include isn't used Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:84: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/01/11 07:52:13, dominickn wrote: > Nit: probably can leave this new line. Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:99: GURL embedding_origin = web_contents->GetLastCommittedURL().GetOrigin(); On 2017/01/11 07:52:13, dominickn wrote: > Nit: probably can leave this new line. Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:115: // Check the content setting first before autoblocking. On 2017/01/11 07:52:13, dominickn wrote: > Remove "before autoblocking" - it's a bit misleading. > > Instead, write it out in more detail: > > 1. Synchronously check the content setting to see if the user has made a > decision, or if the origin is under embargo. If so, respect that decision. > > 2. Asynchronously check whether the origin should be blocked from making a > request. It may be on the Safe Browsing API blacklist, or it may have been > dismissed too many times in a row. If the origin is allowed to request, that > request will be made in ContinueRequestPermission Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:137: PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock( On 2017/01/11 07:52:13, dominickn wrote: > db_manager_ could still be null here (e.g. if sb_service is null). I guess > ShouldAutomaticallyBlock does null-check it before using it, and your TODO > covers moving this from here into the auto blocker (i.e. changing all the tests) For now should I move this inside an if statement that does a null check? https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:190: if (content_setting == CONTENT_SETTING_ASK) { On 2017/01/11 07:52:13, dominickn wrote: > Nit: combine the two if statements using && Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:40: origin_url, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, On 2017/01/11 07:52:13, dominickn wrote: > Do you want to rename the content setting in this CL too? raymes is the relevant > OWNER for all of that code as well (aside from webui which you can TBR) Raymes left a comment on the design doc pointing out that it may be difficult to change the name due to underlying prefs, and we may want to deprecate this one and adding a new one. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:186: int embargo_time = -1; On 2017/01/11 07:52:13, dominickn wrote: > Nit: call this embargo_days? Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:266: if (current_time < base::Time::FromInternalValue(embargo_time)) { On 2017/01/11 07:52:13, dominickn wrote: > Nit: remove braces from this if Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:283: double embargo_end = On 2017/01/11 07:52:13, dominickn wrote: > Minor nit: I'd find this clearer as: > > base::Time embargo_end = current_time + > base::TimeDelta::FromDays(g_embargo_days); > permission_dict->SetDouble(kPermissionOriginEmbargoKey, > embargo_end.ToInternalValue()) Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:9: #include "base/callback_forward.h" On 2017/01/11 07:52:13, dominickn wrote: > Remove callback_forward.h Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:31: On 2017/01/11 07:52:13, dominickn wrote: > Add a class comment here explaining what embargo is. > > "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." etc. Yep! I've also left a TODO explaining that this will incorporate the block on repeated dismissals behaviour, which is my next task. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:95: // Get the result of the Safe Browsing check, if |should_be_embargoed| then On 2017/01/11 07:52:13, dominickn wrote: > "if |should_be_embargoed| is true then". Done. https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:127: EXPECT_TRUE(PermissionDecisionAutoBlocker::IsUnderEmbargo( On 2017/01/11 07:52:13, dominickn wrote: > Perhaps add a check that a different permission is not under embargo at this > point? Done. I also added a few extra as well.
lgtm. thanks! https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2622983003/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_context_base.cc:137: PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock( It should be fine given that this will move soon (and the consumer does a null check)
https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:218: } else if (!db_manager) { Can this be incorporated into the feature enabled check above? https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:220: return; Rather than all the return statements, the below statement could just be put into an else {} which might be a bit clearer https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:284: request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, 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? https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:37: // embargoed if it appears on Safe Browsing's API blacklist. 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:83: content::PermissionType permission, 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.
https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:202: void PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock( 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:260: double embargo_days = -1; nit: would this be better named embargo_time? https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:263: if (current_time < base::Time::FromInternalValue(embargo_days)) 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.
https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:263: if (current_time < base::Time::FromInternalValue(embargo_days)) On 2017/01/12 02:34:10, raymes wrote: > 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. 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:280: current_time + base::TimeDelta::FromDays(g_embargo_days); 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:151: } Could we add a simple test for the function to update the embargo status?
Thanks! https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:202: void PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock( On 2017/01/12 02:34:10, raymes wrote: > 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. 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? https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:218: } else if (!db_manager) { On 2017/01/12 00:53:14, raymes wrote: > Can this be incorporated into the feature enabled check above? 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:220: return; On 2017/01/12 00:53:15, raymes wrote: > Rather than all the return statements, the below statement could just be put > into an else {} which might be a bit clearer Done. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:260: double embargo_days = -1; On 2017/01/12 02:34:10, raymes wrote: > nit: would this be better named embargo_time? 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:263: if (current_time < base::Time::FromInternalValue(embargo_days)) On 2017/01/12 03:19:30, raymes wrote: > On 2017/01/12 02:34:10, raymes wrote: > > 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. > > 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. 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:280: current_time + base::TimeDelta::FromDays(g_embargo_days); On 2017/01/12 03:19:30, raymes wrote: > 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. Done. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:284: request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, On 2017/01/12 00:53:15, raymes wrote: > 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? 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... https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:37: // embargoed if it appears on Safe Browsing's API blacklist. On 2017/01/12 00:53:15, raymes wrote: > 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. 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? https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.h:83: content::PermissionType permission, On 2017/01/12 00:53:15, raymes wrote: > 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. 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. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:151: } On 2017/01/12 03:19:30, raymes wrote: > Could we add a simple test for the function to update the embargo status? PlaceUnderEmbargo is what updates the embargo status, which is tested here. Were you after a test for ShouldAutomaticallyBlock or the client callback method?
Thanks Meredith! Overall the approach makes sense now although I still have a couple of questions about the pref and storing data which I think would be better to sort out before we land it. I hope we can chat about these in the morning tomorrow :) https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:202: void PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock( On 2017/01/12 05:59:19, meredithl wrote: > On 2017/01/12 02:34:10, raymes wrote: > > 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. > > 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? > 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"? https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:218: } else if (!db_manager) { On 2017/01/12 05:59:19, meredithl wrote: > On 2017/01/12 00:53:14, raymes wrote: > > Can this be incorporated into the feature enabled check above? > > 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. That makes sense, it might be worth adding a comment to this effect. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:260: double embargo_days = -1; On 2017/01/12 05:59:19, meredithl wrote: > On 2017/01/12 02:34:10, raymes wrote: > > nit: would this be better named embargo_time? > > 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. I see you've changed it to embargo_date which makes much more sense to me :) https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:284: request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, On 2017/01/12 05:59:19, meredithl wrote: > On 2017/01/12 00:53:15, raymes wrote: > > 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? > > 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... 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? https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:151: } On 2017/01/12 05:59:19, meredithl wrote: > On 2017/01/12 03:19:30, raymes wrote: > > Could we add a simple test for the function to update the embargo status? > > PlaceUnderEmbargo is what updates the embargo status, which is tested here. Were > you after a test for ShouldAutomaticallyBlock or the client callback method? Yep for ShouldAutomaticallyBlock https://codereview.chromium.org/2622983003/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:282: request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, We need to decide what to do about this pref. It doesn't seem good to store this new data in there right now, since it's so unrelated to the name. And once you start storing pref data somewhere you can't easily change it later. Perhaps we can discuss the options tomorrow morning too.
A bit of surgery happened, PTAL! https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:202: void PermissionDecisionAutoBlocker::ShouldAutomaticallyBlock( On 2017/01/16 02:23:42, raymes wrote: > On 2017/01/12 05:59:19, meredithl wrote: > > On 2017/01/12 02:34:10, raymes wrote: > > > 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. > > > > 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? > > > > 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"? Agreed, the name is bad. I called it this to tie in with the ShouldBlockOnRepeatedDismissals, which I've deleted anyway. Done. https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:218: } else if (!db_manager) { On 2017/01/16 02:23:42, raymes wrote: > On 2017/01/12 05:59:19, meredithl wrote: > > On 2017/01/12 00:53:14, raymes wrote: > > > Can this be incorporated into the feature enabled check above? > > > > 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. > > That makes sense, it might be worth adding a comment to this effect. 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! https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:260: double embargo_days = -1; On 2017/01/16 02:23:42, raymes wrote: > On 2017/01/12 05:59:19, meredithl wrote: > > On 2017/01/12 02:34:10, raymes wrote: > > > nit: would this be better named embargo_time? > > > > 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. > > I see you've changed it to embargo_date which makes much more sense to me :) Yes, as per your other suggestion to store the date of embargoing, rather than the date of the embargo being lifted :) https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:284: request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, On 2017/01/16 02:23:42, raymes wrote: > On 2017/01/12 05:59:19, meredithl wrote: > > On 2017/01/12 00:53:15, raymes wrote: > > > 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? > > > > 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... > > 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? Sgtm! https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:151: } On 2017/01/16 02:23:42, raymes wrote: > On 2017/01/12 05:59:19, meredithl wrote: > > On 2017/01/12 03:19:30, raymes wrote: > > > Could we add a simple test for the function to update the embargo status? > > > > PlaceUnderEmbargo is what updates the embargo status, which is tested here. > Were > > you after a test for ShouldAutomaticallyBlock or the client callback method? > > Yep for ShouldAutomaticallyBlock > Done. https://codereview.chromium.org/2622983003/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker.cc:282: request_origin, GURL(), CONTENT_SETTINGS_TYPE_PROMPT_NO_DECISION_COUNT, On 2017/01/16 02:23:42, raymes wrote: > We need to decide what to do about this pref. It doesn't seem good to store this > new data in there right now, since it's so unrelated to the name. And once you > start storing pref data somewhere you can't easily change it later. Perhaps we > can discuss the options tomorrow morning too. Sgtm. The original plan was to rename it, but if that's too difficult then happy to do something else.
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thaaaaanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+nathan so he's aware what's happening with this project. Is there a check somewhere that this is only turned on for SafeBrowsing users? Or is the service only available when the user is opted in?
On 2017/01/17 23:03:51, kcarattini wrote: > +nathan so he's aware what's happening with this project. > > Is there a check somewhere that this is only turned on for SafeBrowsing users? > Or is the service only available when the user is opted in? The only checks I've included is that the features for block on repeated dismissals and permissions blacklisting are enabled.
meredithl@google.com changed reviewers: + nparker@chromium.org
Hi Nathan, PTAL. Thanks :)
On 2017/01/18 00:09:43, meredithl wrote: > On 2017/01/17 23:03:51, kcarattini wrote: > > +nathan so he's aware what's happening with this project. > > > > Is there a check somewhere that this is only turned on for SafeBrowsing users? > > Or is the service only available when the user is opted in? > > The only checks I've included is that the features for block on repeated > dismissals and permissions blacklisting are enabled. Nathan, do we need to explicitly check that a user is opted into Safe Browsing, or is the SB Service null when the user is opted out?
Thanks! A few comments due to the reworking. Happy to chat about them if needed. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:368: CONTENT_SETTING_ASK)); It looks like it takes one additional request to be blocked now? Is that intended? Should we just update the loop above to have an extra iteration? If that's not possible could we just reuse |id| from the loop? https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:514: base::Unretained(&permission_context))); Same here https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:228: bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock( Does this function need to be removed now? We should at least add a note to remove it in a followup CL. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:257: g_prompt_dismissals_before_block = prompt_dismissals; nit: (here and below) always use {} around multi-line if statements https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:281: // of inconsistency will at most be g_embargo_days. 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? https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:287: // If the site is not currently under embargo, then it is either expired or nit: clarify with "under dismissal embargo" nit: "expired or has" https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:290: // dismissals. 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. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:292: permission, profile, request_origin, kPermissionDismissalEmbargoKey); nit: move this down right before it is needed https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:294: GetDismissCount(request_origin, permission, profile); nit: move this down right before it is needed https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:311: } 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. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:314: // through to allow for another dismiss. 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? https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:321: kPermissionBlacklistEmbargoKey)); is it necessary to pass the kPermissionBlacklistEmbargoKey? Could we just use it directly in CheckSafeBrowsingResult? https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:344: return true; nit: always use {} for multi-line if-statements - here and below https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:349: return true; 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. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:365: bool PermissionDecisionAutoBlocker::GetEmbargoStatusForTest( 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. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:38: // TODO(meredithl): Incorporate embargoing into blocking on repeated dismissals. I think the TODO is less of a TODO now? Could we just describe the repeated dismissals part here instead? https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:92: // is under embargo for the |permission|. This check is done synchronously. nit: the last sentence is probably unnecessary as it's implied from the API design https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:177: TEST_F(PermissionDecisionAutoBlockerUnitTest, TestUpdatingEmbargoStatus) { Does this test anything more than the above test? https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:212: false); nit: document booleans
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Raymes, let me know if you want to talk more about the testing etc :) https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:368: CONTENT_SETTING_ASK)); On 2017/01/18 03:15:20, raymes wrote: > It looks like it takes one additional request to be blocked now? Is that > intended? Should we just update the loop above to have an extra iteration? > > If that's not possible could we just reuse |id| from the loop? The embargo status wasn't set until the next permission request. We have now updated it so that it embargoes on the Nth dismissal, rather than waiting until the next one so this has been taken care of anyway and we've been able to go back to what we were doing previously, setting the expected value using a ternary, and not needing the extra request. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:514: base::Unretained(&permission_context))); On 2017/01/18 03:15:20, raymes wrote: > Same here Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:228: bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock( On 2017/01/18 03:15:20, raymes wrote: > Does this function need to be removed now? We should at least add a note to > remove it in a followup CL. Yes it does. It looks like there are a few gnarly tests in BrowsingDataRemoverUnitTest that rely on this, am I clear to delete them? For now I've just added a TODO to remove it. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:257: g_prompt_dismissals_before_block = prompt_dismissals; On 2017/01/18 03:15:20, raymes wrote: > nit: (here and below) always use {} around multi-line if statements Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:281: // of inconsistency will at most be g_embargo_days. On 2017/01/18 03:15:20, raymes wrote: > 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? I've moved the feature check into IsUnderEmbargo, good suggestion, thanks! https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:287: // If the site is not currently under embargo, then it is either expired or On 2017/01/18 03:15:20, raymes wrote: > nit: clarify with "under dismissal embargo" > nit: "expired or has" Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:290: // dismissals. On 2017/01/18 03:15:20, raymes wrote: > 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. Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:292: permission, profile, request_origin, kPermissionDismissalEmbargoKey); On 2017/01/18 03:15:20, raymes wrote: > nit: move this down right before it is needed Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:294: GetDismissCount(request_origin, permission, profile); On 2017/01/18 03:15:20, raymes wrote: > nit: move this down right before it is needed Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:311: } On 2017/01/18 03:15:20, raymes wrote: > 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. Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:314: // through to allow for another dismiss. On 2017/01/18 03:15:20, raymes wrote: > 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? 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. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:321: kPermissionBlacklistEmbargoKey)); On 2017/01/18 03:15:20, raymes wrote: > is it necessary to pass the kPermissionBlacklistEmbargoKey? Could we just use it > directly in CheckSafeBrowsingResult? 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. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:344: return true; On 2017/01/18 03:15:20, raymes wrote: > nit: always use {} for multi-line if-statements - here and below Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:349: return true; On 2017/01/18 03:15:20, raymes wrote: > 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. I've made the suggested changes and added a test which (i think) covers it. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:365: bool PermissionDecisionAutoBlocker::GetEmbargoStatusForTest( On 2017/01/18 03:15:20, raymes wrote: > 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. 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? https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:38: // TODO(meredithl): Incorporate embargoing into blocking on repeated dismissals. On 2017/01/18 03:15:20, raymes wrote: > I think the TODO is less of a TODO now? Could we just describe the repeated > dismissals part here instead? Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:92: // is under embargo for the |permission|. This check is done synchronously. On 2017/01/18 03:15:20, raymes wrote: > nit: the last sentence is probably unnecessary as it's implied from the API > design Done. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:177: TEST_F(PermissionDecisionAutoBlockerUnitTest, TestUpdatingEmbargoStatus) { On 2017/01/18 03:15:20, raymes wrote: > Does this test anything more than the above test? Dom and I thought you were looking for a way just to verify that the PlaceUnderEmbargo test was working correctly, can remove if not relevant/helpful. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:212: false); On 2017/01/18 03:15:20, raymes wrote: > nit: document booleans Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks close. Thanks! https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:228: bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock( On 2017/01/18 08:28:16, meredithl wrote: > On 2017/01/18 03:15:20, raymes wrote: > > Does this function need to be removed now? We should at least add a note to > > remove it in a followup CL. > > Yes it does. It looks like there are a few gnarly tests in > BrowsingDataRemoverUnitTest that rely on this, am I clear to delete them? For > now I've just added a TODO to remove it. Let's remove it in a followup CL. We should make sure that we have equivalent coverage of the tests we're removing. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:365: bool PermissionDecisionAutoBlocker::GetEmbargoStatusForTest( On 2017/01/18 08:28:16, meredithl wrote: > On 2017/01/18 03:15:20, raymes wrote: > > 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. > > 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? 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. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:177: TEST_F(PermissionDecisionAutoBlockerUnitTest, TestUpdatingEmbargoStatus) { On 2017/01/18 08:28:16, meredithl wrote: > On 2017/01/18 03:15:20, raymes wrote: > > Does this test anything more than the above test? > > Dom and I thought you were looking for a way just to verify that the > PlaceUnderEmbargo test was working correctly, can remove if not > relevant/helpful. I'm not sure I understand completely, but I think the coverage in the above test is almost equivalent and would allow us to remove the GetEmbargoStatus test function. https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:289: bool is_under_dismiss_embargo = false, is_under_blacklist_embargo = false; nit: Put each declaration on a new line. https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:309: // If either embargoes are still in effect, return true. nit: // If either of the embargoes is still in effect, return true. https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:39: // through variations or |g_prompt_dismissals_before_block|. nit: I would leave out the |g_prompt_dismissals_before_block| because it's an implementation detail. We can just say "greater than a threshhold". https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:59: // total number of dismissals exceeds |g_prompt_dismissals_before_block| and nit: same here https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:281: {}); nit: it might be worth just setting up these feature lists in the SetUp function of PermissionDecisionAutoBlockerUnitTest so we don't need to in each test. It's not too important to do in this CL.
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:228: bool PermissionDecisionAutoBlocker::ShouldChangeDismissalToBlock( On 2017/01/18 23:35:00, raymes wrote: > On 2017/01/18 08:28:16, meredithl wrote: > > On 2017/01/18 03:15:20, raymes wrote: > > > Does this function need to be removed now? We should at least add a note to > > > remove it in a followup CL. > > > > Yes it does. It looks like there are a few gnarly tests in > > BrowsingDataRemoverUnitTest that rely on this, am I clear to delete them? For > > now I've just added a TODO to remove it. > > Let's remove it in a followup CL. We should make sure that we have equivalent > coverage of the tests we're removing. Acknowledged. https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:365: bool PermissionDecisionAutoBlocker::GetEmbargoStatusForTest( On 2017/01/18 23:35:00, raymes wrote: > On 2017/01/18 08:28:16, meredithl wrote: > > On 2017/01/18 03:15:20, raymes wrote: > > > 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. > > > > 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? > > 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. 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). https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:177: TEST_F(PermissionDecisionAutoBlockerUnitTest, TestUpdatingEmbargoStatus) { On 2017/01/18 23:35:00, raymes wrote: > On 2017/01/18 08:28:16, meredithl wrote: > > On 2017/01/18 03:15:20, raymes wrote: > > > Does this test anything more than the above test? > > > > Dom and I thought you were looking for a way just to verify that the > > PlaceUnderEmbargo test was working correctly, can remove if not > > relevant/helpful. > > I'm not sure I understand completely, but I think the coverage in the above test > is almost equivalent and would allow us to remove the GetEmbargoStatus test > function. Done (removed). https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:289: bool is_under_dismiss_embargo = false, is_under_blacklist_embargo = false; On 2017/01/18 23:35:00, raymes wrote: > nit: Put each declaration on a new line. Done. https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:309: // If either embargoes are still in effect, return true. On 2017/01/18 23:35:00, raymes wrote: > nit: // If either of the embargoes is still in effect, return true. Done. https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:39: // through variations or |g_prompt_dismissals_before_block|. On 2017/01/18 23:35:00, raymes wrote: > nit: I would leave out the |g_prompt_dismissals_before_block| because it's an > implementation detail. We can just say "greater than a threshhold". Done. https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.h:59: // total number of dismissals exceeds |g_prompt_dismissals_before_block| and On 2017/01/18 23:35:00, raymes wrote: > nit: same here Done. https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2622983003/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:281: {}); On 2017/01/18 23:35:00, raymes wrote: > nit: it might be worth just setting up these feature lists in the SetUp function > of PermissionDecisionAutoBlockerUnitTest so we don't need to in each test. It's > not too important to do in this CL. Acknowledged.
Description was changed from ========== Implement embargo in PermissionDecisionAutoBlocker Creates an embargo state for a permission request for a given origin. An origin will remain under embargo for a certain amount of time, where any requests it makes for the permission it is under embargo for will be automatically blocked. After the embargo state is lifted, it will either be allowed to request permissions again, or be placed under embargo again if it remains blacklisted by Safe Browsing. In future, there may be more cases for assigning the embargo state, but for now it is just for Permissions Blacklisting. The embargo state is stored as a date for when the embargo should be lifted, which is kept inside the permission dictionary for the given origin and profile. BUG=679877 ========== to ========== Implement embargo in PermissionDecisionAutoBlocker Creates an embargo state for a permission request for a given origin. An origin will remain under embargo for a certain amount of time, where any requests it makes for the permission it is under embargo for will be automatically blocked. After the embargo state is lifted, it will either be allowed to request permissions again, or be placed under embargo again if it remains blacklisted by Safe Browsing. In future, there may be more cases for assigning the embargo state, but for now it is just for Permissions Blacklisting. The embargo state is stored as a date for when the embargo was placed, which is kept inside the permission dictionary for the given origin and profile. BUG=679877 ==========
lgtm!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by meredithl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2622983003/#ps140001 (title: "Testing clean up and nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484794714984900,
"parent_rev": "b5ea8586bfa95e4a03b231a764b87ee374f7b018", "commit_rev":
"cda94daf85133a29be61558d556d1fdc3412d088"}
Message was sent while issue was closed.
Description was changed from ========== Implement embargo in PermissionDecisionAutoBlocker Creates an embargo state for a permission request for a given origin. An origin will remain under embargo for a certain amount of time, where any requests it makes for the permission it is under embargo for will be automatically blocked. After the embargo state is lifted, it will either be allowed to request permissions again, or be placed under embargo again if it remains blacklisted by Safe Browsing. In future, there may be more cases for assigning the embargo state, but for now it is just for Permissions Blacklisting. The embargo state is stored as a date for when the embargo was placed, which is kept inside the permission dictionary for the given origin and profile. BUG=679877 ========== to ========== Implement embargo in PermissionDecisionAutoBlocker Creates an embargo state for a permission request for a given origin. An origin will remain under embargo for a certain amount of time, where any requests it makes for the permission it is under embargo for will be automatically blocked. After the embargo state is lifted, it will either be allowed to request permissions again, or be placed under embargo again if it remains blacklisted by Safe Browsing. In future, there may be more cases for assigning the embargo state, but for now it is just for Permissions Blacklisting. The embargo state is stored as a date for when the embargo was placed, which is kept inside the permission dictionary for the given origin and profile. BUG=679877 Review-Url: https://codereview.chromium.org/2622983003 Cr-Commit-Position: refs/heads/master@{#444616} Committed: https://chromium.googlesource.com/chromium/src/+/cda94daf85133a29be61558d556d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/cda94daf85133a29be61558d556d...
Message was sent while issue was closed.
LGTM in general. I chatted w/ kcarattini a change we need for properly handling when SB is disabled. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
