Add recommended extension installation support

Add recommended extension installation support

Extensions on recommended installation list will be automatically installed just like forced installed extensions, but are allowed to be disabled (not allowed to be uninstalled).

This CL adds a new ExternalLoader for recommended extension (use EXTERNAL_PREF_DOWNLOAD as location), another MustRemainInstalled() function to interface of ManagementPolicy, and related WebUI changes.

BUG=177351
Committed: https://crrev.com/cccacefd3fc1ff73b976144fe1aeb74e47e848e6
Cr-Commit-Position: refs/heads/master@{#299336}

[binjin, 2014-09-17 19:26:24]

binjin@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[Joao da Silva, 2014-09-18 12:21:23]

Make the CL description more informative and add a BUG entry for this.

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:152:
scoped_ptr<base::DictionaryValue> GetAutoInstallList(bool forced) const;
Use two methods instead of having the argument:

scoped_ptr<base::DictionaryValue> GetForceInstallList() const;

scoped_ptr<base::DictionaryValue> GetRecommendedInstallList() const;

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_policy_loader.h (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.h:27: enum InstallationType {
FORCED, RECOMMENDED };
Break each constant into a line and document what it does

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_provider_impl.cc:454:
Extension::WAS_INSTALLED_BY_DEFAULT)));
Why? The documentation for this flag says:

"|WAS_INSTALLED_BY_DEFAULT| installed by default when the profile was created"

But the extension may be installed much later, when the policy for the profile
is updated.

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/standard_management_policy_provider.cc:148: return
false;
Why? Shouldn't this be true?

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

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:243:
management_policy_->MustRemainInstalled(extension, NULL);
This is conflating logic for different things. It would be much clearer if
management_policy_ simply had 2 new calls: IsManagedInstall() and
IsRecommendedInstall()

[binjin, 2014-09-18 16:25:16]

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:152:
scoped_ptr<base::DictionaryValue> GetAutoInstallList(bool forced) const;
On 2014/09/18 12:21:23, Joao da Silva wrote:
> Use two methods instead of having the argument:
> 
> scoped_ptr<base::DictionaryValue> GetForceInstallList() const;
> 
> scoped_ptr<base::DictionaryValue> GetRecommendedInstallList() const;

Done. I choosed to duplicated code here. since adding helper function to private
section will complicated interface again.

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_policy_loader.h (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.h:27: enum InstallationType {
FORCED, RECOMMENDED };
On 2014/09/18 12:21:23, Joao da Silva wrote:
> Break each constant into a line and document what it does

Done.

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_provider_impl.cc:454:
Extension::WAS_INSTALLED_BY_DEFAULT)));
On 2014/09/18 12:21:23, Joao da Silva wrote:
> Why? The documentation for this flag says:
> 
> "|WAS_INSTALLED_BY_DEFAULT| installed by default when the profile was created"
> 
> But the extension may be installed much later, when the policy for the profile
> is updated.

Done.

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/standard_management_policy_provider.cc:148: return
false;
On 2014/09/18 12:21:23, Joao da Silva wrote:
> Why? Shouldn't this be true?

Done.

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

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:243:
management_policy_->MustRemainInstalled(extension, NULL);
On 2014/09/18 12:21:23, Joao da Silva wrote:
> This is conflating logic for different things. It would be much clearer if
> management_policy_ simply had 2 new calls: IsManagedInstall() and
> IsRecommendedInstall()

How about management_policy_->UserMayModifySettings() &&
management_policy_->MustRemainInstalled?

[binjin, 2014-09-18 17:44:07]

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

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:243:
management_policy_->MustRemainInstalled(extension, NULL);
On 2014/09/18 16:25:16, bjin wrote:
> On 2014/09/18 12:21:23, Joao da Silva wrote:
> > This is conflating logic for different things. It would be much clearer if
> > management_policy_ simply had 2 new calls: IsManagedInstall() and
> > IsRecommendedInstall()
> 
> How about management_policy_->UserMayModifySettings() &&
> management_policy_->MustRemainInstalled?

