Apps&Extensions for supervised users: Allow the custodian to remotely uninstall extensions.

Apps&Extensions for supervised users: Allow the custodian to remotely uninstall extensions.

This is a followup to https://codereview.chromium.org/353493002/

BUG=390520
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284744

[Marc Treib, 2014-07-17 15:17:12]

PTAL!

[not at google - send to devlin, 2014-07-17 16:52:59]

looks fine but do you have tests for any of this stuff which you can augment?

And another thing, there are a couple of other places where
UNINSTALL_REASON_SYNC is used:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/app...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

are either of those noteworthy?

[Marc Treib, 2014-07-18 09:47:07]

On 2014/07/17 16:52:59, kalman wrote:
> looks fine but do you have tests for any of this stuff which you can augment?

Hm, I suppose I could add a test for ExtensionService to make sure
UNINSTALL_.._CUSTODIAN (as well as the other "internal" reasons) result in
bypassing the UserMayModifySettings check?
ExtensionSyncService doesn't seem to have any tests.

> And another thing, there are a couple of other places where
> UNINSTALL_REASON_SYNC is used:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/app...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> are either of those noteworthy?

I've added code to handle the installed_by_custodian case there as well.

[not at google - send to devlin, 2014-07-18 14:45:09]

> Hm, I suppose I could add a test for ExtensionService to make sure
> UNINSTALL_.._CUSTODIAN (as well as the other "internal" reasons) result in
> bypassing the UserMayModifySettings check?
> ExtensionSyncService doesn't seem to have any tests.

I guess the test would be that if you try to uninstall an extension that was
installed by a custodian, it doesn't work?

unfortunately extension_service_unittest.cc is quite a maze.

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

https://codereview.chromium.org/405433003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:703: (reason ==
UNINSTALL_REASON_SYNC_BY_CUSTODIAN);
Thanks for updating the other call sites.

With that in mind why do you need to add UNINSTALL_REASON_SYNC_BY_CUSTODIAN at
all, can't this check be just "extension->was_installed_by_custodian()"?

[Marc Treib, 2014-07-21 08:02:02]

On 2014/07/18 14:45:09, kalman wrote:
> > Hm, I suppose I could add a test for ExtensionService to make sure
> > UNINSTALL_.._CUSTODIAN (as well as the other "internal" reasons) result in
> > bypassing the UserMayModifySettings check?
> > ExtensionSyncService doesn't seem to have any tests.
> 
> I guess the test would be that if you try to uninstall an extension that was
> installed by a custodian, it doesn't work?

Right now, supervised users can not uninstall any extensions. We already have a
test for that
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sup...).
Here I'd need to test that if an extension with was_installed_by_custodian is
removed from sync, the uninstall does work. I'll have a look at
extension_service_unittest.cc and see if that's reasonable to add.

> unfortunately extension_service_unittest.cc is quite a maze.
> 
>
https://codereview.chromium.org/405433003/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/405433003/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.cc:703: (reason ==
> UNINSTALL_REASON_SYNC_BY_CUSTODIAN);
> Thanks for updating the other call sites.
> 
> With that in mind why do you need to add UNINSTALL_REASON_SYNC_BY_CUSTODIAN at
> all, can't this check be just "extension->was_installed_by_custodian()"?

No, that would mean that extensions with was_installed_by_custodian can always
be uninstalled. We only want to allow that when the uninstall comes from sync,
i.e. from the custodian.

[Marc Treib, 2014-07-21 16:27:17]

I've added a test. Please take a look!

Also, +stevenjb@chromium.org: Please review changes in
app_list_syncable_service. Thanks!

[not at google - send to devlin, 2014-07-21 16:48:24]

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

https://codereview.chromium.org/405433003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:703: (reason ==
UNINSTALL_REASON_SYNC_BY_CUSTODIAN);
On 2014/07/18 14:45:08, kalman wrote:
> Thanks for updating the other call sites.
> 
> With that in mind why do you need to add UNINSTALL_REASON_SYNC_BY_CUSTODIAN at
> all, can't this check be just "extension->was_installed_by_custodian()"?

I guess I mean 2 things:
- you could not even trigger the uninstall in the first place if
was_installed_by_custodian is true. but maybe this is already the case.
- instead of doing (reason == UNINSTALL_REASON_SYNC_BY_CUSTODIAN) you could do
(reason == UNINSTALL_REASON_SYNC && was_installed_by_custodian).

[stevenjb, 2014-07-22 00:47:58]

https://codereview.chromium.org/405433003/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/405433003/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:128:
service->UninstallExtension(id, reason, NULL);
It seems like this should be automatic, or there should be a helper function for
this, since the logic is not specific to the app list sync code.

[Marc Treib, 2014-07-22 09:30:07]

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

https://codereview.chromium.org/405433003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:703: (reason ==
UNINSTALL_REASON_SYNC_BY_CUSTODIAN);
On 2014/07/21 16:48:23, kalman wrote:
> On 2014/07/18 14:45:08, kalman wrote:
> > Thanks for updating the other call sites.
> > 
> > With that in mind why do you need to add UNINSTALL_REASON_SYNC_BY_CUSTODIAN
at
> > all, can't this check be just "extension->was_installed_by_custodian()"?
> 
> I guess I mean 2 things:
> - you could not even trigger the uninstall in the first place if
> was_installed_by_custodian is true. but maybe this is already the case.
> - instead of doing (reason == UNINSTALL_REASON_SYNC_BY_CUSTODIAN) you could do
> (reason == UNINSTALL_REASON_SYNC && was_installed_by_custodian).

Ah, right, that'll work, and not require changes to all call sites. Thanks,
done!

https://codereview.chromium.org/405433003/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/405433003/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:128:
service->UninstallExtension(id, reason, NULL);
On 2014/07/22 00:47:58, stevenjb wrote:
> It seems like this should be automatic, or there should be a helper function
for
> this, since the logic is not specific to the app list sync code.

Ack. With the change suggested by kalman, this has become unnecessary. Thanks
for the feedback!

[not at google - send to devlin, 2014-07-22 15:41:44]

lgtm, and thanks for adding the test.

[stevenjb, 2014-07-22 15:46:26]

lgtm

[Marc Treib, 2014-07-22 16:15:39]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-07-22 16:16:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/405433003/100001

[commit-bot: I haz the power, 2014-07-22 19:08:19]

Change committed as 284744