Exempt component and policy-installed extensions from policy blacklist check.

Exempt component and policy-installed extensions from policy blacklist check.

Component extensions and policy-installed extensions shouldn't be affected by the policy blacklist: The former are considered internal UI that happens to be part of an extension, and they should have first-class policy knobs if we want to disable them. For the latter, the admin specifying them is enough indication that they should be installed, and requiring them to explicitly white-listed in some cases is redundant.

BUG=chromium-os:16702, chromium-os:21281
TEST=On ChromeOS, configure ExtensionInstallBlacklist = * and load chrome://bookmarks


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107707

[Mattias Nissler (ping if slow), 2011-10-24 13:46:59]

Please review.

[Finnur, 2011-10-24 15:08:13]

These two bugs you refer to in the CL look a bit conflicting. One says we should
not do this (with cmassone chiming in) and the other says we should do this
(with cmassone not on it). Has this been resolved or am I reading this
incorrectly?

http://codereview.chromium.org/8380006/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8380006/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2420: 
I am repulsed by this particular change. The effects it will have on our
userbase is enormous!

[Mattias Nissler (ping if slow), 2011-10-24 15:28:14]

Regarding the bugs, I've posted a follow-up on the one that says won't-fix.
While the behavior is intentional, I feel it's rather unintuitive and now even
lacking a technical reason to keep it.

http://codereview.chromium.org/8380006/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8380006/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:2420: 
On 2011/10/24 15:08:13, Finnur wrote:
> I am repulsed by this particular change. The effects it will have on our
> userbase is enormous!

lol :) Done.

[Aaron Boodman, 2011-10-24 15:39:05]

Mattias' argument seems reasonable. It especially does not make sense to block
component extensions.

This change needs tests.

