Add minimum version to extension management

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 283f42a1fc3059b09f965b156b5455ac1dbfa62c..e9f6ba3cf4c1aa6795f619868825d3e1e4287b65 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -853,6 +853,7 @@ void ExtensionService::DisableExtension(
   // preferences.
   if (extension &&
       disable_reason != Extension::DISABLE_RELOAD &&
+      disable_reason != Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY &&
       !system_->management_policy()->UserMayModifySettings(extension, NULL) &&
       extension->location() != Manifest::EXTERNAL_COMPONENT) {
     return;
@@ -1111,27 +1112,71 @@ base::SequencedTaskRunner* ExtensionService::GetFileTaskRunner() {
 void ExtensionService::CheckManagementPolicy() {
   std::vector<std::string> to_unload;
   std::map<std::string, Extension::DisableReason> to_disable;
+  std::vector<std::string> to_enable;
 
   // Loop through the extensions list, finding extensions we need to unload or
   // disable.
-  const ExtensionSet& extensions = registry_->enabled_extensions();
-  for (ExtensionSet::const_iterator iter = extensions.begin();
-       iter != extensions.end(); ++iter) {
-    const Extension* extension = (iter->get());
-    if (!system_->management_policy()->UserMayLoad(extension, NULL))
+  for (scoped_refptr<const Extension> extension :
+       registry_->enabled_extensions()) {
+    if (!system_->management_policy()->UserMayLoad(extension.get(), nullptr))
       to_unload.push_back(extension->id());
     Extension::DisableReason disable_reason = Extension::DISABLE_NONE;
     if (system_->management_policy()->MustRemainDisabled(
-            extension, &disable_reason, NULL))
+            extension.get(), &disable_reason, nullptr))
       to_disable[extension->id()] = disable_reason;
   }
 
-  for (size_t i = 0; i < to_unload.size(); ++i)
-    UnloadExtension(to_unload[i], UnloadedExtensionInfo::REASON_DISABLE);
+  extensions::ExtensionManagement* management =
+      extensions::ExtensionManagementFactory::GetForBrowserContext(profile());
+
+  // Loop through the disabled extension list, find extensions to re-enable
+  // automatically. These extensions are exclusive from the |to_disable| and
+  // |to_unload| lists constructed above, since disabled_extensions() and
+  // enabled_extensions() are supposed to be mutually exclusive.
+  for (scoped_refptr<const Extension> extension :
+       registry_->disabled_extensions()) {
+    // Find all disabled extensions disabled due to minimum version requirement,
+    // but now satisfying it.
+    if (management->CheckMinimumVersionRequirement(extension.get(), nullptr) &&
+        extension_prefs_->HasDisableReason(
+            extension->id(), Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY)) {
+      // Is DISABLE_UPDATE_REQUIRED_BY_POLICY the *only* reason?
+      if (extension_prefs_->GetDisableReasons(extension->id()) ==
+          Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY) {
+        // We need to enable those disabled *only* due to minimum version
+        // requirement.
+        to_enable.push_back(extension->id());
+      }
+      extension_prefs_->RemoveDisableReason(
+          extension->id(), Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY);
+    }
+  }
+
+  for (const std::string& id : to_unload)
+    UnloadExtension(id, UnloadedExtensionInfo::REASON_DISABLE);
 
   for (std::map<std::string, Extension::DisableReason>::const_iterator i =
            to_disable.begin(); i != to_disable.end(); ++i)
     DisableExtension(i->first, i->second);
+
+  for (const std::string& id : to_enable)
+    EnableExtension(id);
+
+  if (updater_.get()) {
+    // Find all extensions (including the ones that got disabled just now)
+    // disabled due to minimum version requirement from policy, and check for
+    // update.
+    extensions::ExtensionUpdater::CheckParams params;

[Finnur, 2014/11/19 20:50:18]

Nit: params is a bit general. To be consistent with the other vars it would make
sense name this: to_recheck (or to_check_again). :)

[Finnur, 2014/11/20 12:25:35]

What about these two (line 1174 also)?

[binjin, 2014/11/20 13:11:54]

Sorry, I yesterday was also trying to make quick reply for things I got
confused. And thus ignored other nits.

[binjin, 2014/11/20 13:11:54]

I yesterday was also trying to make quick response for things I got confused. I
didn't make any changes.

+    for (scoped_refptr<const Extension> extension :
+         registry_->disabled_extensions()) {
+      if (extension_prefs_->GetDisableReasons(extension->id()) ==
+          Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY) {
+        params.ids.push_back(extension->id());

[Finnur, 2014/11/19 20:50:18]

Nit: Comment this line with:
// The minimum version check is the only thing holding this 
// extension back, so check if it can be updated to fix that.
...  or something to that effect.

[binjin, 2014/11/20 13:11:54]

Done.

+      }
+    }
+    if (!params.ids.empty())
+      updater_->CheckNow(params);
+  }
 }
 
 void ExtensionService::CheckForUpdatesSoon() {
@@ -1607,13 +1652,22 @@ void ExtensionService::OnExtensionInstalled(
   // Unsupported requirements overrides the management policy.
   if (install_flags & extensions::kInstallFlagHasRequirementErrors) {
     disable_reasons |= Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
-  // If the extension was disabled because of unsupported requirements but
-  // now supports all requirements after an update and there are not other
-  // disable reasons, enable it.
-  } else if (extension_prefs_->GetDisableReasons(id) ==
-             Extension::DISABLE_UNSUPPORTED_REQUIREMENT) {
-    disable_reasons = Extension::DISABLE_NONE;
-    extension_prefs_->ClearDisableReasons(id);
+  } else {
+    // Requirement is supported now, remove the corresponding disable reason
+    // instead.
+    extension_prefs_->RemoveDisableReason(
+        id, Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
+    disable_reasons &= ~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
+  }
+
+  // Check if the extension was disabled because of the minimum version
+  // requirements from enterprise policy, and satisfies it now.
+  if (extensions::ExtensionManagementFactory::GetForBrowserContext(profile())
+          ->CheckMinimumVersionRequirement(extension, nullptr)) {
+    // And remove the corresponding disable reason.
+    extension_prefs_->RemoveDisableReason(
+        id, Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY);
+    disable_reasons &= ~Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY;
   }

[Finnur, 2014/11/20 12:25:35]

So, I agree with you, that this looks like what we should be doing. If we detect
that the faulty condition no longer applies, we can remove these disable
reasons. 

However, can you track down the author of the original check (lines 1607-1617 on
the left) and ask why DISABLE_UNSUPPORTED_REQUIREMENT was not being removed
unless it was the *last* disable reason? Just to make sure we're not introducing
a bug here? I suspect the answer is "because it won't be enabled anyway, unless
it is the last one", but we should make sure.

[binjin, 2014/11/20 13:11:54]

Acknowledged.

[binjin, 2014/11/20 13:17:16]

@eaugusti, sorry for bothering, you are the original author of code removing
Extension::DISABLE_UNSUPPORTED_REQUIREMENT here. See
https://codereview.chromium.org/10908184/diff/1/chrome/browser/extensions/ext...

Could you clarify the reason we only remove this as the LAST disable reason? The
current modified behavior is that we will remove this disable reason even if
there are other reasons. And only re-enable it if there are no other disable
reasons.

 
   if (install_flags & extensions::kInstallFlagIsBlacklistedForMalware) {