Okay, I got what you mean. But I still don't think it's a good idea to modify
interfaces of ManagementPolicy. What do you think of accessing
ExtensinManagement service here?

[Joao da Silva, 2014-09-19 07:55:52]

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

https://codereview.chromium.org/536753003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:243:
management_policy_->MustRemainInstalled(extension, NULL);
On 2014/09/18 17:44:07, bjin wrote:
> On 2014/09/18 16:25:16, bjin wrote:
> > On 2014/09/18 12:21:23, Joao da Silva wrote:
> > > This is conflating logic for different things. It would be much clearer if
> > > management_policy_ simply had 2 new calls: IsManagedInstall() and
> > > IsRecommendedInstall()
> > 
> > How about management_policy_->UserMayModifySettings() &&
> > management_policy_->MustRemainInstalled?
> 
> Okay, I got what you mean. But I still don't think it's a good idea to modify
> interfaces of ManagementPolicy. What do you think of accessing
> ExtensinManagement service here?

The point is that ManagementPolicy has more than one provider (including
supervised users, device local accounts, and the normal policy source). This
code must handle extensions that are forced-installed from any of those
providers.

So I really think the right fix is to let the ManagementPolicy class tell
exactly: is this extension a mandatory or a recommended install.

Anyway, I think the current code is fine but a bit subtle. It's not a big
problem for now.

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

https://codereview.chromium.org/536753003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:293: by_id->installation_mode
= INSTALLATION_RECOMMENDED;
Why? This is the force list, so should be FORCED here.

[binjin, 2014-09-19 09:25:28]

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

https://codereview.chromium.org/536753003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:293: by_id->installation_mode
= INSTALLATION_RECOMMENDED;
On 2014/09/19 07:55:52, Joao da Silva wrote:
> Why? This is the force list, so should be FORCED here.

it's added by mistake, it's for manual testing.

[Joao da Silva, 2014-09-19 09:27:48]

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

https://codereview.chromium.org/536753003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:293: by_id->installation_mode
= INSTALLATION_RECOMMENDED;
On 2014/09/19 09:25:28, bjin wrote:
> On 2014/09/19 07:55:52, Joao da Silva wrote:
> > Why? This is the force list, so should be FORCED here.
> 
> it's added by mistake, it's for manual testing.

Ok. Please remove it and send an updated CL when ready.

[binjin, 2014-09-19 10:09:56]

Forgot to mention that there are additional changes on
extension_settings_handler.

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

https://codereview.chromium.org/536753003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:293: by_id->installation_mode
= INSTALLATION_RECOMMENDED;
On 2014/09/19 09:27:48, Joao da Silva wrote:
> On 2014/09/19 09:25:28, bjin wrote:
> > On 2014/09/19 07:55:52, Joao da Silva wrote:
> > > Why? This is the force list, so should be FORCED here.
> > 
> > it's added by mistake, it's for manual testing.
> 
> Ok. Please remove it and send an updated CL when ready.

Done.

[Joao da Silva, 2014-09-19 12:14:04]

lgtm

Have a look at PolicyTest.ExtensionInstallForcelist in policy_browsertest.cc. We
should add a new test there for the recommended install, that verifies that an
extension can't be uninstalled but can be disabled. If you can add that in this
CL it would be awesome, but it's fine to add in a follow-up CL.

[Joao da Silva, 2014-09-25 09:43:22]

joaodasilva@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[Joao da Silva, 2014-09-25 09:43:22]

+pastarmovj to review while I'm away.

This CL is ready to go from my side but Bin may be working on a new test. Feel
free to wait for an update or do a 2nd pass.

[pastarmovj, 2014-10-02 10:13:44]

lgtm

[pastarmovj, 2014-10-02 10:13:46]

lgtm

