Reinstall extensions if they become policy-controlled

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2011 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 <set>
 
 #include "base/basictypes.h"
@@ skipping 392 matching lines @@
       linked_ptr<ExternalExtensionProviderInterface>(test_provider));
 }
 
 void ExtensionService::OnExternalExtensionUpdateUrlFound(
     const std::string& id,
     const GURL& update_url,
     Extension::Location location) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   CHECK(Extension::IdIsValid(id));
 
-  if (GetExtensionById(id, true)) {
-    // Already installed.  Do not change the update URL that the extension set.
-    return;
+  const Extension* extension = GetExtensionById(id, true);
+  if (extension) {
+    if (location != extension->location() &&

[Sam Kerner (Chrome), 2011/06/24 20:03:34]

Uninstalling wipes any extension state in localstorage.  It would be better to
install the new CRX without uninstalling the old one.  A quick read of the code
makes me think that if you let the install run without uninstalling, the new
location be used.  There may be some logic to stop the fetch or install if the
extension is already installed.  You could use a check that the pending
extension's location is different to override such a check.



Testing against Extension::EXTERNAL_POLICY_DOWNLOAD works, but it can be made
more general.  The function Extension::GetHigherPriorityLocation() takes two
Extension::Locations, and returns the one chrome should choose if the same
extension is installed with the two locations.  Policy is the highest priority. 
You could use that function to see if the new location outranks the current one,
and allow the install to continue if it does.

It is important not to allow the install if the locations are the same, because
that happens on every reading of the external sources.  Something like:

if (extension)
  Extension::Location current = extension->location();
  // If the current location outranks the new one, skip this install.
  if (current == Extension::GetHigherPriorityLocation(location, current))
    return;
}

This will do the right thing when a user-installed extension is added as
EXTERNAL_UPDATE_URL, which also outranks a user install.

[gfeher, 2011/06/27 12:59:49]

Your comment got me thinking. In the case you describe, consider:
1. user installs extension X
2. an application registers extension X as EXTERNAL_UPDATE_URL
3. the application gets uninstalled and removes extension X as
EXTERNAL_UPDATE_URL
4. extension X gets uninstalled
(I guess what we are missing is extensions having multiple locations, including
user install.)

This is actually also a problem for policy-controlled extensions, although it
may be acceptable in their special case. (I'll have to talk with gwilson about
it.)

WDYT?

[Sam Kerner (Chrome), 2011/06/27 14:26:30]

Yes, we can't do the right thing here without some way to represent the fact
that extensions are installed for more than one reason.  It was decided that the
complexity of supporting multiple locations was not worth the benefit of fixing
this.  At the time, we were not thinking about policy extensions.

If you feel that this is a significant issue for policy extensions, send an
email to Erik Kay and me, and we can reevaluate.

+        location == Extension::EXTERNAL_POLICY_DOWNLOAD) {
+      // Already installed, but not as a policy controlled extension.
+      // Trigger a reainstall to make its location right, and thus disable its
+      // manual uninstall.
+      std::string error;
+      UninstallExtension(extension->id(), false, &error);
+    } else {
+      // Already installed.  Do not change the update URL that the extension
+      // set.
+      return;
+    }
   }
   pending_extension_manager()->AddFromExternalUpdateUrl(
       id, update_url, location);
   external_extension_url_added_ |= true;
 }
 
 bool ExtensionService::IsDownloadFromGallery(const GURL& download_url,
                                              const GURL& referrer_url) {
   // Special-case the themes mini-gallery.
   // TODO(erikkay) When that gallery goes away, remove this code.
@@ skipping 2004 matching lines @@
 
 ExtensionService::NaClModuleInfoList::iterator
     ExtensionService::FindNaClModule(const GURL& url) {
   for (NaClModuleInfoList::iterator iter = nacl_module_list_.begin();
        iter != nacl_module_list_.end(); ++iter) {
     if (iter->url == url)
       return iter;
   }
   return nacl_module_list_.end();
 }