Supervised users: Slightly relax restrictions around extensions.

Supervised users: Slightly relax restrictions around extensions.

Previously, SupervisedUserService implemented extensions::ManagementPolicy::Provider::UserMayModifySettings, preventing any modifications to extensions (including e.g. disabling on permissions increase). This CL switches to MustRemainInstalled, to prevent uninstallation of custodian-installed extensions, and disables all UI that would allow re-enabling.

As a side-effect, this fixes crbug.com/458082 :)

BUG=397951, 458082, 461261
Committed: https://crrev.com/bb9a196b882764b9984ae1b924683361fb00ea09
Cr-Commit-Position: refs/heads/master@{#318037}

[Marc Treib, 2015-02-12 17:21:49]

treib@chromium.org changed reviewers:
+ pam@chromium.org

[Marc Treib, 2015-02-12 17:21:50]

PTAL when you have some spare time! I'll add extensions OWNERS after you're
satisfied.

[Pam (message me for reviews), 2015-02-23 10:18:50]

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:310: // TODO(treib): Add an
extra message for supervised users?
+1

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:369: // supervised profile.
What other kind of extension in a supervised profile should it exist for? Surely
not enterprise-forced. Or is this just allowing for a future change, e.g. if we
decide that permissionless extensions can be freely installed?

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:360: var
enableCheckboxDisabled = extension.managedInstall ||
Maintainability = ick. Can we get most of this from UserMayModifySettings?

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:98: bool
was_installed_by_default = extension->was_installed_by_default();
If indeed |extension| can be NULL in unit tests, kablooie.

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:506: bool
may_uninstall = (result != EXTENSION_FORCED);
Please change the sense of this (i.e. use may_not_uninstall). The number of
layered negatives is hard to parse.

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1091:
extension->was_installed_by_custodian()) {
It really seems like being installed by a custodian should mean
UserMayModifySettings = false, which would simplify both these.

[Marc Treib, 2015-02-23 12:07:38]

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:310: // TODO(treib): Add an
extra message for supervised users?
On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> +1

I'd like to do that in a different CL, though.

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:369: // supervised profile.
On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> What other kind of extension in a supervised profile should it exist for?
Surely
> not enterprise-forced. Or is this just allowing for a future change, e.g. if
we
> decide that permissionless extensions can be freely installed?

Or if we decide to have an "allow everything, even if it requires permissions"
mode. Either way, yes, this is meant for future changes.

There are also default-installed extensions, but for those, this UI doesn't show
up anyway.

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_list.js:360: var
enableCheckboxDisabled = extension.managedInstall ||
On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> Maintainability = ick. Can we get most of this from UserMayModifySettings?

Agreed re maintainability, but I don't see a better way.

I removed SupervisedUserService::UserMayModifySettings because that means the
extension can never be disabled, even if it requires new permissions. (The
"User" in the name is misleading :-/)

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:98: bool
was_installed_by_default = extension->was_installed_by_default();
On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> If indeed |extension| can be NULL in unit tests, kablooie.

Huh. Since I got green try runs, looks like extension can't actually be null.
I've removed the check and comment above. Good catch!

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:506: bool
may_uninstall = (result != EXTENSION_FORCED);
On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> Please change the sense of this (i.e. use may_not_uninstall). The number of
> layered negatives is hard to parse.

Done.

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1091:
extension->was_installed_by_custodian()) {
On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> It really seems like being installed by a custodian should mean
> UserMayModifySettings = false, which would simplify both these.

As mentioned above: UserMayModifySettings = false really means "nothing may ever
change the enabled state of this extension, ever", which breaks our "update with
new permissions" flow.

[Pam (message me for reviews), 2015-02-23 12:20:17]

LGTM + comment request

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1091:
extension->was_installed_by_custodian()) {
On 2015/02/23 12:07:38, Marc Treib wrote:
> On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> > It really seems like being installed by a custodian should mean
> > UserMayModifySettings = false, which would simplify both these.
> 
> As mentioned above: UserMayModifySettings = false really means "nothing may
ever
> change the enabled state of this extension, ever", which breaks our "update
with
> new permissions" flow.

So that's something we should sort out fairly soon -- figure out whether it
really should be only user-initiated changes, and then either fix the semantics
or fix the name.  But not necessarily for this CL. For now, a comment on the
method in management_policy.h would be good.

[Marc Treib, 2015-02-23 12:30:37]

New patchsets have been uploaded after l-g-t-m from pam@chromium.org

[Marc Treib, 2015-02-23 12:33:59]

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/925563002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1091:
extension->was_installed_by_custodian()) {
On 2015/02/23 12:20:17, Pam (also PM for reviews) wrote:
> On 2015/02/23 12:07:38, Marc Treib wrote:
> > On 2015/02/23 10:18:50, Pam (also PM for reviews) wrote:
> > > It really seems like being installed by a custodian should mean
> > > UserMayModifySettings = false, which would simplify both these.
> > 
> > As mentioned above: UserMayModifySettings = false really means "nothing may
> ever
> > change the enabled state of this extension, ever", which breaks our "update
> with
> > new permissions" flow.
> 
> So that's something we should sort out fairly soon -- figure out whether it
> really should be only user-initiated changes, and then either fix the
semantics
> or fix the name.  But not necessarily for this CL. For now, a comment on the
> method in management_policy.h would be good.

Done.

[Marc Treib, 2015-02-23 12:35:51]

treib@chromium.org changed reviewers:
+ benwells@chromium.org

[Marc Treib, 2015-02-23 12:35:51]

+benwells for OWNERS review of extensions changes.
Please look at:
chrome/browser/extensions/extension_disabled_ui.cc
chrome/browser/ui/extensions/extension_enable_flow.cc
extensions/browser/management_policy.h
Thanks!

[benwells, 2015-02-24 07:33:47]

https://codereview.chromium.org/925563002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/925563002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:326:
extension_->was_installed_by_custodian()) {
This logic is repeated a few times. You should put this into a well named
function in extension_util.

https://codereview.chromium.org/925563002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:327: // TODO(treib): Probably
use a new string here once we get UX design.
These TODOs are a bit concerning. If this doesn't have UX design, should it be
behind a flag?

If the TODOs stay you sould add a crbug link.

[Marc Treib, 2015-02-24 10:33:40]

https://codereview.chromium.org/925563002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/925563002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:326:
extension_->was_installed_by_custodian()) {
On 2015/02/24 07:33:47, benwells (OOO to 24 Feb) wrote:
> This logic is repeated a few times. You should put this into a well named
> function in extension_util.

The "well-named" is the hard part ;)
Done (I hope)