[Joao da Silva, 2014-10-06 13:19:30]

What is the status of this CL?

[binjin, 2014-10-06 13:23:17]

On 2014/10/06 13:19:30, Joao da Silva wrote:
> What is the status of this CL?

I didn't land this CL two weeks ago due to the branch cut, and I'm working on
permissions stuff last week. I plan to land this CL these two days.

[binjin, 2014-10-06 13:25:40]

binjin@chromium.org changed reviewers:
+ finnur@chromium.org

[binjin, 2014-10-06 13:25:41]

Finnur, could you have a look at this CL? Please pay attention that I made some
changes to interface of ManagementPolicy.

Thanks

[Joao da Silva, 2014-10-06 13:48:11]

Can you add a new test in policy_browsertest for this policy?

[binjin, 2014-10-06 14:00:16]

On 2014/10/06 13:48:11, Joao da Silva wrote:
> Can you add a new test in policy_browsertest for this policy?

Oh, thanks for reminding. I agree that this CL better comes with a browser test.
But then it's better to land the actual policy first.

@Finnur, sorry that I might add a browser test later(this CL is manually tested
though). But instead of entire owner code review, could you have a glance at
changes to ManagementPolicy?

[Finnur, 2014-10-06 14:01:42]

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:88: // instead.
I don't get it... 
GetRecommendedInstallList is like GetRecommendedInstallList?

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/external_policy_loader.cc (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/external_policy_loader.cc:40: prefs_ =
settings_->GetRecommendedInstallList();
I usually prefer a switch with no default: case for things like this. Then the
person who adds a new value to the InstallationType enum gets a compile error
here and realizes the need to update this function.

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.cc:142: //
Disallowing removing of recommended extension, to avoid re-install it
nit: s/Disallowing/Disallow/

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:40: *           
recommendedInstall: boolean,
It is pretty clear this list should be alphabetically ordered. :)

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:400: // Then the 'managed,
cannot uninstall/disable' message.
nit: This comment is out of date now, no?

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

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:246: // We
should not get into a state where both are true.
Nit: This comment is now out of date. Perhaps:

