From 8a87a533efa7ac622a839499ada717f35f14dda2 Mon Sep 17 00:00:00 2001
From: "joaodasilva@chromium.org"
<joaodasilva@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu, 11 Aug 2011 17:54:59 +0000
Subject: Extensions installed by policy overrun previously installed
extensions.
BUG=86519
TEST=Extensions tests, should all work as before. The test described in the bug should now work as expected.
Review URL: http://codereview.chromium.org/7605001
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@96420 0039d316-1c4b-4281-b951-d872f2087c98
---
.../extensions/extension_management_browsertest.cc | 113 ++++++++++++++++++++-
chrome/browser/extensions/extension_service.cc | 17 +++-
.../extensions/extension_service_unittest.cc | 27 +++++
chrome/browser/extensions/extension_updater.cc | 22 ++--
.../extensions/pending_extension_manager.cc | 22 ++--
5 files changed, 178 insertions(+), 23 deletions(-)
(limited to 'chrome')
diff --git a/chrome/browser/extensions/extension_management_browsertest.cc b/chrome/browser/extensions/extension_management_browsertest.cc
index cace228..cd018a42 100644
--- a/chrome/browser/extensions/extension_management_browsertest.cc
+++ b/chrome/browser/extensions/extension_management_browsertest.cc
@@ -442,6 +442,16 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalUrlUpdate) {
<< "Uninstalling non-external extension should not set kill bit.";
}
+namespace {
+
+const char* kForceInstallNotEmptyHelp =
+ "A policy may already be controlling the list of force-installed "
+ "extensions. Please remove all policy settings from your computer "
+ "before running tests. E.g. from /etc/chromium/policies Linux or "
+ "from the registry on Windows, etc.";
+
+}
+
// See http://crbug.com/57378 for flakiness details.
IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalPolicyRefresh) {
ExtensionService* service = browser()->profile()->GetExtensionService();
@@ -466,11 +476,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalPolicyRefresh) {
PrefService* prefs = browser()->profile()->GetPrefs();
const ListValue* forcelist =
prefs->GetList(prefs::kExtensionInstallForceList);
- ASSERT_TRUE(forcelist->empty())
- << "A policy may already be controlling the list of force-installed "
- << "extensions. Please remove all policy settings from your computer "
- << "before running tests. E.g. from /etc/chromium/policies Linux or "
- << "from the registry on Windows, etc.";
+ ASSERT_TRUE(forcelist->empty()) << kForceInstallNotEmptyHelp;
{
// Set the policy as a user preference and fire notification observers.
@@ -516,3 +522,100 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalPolicyRefresh) {
for (i = extensions->begin(); i != extensions->end(); ++i)
EXPECT_NE(kExtensionId, (*i)->id());
}
+
+IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, PolicyOverridesUserInstall) {
+ ExtensionService* service = browser()->profile()->GetExtensionService();
+ const char* kExtensionId = "ogjcoiohnmldgjemafoockdghcjciccf";
+ service->updater()->set_blacklist_checks_enabled(false);
+ const size_t size_before = service->extensions()->size();
+ FilePath basedir = test_data_dir_.AppendASCII("autoupdate");
+ ASSERT_TRUE(service->disabled_extensions()->empty());
+
+ // Note: This interceptor gets requests on the IO thread.
+ scoped_refptr<AutoUpdateInterceptor> interceptor(new AutoUpdateInterceptor());
+ URLFetcher::enable_interception_for_tests(true);
+
+ interceptor->SetResponseOnIOThread("http://localhost/autoupdate/manifest",
+ basedir.AppendASCII("manifest_v2.xml"));
+ interceptor->SetResponseOnIOThread("http://localhost/autoupdate/v2.crx",
+ basedir.AppendASCII("v2.crx"));
+
+ // Check that the policy is initially empty.
+ PrefService* prefs = browser()->profile()->GetPrefs();
+ const ListValue* forcelist =
+ prefs->GetList(prefs::kExtensionInstallForceList);
+ ASSERT_TRUE(forcelist->empty()) << kForceInstallNotEmptyHelp;
+
+ // User install of the extension.
+ ASSERT_TRUE(InstallExtension(basedir.AppendASCII("v2.crx"), 1));
+ const ExtensionList* extensions = service->extensions();
+ ASSERT_EQ(size_before + 1, extensions->size());
+ const Extension* extension = extensions->at(size_before);
+ ASSERT_EQ(kExtensionId, extension->id());
+ EXPECT_EQ(Extension::INTERNAL, extension->location());
+ EXPECT_TRUE(service->IsExtensionEnabled(kExtensionId));
+
+ // Setup the force install policy. It should override the location.
+ {
+ ListPrefUpdate pref_update(prefs, prefs::kExtensionInstallForceList);
+ ListValue* forcelist = pref_update.Get();
+ ASSERT_TRUE(forcelist->empty());
+ forcelist->Append(Value::CreateStringValue(
+ std::string(kExtensionId) + ";http://localhost/autoupdate/manifest"));
+ }
+ ASSERT_TRUE(WaitForExtensionInstall());
+ extensions = service->extensions();
+ ASSERT_EQ(size_before + 1, extensions->size());
+ extension = extensions->at(size_before);
+ ASSERT_EQ(kExtensionId, extension->id());
+ EXPECT_EQ(Extension::EXTERNAL_POLICY_DOWNLOAD, extension->location());
+ EXPECT_TRUE(service->IsExtensionEnabled(kExtensionId));
+
+ // Remove the policy, and verify that the extension was uninstalled.
+ // TODO(joaodasilva): it would be nicer if the extension was kept instead,
+ // and reverted location to INTERNAL or whatever it was before the policy
+ // was applied.
+ {
+ ListPrefUpdate pref_update(prefs, prefs::kExtensionInstallForceList);
+ ListValue* forcelist = pref_update.Get();
+ ASSERT_TRUE(!forcelist->empty());
+ forcelist->Clear();
+ }
+ extensions = service->extensions();
+ ASSERT_EQ(size_before, extensions->size());
+ extension = service->GetExtensionById(kExtensionId, true);
+ EXPECT_TRUE(extension == NULL);
+
+ // User install again, but have it disabled too before setting the policy.
+ ASSERT_TRUE(InstallExtension(basedir.AppendASCII("v2.crx"), 1));
+ extensions = service->extensions();
+ ASSERT_EQ(size_before + 1, extensions->size());
+ extension = extensions->at(size_before);
+ ASSERT_EQ(kExtensionId, extension->id());
+ EXPECT_EQ(Extension::INTERNAL, extension->location());
+ EXPECT_TRUE(service->IsExtensionEnabled(kExtensionId));
+ EXPECT_TRUE(service->disabled_extensions()->empty());
+
+ service->DisableExtension(kExtensionId);
+ EXPECT_EQ(1u, service->disabled_extensions()->size());
+ EXPECT_EQ(kExtensionId, service->disabled_extensions()->at(0)->id());
+ EXPECT_FALSE(service->IsExtensionEnabled(kExtensionId));
+
+ // Install the policy again. It should overwrite the extension's location,
+ // and force enable it too.
+ {
+ ListPrefUpdate pref_update(prefs, prefs::kExtensionInstallForceList);
+ ListValue* forcelist = pref_update.Get();
+ ASSERT_TRUE(forcelist->empty());
+ forcelist->Append(Value::CreateStringValue(
+ std::string(kExtensionId) + ";http://localhost/autoupdate/manifest"));
+ }
+ ASSERT_TRUE(WaitForExtensionInstall());
+ extensions = service->extensions();
+ ASSERT_EQ(size_before + 1, extensions->size());
+ extension = extensions->at(size_before);
+ ASSERT_EQ(kExtensionId, extension->id());
+ EXPECT_EQ(Extension::EXTERNAL_POLICY_DOWNLOAD, extension->location());
+ EXPECT_TRUE(service->IsExtensionEnabled(kExtensionId));
+ EXPECT_TRUE(service->disabled_extensions()->empty());
+}
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index c729867..b49f5bd 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -415,9 +415,14 @@ void ExtensionService::OnExternalExtensionUpdateUrlFound(
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
CHECK(Extension::IdIsValid(id));
- if (GetExtensionById(id, true)) {
- // Already installed. Do not change the update URL that the extension set.
- return;
+ const Extension* extension = GetExtensionById(id, true);
+ if (extension) {
+ // Already installed. Skip this install if the current location has
+ // higher priority than |location|.
+ Extension::Location current = extension->location();
+ if (current == Extension::GetHigherPriorityLocation(current, location))
+ return;
+ // Otherwise, overwrite the current installation.
}
pending_extension_manager()->AddFromExternalUpdateUrl(
id, update_url, location);
@@ -2163,7 +2168,11 @@ void ExtensionService::OnExtensionInstalled(
// Ensure extension is deleted unless we transfer ownership.
scoped_refptr<const Extension> scoped_extension(extension);
const std::string& id = extension->id();
- bool initial_enable = !extension_prefs_->IsExtensionDisabled(id);
+ // Extensions installed by policy can't be disabled. So even if a previous
+ // installation disabled the extension, make sure it is now enabled.
+ bool initial_enable =
+ !extension_prefs_->IsExtensionDisabled(id) ||
+ !Extension::UserMayDisable(extension->location());
PendingExtensionInfo pending_extension_info;
if (pending_extension_manager()->GetById(id, &pending_extension_info)) {
pending_extension_manager()->Remove(id);
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index edb7772..5111f31 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -3704,6 +3704,33 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNotInstalled) {
// TODO(akalin): Figure out a way to test |info.ShouldAllowInstall()|.
}
+TEST_F(ExtensionServiceTest, HigherPriorityInstall) {
+ InitializeEmptyExtensionService();
+
+ FilePath path = data_dir_.AppendASCII("good.crx");
+ InstallCrx(path, true);
+ ValidatePrefKeyCount(1u);
+ ValidateIntegerPref(good_crx, "state", Extension::ENABLED);
+ ValidateIntegerPref(good_crx, "location", Extension::INTERNAL);
+
+ PendingExtensionManager* pending = service_->pending_extension_manager();
+ EXPECT_FALSE(pending->IsIdPending(kGoodId));
+
+ // Skip install when the location is the same.
+ service_->OnExternalExtensionUpdateUrlFound(kGoodId, GURL(kGoodUpdateURL),
+ Extension::INTERNAL);
+ EXPECT_FALSE(pending->IsIdPending(kGoodId));
+ // Force install when the location has higher priority.
+ service_->OnExternalExtensionUpdateUrlFound(kGoodId, GURL(kGoodUpdateURL),
+ Extension::EXTERNAL_POLICY_DOWNLOAD);
+ EXPECT_TRUE(pending->IsIdPending(kGoodId));
+ pending->Remove(kGoodId);
+ // Skip install when the location has lower priority.
+ service_->OnExternalExtensionUpdateUrlFound(kGoodId, GURL(kGoodUpdateURL),
+ Extension::INTERNAL);
+ EXPECT_FALSE(pending->IsIdPending(kGoodId));
+}
+
// Test that when multiple sources try to install an extension,
// we consistently choose the right one. To make tests easy to read,
// methods that fake requests to install crx files in several ways
diff --git a/chrome/browser/extensions/extension_updater.cc b/chrome/browser/extensions/extension_updater.cc
index ae62dca..32679af 100644
--- a/chrome/browser/extensions/extension_updater.cc
+++ b/chrome/browser/extensions/extension_updater.cc
@@ -1000,14 +1000,9 @@ void ExtensionUpdater::CheckNow() {
NotifyStarted();
ManifestFetchesBuilder fetches_builder(service_, extension_prefs_);
- const ExtensionList* extensions = service_->extensions();
- for (ExtensionList::const_iterator iter = extensions->begin();
- iter != extensions->end(); ++iter) {
- fetches_builder.AddExtension(**iter);
- }
-
const PendingExtensionManager* pending_extension_manager =
service_->pending_extension_manager();
+ std::set<std::string> pending_ids;
PendingExtensionManager::const_iterator iter;
for (iter = pending_extension_manager->begin();
@@ -1016,8 +1011,21 @@ void ExtensionUpdater::CheckNow() {
// class PendingExtensionManager.
Extension::Location location = iter->second.install_source();
if (location != Extension::EXTERNAL_PREF &&
- location != Extension::EXTERNAL_REGISTRY)
+ location != Extension::EXTERNAL_REGISTRY) {
fetches_builder.AddPendingExtension(iter->first, iter->second);
+ pending_ids.insert(iter->first);
+ }
+ }
+
+ const ExtensionList* extensions = service_->extensions();
+ for (ExtensionList::const_iterator iter = extensions->begin();
+ iter != extensions->end(); ++iter) {
+ // An extension might be overwritten by policy, and have its update url
+ // changed. Make sure existing extensions aren't fetched again, if a
+ // pending fetch for an extension with the same id already exists.
+ if (!ContainsKey(pending_ids, (*iter)->id())) {
+ fetches_builder.AddExtension(**iter);
+ }
}
fetches_builder.ReportStats();
diff --git a/chrome/browser/extensions/pending_extension_manager.cc b/chrome/browser/extensions/pending_extension_manager.cc
index d4446a9..c978415 100644
--- a/chrome/browser/extensions/pending_extension_manager.cc
+++ b/chrome/browser/extensions/pending_extension_manager.cc
@@ -74,13 +74,21 @@ void PendingExtensionManager::AddFromExternalUpdateUrl(
const bool kIsFromSync = false;
const bool kInstallSilently = true;
- if (service_.IsExternalExtensionUninstalled(id))
- return;
-
- if (service_.GetInstalledExtension(id)) {
- LOG(DFATAL) << "Trying to add extension " << id
- << " by external update, but it is already installed.";
- return;
+ const Extension* extension = service_.GetInstalledExtension(id);
+ if (extension &&
+ location == Extension::GetHigherPriorityLocation(location,
+ extension->location())) {
+ // If the new location has higher priority than the location of an existing
+ // extension, let the update process overwrite the existing extension.
+ } else {
+ if (service_.IsExternalExtensionUninstalled(id)) {
+ return;
+ }
+ if (extension) {
+ LOG(DFATAL) << "Trying to add extension " << id
+ << " by external update, but it is already installed.";
+ return;
+ }
}
AddExtensionImpl(id, update_url, &AlwaysInstall,
--
cgit v1.1