developerPrivate.repair: Skip not-corrupted and policy extensions

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/developer_private/developer_private_api.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 57 matching lines @@
 #include "extensions/browser/file_highlighter.h"
 #include "extensions/browser/management_policy.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/warning_service.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/install_warning.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_handlers/options_page_info.h"
+#include "extensions/common/manifest_url_handlers.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/grit/extensions_browser_resources.h"
 #include "storage/browser/fileapi/external_mount_points.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/fileapi/file_system_operation.h"
 #include "storage/browser/fileapi/file_system_operation_runner.h"
 #include "storage/browser/fileapi/isolated_context.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace extensions {
@@ skipping 14 matching lines @@
 const char kNoSuchRendererError[] = "No such renderer.";
 const char kInvalidPathError[] = "Invalid path.";
 const char kManifestKeyIsRequiredError[] =
     "The 'manifestKey' argument is required for manifest files.";
 const char kCouldNotFindWebContentsError[] =
     "Could not find a valid web contents.";
 const char kCannotUpdateSupervisedProfileSettingsError[] =
     "Cannot change settings for a supervised profile.";
 const char kNoOptionsPageForExtensionError[] =
     "Extension does not have an options page.";
+const char kCannotRepairNotCorruptedExtension[] =
+    "Cannot repair a not corrupted extension.";
 
 const char kUnpackedAppsFolder[] = "apps_target";
 const char kManifestFile[] = "manifest.json";
 
 ExtensionService* GetExtensionService(content::BrowserContext* context) {
   return ExtensionSystem::Get(context)->extension_service();
 }
 
 std::string ReadFileToString(const base::FilePath& path) {
   std::string data;
@@ skipping 1332 matching lines @@
 
 ExtensionFunction::ResponseAction
 DeveloperPrivateRepairExtensionFunction::Run() {
   std::unique_ptr<developer::RepairExtension::Params> params(
       developer::RepairExtension::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
   const Extension* extension = GetExtensionById(params->extension_id);
   if (!extension)
     return RespondNow(Error(kNoSuchExtensionError));
 
+  ExtensionSystem* extension_system = ExtensionSystem::Get(browser_context());
+  if (extension_system->management_policy()->MustRemainEnabled(extension,

[Devlin, 2017/04/06 00:54:06]

Should we also check MustRemainInstalled()?

[lazyboy, 2017/04/06 01:33:53]

I'm not entirely sure, I copied the logic from verifier:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content...

Note that there's also !UserMayModifySettings checks somewhere.

[Devlin, 2017/04/06 15:12:31]

Yeah, the differences differences between all of these possible policy settings
are pretty subtle, and sometimes unclear.  Since this inherently uninstalls the
extension (and only performs a one-off attempt), let's also check
MustRemainInstalled(), but we can probably avoid UserMayModifySettings (at least
for now). 

[lazyboy, 2017/04/06 18:06:05]

OK, done MustRemainEnabled || MustRemainInstalled
As this should match our content verifier code in ChromeContentVerifierDelegate,
I've extracted this to a separate function
ContentVerifier::ShouldRepairIfCorrupted and used it in CCVDelegate.
I wanted the name to sound more tied to content verification than policy, but
not sure. WDYT?

+                                                               nullptr)) {
+    int disable_reason = ExtensionPrefs::Get(browser_context())
+                             ->GetDisableReasons(extension->id());
+    // Do not let a policy extension that is not corrupted to be
+    // reinstalled/repaired. Currently this would uninstall the extension but

[Devlin, 2017/04/06 00:54:06]

Should we even allow this in the case of corruption?  If it's going to fail on
reinstall, it sounds like something we shouldn't do - especially now that we
should be repairing corrupt policy extensions.

[lazyboy, 2017/04/06 01:33:53]

"Currently" is misleading here, I meant to put that in CL description to
describe what happens without the CL. With the CL, we do not uninstall the
extension at all. Updated the comment.

[Devlin, 2017/04/06 15:12:31]

Yep, I figured. :)  But my main question still stands - it sounds like, even in
the case of corruption, the reinstall here will fail due to a policy check. 
This accounts for the case of a call to repair an uncorrupted extension, but
even with a corrupted extension, perhaps we shouldn't do this?  If I have a
corrupted policy extension and do this, won't it just uninstall it and fail?  Or
am I misunderstanding the policy check?

[lazyboy, 2017/04/06 18:06:05]

Ah, that is correct. I've disallowed policy extension altogether.
 Or

+    // then we will fail to reinstall it for policy check.
+    if ((disable_reason & Extension::DISABLE_CORRUPTED) == 0)

[Devlin, 2017/04/06 00:55:24]

And actually, we should probably have this check either here, in
WebstoreReinstaller, or both, for all extensions (regardless of policy or not)

[lazyboy, 2017/04/06 01:33:53]

Putting it in WebstoreReinstaller might other cases where we try to reinstall
from somewhere else than here?

Oh, this function is the only consumer of WebstoreReinstaller, so added the
check there.

+      return RespondNow(Error(kCannotRepairNotCorruptedExtension));
+  }
+
   content::WebContents* web_contents = GetSenderWebContents();
   if (!web_contents)
     return RespondNow(Error(kCouldNotFindWebContentsError));
 
   scoped_refptr<WebstoreReinstaller> reinstaller(new WebstoreReinstaller(
       web_contents,
       params->extension_id,
       base::Bind(&DeveloperPrivateRepairExtensionFunction::OnReinstallComplete,
                  this)));
   reinstaller->BeginReinstall();
@@ skipping 83 matching lines @@
         update.extension_id, update.command_name, *update.keybinding);
   }
 
   return RespondNow(NoArguments());
 }
 
 
 }  // namespace api
 
 }  // namespace extensions