// Suspicious install should always be mutually exclusive to
// managed and/or recommended install.

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1144: if
(!management_policy_->UserMayModifySettings(extension, NULL)) {
We could at some point get the css wrong or the user might change it through the
web inspector, allowing the trash icon to become enabled. Now, I may be
misreading this CL, but as I understand it, UserMayModifySettings is true for
recommended policy managed extensions. Don't we need to check if they are
allowed to uninstall the extension instead?

https://codereview.chromium.org/536753003/diff/100001/extensions/browser/test...
File extensions/browser/test_management_policy.h (right):

https://codereview.chromium.org/536753003/diff/100001/extensions/browser/test...
extensions/browser/test_management_policy.h:25: MUST_REMAIN_INSTALLED = 1 << 4
nit: Add a trailing comma, so that the next time we add a value it doesn't have
to change line 25 (less churn).

[binjin, 2014-10-06 18:48:48]

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_management.h:88: // instead.
On 2014/10/06 14:01:42, Finnur wrote:
> I don't get it... 
> GetRecommendedInstallList is like GetRecommendedInstallList?

Ooops, it's a mistake due to auto-completion. Done.

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/external_policy_loader.cc (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/external_policy_loader.cc:40: prefs_ =
settings_->GetRecommendedInstallList();
On 2014/10/06 14:01:42, Finnur wrote:
> I usually prefer a switch with no default: case for things like this. Then the
> person who adds a new value to the InstallationType enum gets a compile error
> here and realizes the need to update this function.

Done.

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/standard_management_policy_provider.cc (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/standard_management_policy_provider.cc:142: //
Disallowing removing of recommended extension, to avoid re-install it
On 2014/10/06 14:01:42, Finnur wrote:
> nit: s/Disallowing/Disallow/

Done.

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:40: *           
recommendedInstall: boolean,
On 2014/10/06 14:01:42, Finnur wrote:
> It is pretty clear this list should be alphabetically ordered. :)

Done.

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_list.js:400: // Then the 'managed,
cannot uninstall/disable' message.
On 2014/10/06 14:01:42, Finnur wrote:
> nit: This comment is out of date now, no?

I think the comment looks fine to me. It's not restrictive as 'cannot uninstall
and disable'.

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

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:246: // We
should not get into a state where both are true.
On 2014/10/06 14:01:42, Finnur wrote:
> Nit: This comment is now out of date. Perhaps:
> 
> // Suspicious install should always be mutually exclusive to
> // managed and/or recommended install.

Done.

https://codereview.chromium.org/536753003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:1144: if
(!management_policy_->UserMayModifySettings(extension, NULL)) {
On 2014/10/06 14:01:42, Finnur wrote:
> We could at some point get the css wrong or the user might change it through
the
> web inspector, allowing the trash icon to become enabled. Now, I may be
> misreading this CL, but as I understand it, UserMayModifySettings is true for
> recommended policy managed extensions. Don't we need to check if they are
> allowed to uninstall the extension instead?

Done.

https://codereview.chromium.org/536753003/diff/100001/extensions/browser/test...
File extensions/browser/test_management_policy.h (right):

https://codereview.chromium.org/536753003/diff/100001/extensions/browser/test...
extensions/browser/test_management_policy.h:25: MUST_REMAIN_INSTALLED = 1 << 4
On 2014/10/06 14:01:42, Finnur wrote:
> nit: Add a trailing comma, so that the next time we add a value it doesn't
have
> to change line 25 (less churn).

Done.

[Finnur, 2014-10-06 19:35:28]

LGTM, with one comment.

https://codereview.chromium.org/536753003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/external_policy_loader.cc (right):

https://codereview.chromium.org/536753003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/external_policy_loader.cc:46: NOTREACHED();
Leaving out the default case altogether is better than adding a default with a
NOTREACHED, because then the mistake (forgetting to add to this switch
statement) is caught at compile time, and not at runtime...

[binjin, 2014-10-07 09:29:23]

https://codereview.chromium.org/536753003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/external_policy_loader.cc (right):

https://codereview.chromium.org/536753003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/external_policy_loader.cc:46: NOTREACHED();
On 2014/10/06 19:35:27, Finnur wrote:
> Leaving out the default case altogether is better than adding a default with a
> NOTREACHED, because then the mistake (forgetting to add to this switch
> statement) is caught at compile time, and not at runtime...

Done.

[binjin, 2014-10-08 16:05:33]

Hello Joao,

PTAL at the new browser test I added, I also fixed a bug which cause the
recommended policy not working in tests.

[Joao da Silva, 2014-10-08 16:52:30]

Nice test. lgtm with a little typo

https://codereview.chromium.org/536753003/diff/220001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/536753003/diff/220001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1779: // But the user are allowed to
disable them.
is allowed

[Joao da Silva, 2014-10-08 16:53:31]

Seems like the build fails because the ExtensionSettings policy hasn't been
added yet. You may have to land the other CL first.

[binjin, 2014-10-09 12:05:20]

https://codereview.chromium.org/536753003/diff/220001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/536753003/diff/220001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:1779: // But the user are allowed to
disable them.
On 2014/10/08 16:52:30, Joao da Silva wrote:
> is allowed

Done.

[binjin, 2014-10-09 18:09:44]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-10-09 18:10:15]

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

[commit-bot: I haz the power, 2014-10-09 20:11:04]

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

[commit-bot: I haz the power, 2014-10-09 20:11:05]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[binjin, 2014-10-13 17:58:34]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-10-13 17:59:32]

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

[commit-bot: I haz the power, 2014-10-13 19:00:39]

Committed patchset #14 (id:420001)

[commit-bot: I haz the power, 2014-10-13 19:01:29]

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