Attempt to repair corrupt enterprise policy force-installed extensions

chrome/browser/extensions/content_verifier_browsertest.cc

Index: chrome/browser/extensions/content_verifier_browsertest.cc
diff --git a/chrome/browser/extensions/content_verifier_browsertest.cc b/chrome/browser/extensions/content_verifier_browsertest.cc
index f688dd4712708f4c9b1023b3580a2c2482d06acc..55bd9f2bdf66a26f0c0155a8882b7bfb626a6610 100644
--- a/chrome/browser/extensions/content_verifier_browsertest.cc
+++ b/chrome/browser/extensions/content_verifier_browsertest.cc
@@ -6,52 +6,104 @@
 #include <set>
 #include <string>
 
+#include "base/callback_helpers.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
 #include "base/scoped_observer.h"
+#include "base/strings/string_split.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/content_verifier.h"
 #include "extensions/browser/content_verify_job.h"
+#include "extensions/browser/crx_file_info.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_registry_observer.h"
+#include "extensions/browser/external_install_info.h"
+#include "extensions/browser/external_provider_interface.h"
+#include "extensions/browser/management_policy.h"
+#include "extensions/browser/updater/extension_downloader.h"
+#include "extensions/browser/updater/extension_downloader_test_delegate.h"
+#include "extensions/browser/updater/manifest_fetch_data.h"
+#include "extensions/common/extension_urls.h"
 
 namespace extensions {
 
 namespace {
 
-// Helper for observing extension unloads.
-class UnloadObserver : public ExtensionRegistryObserver {
+// Helper for observing extension registry events.
+class RegistryObserver : public ExtensionRegistryObserver {
  public:
-  explicit UnloadObserver(ExtensionRegistry* registry) : observer_(this) {
+  explicit RegistryObserver(ExtensionRegistry* registry) : observer_(this) {
     observer_.Add(registry);
   }
-  ~UnloadObserver() override {}
+  ~RegistryObserver() override {}
+
+  // Waits until we've seen an unload for extension with |id|, returning true
+  // if we saw one or false otherwise (typically because of test timeout).
+  bool WaitForUnload(const ExtensionId& id) {
+    if (base::ContainsKey(unloaded_, id))
+      return true;
+
+    base::RunLoop run_loop;
+    awaited_unload_id_ = id;
+    quit_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+    return base::ContainsKey(unloaded_, id);
+  }
 
-  void WaitForUnload(const ExtensionId& id) {
-    if (base::ContainsKey(observed_, id))
-      return;
+  // Same as WaitForUnload, but for an install.
+  bool WaitForInstall(const ExtensionId& id) {
+    if (base::ContainsKey(installed_, id))
+      return true;
 
-    ASSERT_TRUE(loop_runner_.get() == NULL);
-    awaited_id_ = id;
-    loop_runner_ = new content::MessageLoopRunner();
-    loop_runner_->Run();
+    base::RunLoop run_loop;
+    awaited_install_id_ = id;
+    quit_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+    return base::ContainsKey(installed_, id);
   }
 
+  // ExtensionRegistryObserver
   void OnExtensionUnloaded(content::BrowserContext* browser_context,
                            const Extension* extension,
                            UnloadedExtensionInfo::Reason reason) override {
-    observed_.insert(extension->id());
-    if (awaited_id_ == extension->id())
-      loop_runner_->Quit();
+    unloaded_.insert(extension->id());
+    if (awaited_unload_id_ == extension->id()) {
+      awaited_unload_id_.clear();
+      base::ResetAndReturn(&quit_closure_).Run();
+    }
+  }
+
+  void OnExtensionInstalled(content::BrowserContext* browser_context,
+                            const Extension* extension,
+                            bool is_update) override {
+    installed_.insert(extension->id());
+    if (awaited_install_id_ == extension->id()) {
+      awaited_install_id_.clear();
+      base::ResetAndReturn(&quit_closure_).Run();
+    }
   }
 
  private:
-  ExtensionId awaited_id_;
-  std::set<ExtensionId> observed_;
-  scoped_refptr<content::MessageLoopRunner> loop_runner_;
-  ScopedObserver<ExtensionRegistry, UnloadObserver> observer_;
+  // The id we're waiting for a load/install of respectively.
+  ExtensionId awaited_unload_id_;
+  ExtensionId awaited_install_id_;
+
+  // The quit closure for stopping a running RunLoop, if we're waiting.
+  base::Closure quit_closure_;
+
+  // The extension id's we've seen unloaded and installed, respectively.
+  std::set<ExtensionId> unloaded_;
+  std::set<ExtensionId> installed_;
+
+  ScopedObserver<ExtensionRegistry, RegistryObserver> observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(RegistryObserver);
 };
 
 // Helper for forcing ContentVerifyJob's to return an error.
@@ -250,6 +302,133 @@ void VerifierObserver::OnFetchComplete(const std::string& extension_id,
     loop_runner_->Quit();
 }
 
+// This lets us intercept requests for update checks of extensions, and
+// substitute a local file as a simulated response.
+class DownloaderTestDelegate : public ExtensionDownloaderTestDelegate {
+ public:
+  DownloaderTestDelegate() {}
+
+  // This makes it so that update check requests for |extension_id| will return
+  // a downloaded file of |crx_path| that is claimed to have version
+  // |version_string|.
+  void AddResponse(const std::string& extension_id,

[lazyboy, 2016/09/02 19:21:53]

ExtensionId instead of std::string for extension ids.

[asargent_no_longer_on_chrome, 2016/09/09 03:30:43]

Done.

+                   const std::string& version_string,
+                   const base::FilePath& crx_path) {
+    responses_[extension_id] = std::make_pair(version_string, crx_path);
+  }
+
+  const std::vector<std::unique_ptr<ManifestFetchData>>& requests() {
+    return requests_;
+  }
+
+  // ExtensionDownloaderTestDelegate:
+  void StartUpdateCheck(
+      ExtensionDownloader* downloader,
+      ExtensionDownloaderDelegate* delegate,
+      std::unique_ptr<ManifestFetchData> fetch_data) override {
+    requests_.push_back(std::move(fetch_data));
+    const ManifestFetchData* data = requests_.back().get();
+
+    for (const auto& id : data->extension_ids()) {
+      if (ContainsKey(responses_, id)) {
+        // We use PostTask here instead of calling OnExtensionDownloadFinished
+        // immeditately, because the calling code isn't expecting a synchronous
+        // response (in non-test situations there are at least 2 network
+        // requests needed before a file could be returned).
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE,
+            base::Bind(
+                &ExtensionDownloaderDelegate::OnExtensionDownloadFinished,
+                base::Unretained(delegate),
+                CRXFileInfo(id, responses_[id].second),
+                false /* pass_file_ownership */, GURL(), responses_[id].first,
+                ExtensionDownloaderDelegate::PingResult(), data->request_ids(),
+                ExtensionDownloaderDelegate::InstallCallback()));
+      }
+    }
+  }
+
+ private:
+  // The requests we've received.
+  std::vector<std::unique_ptr<ManifestFetchData>> requests_;
+
+  // The prepared responses - this maps an extension id to a (version string,
+  // crx file path) pair.
+  std::map<std::string, std::pair<std::string, base::FilePath>> responses_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloaderTestDelegate);
+};
+
+// This lets us simulate the behavior of an enterprise policy that wants
+// a given extension to be installed via the webstore.
+class TestExternalProvider : public ExternalProviderInterface {
+ public:
+  TestExternalProvider(VisitorInterface* visitor,
+                       const std::string& extension_id)
+      : visitor_(visitor), extension_id_(extension_id) {}
+
+  ~TestExternalProvider() override {}
+
+  // ExternalProviderInterface:
+  void ServiceShutdown() override {}
+
+  void VisitRegisteredExtension() override {
+    visitor_->OnExternalExtensionUpdateUrlFound(
+        ExternalInstallInfoUpdateUrl(
+            extension_id_, std::string() /* install_parameter */,
+            base::MakeUnique<GURL>(extension_urls::GetWebstoreUpdateUrl()),
+            Manifest::EXTERNAL_POLICY_DOWNLOAD, 0 /* creation_flags */,
+            true /* mark_acknowledged */),
+        true /* is_initial_load */);
+    visitor_->OnExternalProviderReady(this);
+  }
+
+  bool HasExtension(const std::string& id) const override {
+    return id == std::string("npnbmohejbjohgpjnmjagbafnjhkmgko");
+  }
+
+  bool GetExtensionDetails(
+      const std::string& id,
+      Manifest::Location* location,
+      std::unique_ptr<base::Version>* version) const override {
+    ADD_FAILURE() << "Unexpected GetExtensionDetails call; id:" << id;
+    return false;
+  }
+
+  bool IsReady() const override { return true; }
+
+ private:
+  VisitorInterface* visitor_;
+  std::string extension_id_;
+  base::Closure quit_closure_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestExternalProvider);
+};
+
+// This lets us simulate a policy-installed extension being "force" installed;
+// ie a user is not allowed to manually uninstall/disable it.
+class ForceInstallProvider : public ManagementPolicy::Provider {
+ public:
+  explicit ForceInstallProvider(const std::string& id) : id_(id) {}
+  ~ForceInstallProvider() override {}
+
+  std::string GetDebugPolicyProviderName() const override {
+    return "ForceInstallProvider";
+  }
+
+  // MananagementPolicy::Provider:
+  bool UserMayModifySettings(const Extension* extension,
+                             base::string16* error) const override {
+    return extension->id() != id_;
+  }
+
+ private:
+  // The extension id we want to disallow uninstall/disable for.
+  std::string id_;
+
+  DISALLOW_COPY_AND_ASSIGN(ForceInstallProvider);
+};
+
 }  // namespace
 
 class ContentVerifierTest : public ExtensionBrowserTest {
@@ -280,7 +459,7 @@ class ContentVerifierTest : public ExtensionBrowserTest {
     std::string id = "npnbmohejbjohgpjnmjagbafnjhkmgko";
     delegate_.set_id(id);
     unload_observer_.reset(
-        new UnloadObserver(ExtensionRegistry::Get(profile())));
+        new RegistryObserver(ExtensionRegistry::Get(profile())));
     const Extension* extension = InstallExtensionFromWebstore(
         test_data_dir_.AppendASCII("content_verifier/v1.crx"), 1);
     ASSERT_TRUE(extension);
@@ -294,7 +473,7 @@ class ContentVerifierTest : public ExtensionBrowserTest {
     AddTabAtIndexToBrowser(browser(), 1, page_url_, ui::PAGE_TRANSITION_LINK,
                            false);
 
-    unload_observer_->WaitForUnload(id);
+    EXPECT_TRUE(unload_observer_->WaitForUnload(id));
     ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
     int reasons = prefs->GetDisableReasons(id);
     EXPECT_TRUE(reasons & Extension::DISABLE_CORRUPTED);
@@ -306,7 +485,7 @@ class ContentVerifierTest : public ExtensionBrowserTest {
 
  protected:
   JobDelegate delegate_;
-  std::unique_ptr<UnloadObserver> unload_observer_;
+  std::unique_ptr<RegistryObserver> unload_observer_;
   GURL page_url_;
 };
 
@@ -423,4 +602,64 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierTest, ContentScripts) {
   ContentVerifyJob::SetObserverForTests(NULL);
 }
 
+// Test the case of a corrupt extension that is force-installed by policy and

[lazyboy, 2016/09/02 19:21:52]

nit: Tests the ...

[asargent_no_longer_on_chrome, 2016/09/09 03:30:43]

Done.

+// should not be allowed to be manually uninstalled/disabled by the user.
+IN_PROC_BROWSER_TEST_F(ContentVerifierTest, PolicyCorrupted) {

[lazyboy, 2016/09/02 19:21:53]

Nice and thorough test!

[asargent_no_longer_on_chrome, 2016/09/09 03:30:43]

thanks!

+  ExtensionSystem* system = ExtensionSystem::Get(profile());
+  ExtensionService* service = system->extension_service();
+
+  // The id of our test extension.
+  std::string id("npnbmohejbjohgpjnmjagbafnjhkmgko");
+
+  // Setup fake policy and update check objects.
+  ForceInstallProvider policy(id);
+  DownloaderTestDelegate downloader;
+  system->management_policy()->RegisterProvider(&policy);
+  ExtensionDownloader::set_test_delegate(&downloader);
+  service->AddProviderForTesting(
+      base::MakeUnique<TestExternalProvider>(service, id));
+
+  base::FilePath crx_path =
+      test_data_dir_.AppendASCII("content_verifier/v1.crx");
+  const Extension* extension =
+      InstallExtension(crx_path, 1, Manifest::EXTERNAL_POLICY_DOWNLOAD);
+  EXPECT_NE(extension, nullptr);
+
+  downloader.AddResponse(id, extension->VersionString(), crx_path);
+
+  RegistryObserver registry_observer(ExtensionRegistry::Get(profile()));
+  ContentVerifier* verifier = system->content_verifier();
+  verifier->VerifyFailed(extension->id(), ContentVerifyJob::HASH_MISMATCH);
+
+  // Make sure the extension first got disabled due to corruption.
+  EXPECT_TRUE(registry_observer.WaitForUnload(id));
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+  int reasons = prefs->GetDisableReasons(id);
+  EXPECT_TRUE(reasons & Extension::DISABLE_CORRUPTED);
+
+  // Make sure the extension then got re-installed, and that after reinstall it
+  // is no longer disabled due to corruption.
+  EXPECT_TRUE(registry_observer.WaitForInstall(id));
+  reasons = prefs->GetDisableReasons(id);
+  EXPECT_FALSE(reasons & Extension::DISABLE_CORRUPTED);
+
+  // Make sure that the update check request properly included a parameter
+  // indicating that this was a corrupt policy reinstall.
+  bool found = false;
+  for (const auto& request : downloader.requests()) {
+    if (request->Includes(id)) {
+      std::string query = request->full_url().query();
+      for (const auto& part : base::SplitString(
+               query, "&", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL)) {
+        if (part.find("x=") == 0 &&

[lazyboy, 2016/09/02 19:21:53]

base::StartWith(part, "x=")

[asargent_no_longer_on_chrome, 2016/09/09 03:30:43]

Done.

+            part.find(std::string("id%3D") + id) != std::string::npos) {
+          found = true;
+          EXPECT_NE(std::string::npos, part.find("%26cpr"));
+        }
+      }
+    }
+  }
+  EXPECT_TRUE(found);
+}
+
 }  // namespace extensions