Add minimum version to extension management

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2013 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/extension_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 835 matching lines @@
   }
 
   const Extension* extension = GetInstalledExtension(extension_id);
   // |extension| can be NULL if sync disables an extension that is not
   // installed yet.
   // EXTERNAL_COMPONENT extensions are not generally modifiable by users, but
   // can be uninstalled by the browser if the user sets extension-specific
   // preferences.
   if (extension &&
       disable_reason != Extension::DISABLE_RELOAD &&
+      disable_reason != Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY &&

[Finnur, 2014/11/19 10:16:26]

This doesn't look quite right. Disable reasons use a bitmask, so there could be
more than one. You need to replace != with &, or use HasDisableReason from
prefs.

[binjin, 2014/11/19 13:36:11]

I think it's desired, the Extension::DisableReason is an enum stating a single
disable reason. For places that we actually need a bitmask, an int is explictly
used.

Also if there are multiple reasons, we don't want to block that since there are
other reasons to disable the extensions.

[Finnur, 2014/11/19 15:47:38]

Ah, right! Sorry.

       !system_->management_policy()->UserMayModifySettings(extension, NULL) &&
       extension->location() != Manifest::EXTERNAL_COMPONENT) {
     return;
   }
 
   extension_prefs_->SetExtensionState(extension_id, Extension::DISABLED);
   extension_prefs_->AddDisableReason(extension_id, disable_reason);
 
   int include_mask =
       ExtensionRegistry::EVERYTHING & ~ExtensionRegistry::DISABLED;
@@ skipping 238 matching lines @@
   file_task_runner_ = BrowserThread::GetBlockingPool()->
       GetSequencedTaskRunnerWithShutdownBehavior(
         BrowserThread::GetBlockingPool()->GetNamedSequenceToken(token),
         base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   return file_task_runner_.get();
 }
 
 void ExtensionService::CheckManagementPolicy() {
   std::vector<std::string> to_unload;
   std::map<std::string, Extension::DisableReason> to_disable;
+  std::vector<std::string> to_enable;

[Finnur, 2014/11/19 10:16:26]

It seems a bit weird to let the system disable extensions that have only the
DISABLE_UPDATE_REQUIRED_BY_POLICY flag set, and then re-enable them if they've
already passed the min version requirement. Is that necessary?

[binjin, 2014/11/19 13:36:11]

|to_enable| and |to_disable| are mutually exclusive, we collected two lists of
extensions then do corresponding operations. And finally we call updater with
certain extensions.

That is, we might disable certain extensins, but won't enable it immediately,
but we call updater to check a newer version if possible.

[Finnur, 2014/11/19 15:47:38]

Hmmm, that's totally non-obvious due to the MustRemainDisabled call. Does that
function return TRUE for extensions failing this new minimum version check? Is
that expected?

[binjin, 2014/11/19 17:21:41]

Yes, we use MustRemainDisabled() for that purpose. We prepare these three lists,
and after that we do the operation. Thus the two enumeration loops below is kind
of independent.

The |to_enable| list are selected from |registry_->disabled_extensions()|, and
|to_disable| list are selected from |registry_->enabled_extensions()|, that's
the reason they will be mutally exclusive.

Added few more comments on this.

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

But... that's my point. Isn't it a bug that MustRemainDisabled returns TRUE for
an extension that fails the min_version check? Will that lead to an extension
getting enabled when it should be disabled?

[binjin, 2014/11/19 21:31:40]

I'm confused. MustRemainDisaabled() will add an extension to |to_disable| list,
but we won't enable extensions from |to_disable|. We only tell updater to check
immediately if there is a newer version for it or not. After fetching and
installed the crx, OnExtensionInstalled() will enable it much later if the new
version meets the minimum version requirement.

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

Sorry, this confusion is, I think, on my part and is a combination of me being
jetlagged and trying to rush through the review at the last minute to give you
feedback before I leave for the day.

I think you are saying that MustRemainDisabled will return TRUE for extensions
with this new DISABLE_UPDATE_REQUIRED_BY_POLICY, in which case I don't have a
concern. If it does not let me know.

 
   // 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.
+  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->CheckMinimumVersion(extension.get(), nullptr) &&
+        extension_prefs_->HasDisableReason(
+            extension->id(), Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY)) {
+      if (extension_prefs_->GetDisableReasons(extension->id()) ==

[Finnur, 2014/11/19 10:16:26]

I would comment this line with:
// Is DISABLE_UPDATE_REQUIRED_BY_POLICY the *only* reason?

[binjin, 2014/11/19 13:36:11]

Done.

+          Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY) {
+          // We need to enable those disabled only due to minimum version

[Finnur, 2014/11/19 10:16:26]

nit: Indentation is wrong here.

[binjin, 2014/11/19 13:36:11]

Oops, Done.

+          // requirement.
+          to_enable.push_back(extension->id());

[Finnur, 2014/11/19 10:16:26]

You've already established here that this is an extension that no longer fails
the min version check -- and that that was the only thing holding it back. Can
we instead remove the id from to_disable so it doesn't get disabled?

[binjin, 2014/11/19 13:36:11]

|to_enable| and |to_disable| are mutually exclusive, see comments above.

+          extension_prefs_->ClearDisableReasons(extension->id());

[Finnur, 2014/11/19 10:16:26]

Delete line 1146, remove the else clause and just always call
RemoveDisableReason for DISABLE_UPDATE_REQUIRED_BY_POLICY (between lines 1151
and 1152).

[binjin, 2014/11/19 13:36:11]

Done.

+        } else {
+          // otherwise, there are other disable reasons, just remove ours.
+          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)

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

I would comment this |for| loop with something like:
// No extension is getting re-enabled here after disabling/unloading
// because to_enable is mutually exclusive to to_disable + to_unload.

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

Done.

+    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.

[Finnur, 2014/11/19 10:16:26]

nit: This comment will probably need to be updated if you follow my
recommendations above.

[binjin, 2014/11/19 13:36:11]

Acknowledged.

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

Suggest for easier reading:
// Find all extensions disabled due to minimum version requirement from
// policy (including the ones that got disabled just now), and check for
// update.

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

Done.

+    extensions::ExtensionUpdater::CheckParams params;
+    for (scoped_refptr<const Extension> extension :
+         registry_->disabled_extensions()) {
+      if (extension_prefs_->HasDisableReason(
+              extension->id(), Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY)) {
+        params.ids.push_back(extension->id());

[Finnur, 2014/11/19 10:16:25]

Is this code *required* to get a disabled extension past the min version or will
that happen anyway at some point? If it is *not* required, then I would not
check *unless* DISABLE_UPDATE_REQUIRED_BY_POLICY is the *only* thing holding it
back (given that we already know it won't become enabled after updating because
more things are holding it back).

[binjin, 2014/11/19 13:36:11]

Done.

+      }
+    }
+    if (!params.ids.empty())
+      updater_->CheckNow(params);
+  }
 }
 
 void ExtensionService::CheckForUpdatesSoon() {
   // This can legitimately happen in unit tests.
   if (!updater_.get())
     return;
 
   if (AreAllExternalProvidersReady()) {
     updater_->CheckSoon();
   } else {
@@ skipping 455 matching lines @@
     // extension; if we're here, that means the user is manually
     // installing the extension.
     if (extension_prefs_->IsExternalExtensionUninstalled(id)) {
       disable_reasons = Extension::DISABLE_NONE;
     }
   }
 
   // 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) {
+    // The extension was disabled because of unsupported requirements but
+    // now supports all requirements after an update.
+    // If there are no other disable reasons, enable it.
     disable_reasons = Extension::DISABLE_NONE;
     extension_prefs_->ClearDisableReasons(id);
+  } else {
+    // Otherwise just remove the corresponding disable reason.
+    extension_prefs_->RemoveDisableReason(
+        id, Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
+    disable_reasons &= ~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
+  }

[Finnur, 2014/11/19 10:16:26]

Both RemoveDisableReason and the bitmask removal of
DISABLE_UNSUPPORTED_REQUIREMENT will result in disable_reasons becoming
DISABLE_NONE -- assuming DISABLE_UNSUPPORTED_REQUIREMENT was the only thing
remaining. Therefore, I don't think we need to explicitly check here (in two
places now) if DISABLE_UNSUPPORTED_REQUIREMENT was the only thing remaining. We
can just do:

