Add ExtensionManagement based ExternalLoader

Add ExtensionManagement based ExternalLoader

The previous approach used pref_names::kInstallForceList directly, change it to an observer of ExtensionManagement class instead.

BUG=177351
TEST=ExtensionServiceTest,ExternalPolicyLoaderTest

Committed: https://crrev.com/30301062fce5fe3a7dc02ede5f00f113b41c0578
Cr-Commit-Position: refs/heads/master@{#293777}

[binjin, 2014-09-03 16:12:39]

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

[binjin, 2014-09-03 16:12:40]

Joao,

PTAL at this CL

[Joao da Silva, 2014-09-04 12:07:46]

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_policy_loader.cc (left):

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.cc:50: DCHECK(type ==
chrome::NOTIFICATION_PROFILE_DESTROYED) <<
I wonder why this was here before. Are we sure that these objects are destroyed
in the right order?

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

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.cc:15: :
settings_(ExtensionManagementFactory::GetInstance()
GetInstance() is not needed here, just do

 ExtensionManagementFactory::GetForBrowserContext(profile)

I think this should also be moved to the owner of the object, instead of passing
the Profile* to this constructor.

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.cc:34: update_url);
Move this method to ExtensionManagement too.

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

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.h:27: explicit
ExternalPolicyLoader(Profile* profile);
This doesn't really need a Profile, just an ExtensionManagement* handle. Pass
that directly here instead, so that the interface clearly indicates what it uses
and tests also become simpler (no need to have a TestingProfile, just a
TestingPrefs)

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

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/install_verifier.cc:327:
->GetForBrowserContext(context_)
ExtensionManagementFactory::GetForBrowserContext

[binjin, 2014-09-04 14:27:06]

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_policy_loader.cc (left):

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.cc:50: DCHECK(type ==
chrome::NOTIFICATION_PROFILE_DESTROYED) <<
On 2014/09/04 12:07:46, Joao da Silva wrote:
> I wonder why this was here before. Are we sure that these objects are
destroyed
> in the right order?

Not sure. Does the destroyed order of ExtensionManagement matters, like here?

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

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.cc:15: :
settings_(ExtensionManagementFactory::GetInstance()
On 2014/09/04 12:07:46, Joao da Silva wrote:
> GetInstance() is not needed here, just do
> 
>  ExtensionManagementFactory::GetForBrowserContext(profile)
> 
> I think this should also be moved to the owner of the object, instead of
passing
> the Profile* to this constructor.

Done.

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.cc:34: update_url);
On 2014/09/04 12:07:46, Joao da Silva wrote:
> Move this method to ExtensionManagement too.

I think it's better to move this to ExternalProviderImpl or ExternalProvider,
the format was generally used and actually defined in ExternalProviderImpl. If
not, leaving it here is still better than moving it to ExternsionManagement, the
latter one will eventually drop support of legacy forcelist policy.

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

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.h:27: explicit
ExternalPolicyLoader(Profile* profile);
On 2014/09/04 12:07:46, Joao da Silva wrote:
> This doesn't really need a Profile, just an ExtensionManagement* handle. Pass
> that directly here instead, so that the interface clearly indicates what it
uses
> and tests also become simpler (no need to have a TestingProfile, just a
> TestingPrefs)

Done. But I don't know how to simplify the tests: seems that initialization of
ExternalProviderImpl requires a Profile.

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

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/install_verifier.cc:327:
->GetForBrowserContext(context_)
On 2014/09/04 12:07:46, Joao da Silva wrote:
> ExtensionManagementFactory::GetForBrowserContext

Done.

[Joao da Silva, 2014-09-04 16:55:30]

lgtm

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/external_policy_loader.cc (left):

https://codereview.chromium.org/536573003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/external_policy_loader.cc:50: DCHECK(type ==
chrome::NOTIFICATION_PROFILE_DESTROYED) <<
On 2014/09/04 14:27:06, bjin wrote:
> On 2014/09/04 12:07:46, Joao da Silva wrote:
> > I wonder why this was here before. Are we sure that these objects are
> destroyed
> > in the right order?
> 
> Not sure. Does the destroyed order of ExtensionManagement matters, like here?

I just worry that |settings_| may be a stray pointers at ~ExternalPolicyLoader.

I looked around in the code and it seems that won't happen, so let's keep this
change as is.

[binjin, 2014-09-05 10:15:11]

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

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

