developerPrivate.repair: Skip not-corrupted and policy extensions

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

Index: chrome/browser/extensions/api/developer_private/developer_private_api.cc
diff --git a/chrome/browser/extensions/api/developer_private/developer_private_api.cc b/chrome/browser/extensions/api/developer_private/developer_private_api.cc
index f2b455a41118817bc33a49a150372e93107a9182..c9c6996a3696864dac192eb0fad739e9c252ac08 100644
--- a/chrome/browser/extensions/api/developer_private/developer_private_api.cc
+++ b/chrome/browser/extensions/api/developer_private/developer_private_api.cc
@@ -75,6 +75,7 @@
 #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"
@@ -109,6 +110,8 @@ 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";
@@ -1461,6 +1464,18 @@ DeveloperPrivateRepairExtensionFunction::Run() {
   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));