When extension is blacklisted by admin policy, it should be removed if already running.

(Attempt 2 at landing this, this time with changes to fix Enabledness unit test)

BUG=51689
TEST=ExtensionsServiceTest.BlacklistedByPolicyRemovedIfRunning


Review URL: http://codereview.chromium.org/3166023

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@56987 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 108 insertions, 26 deletions

diff --git a/chrome/browser/extensions/extension_message_service.cc b/chrome/browser/extensions/extension_message_service.cc
index c4189b1..9775551 100644
--- a/chrome/browser/extensions/extension_message_service.cc
+++ b/chrome/browser/extensions/extension_message_service.cc
@@ -153,7 +153,7 @@ ExtensionMessageService::~ExtensionMessageService() {
   channels_.clear();
 }
 
-void ExtensionMessageService::ProfileDestroyed() {
+void ExtensionMessageService::DestroyingProfile() {
   profile_ = NULL;
   if (!registrar_.IsEmpty())
     registrar_.RemoveAll();
diff --git a/chrome/browser/extensions/extension_message_service.h b/chrome/browser/extensions/extension_message_service.h
index 5460344..e18140b 100644
--- a/chrome/browser/extensions/extension_message_service.h
+++ b/chrome/browser/extensions/extension_message_service.h
@@ -71,7 +71,7 @@ class ExtensionMessageService
   explicit ExtensionMessageService(Profile* profile);
 
   // Notification that our owning profile is going away.
-  void ProfileDestroyed();
+  void DestroyingProfile();
 
   // Add or remove |render_process_pid| as a listener for |event_name|.
   void AddEventListener(const std::string& event_name, int render_process_id);
diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index 9b28456..c7799c7 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -241,6 +241,10 @@ bool ExtensionPrefs::IsExtensionAllowedByPolicy(
     const std::string& extension_id) {
   std::string string_value;
 
+  const ListValue* blacklist = prefs_->GetList(kExtensionInstallDenyList);
+  if (!blacklist || blacklist->empty())
+    return true;
+
   // Check the whitelist first.
   const ListValue* whitelist = prefs_->GetList(kExtensionInstallAllowList);
   if (whitelist) {
@@ -254,7 +258,6 @@ bool ExtensionPrefs::IsExtensionAllowedByPolicy(
   }
 
   // Then check the blacklist (the admin blacklist, not the Google blacklist).
-  const ListValue* blacklist = prefs_->GetList(kExtensionInstallDenyList);
   if (blacklist) {
     for (ListValue::const_iterator it = blacklist->begin();
          it != blacklist->end(); ++it) {
diff --git a/chrome/browser/extensions/extension_updater_unittest.cc b/chrome/browser/extensions/extension_updater_unittest.cc
index 2d2c6d03..3de0099 100644
--- a/chrome/browser/extensions/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/extension_updater_unittest.cc
@@ -40,33 +40,37 @@ class MockService : public ExtensionUpdateService {
   virtual ~MockService() {}
 
   virtual const ExtensionList* extensions() const {
-    EXPECT_TRUE(false);
+    ADD_FAILURE();
     return NULL;
   }
 
   virtual const PendingExtensionMap& pending_extensions() const {
-    EXPECT_TRUE(false);
+    ADD_FAILURE();
     return pending_extensions_;
   }
 
   virtual void UpdateExtension(const std::string& id,
                                const FilePath& extension_path,
                                const GURL& download_url) {
-    EXPECT_TRUE(false);
+    FAIL();
   }
 
   virtual Extension* GetExtensionById(const std::string& id, bool) {
-    EXPECT_TRUE(false);
+    ADD_FAILURE();
     return NULL;
   }
 
   virtual void UpdateExtensionBlacklist(
-    const std::vector<std::string>& blacklist) {
-    EXPECT_TRUE(false);
+      const std::vector<std::string>& blacklist) {
+    FAIL();
+  }
+
+  virtual void CheckAdminBlacklist() {
+    FAIL();
   }
 
   virtual bool HasInstalledExtensions() {
-    EXPECT_TRUE(false);
+    ADD_FAILURE();
     return false;
   }
 
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index e9d635c..58ead19 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -204,6 +204,8 @@ ExtensionsService::ExtensionsService(Profile* profile,
                  NotificationService::AllSources());
   registrar_.Add(this, NotificationType::EXTENSION_PROCESS_TERMINATED,
                  Source<Profile>(profile_));
+  prefs->AddPrefObserver(prefs::kExtensionInstallAllowList, this);
+  prefs->AddPrefObserver(prefs::kExtensionInstallDenyList, this);
 
   // Set up the ExtensionUpdater
   if (autoupdate_enabled) {
@@ -218,7 +220,7 @@ ExtensionsService::ExtensionsService(Profile* profile,
 
   backend_ = new ExtensionsServiceBackend(install_directory_);
 
-  // Use monochrome icons for omnibox icons.
+  // Use monochrome icons for Omnibox icons.
   omnibox_icon_manager_.set_monochrome(true);
 }
 
@@ -794,6 +796,31 @@ void ExtensionsService::UpdateExtensionBlacklist(
   }
 }
 
+void ExtensionsService::DestroyingProfile() {
+  profile_->GetPrefs()->RemovePrefObserver(
+      prefs::kExtensionInstallAllowList, this);
+  profile_->GetPrefs()->RemovePrefObserver(
+      prefs::kExtensionInstallDenyList, this);
+
+  profile_ = NULL;
+}
+
+void ExtensionsService::CheckAdminBlacklist() {
+  std::vector<std::string> to_be_removed;
+  // Loop through extensions list, unload installed extensions.
+  for (ExtensionList::const_iterator iter = extensions_.begin();
+       iter != extensions_.end(); ++iter) {
+    Extension* extension = (*iter);
+    if (!extension_prefs_->IsExtensionAllowedByPolicy(extension->id()))
+      to_be_removed.push_back(extension->id());
+  }
+
+  // UnloadExtension will change the extensions_ list. So, we should
+  // call it outside the iterator loop.
+  for (unsigned int i = 0; i < to_be_removed.size(); ++i)
+    UnloadExtension(to_be_removed[i]);
+}
+
 bool ExtensionsService::IsIncognitoEnabled(const Extension* extension) {
   // If this is a component extension we always allow it to work in incognito
   // mode.
@@ -1337,6 +1364,14 @@ void ExtensionsService::Observe(NotificationType type,
       break;
     }
 
+    case NotificationType::PREF_CHANGED: {
+      std::string* pref_name = Details<std::string>(details).ptr();
+      DCHECK(*pref_name == prefs::kExtensionInstallAllowList ||
+             *pref_name == prefs::kExtensionInstallDenyList);
+      CheckAdminBlacklist();
+      break;
+    }
+
     default:
       NOTREACHED() << "Unexpected notification type.";
   }
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index 3191a06..45110d4 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -88,6 +88,7 @@ class ExtensionUpdateService {
                                       bool include_disabled) = 0;
   virtual void UpdateExtensionBlacklist(
     const std::vector<std::string>& blacklist) = 0;
+  virtual void CheckAdminBlacklist() = 0;
   virtual bool HasInstalledExtensions() = 0;
 
   virtual ExtensionPrefs* extension_prefs() = 0;
@@ -320,6 +321,11 @@ class ExtensionsService
   virtual void UpdateExtensionBlacklist(
     const std::vector<std::string>& blacklist);
 
+  // Go through each extension and unload those that the network admin has
+  // put on the blacklist (not to be confused with the Google managed blacklist
+  // set of extensions.
+  virtual void CheckAdminBlacklist();
+
   void set_extensions_enabled(bool enabled) { extensions_enabled_ = enabled; }
   bool extensions_enabled() { return extensions_enabled_; }
 
@@ -333,8 +339,9 @@ class ExtensionsService
 
   Profile* profile() { return profile_; }
 
-  // Profile calls this when it is destroyed so that we know not to call it.
-  void ProfileDestroyed() { profile_ = NULL; }
+  // Profile calls this when it is being destroyed so that we know not to call
+  // it.
+  void DestroyingProfile();
 
   ExtensionPrefs* extension_prefs() { return extension_prefs_.get(); }
 
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index e1b77c4..272e840 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -1844,6 +1844,35 @@ TEST_F(ExtensionsServiceTest, BlacklistedByPolicyWillNotInstall) {
   EXPECT_EQ(1u, service_->extensions()->size());
 }
 
+// Extension blacklisted by policy get unloaded after installing.
+TEST_F(ExtensionsServiceTest, BlacklistedByPolicyRemovedIfRunning) {
+  InitializeEmptyExtensionsService();
+
+  // Install good_crx.
+  FilePath extensions_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
+  extensions_path = extensions_path.AppendASCII("extensions");
+  FilePath path = extensions_path.AppendASCII("good.crx");
+  service_->InstallExtension(path);
+  loop_.RunAllPending();
+  EXPECT_EQ(1u, service_->extensions()->size());
+
+  ListValue* blacklist = prefs_->GetMutableList("extensions.install.denylist");
+  ASSERT_TRUE(blacklist != NULL);
+
+  // Blacklist this extension.
+  blacklist->Append(Value::CreateStringValue(good_crx));
+  prefs_->ScheduleSavePersistentPrefs();
+
+  // Programmatically appending to the prefs doesn't seem to notify the
+  // observers... :/
+  prefs_->pref_notifier()->FireObservers("extensions.install.denylist");
+
+  // Extension should not be running now.
+  loop_.RunAllPending();
+  EXPECT_EQ(0u, service_->extensions()->size());
+}
+
 // Tests disabling extensions
 TEST_F(ExtensionsServiceTest, DisableExtension) {
   InitializeEmptyExtensionsService();
@@ -2365,19 +2394,19 @@ class ExtensionsReadyRecorder : public NotificationObserver {
 // enabled or not.
 TEST(ExtensionsServiceTestSimple, Enabledness) {
   ExtensionsReadyRecorder recorder;
-  TestingProfile profile;
+  scoped_ptr<TestingProfile> profile(new TestingProfile());
   MessageLoop loop;
   ChromeThread ui_thread(ChromeThread::UI, &loop);
   ChromeThread file_thread(ChromeThread::FILE, &loop);
   scoped_ptr<CommandLine> command_line;
   scoped_refptr<ExtensionsService> service;
-  FilePath install_dir = profile.GetPath()
+  FilePath install_dir = profile->GetPath()
       .AppendASCII(ExtensionsService::kInstallDirectoryName);
 
   // By default, we are enabled.
   command_line.reset(new CommandLine(CommandLine::ARGUMENTS_ONLY));
-  service = new ExtensionsService(&profile, command_line.get(),
-      profile.GetPrefs(), install_dir, false);
+  service = new ExtensionsService(profile.get(), command_line.get(),
+      profile->GetPrefs(), install_dir, false);
   EXPECT_TRUE(service->extensions_enabled());
   service->Init();
   loop.RunAllPending();
@@ -2385,27 +2414,31 @@ TEST(ExtensionsServiceTestSimple, Enabledness) {
 
   // If either the command line or pref is set, we are disabled.
   recorder.set_ready(false);
+  profile.reset(new TestingProfile());
   command_line->AppendSwitch(switches::kDisableExtensions);
-  service = new ExtensionsService(&profile, command_line.get(),
-      profile.GetPrefs(), install_dir, false);
+  service = new ExtensionsService(profile.get(), command_line.get(),
+      profile->GetPrefs(), install_dir, false);
   EXPECT_FALSE(service->extensions_enabled());
   service->Init();
   loop.RunAllPending();
   EXPECT_TRUE(recorder.ready());
 
   recorder.set_ready(false);
-  profile.GetPrefs()->SetBoolean(prefs::kDisableExtensions, true);
-  service = new ExtensionsService(&profile, command_line.get(),
-      profile.GetPrefs(), install_dir, false);
+  profile.reset(new TestingProfile());
+  profile->GetPrefs()->SetBoolean(prefs::kDisableExtensions, true);
+  service = new ExtensionsService(profile.get(), command_line.get(),
+      profile->GetPrefs(), install_dir, false);
   EXPECT_FALSE(service->extensions_enabled());
   service->Init();
   loop.RunAllPending();
   EXPECT_TRUE(recorder.ready());
 
   recorder.set_ready(false);
+  profile.reset(new TestingProfile());
+  profile->GetPrefs()->SetBoolean(prefs::kDisableExtensions, true);
   command_line.reset(new CommandLine(CommandLine::ARGUMENTS_ONLY));
-  service = new ExtensionsService(&profile, command_line.get(),
-      profile.GetPrefs(), install_dir, false);
+  service = new ExtensionsService(profile.get(), command_line.get(),
+      profile->GetPrefs(), install_dir, false);
   EXPECT_FALSE(service->extensions_enabled());
   service->Init();
   loop.RunAllPending();
diff --git a/chrome/browser/profile_impl.cc b/chrome/browser/profile_impl.cc
index 84a2b17..5680658 100644
--- a/chrome/browser/profile_impl.cc
+++ b/chrome/browser/profile_impl.cc
@@ -509,10 +509,10 @@ ProfileImpl::~ProfileImpl() {
   favicon_service_ = NULL;
 
   if (extension_message_service_)
-    extension_message_service_->ProfileDestroyed();
+    extension_message_service_->DestroyingProfile();
 
   if (extensions_service_)
-    extensions_service_->ProfileDestroyed();
+    extensions_service_->DestroyingProfile();
 
   // This causes the Preferences file to be written to disk.
   MarkAsCleanShutdown();