https://codereview.chromium.org/925563002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_disabled_ui.cc:327: // TODO(treib): Probably
use a new string here once we get UX design.
On 2015/02/24 07:33:47, benwells (OOO to 24 Feb) wrote:
> These TODOs are a bit concerning. If this doesn't have UX design, should it be
> behind a flag?

There's currently no way for a user to trigger this UI, so no need for an
explicit flag.

> If the TODOs stay you sould add a crbug link.

Done.

[benwells, 2015-02-25 03:23:32]

lgtm with the comments addressed.

https://codereview.chromium.org/925563002/diff/80001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/extension_enable_flow.cc (right):

https://codereview.chromium.org/925563002/diff/80001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/extension_enable_flow.cc:91: if
(profile_->IsSupervised() && extension->was_installed_by_custodian()) {
Nit: You should use your cool new function here too.

https://codereview.chromium.org/925563002/diff/80001/extensions/browser/manag...
File extensions/browser/management_policy.h (right):

https://codereview.chromium.org/925563002/diff/80001/extensions/browser/manag...
extensions/browser/management_policy.h:56: // extension installations, not just
user-initiated ones. Fix either the
Please file a bug to cover all these todos as well.

[Marc Treib, 2015-02-25 09:48:36]

Patchset #7 (id:120001) has been deleted

[Marc Treib, 2015-02-25 09:48:56]

https://codereview.chromium.org/925563002/diff/80001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/extension_enable_flow.cc (right):

https://codereview.chromium.org/925563002/diff/80001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/extension_enable_flow.cc:91: if
(profile_->IsSupervised() && extension->was_installed_by_custodian()) {
On 2015/02/25 03:23:32, benwells (OOO to 24 Feb) wrote:
> Nit: You should use your cool new function here too.

Done.

https://codereview.chromium.org/925563002/diff/80001/extensions/browser/manag...
File extensions/browser/management_policy.h (right):

https://codereview.chromium.org/925563002/diff/80001/extensions/browser/manag...
extensions/browser/management_policy.h:56: // extension installations, not just
user-initiated ones. Fix either the
On 2015/02/25 03:23:32, benwells (OOO to 24 Feb) wrote:
> Please file a bug to cover all these todos as well.

Done.

[Marc Treib, 2015-02-25 09:49:30]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-02-25 09:49:30]

The patchset sent to the CQ was uploaded after l-g-t-m from pam@chromium.org,
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/925563002/#ps100001
(title: "review")

[commit-bot: I haz the power, 2015-02-25 09:50:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/925563002/100001

[commit-bot: I haz the power, 2015-02-25 09:56:29]

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

[commit-bot: I haz the power, 2015-02-25 09:56:31]

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

[Marc Treib, 2015-02-25 10:31:32]

Patchset #7 (id:140001) has been deleted

[Marc Treib, 2015-02-25 12:51:24]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-02-25 12:51:24]

The patchset sent to the CQ was uploaded after l-g-t-m from pam@chromium.org,
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/925563002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-02-25 12:52:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/925563002/160001

[commit-bot: I haz the power, 2015-02-25 13:41:10]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-02-25 13:41:40]

Patchset 7 (id:??) landed as
https://crrev.com/bb9a196b882764b9984ae1b924683361fb00ea09
Cr-Commit-Position: refs/heads/master@{#318037}