Track permissions granted to extensions in prefs

chrome/browser/extensions/extensions_service.cc

Index: chrome/browser/extensions/extensions_service.cc
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 06fe0687551e60f658171c20d7ae2d9920fda1d5..bb358ec4481473c601533dbf34d552b37ffd8a65 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -927,6 +927,22 @@ void ExtensionsService::DisableExtension(const std::string& extension_id) {
   UpdateActiveExtensionsInCrashReporter();
 }
 
+void ExtensionsService::GrantPermissions(const Extension* extension) {
+  DCHECK(extension);
+  ExtensionExtent effective_hosts = extension->GetEffectiveHostPermissions();
+  extension_prefs_->AddGrantedPermissions(extension->id(),
+                                          extension->api_permissions(),
+                                          effective_hosts);
+}
+
+void ExtensionsService::GrantPermissionsAndEnableExtension(
+    const Extension* extension) {
+  DCHECK(extension);
+  GrantPermissions(extension);
+  extension_prefs_->SetDidExtensionEscalatePermissions(extension, false);
+  EnableExtension(extension->id());
+}
+
 void ExtensionsService::LoadExtension(const FilePath& extension_path) {
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ -1104,7 +1120,6 @@ void ExtensionsService::LoadAllExtensions() {
                            browser_action_count);
 }
 