// Unsupported requirements overrides the management policy.
if (install_flags & extensions::kInstallFlagHasRequirementErrors) {
  disable_reasons |= Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
} else {
  // Otherwise just remove the corresponding disable reason.
  extension_prefs_->RemoveDisableReason(
      id, Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
  disable_reasons &= ~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
}

Right? Similar logic below.

[binjin, 2014/11/19 13:36:11]

Done.

+
+  // Check if the extension was disabled because of the minimum version
+  // requirements from enterprise policy, and satisfies it now.
+  if (extensions::ExtensionManagementFactory::GetForBrowserContext(profile())
+          ->CheckMinimumVersion(extension, nullptr)) {
+    if (extension_prefs_->GetDisableReasons(id) ==
+        Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY) {
+      // There are no other disable reasons, re-enable the extension.
+      disable_reasons = Extension::DISABLE_NONE;
+      extension_prefs_->ClearDisableReasons(id);
+    } else {
+      // Otherwise just clear the corresponding disable reason.
+      extension_prefs_->RemoveDisableReason(
+          id, Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY);
+      disable_reasons &= ~Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY;
+    }
   }
 
   if (install_flags & extensions::kInstallFlagIsBlacklistedForMalware) {
     // Installation of a blacklisted extension can happen from sync, policy,
     // etc, where to maintain consistency we need to install it, just never
     // load it (see AddExtension). Usually it should be the job of callers to
     // incercept blacklisted extension earlier (e.g. CrxInstaller, before even
     // showing the install dialogue).
     extension_prefs_->AcknowledgeBlacklistedExtension(id);
     UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.SilentInstall",
@@ skipping 778 matching lines @@
 }
 
 void ExtensionService::OnProfileDestructionStarted() {
   ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs();
   for (ExtensionIdSet::iterator it = ids_to_unload.begin();
        it != ids_to_unload.end();
        ++it) {
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN);
   }
 }