Finnur, please take a look at chrome/browser/chromeos/extensions/* and
chrome/browser/extensions/* changes.

Thanks,

-bjin

[Finnur, 2014-09-05 10:27:14]

I presume Joao is verifying for correctness and I'm providing more of an OWNERS
check. 
As such, LGTM.

[binjin, 2014-09-05 15:27:29]

The CQ bit was checked by binjin@chromium.org

[binjin, 2014-09-05 15:27:52]

The CQ bit was unchecked by binjin@chromium.org

[binjin, 2014-09-05 18:49:16]

Hello Joao,

I pushed another patchset, fixing a broken browser_tests, PTAL.

-bjin

[Joao da Silva, 2014-09-07 14:30:21]

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_browsertest.cc (right):

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_browsertest.cc:514:
browser()->profile())->AllowUserPreferenceForTesting();
Don't do this. The right way to mock policies for tests is to use
BrowserPolicyConnector::SetPolicyProviderForTesting. See policy_browsertest.cc
for an example.

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

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:128: void
AllowUserPreferenceForTesting() const;
Revert these changes. See the comment on the browsertest for a better way to do
this.

[binjin, 2014-09-08 12:59:39]

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_browsertest.cc (right):

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_browsertest.cc:514:
browser()->profile())->AllowUserPreferenceForTesting();
On 2014/09/07 14:30:21, Joao da Silva wrote:
> Don't do this. The right way to mock policies for tests is to use
> BrowserPolicyConnector::SetPolicyProviderForTesting. See policy_browsertest.cc
> for an example.

It's browser tests for extensions system, user preference kInstallForcelist was
used here, I don't think it's easy to integrate PolicyProviders here.

Another options is providing SetForceInstallListForTesting(), what do you think?

[Joao da Silva, 2014-09-08 13:18:29]

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_browsertest.cc (right):

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_browsertest.cc:514:
browser()->profile())->AllowUserPreferenceForTesting();
On 2014/09/08 12:59:39, bjin wrote:
> On 2014/09/07 14:30:21, Joao da Silva wrote:
> > Don't do this. The right way to mock policies for tests is to use
> > BrowserPolicyConnector::SetPolicyProviderForTesting. See
policy_browsertest.cc
> > for an example.
> 
> It's browser tests for extensions system, user preference kInstallForcelist
was
> used here, I don't think it's easy to integrate PolicyProviders here.
> 
> Another options is providing SetForceInstallListForTesting(), what do you
think?

This test code needs to be updated, because it was relying on setting that pref
at a user level but it should be set at a mandatory policy level.

The way to do that in a test is with a MockConfigurationPolicyProvider passed to
BrowserPolicyConnector::SetPolicyProviderForTesting, and then you can set any
policies in that mock object. policy_browsertest.cc has example code that you
can just copy and paste here.

Note that this test is testing how the extensions code handles an
ExternalPolicyRefresh, so it's fair to require some setup for the policy parts.

[binjin, 2014-09-08 14:52:05]

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_browsertest.cc (right):

https://codereview.chromium.org/536573003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_browsertest.cc:514:
browser()->profile())->AllowUserPreferenceForTesting();
On 2014/09/08 13:18:29, Joao da Silva wrote:
> On 2014/09/08 12:59:39, bjin wrote:
> > On 2014/09/07 14:30:21, Joao da Silva wrote:
> > > Don't do this. The right way to mock policies for tests is to use
> > > BrowserPolicyConnector::SetPolicyProviderForTesting. See
> policy_browsertest.cc
> > > for an example.
> > 
> > It's browser tests for extensions system, user preference kInstallForcelist
> was
> > used here, I don't think it's easy to integrate PolicyProviders here.
> > 
> > Another options is providing SetForceInstallListForTesting(), what do you
> think?
> 
> This test code needs to be updated, because it was relying on setting that
pref
> at a user level but it should be set at a mandatory policy level.
> 
> The way to do that in a test is with a MockConfigurationPolicyProvider passed
to
> BrowserPolicyConnector::SetPolicyProviderForTesting, and then you can set any
> policies in that mock object. policy_browsertest.cc has example code that you
> can just copy and paste here.
> 
> Note that this test is testing how the extensions code handles an
> ExternalPolicyRefresh, so it's fair to require some setup for the policy
parts.

Got it. Done.

[Joao da Silva, 2014-09-08 14:59:47]

Thanks for cleaning up the test. lgtm

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_browsertest.cc (right):

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_browsertest.cc:65: void
UpdateProviderPolicy(const PolicyMap& policy) {
This is a helper method like the method below, so it should also be in the
protected: section

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_browsertest.cc:67:
DCHECK(base::MessageLoop::current());
This DCHECK isn't needed (nor #include message_loop.h)

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_browsertest.cc:69:
loop.RunUntilIdle();
These 2 lines can be combined:

base::RunLoop().RunUntilIdle();

[binjin, 2014-09-08 15:08:47]

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_browsertest.cc (right):

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_browsertest.cc:65: void
UpdateProviderPolicy(const PolicyMap& policy) {
On 2014/09/08 14:59:47, Joao da Silva wrote:
> This is a helper method like the method below, so it should also be in the
> protected: section

Done.

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_browsertest.cc:67:
DCHECK(base::MessageLoop::current());
On 2014/09/08 14:59:47, Joao da Silva wrote:
> This DCHECK isn't needed (nor #include message_loop.h)

Done.

https://codereview.chromium.org/536573003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_browsertest.cc:69:
loop.RunUntilIdle();
On 2014/09/08 14:59:47, Joao da Silva wrote:
> These 2 lines can be combined:
> 
> base::RunLoop().RunUntilIdle();

Done.

[binjin, 2014-09-08 15:08:55]

The CQ bit was checked by binjin@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/536573003/120001

[commit-bot: I haz the power, 2014-09-08 16:14:56]

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

[commit-bot: I haz the power, 2014-09-08 16:14:57]

Failed to apply patch for chrome/browser/extensions/extension_management.cc:
While running git apply --index -p1;
  error: patch failed: chrome/browser/extensions/extension_management.cc:8
  error: chrome/browser/extensions/extension_management.cc: patch does not apply

Patch:       chrome/browser/extensions/extension_management.cc
Index: chrome/browser/extensions/extension_management.cc
diff --git a/chrome/browser/extensions/extension_management.cc
b/chrome/browser/extensions/extension_management.cc
index
abb4d90d6542f941df4e04b00cee67a25fb26390..7a214d96e87990c063553ad15e5bded37831ac7a
100644
--- a/chrome/browser/extensions/extension_management.cc
+++ b/chrome/browser/extensions/extension_management.cc
@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
+#include "chrome/browser/extensions/external_policy_loader.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/extensions/standard_management_policy_provider.h"
 #include "chrome/browser/profiles/profile.h"
@@ -74,6 +75,24 @@ bool ExtensionManagement::BlacklistedByDefault() {
   return default_settings_.installation_mode == INSTALLATION_BLOCKED;
 }
 
+scoped_ptr<base::DictionaryValue> ExtensionManagement::GetForceInstallList()
+    const {
+  scoped_ptr<base::DictionaryValue> forcelist(new base::DictionaryValue());
+  for (SettingsIdMap::const_iterator it = settings_by_id_.begin();
+       it != settings_by_id_.end();
+       ++it) {
+    if (it->second.installation_mode == INSTALLATION_FORCED) {
+      ExternalPolicyLoader::AddExtension(
+          forcelist.get(), it->first, it->second.update_url);
+    }
+  }
+  return forcelist.Pass();
+}
+
+bool ExtensionManagement::IsInstallationAllowed(const ExtensionId& id) const {
+  return ReadById(id).installation_mode != INSTALLATION_BLOCKED;
+}
+
 const ExtensionManagement::IndividualSettings& ExtensionManagement::ReadById(
     const ExtensionId& id) const {
   DCHECK(crx_file::id_util::IdIsValid(id)) << "Invalid ID: " << id;

[binjin, 2014-09-08 16:20:00]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:30:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/536573003/140001

[commit-bot: I haz the power, 2014-09-08 17:42:26]

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

[commit-bot: I haz the power, 2014-09-08 17:42:27]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[binjin, 2014-09-08 18:27:19]

The CQ bit was checked by binjin@chromium.org

[commit-bot: I haz the power, 2014-09-08 18:48:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/binjin@chromium.org/536573003/140001

[commit-bot: I haz the power, 2014-09-08 20:28:28]

Committed patchset #8 (id:140001) as ec1dadbc7bd9246490659493c9bb4cc1fa107d8e

[commit-bot: I haz the power, 2014-09-10 03:47:54]

Patchset 8 (id:??) landed as
https://crrev.com/30301062fce5fe3a7dc02ede5f00f113b41c0578
Cr-Commit-Position: refs/heads/master@{#293777}