http://codereview.chromium.org/8380006/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8380006/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1697: void
ExtensionService::CheckAdminBlacklist() {
I don't see any non-test call of this method in Chrome. Should we remove it?

[Mattias Nissler (ping if slow), 2011-10-24 15:44:34]

On 2011/10/24 15:39:05, Aaron Boodman wrote:
> Mattias' argument seems reasonable. It especially does not make sense to block
> component extensions.

That's for sure, and the policy-forced extensions white listing part is really
only that one line in IsExtensionAllowedByPolicy() which I'll remove if we
decide to keep behavior as it is.

> 
> This change needs tests.

I'll write some.

> 
>
http://codereview.chromium.org/8380006/diff/5001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
http://codereview.chromium.org/8380006/diff/5001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_service.cc:1697: void
> ExtensionService::CheckAdminBlacklist() {
> I don't see any non-test call of this method in Chrome. Should we remove it?

[Finnur, 2011-10-24 16:31:04]

> Mattias' argument seems reasonable.

I think we are all on the same page. I was worried about other people, who
contributed to the WontFix bug, but Mattias' comment there I think that's
addressed, or at least made visible to those people.

I'm fine with this change when Aaron is happy with it, so I'll give my LGTM.

[Mattias Nissler (ping if slow), 2011-10-24 16:50:31]

I wrote unit tests and make sure they fail if the patch is not present. Please
take another look.

http://codereview.chromium.org/8380006/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8380006/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1697: void
ExtensionService::CheckAdminBlacklist() {
On 2011/10/24 15:39:05, Aaron Boodman wrote:
> I don't see any non-test call of this method in Chrome. Should we remove it?

It's called from the pref change handler in order to uninstall extensions when
policy changes at runtime.

[Aaron Boodman, 2011-10-24 18:14:44]

So if an extension is installed, and then becomes blacklisted by admin policy,
do you uninstall it, or only stop loading it?

The reason that I ask is that this feature is similar to Chrome's internal
extension blacklist, but works differently. The internal blacklist is enforced
by uninstalling the offending extension when the change is noticed. There is no
code in the load path.

I don't have a strong sense of which way is better, but I'd rather that similar
features be implemented similar, just for sanity's sake. Unless there's some
reason not to.

[Mattias Nissler (ping if slow), 2011-10-25 09:30:54]

On 2011/10/24 18:14:44, Aaron Boodman wrote:
> So if an extension is installed, and then becomes blacklisted by admin policy,
> do you uninstall it, or only stop loading it?

We just stop loading it. Thinking some more about this, it might be nicer if we
did something similar to the way we handle plugins: Keep showing the extension
in the extension list, but put a note that it's been disabled through policy.
Would that be doable?

> The reason that I ask is that this feature is similar to Chrome's internal
> extension blacklist, but works differently. The internal blacklist is enforced
> by uninstalling the offending extension when the change is noticed. There is
no
> code in the load path.

It may happen that policy changes when Chrome is not running. So we need to be
prepared in to load path to make the right decision in case policy has changed.
In theory, we could just load all extensions first and then call
ExtensionService::CheckAdminBlacklist(), but IMHO not loading in the first place
seems the better choice, given that the admin usually wants to keep some
offending code in the extension from executing.

> 
> I don't have a strong sense of which way is better, but I'd rather that
similar
> features be implemented similar, just for sanity's sake. Unless there's some
> reason not to.

Fair point. One more thing to consider is sync: Actually uninstalling a
policy-disabled extension would result in sync picking up the change and
removing the extension on all other syncing browser instances as well. This is
clearly not what we want, since policy would then travel through sync to other
machines (think personal account on corp machine that disallows extensions but
sync).

In any case, this CL is about fixing an M16 issue, so in the interest of keeping
it merge-able, I'd like to just fix brokenness and leave additional work to
follow-ups. Good to go?

[Mattias Nissler (ping if slow), 2011-10-26 08:52:37]

On 2011/10/25 09:30:54, Mattias Nissler wrote:
> On 2011/10/24 18:14:44, Aaron Boodman wrote:
> > So if an extension is installed, and then becomes blacklisted by admin
policy,
> > do you uninstall it, or only stop loading it?
> 
> We just stop loading it. Thinking some more about this, it might be nicer if
we
> did something similar to the way we handle plugins: Keep showing the extension
> in the extension list, but put a note that it's been disabled through policy.
> Would that be doable?
> 
> > The reason that I ask is that this feature is similar to Chrome's internal
> > extension blacklist, but works differently. The internal blacklist is
enforced
> > by uninstalling the offending extension when the change is noticed. There is
> no
> > code in the load path.
> 
> It may happen that policy changes when Chrome is not running. So we need to be
> prepared in to load path to make the right decision in case policy has
changed.
> In theory, we could just load all extensions first and then call
> ExtensionService::CheckAdminBlacklist(), but IMHO not loading in the first
place
> seems the better choice, given that the admin usually wants to keep some
> offending code in the extension from executing.
> 
> > 
> > I don't have a strong sense of which way is better, but I'd rather that
> similar
> > features be implemented similar, just for sanity's sake. Unless there's some
> > reason not to.
> 
> Fair point. One more thing to consider is sync: Actually uninstalling a
> policy-disabled extension would result in sync picking up the change and
> removing the extension on all other syncing browser instances as well. This is
> clearly not what we want, since policy would then travel through sync to other
> machines (think personal account on corp machine that disallows extensions but
> sync).
> 
> In any case, this CL is about fixing an M16 issue, so in the interest of
keeping
> it merge-able, I'd like to just fix brokenness and leave additional work to
> follow-ups. Good to go?

Aaron: friendly ping

[Aaron Boodman, 2011-10-27 16:53:05]

On 2011/10/26 08:52:37, Mattias Nissler wrote:
> On 2011/10/25 09:30:54, Mattias Nissler wrote:
> > On 2011/10/24 18:14:44, Aaron Boodman wrote:
> > > So if an extension is installed, and then becomes blacklisted by admin
> policy,
> > > do you uninstall it, or only stop loading it?
> > 
> > We just stop loading it. Thinking some more about this, it might be nicer if
> we
> > did something similar to the way we handle plugins: Keep showing the
extension
> > in the extension list, but put a note that it's been disabled through
policy.
> > Would that be doable?
> > 
> > > The reason that I ask is that this feature is similar to Chrome's internal
> > > extension blacklist, but works differently. The internal blacklist is
> enforced
> > > by uninstalling the offending extension when the change is noticed. There
is
> > no
> > > code in the load path.
> > 
> > It may happen that policy changes when Chrome is not running. So we need to
be
> > prepared in to load path to make the right decision in case policy has
> changed.
> > In theory, we could just load all extensions first and then call
> > ExtensionService::CheckAdminBlacklist(), but IMHO not loading in the first
> place
> > seems the better choice, given that the admin usually wants to keep some
> > offending code in the extension from executing.
> > 
> > > 
> > > I don't have a strong sense of which way is better, but I'd rather that
> > similar
> > > features be implemented similar, just for sanity's sake. Unless there's
some
> > > reason not to.
> > 
> > Fair point. One more thing to consider is sync: Actually uninstalling a
> > policy-disabled extension would result in sync picking up the change and
> > removing the extension on all other syncing browser instances as well. This
is
> > clearly not what we want, since policy would then travel through sync to
other
> > machines (think personal account on corp machine that disallows extensions
but
> > sync).
> > 
> > In any case, this CL is about fixing an M16 issue, so in the interest of
> keeping
> > it merge-able, I'd like to just fix brokenness and leave additional work to
> > follow-ups. Good to go?
> 
> Aaron: friendly ping

Sorry, did not see your reply. LGTM.

Can you please add a comment where you check this that the changes can happen
while chrome is not running and that is why we're checking there?

A year from now when someone cleans up this code, at least it will be clear why
it is done the way it is done.

Regarding your UI suggestion: possible, yeah. practical, not really. The
extension-service related code right now is so hairy, I'm really reluctant to
allow any additional complexity at all until we get it untangled.

[Mattias Nissler (ping if slow), 2011-10-27 17:49:28]

On 2011/10/27 16:53:05, Aaron Boodman wrote:
> On 2011/10/26 08:52:37, Mattias Nissler wrote:
> > On 2011/10/25 09:30:54, Mattias Nissler wrote:
> > > On 2011/10/24 18:14:44, Aaron Boodman wrote:
> > > > So if an extension is installed, and then becomes blacklisted by admin
> > policy,
> > > > do you uninstall it, or only stop loading it?
> > > 
> > > We just stop loading it. Thinking some more about this, it might be nicer
if
> > we
> > > did something similar to the way we handle plugins: Keep showing the
> extension
> > > in the extension list, but put a note that it's been disabled through
> policy.
> > > Would that be doable?
> > > 
> > > > The reason that I ask is that this feature is similar to Chrome's
internal
> > > > extension blacklist, but works differently. The internal blacklist is
> > enforced
> > > > by uninstalling the offending extension when the change is noticed.
There
> is
> > > no
> > > > code in the load path.
> > > 
> > > It may happen that policy changes when Chrome is not running. So we need
to
> be
> > > prepared in to load path to make the right decision in case policy has
> > changed.
> > > In theory, we could just load all extensions first and then call
> > > ExtensionService::CheckAdminBlacklist(), but IMHO not loading in the first
> > place
> > > seems the better choice, given that the admin usually wants to keep some
> > > offending code in the extension from executing.
> > > 
> > > > 
> > > > I don't have a strong sense of which way is better, but I'd rather that
> > > similar
> > > > features be implemented similar, just for sanity's sake. Unless there's
> some
> > > > reason not to.
> > > 
> > > Fair point. One more thing to consider is sync: Actually uninstalling a
> > > policy-disabled extension would result in sync picking up the change and
> > > removing the extension on all other syncing browser instances as well.
This
> is
> > > clearly not what we want, since policy would then travel through sync to
> other
> > > machines (think personal account on corp machine that disallows extensions
> but
> > > sync).
> > > 
> > > In any case, this CL is about fixing an M16 issue, so in the interest of
> > keeping
> > > it merge-able, I'd like to just fix brokenness and leave additional work
to
> > > follow-ups. Good to go?
> > 
> > Aaron: friendly ping
> 
> Sorry, did not see your reply. LGTM.
> 
> Can you please add a comment where you check this that the changes can happen
> while chrome is not running and that is why we're checking there?
> 
> A year from now when someone cleans up this code, at least it will be clear
why
> it is done the way it is done.

Added the comment in ExtensionService::LoadInstalledExtension(). I plan to land
(and hopefully merge) this tomorrow.

> Regarding your UI suggestion: possible, yeah. practical, not really. The
> extension-service related code right now is so hairy, I'm really reluctant to
> allow any additional complexity at all until we get it untangled.

I see. I'll keep in mind for the future.

[commit-bot: I haz the power, 2011-10-28 09:05:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/8380006/9002

[commit-bot: I haz the power, 2011-10-28 10:12:39]

Change committed as 107707