[HBD] If DefaultPluginPolicy set to 3, prompt should allow flash for the next page load only

[HBD] If DefaultPluginPolicy set to 3, prompt should allow flash for the next page load only

Currently the flash permission prompt doesn't work with an enterprise policy set
to ASK. The reason is that content settings that are stored as a result of
answering the prompt aren't honored because they are overriden by the enterprise
default. This CL changes that, such that when enterprise policy is enabled,
accepting the permission prompt will allow flash on that origin for the duration
of the next page load only. After the prompt is accepted, the page will
automatically be refreshed, we don't revoke access at this point, but any future
navigations of the page will cause permission to be revoked from the origin.
Note that because permission is granted on a per-origin basis, other frames open
to the same origin will also have access during the period of the grant. This
can't be avoided right now due to the way plugin permission checks work (the
WebConents isn't always available at the time of the check).

BUG=654148
Committed: https://crrev.com/4f6714cc01b08469c748536cc70d1fd022863217
Cr-Commit-Position: refs/heads/master@{#425873}

[raymes, 2016-10-13 12:38:12]

raymes@chromium.org changed reviewers:
+ bauerb@chromium.org

[raymes, 2016-10-13 12:38:13]

Bernhard, would you be able to take a first pass over this to review the
approach? I still need to iron out a couple of bugs in the test and have a
browser test I want to add, but I've tested it locally. Thanks!

[Bernhard Bauer, 2016-10-13 13:38:57]

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:247:
context_info_it->second->profile)
Wait, this is being called on the IO thread, right? In that case, you can't
access the profile there to get a KeyedService from it (Profile is UI-thread
only). There is a reason why there was no Profile in the ContextInfo :) What you
want to do here is make the FTPT a RefCountedKeyedService and store a
scoped_refptr to it in the ContextInfo instead (PluginPrefs and HCSM are
examples of that).

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_context.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_context.cc:28: if (provider_id ==
site_settings::kPolicyProviderId)
Just return provider_id == site_settings::kPolicyProviderId?

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_temporary_permission_tracker.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:44:
granted_origins_.insert(std::make_pair(
It might be easier to just do `granted_origins_[origin] = ...;`?

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:58: class
FlashTemporaryPermissionTracker::Tracker : content::WebContentsObserver {
It might be clearer to move the definition of this class to the top of the file,
then out-of-line the method definitions and leave those here, so that this part
only contains method definitions.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:67:
tracker_(tracker) {}
This is a bit difficult to understand... what is the tracker? The outer object
or this one? :)

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_temporary_permission_tracker.h (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:25: // should revoke
access.
Add a comment about the thread-safety of this class?

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:30: explicit
FlashTemporaryPermissionTracker(Profile* profile);
You might want to make it clear which method clients should use to get an
instance of this class. Alternatively, declare the factory a friend and make
constructor/destructor private.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:40: void
RevokeAccess(const GURL& origin);
I think this can be private.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:51: typedef
std::map<GURL, std::unique_ptr<Tracker>> GrantedOriginsMap;
`using GrantedOriginsMap = ...` is preferred now, but really I'm not sure you
need the type alias at all.

[raymes, 2016-10-14 01:49:17]

Thanks Bernhard.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/chrome_plugin_service_filter.cc:247:
context_info_it->second->profile)
On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> Wait, this is being called on the IO thread, right? In that case, you can't
> access the profile there to get a KeyedService from it (Profile is UI-thread
> only). There is a reason why there was no Profile in the ContextInfo :) What
you
> want to do here is make the FTPT a RefCountedKeyedService and store a
> scoped_refptr to it in the ContextInfo instead (PluginPrefs and HCSM are
> examples of that).

Ah right, thank you. Threading is hard :(

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_permission_context.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_permission_context.cc:28: if (provider_id ==
site_settings::kPolicyProviderId)
On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> Just return provider_id == site_settings::kPolicyProviderId?

Done.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_temporary_permission_tracker.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:44:
granted_origins_.insert(std::make_pair(
On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> It might be easier to just do `granted_origins_[origin] = ...;`?

That will cause an existing entry to be overwritten but I think I'd rather it
not insert the new element in that case. See the comment above (let me know if I
can make it clearer)

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:58: class
FlashTemporaryPermissionTracker::Tracker : content::WebContentsObserver {
On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> It might be clearer to move the definition of this class to the top of the
file,
> then out-of-line the method definitions and leave those here, so that this
part
> only contains method definitions.

Done.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:67:
tracker_(tracker) {}
On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> This is a bit difficult to understand... what is the tracker? The outer object
> or this one? :)

Good point - I've renamed some things :)

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_temporary_permission_tracker.h (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:25: // should revoke
access.
On 2016/10/13 13:38:57, Bernhard Bauer wrote:
> Add a comment about the thread-safety of this class?

Done.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:30: explicit
FlashTemporaryPermissionTracker(Profile* profile);
On 2016/10/13 13:38:57, Bernhard Bauer wrote:
> You might want to make it clear which method clients should use to get an
> instance of this class. Alternatively, declare the factory a friend and make
> constructor/destructor private.

Done - I made them private.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:40: void
RevokeAccess(const GURL& origin);
On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> I think this can be private.

Done.

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.h:51: typedef
std::map<GURL, std::unique_ptr<Tracker>> GrantedOriginsMap;
On 2016/10/13 13:38:57, Bernhard Bauer wrote:
> `using GrantedOriginsMap = ...` is preferred now, but really I'm not sure you
> need the type alias at all.

Done.

[raymes, 2016-10-14 02:17:13]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 02:17:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 03:03:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 03:03:28]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-14 11:02:25]

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_temporary_permission_tracker.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:44:
granted_origins_.insert(std::make_pair(
On 2016/10/14 01:49:17, raymes wrote:
> On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> > It might be easier to just do `granted_origins_[origin] = ...;`?
> 
> That will cause an existing entry to be overwritten but I think I'd rather it
> not insert the new element in that case. See the comment above (let me know if
I
> can make it clearer)

Oh, I see. Hm, it might be clearer to just have an explicit check for
base::ContainsKey() for that. I don't think the second map lookup is that much
more overhead, and you wouldn't construct a GrantObserver only to throw it away
immediately when the key already exists.

(Okay, technically you could use lower_bound() to find an iterator to either the
existing entry or the insertion point, and then use the hint-variant of insert()
in the second case, but that's probably bordering on pathologically premature
optimization. Ask me how I know ;-))

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/chrome_plugin_service_filter.cc:69: const
scoped_refptr<PluginPrefs>& plugin_prefs,
Oh, sorry, I also forgot to mention: Can you change these to plain scoped_refptr
(and assign them with move)? That's the preferred way to pass them now.

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/chrome_plugin_service_filter.cc:92:
observer(ProfileContentSettingObserver(profile)) {
Also... do you actually need to call an explicit constructor for
ProfileContentSettingObserver here? o_O I think the |observer| member could be
initialized directly with the profile.

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/flash_temporary_permission_tracker.cc (right):

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:19: :
content::WebContentsObserver {
Do we now need to be careful about these getting destroyed on a non-UI thread? I
think not, because they should all be gone by the time the profile shuts down,
but it might still be worth to have at least a DCHECK in ShutdownOnUIThread()
that there are no grant observers left.

[raymes, 2016-10-15 03:49:59]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-15 03:50:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[raymes, 2016-10-15 03:51:10]

Thanks!

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/flash_temporary_permission_tracker.cc (right):

https://codereview.chromium.org/2413683005/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:44:
granted_origins_.insert(std::make_pair(
On 2016/10/14 11:02:25, Bernhard Bauer wrote:
> On 2016/10/14 01:49:17, raymes wrote:
> > On 2016/10/13 13:38:56, Bernhard Bauer wrote:
> > > It might be easier to just do `granted_origins_[origin] = ...;`?
> > 
> > That will cause an existing entry to be overwritten but I think I'd rather
it
> > not insert the new element in that case. See the comment above (let me know
if
> I
> > can make it clearer)
> 
> Oh, I see. Hm, it might be clearer to just have an explicit check for
> base::ContainsKey() for that. I don't think the second map lookup is that much
> more overhead, and you wouldn't construct a GrantObserver only to throw it
away
> immediately when the key already exists.
> 

Done. 

> (Okay, technically you could use lower_bound() to find an iterator to either
the
> existing entry or the insertion point, and then use the hint-variant of
insert()
> in the second case, but that's probably bordering on pathologically premature
> optimization. Ask me how I know ;-))

I've done that kind of thing before too :P

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/chrome_plugin_service_filter.cc:69: const
scoped_refptr<PluginPrefs>& plugin_prefs,
On 2016/10/14 11:02:25, Bernhard Bauer wrote:
> Oh, sorry, I also forgot to mention: Can you change these to plain
scoped_refptr
> (and assign them with move)? That's the preferred way to pass them now.

I think I've done what you asked, but let me know if I'm confused :)

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/chrome_plugin_service_filter.cc:92:
observer(ProfileContentSettingObserver(profile)) {
On 2016/10/14 11:02:25, Bernhard Bauer wrote:
> Also... do you actually need to call an explicit constructor for
> ProfileContentSettingObserver here? o_O I think the |observer| member could be
> initialized directly with the profile.

Done.

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/flash_temporary_permission_tracker.cc (right):

https://codereview.chromium.org/2413683005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/flash_temporary_permission_tracker.cc:19: :
content::WebContentsObserver {
On 2016/10/14 11:02:25, Bernhard Bauer wrote:
> Do we now need to be careful about these getting destroyed on a non-UI thread?
I
> think not, because they should all be gone by the time the profile shuts down,
> but it might still be worth to have at least a DCHECK in ShutdownOnUIThread()
> that there are no grant observers left.

That was also my reasoning. Done.

[commit-bot: I haz the power, 2016-10-15 04:07:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-15 04:07:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2016-10-16 21:42:26]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-16 21:42:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-16 21:58:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-16 21:58:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bernhard Bauer, 2016-10-17 16:06:01]

LGTM!

[raymes, 2016-10-17 22:37:39]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2016-10-17 22:38:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 00:17:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 00:17:21]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2016-10-18 01:28:14]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-10-18 01:28:14]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2413683005/#ps180001
(title: "[HBD] If DefaultPluginPolicy set to 3, prompt should allow flash for
the next page load only")

[commit-bot: I haz the power, 2016-10-18 01:28:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 02:52:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-10-18 02:54:59]

Description was changed from

==========
[HBD] If DefaultPluginPolicy set to 3, prompt should allow flash for the next
page load only

Currently the flash permission prompt doesn't work with an enterprise policy set
to ASK. The reason is that content settings that are stored as a result of
answering the prompt aren't honored because they are overriden by the enterprise
default. This CL changes that, such that when enterprise policy is enabled,
accepting the permission prompt will allow flash on that origin for the duration
of the next page load only. After the prompt is accepted, the page will
automatically be refreshed, we don't revoke access at this point, but any future
navigations of the page will cause permission to be revoked from the origin.
Note that because permission is granted on a per-origin basis, other frames open
to the same origin will also have access during the period of the grant. This
can't be avoided right now due to the way plugin permission checks work (the
WebConents isn't always available at the time of the check).

BUG=654148
==========

to

==========
[HBD] If DefaultPluginPolicy set to 3, prompt should allow flash for the next
page load only

Currently the flash permission prompt doesn't work with an enterprise policy set
to ASK. The reason is that content settings that are stored as a result of
answering the prompt aren't honored because they are overriden by the enterprise
default. This CL changes that, such that when enterprise policy is enabled,
accepting the permission prompt will allow flash on that origin for the duration
of the next page load only. After the prompt is accepted, the page will
automatically be refreshed, we don't revoke access at this point, but any future
navigations of the page will cause permission to be revoked from the origin.
Note that because permission is granted on a per-origin basis, other frames open
to the same origin will also have access during the period of the grant. This
can't be avoided right now due to the way plugin permission checks work (the
WebConents isn't always available at the time of the check).

BUG=654148

Committed: https://crrev.com/4f6714cc01b08469c748536cc70d1fd022863217
Cr-Commit-Position: refs/heads/master@{#425873}
==========

[commit-bot: I haz the power, 2016-10-18 02:55:00]

Patchset 10 (id:??) landed as
https://crrev.com/4f6714cc01b08469c748536cc70d1fd022863217
Cr-Commit-Position: refs/heads/master@{#425873}