-
 void ExtensionsService::LoadInstalledExtension(const ExtensionInfo& info,
                                                bool write_to_prefs) {
   std::string error;
@@ -1131,9 +1146,10 @@ void ExtensionsService::LoadInstalledExtension(const ExtensionInfo& info,
   if (write_to_prefs)
     extension_prefs_->UpdateManifest(extension);
 
-  OnExtensionLoaded(extension, true);
-
+  // External extensions are allowed to increase their privileges without
+  // prompting users.
   if (Extension::IsExternalLocation(info.extension_location)) {
+    OnExtensionLoaded(extension, true);
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(
@@ -1141,6 +1157,8 @@ void ExtensionsService::LoadInstalledExtension(const ExtensionInfo& info,
             &ExtensionsServiceBackend::CheckExternalUninstall,
             scoped_refptr<ExtensionsService>(this),
             info.extension_id));
+  } else {
+    OnExtensionLoaded(extension, false);
   }
 }
 
@@ -1514,17 +1532,64 @@ void ExtensionsService::OnExtensionLoaded(const Extension* extension,
       Extension::IsExternalLocation(extension->location())) {
     const Extension* old = GetExtensionByIdInternal(extension->id(),
                                                     true, true);
-    if (old) {
+
+    // We keep track of all permissions the user has granted each extension.

[Aaron Boodman, 2010/11/22 07:57:53]

How about factoring all this big chunk of upgrade logic into its own method.
That way this comment can go there.

[jstritar, 2010/11/22 23:01:08]

Done.

+    // This allows extensions to gracefully support backwards compatibility
+    // by including unknown permissions in their manifests. When the user
+    // installs the extension, only the recognized permissions are recorded.
+    // When the unknown permissions become recognized (e.g., through browser
+    // upgrade), we can prompt the user to accept these new permissions.
+    // Extensions can also silently upgrade to less permissions, and then
+    // silently upgrade to a version that adds these permissions back.
+    //
+    // For example, pretend that Chrome 10 includes a permission "omnibox"
+    // for an API that adds suggestions to the omnibox. An extension can
+    // maintain backwards compatibility while still having "omnibox" in the
+    // manifest. If a user installs the extension on Chrome 9, the browser
+    // will record the permissions it recognized, not including "omnibox."
+    // When upgrading to Chrome 10, "omnibox" will be recognized and Chrome
+    // will disable the extension and prompt the user to approve the increase
+    // in privileges. The extension could then release a new version that
+    // removes the "omnibox" permission. When the user upgrades, Chrome will
+    // still remember that "omnibox" had been granted, so that if the
+    // extension once again includes "omnibox" in an upgrade, the extension
+    // can upgrade without requiring this user's approval.
+
+    std::set<std::string> granted_apis;
+    ExtensionExtent granted_extent;
+
+    // Add all the recognized permissions to the extension's granted
+    // permissions list if the granted permissions list hasn't been
+    // initialized yet or privilege increases are allowed.
+    if (extension->location() != Extension::COMPONENT &&

[Aaron Boodman, 2010/11/22 07:57:53]

It seems like we only want to store the privileges in the case of INTERNAL
extensions, to help keep the Preferences file size down, not COMPONENT or
EXTERNAL_*.

In that case, the call site for OnExtensionLoaded for component extensions could
be changed to pass |true| for allow_privilege_increase, then this code could be
simpler:

if (!allow_privilege_increase || !GetGrantedPermissions()) {
  GrantPermissions();
}

[jstritar, 2010/11/22 23:01:08]

Done. I ended up needing to change the allow_privilege_increase value in a
couple more places (Extension::InstallExtension and
download_util::OpenChromeExtension).

+        (!extension_prefs_->GetGrantedPermissions(
+            extension->id(), &granted_apis, &granted_extent) ||
+         allow_privilege_increase)) {
+      GrantPermissions(extension);
+      DCHECK(extension_prefs_->GetGrantedPermissions(

[Aaron Boodman, 2010/11/22 07:57:53]

Danger! Never do work with side-effects in DCHECK. This gets compiled out in
release builds!

[jstritar, 2010/11/22 23:01:08]

Wow, yeah good point. Fixed.

+          extension->id(), &granted_apis, &granted_extent));
+    }
+
+    bool is_extension_upgrade = old != NULL;
+    bool is_privilege_increase = false;
+
+    // Here, we check if an extension's privileges have increased in a manner
+    // that requires the user's approval. This could occur because the browser
+    // upgraded and recognized additional privileges, or an extension upgrades
+    // to a version that requires additional privileges.
+    if ((extension->location() == Extension::INTERNAL || is_extension_upgrade)

[Aaron Boodman, 2010/11/22 07:57:53]

And we could remove the location() check here too:

if (is_extension_upgrade && !allow_privilege_increase) {

}

[jstritar, 2010/11/22 23:01:08]

Done.

+        && !allow_privilege_increase) {
+      is_privilege_increase = Extension::IsPrivilegeIncrease(
+          granted_apis, granted_extent, old, extension);
+    }
+
+    if (is_extension_upgrade) {
       // CrxInstaller should have guaranteed that we aren't downgrading.
       CHECK(extension->version()->CompareTo(*(old->version())) >= 0);
 
-      bool allow_silent_upgrade =
-          allow_privilege_increase || !Extension::IsPrivilegeIncrease(
-              old, extension);
-
-      // Extensions get upgraded if silent upgrades are allowed, otherwise
-      // they get disabled.
-      if (allow_silent_upgrade) {
+      // Extensions get upgraded if the privileges are allowed to increase or
+      // the privileges haven't increased.
+      if (allow_privilege_increase || !is_privilege_increase) {
         SetBeingUpgraded(old, true);
         SetBeingUpgraded(extension, true);
       }
@@ -1533,13 +1598,13 @@ void ExtensionsService::OnExtensionLoaded(const Extension* extension,
       // then load the new one.
       UnloadExtension(old->id());
       old = NULL;
+    }
 
-      if (!allow_silent_upgrade) {
-        // Extension has changed permissions significantly. Disable it. We
-        // send a notification below.
-        extension_prefs_->SetExtensionState(extension, Extension::DISABLED);
-        extension_prefs_->SetDidExtensionEscalatePermissions(extension, true);
-      }
+    // Extension has changed permissions significantly. Disable it. We
+    // send a notification below.
+    if (is_privilege_increase) {
+      extension_prefs_->SetExtensionState(extension, Extension::DISABLED);
+      extension_prefs_->SetDidExtensionEscalatePermissions(extension, true);
     }
 
     switch (extension_prefs_->GetExtensionState(extension->id())) {