developerPrivate.repair: Skip not-corrupted and policy extensions

chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc

Index: chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
diff --git a/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc b/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
index 33e2765c06958b5720bddba0e4bd5feee84c2700..0132b156babf8868c60c6943ecab97a0e5ace781 100644
--- a/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
+++ b/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
@@ -11,10 +11,11 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/error_console/error_console.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/extensions/extension_service_test_base.h"
+#include "chrome/browser/extensions/extension_service_test_with_install.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/scripting_permissions_modifier.h"
 #include "chrome/browser/extensions/test_extension_dir.h"
@@ -32,6 +33,7 @@
 #include "components/policy/core/common/policy_types.h"
 #include "components/policy/policy_constants.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/test/web_contents_tester.h"
 #include "extensions/browser/api_test_utils.h"
 #include "extensions/browser/event_router_factory.h"
@@ -40,6 +42,7 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_util.h"
+#include "extensions/browser/external_install_info.h"
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
@@ -56,6 +59,8 @@ namespace extensions {
 
 namespace {
 
+const char good_crx[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
+
 std::unique_ptr<KeyedService> BuildAPI(content::BrowserContext* context) {
   return base::MakeUnique<DeveloperPrivateAPI>(context);
 }
@@ -77,13 +82,84 @@ bool HasPrefsPermission(bool (*has_pref)(const std::string&,
   return has_pref(id, context);
 }
 
+// This class is almost entirely copied from extension_service_unittest.cc.
+// TODO(lazyboy): ) Extract to a separate file.
+class MockExtensionProvider : public extensions::ExternalProviderInterface {
+ public:
+  MockExtensionProvider(VisitorInterface* visitor, Manifest::Location location)
+      : location_(location), visitor_(visitor) {}
+
+  ~MockExtensionProvider() override {}
+
+  void AddExtension(const std::string& id,
+                    const std::string& version,
+                    const base::FilePath& path) {
+    extension_map_[id] = std::make_pair(version, path);
+  }
+
+  void RemoveExtension(const std::string& id) { extension_map_.erase(id); }
+
+  // ExternalProvider implementation:
+  void VisitRegisteredExtension() override {
+    for (DataMap::const_iterator i = extension_map_.begin();
+         i != extension_map_.end(); ++i) {
+      std::unique_ptr<base::Version> version(
+          new base::Version(i->second.first));
+
+      std::unique_ptr<ExternalInstallInfoFile> info(new ExternalInstallInfoFile(
+          i->first, std::move(version), i->second.second, location_,
+          Extension::NO_FLAGS, false, false));
+      visitor_->OnExternalExtensionFileFound(*info);
+    }
+    visitor_->OnExternalProviderReady(this);
+  }
+
+  bool HasExtension(const std::string& id) const override {
+    return extension_map_.find(id) != extension_map_.end();
+  }
+
+  bool GetExtensionDetails(
+      const std::string& id,
+      Manifest::Location* location,
+      std::unique_ptr<base::Version>* version) const override {
+    DataMap::const_iterator it = extension_map_.find(id);
+    if (it == extension_map_.end())
+      return false;
+
+    if (version)
+      version->reset(new base::Version(it->second.first));
+
+    if (location)
+      *location = location_;
+
+    return true;
+  }
+
+  bool IsReady() const override { return true; }
+
+  void ServiceShutdown() override {}
+
+ private:
+  using DataMap = std::map<std::string, std::pair<std::string, base::FilePath>>;
+  DataMap extension_map_;
+  Manifest::Location location_;
+  VisitorInterface* visitor_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockExtensionProvider);
+};
+
 }  // namespace
 
-class DeveloperPrivateApiUnitTest : public ExtensionServiceTestBase {
+class DeveloperPrivateApiUnitTest : public ExtensionServiceTestWithInstall {
  protected:
   DeveloperPrivateApiUnitTest() {}
   ~DeveloperPrivateApiUnitTest() override {}
 
+  void AddMockExternalProvider(
+      std::unique_ptr<ExternalProviderInterface> provider) {
+    service()->AddProviderForTesting(std::move(provider));
+  }
+
   // A wrapper around extension_function_test_utils::RunFunction that runs with
   // the associated browser, no flags, and can take stack-allocated arguments.
   bool RunFunction(const scoped_refptr<UIThreadExtensionFunction>& function,
@@ -854,6 +930,45 @@ TEST_F(DeveloperPrivateApiUnitTest, DeveloperPrivateDeleteExtensionErrors) {
   EXPECT_TRUE(error_console->GetErrorsForExtension(extension->id()).empty());
 }
 
+// Tests that developerPrivate.repair does not succeed for a non-corrupted
+// extension and any policy extension.
+// Regression test for https://crbug.com/577959.
+TEST_F(DeveloperPrivateApiUnitTest, RepairNotBrokenExtension) {
+  std::string extension_id(good_crx);
+
+  // Set up a mock provider with a policy extension.
+  std::unique_ptr<MockExtensionProvider> mock_provider =
+      base::MakeUnique<MockExtensionProvider>(
+          service(), Manifest::EXTERNAL_POLICY_DOWNLOAD);
+  MockExtensionProvider* mock_provider_ptr = mock_provider.get();
+  AddMockExternalProvider(std::move(mock_provider));
+  mock_provider_ptr->AddExtension(extension_id, "1.0.0.0",
+                                  data_dir().AppendASCII("good.crx"));
+  // Reloading extensions should find our externally registered extension
+  // and install it.
+  content::WindowedNotificationObserver observer(
+      extensions::NOTIFICATION_CRX_INSTALLER_DONE,
+      content::NotificationService::AllSources());
+  service()->CheckForExternalUpdates();
+  observer.Wait();
+
+  // Attempt to repair the good extension, expect failure.
+  std::unique_ptr<base::ListValue> args =
+      ListBuilder().Append(extension_id).Build();
+  scoped_refptr<UIThreadExtensionFunction> function =
+      new api::DeveloperPrivateRepairExtensionFunction();
+  EXPECT_FALSE(RunFunction(function, *args));
+  EXPECT_EQ("Cannot repair a not corrupted extension.", function->GetError());
+
+  // Corrupt the extension , still expect repair failure because this is a
+  // policy extension.
+  service()->DisableExtension(extension_id, Extension::DISABLE_CORRUPTED);
+  args = ListBuilder().Append(extension_id).Build();
+  function = new api::DeveloperPrivateRepairExtensionFunction();
+  EXPECT_FALSE(RunFunction(function, *args));
+  EXPECT_EQ("Cannot repair a policy extension.", function->GetError());

[Devlin, 2017/04/06 21:01:32]

I'd slightly prefer two separate tests - one for uncorrupted, and one for policy
- so that we know we don't allow repair of uncorrupted extensions independent of
policy status.

[lazyboy, 2017/04/06 22:32:56]

Done.

+}
+
 // Test developerPrivate.updateProfileConfiguration: Try to turn on devMode
 // when DeveloperToolsDisabled policy is active.
 TEST_F(DeveloperPrivateApiUnitTest, DeveloperPrivateDevModeDisabledPolicy) {