Change the web store private install API to accept a localized extension name.

chrome/browser/extensions/crx_installer.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/crx_installer.h"
 
 #include <map>
 #include <set>
 
 #include "base/file_util.h"
@@ skipping 23 matching lines @@
 #include "content/common/notification_type.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
 namespace {
 
-struct WhitelistedInstallData {
-  WhitelistedInstallData() {}
+struct Whitelist {
+  Whitelist() {}
   std::set<std::string> ids;
-  std::map<std::string, linked_ptr<DictionaryValue> > manifests;
+  std::map<std::string, linked_ptr<CrxInstaller::WhitelistEntry> > entries;
 };
 
-static base::LazyInstance<WhitelistedInstallData>
+static base::LazyInstance<Whitelist>
     g_whitelisted_install_data(base::LINKER_INITIALIZED);
 
 }  // namespace
 
 // static
 void CrxInstaller::SetWhitelistedInstallId(const std::string& id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   g_whitelisted_install_data.Get().ids.insert(id);
 }
 
 // static
-void CrxInstaller::SetWhitelistedManifest(const std::string& id,
-                                          DictionaryValue* parsed_manifest) {
+void CrxInstaller::SetWhitelistEntry(const std::string& id,
+                                     CrxInstaller::WhitelistEntry* entry) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  WhitelistedInstallData& data = g_whitelisted_install_data.Get();
-  data.manifests[id] = linked_ptr<DictionaryValue>(parsed_manifest);
+  Whitelist& data = g_whitelisted_install_data.Get();
+  data.entries[id] = linked_ptr<CrxInstaller::WhitelistEntry>(entry);
 }
 
 // static
-const DictionaryValue* CrxInstaller::GetWhitelistedManifest(
+const CrxInstaller::WhitelistEntry* CrxInstaller::GetWhitelistEntry(
     const std::string& id) {
-  WhitelistedInstallData& data = g_whitelisted_install_data.Get();
-  if (ContainsKey(data.manifests, id))
-    return data.manifests[id].get();
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  Whitelist& data = g_whitelisted_install_data.Get();
+  if (ContainsKey(data.entries, id))
+    return data.entries[id].get();
   else
     return NULL;
 }
 
 // static
-DictionaryValue* CrxInstaller::RemoveWhitelistedManifest(
+CrxInstaller::WhitelistEntry* CrxInstaller::RemoveWhitelistEntry(
     const std::string& id) {
-  WhitelistedInstallData& data = g_whitelisted_install_data.Get();
-  if (ContainsKey(data.manifests, id)) {
-    DictionaryValue* manifest = data.manifests[id].release();
-    data.manifests.erase(id);
-    return manifest;
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  Whitelist& data = g_whitelisted_install_data.Get();
+  if (ContainsKey(data.entries, id)) {
+    CrxInstaller::WhitelistEntry* entry = data.entries[id].release();
+    data.entries.erase(id);
+    return entry;
   }
   return NULL;
 }
 
 // static
 bool CrxInstaller::IsIdWhitelisted(const std::string& id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   std::set<std::string>& ids = g_whitelisted_install_data.Get().ids;
   return ContainsKey(ids, id);
 }
 
 // static
 bool CrxInstaller::ClearWhitelistedInstallId(const std::string& id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   std::set<std::string>& ids = g_whitelisted_install_data.Get().ids;
   if (ContainsKey(ids, id)) {
     ids.erase(id);
     return true;
   }
   return false;
 }
 
 CrxInstaller::CrxInstaller(base::WeakPtr<ExtensionService> frontend_weak,
                            ExtensionInstallUI* client)
@@ skipping 67 matching lines @@
 
 void CrxInstaller::ConvertUserScriptOnFileThread() {
   std::string error;
   scoped_refptr<Extension> extension =
       ConvertUserScriptToExtension(source_file_, original_url_, &error);
   if (!extension) {
     ReportFailureFromFileThread(error);
     return;
   }
 
-  OnUnpackSuccess(extension->path(), extension->path(), extension);
+  OnUnpackSuccess(extension->path(), extension->path(), NULL, extension);
 }
 
 void CrxInstaller::InstallWebApp(const WebApplicationInfo& web_app) {
   if (!BrowserThread::PostTask(
           BrowserThread::FILE, FROM_HERE,
           NewRunnableMethod(this, &CrxInstaller::ConvertWebAppOnFileThread,
                             web_app)))
     NOTREACHED();
 }
 
 void CrxInstaller::ConvertWebAppOnFileThread(
     const WebApplicationInfo& web_app) {
   std::string error;
   scoped_refptr<Extension> extension(
       ConvertWebAppToExtension(web_app, base::Time::Now()));
   if (!extension) {
     // Validation should have stopped any potential errors before getting here.
     NOTREACHED() << "Could not convert web app to extension.";
     return;
   }
 
   // TODO(aa): conversion data gets lost here :(
 
-  OnUnpackSuccess(extension->path(), extension->path(), extension);
+  OnUnpackSuccess(extension->path(), extension->path(), NULL, extension);
 }
 
 bool CrxInstaller::AllowInstall(const Extension* extension,
                                 std::string* error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(error);
 
   // Make sure the expected id matches.
   if (!expected_id_.empty() && expected_id_ != extension->id()) {
     *error = base::StringPrintf(
@@ skipping 81 matching lines @@
 
   UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackFailureInstallCause",
                             install_cause(),
                             extension_misc::NUM_INSTALL_CAUSES);
 
   ReportFailureFromFileThread(error_message);
 }
 
 void CrxInstaller::OnUnpackSuccess(const FilePath& temp_dir,
                                    const FilePath& extension_dir,
+                                   const DictionaryValue* original_manifest,
                                    const Extension* extension) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackSuccessInstallSource",
                             install_source(), Extension::NUM_LOCATIONS);
 
 
   UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackSuccessInstallCause",
                             install_cause(),
                             extension_misc::NUM_INSTALL_CAUSES);
 
   // Note: We take ownership of |extension| and |temp_dir|.
   extension_ = extension;
   temp_dir_ = temp_dir;
 
+  original_manifest_.reset(original_manifest->DeepCopy());
+
   // We don't have to delete the unpack dir explicity since it is a child of
   // the temp dir.
   unpacked_extension_root_ = extension_dir;
 
   std::string error;
   if (!AllowInstall(extension, &error)) {
     ReportFailureFromFileThread(error);
     return;
   }
 
   if (client_) {
     Extension::DecodeIcon(extension_.get(), Extension::EXTENSION_ICON_LARGE,
                           &install_icon_);
   }
 
   if (!BrowserThread::PostTask(
           BrowserThread::UI, FROM_HERE,
           NewRunnableMethod(this, &CrxInstaller::ConfirmInstall)))
     NOTREACHED();
 }
 
+#if 0

[Matt Perry, 2011/05/25 01:46:06]

Remove dead code, unless you mean to use it. In the latter case, my personal
preference is the original implementation.

[asargent_no_longer_on_chrome, 2011/05/25 04:42:00]

Oops, I had meant to remove this whole block. Done.

 // Helper method to let us compare a whitelisted manifest with the actual
-// downloaded extension's manifest, but ignoring the kPublicKey since the
-// whitelisted manifest doesn't have that value.
-static bool EqualsIgnoringPublicKey(
+// downloaded extension's manifest, but ignoring certain keys.
+static bool WhitelistMatches(
     const DictionaryValue& extension_manifest,
     const DictionaryValue& whitelisted_manifest) {
   scoped_ptr<DictionaryValue> manifest_copy(extension_manifest.DeepCopy());
   manifest_copy->Remove(extension_manifest_keys::kPublicKey, NULL);
-  return manifest_copy->Equals(&whitelisted_manifest);
+  manifest_copy->Remove(extension_manifest_keys::kCurrentLocale, NULL);
+
+  for (DictionaryValue::key_iterator i = extension_manifest.begin_keys();
+       i != extension_manifest.end_keys();
+       ++i) {
+    const std::string& key = *i;
+    if (key == extension_manifest_keys::kPublicKey ||
+        key == extension_manifest_keys::kCurrentLocale)
+      continue;
+
+    Value* extension_value = NULL;
+    Value* whitelist_value = NULL;
+
+    if (!extension_manifest.Get(key, &extension_value) ||
+        !whitelisted_manifest.Get(key, &whitelist_value))
+      return false;
+
+    if (!extension_value->Equals(whitelist_value))
+      return false;
+  }
+  return true;
 }
+#endif
 
 void CrxInstaller::ConfirmInstall() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (!frontend_weak_.get())
     return;
 
   if (frontend_weak_->extension_prefs()
       ->IsExtensionBlacklisted(extension_->id())) {
     VLOG(1) << "This extension: " << extension_->id()
             << " is blacklisted. Install failed.";
@@ skipping 21 matching lines @@
     return;
   }
 
   current_version_ =
       frontend_weak_->extension_prefs()->GetVersionString(extension_->id());
 
   // First see if it's whitelisted by id (the old mechanism).
   bool whitelisted = ClearWhitelistedInstallId(extension_->id()) &&
       extension_->plugins().empty() && is_gallery_install_;
 
-  // Now check if it's whitelisted by manifest.
-  scoped_ptr<DictionaryValue> whitelisted_manifest(
-      RemoveWhitelistedManifest(extension_->id()));
-  if (is_gallery_install_ && whitelisted_manifest.get()) {
-    if (!EqualsIgnoringPublicKey(*extension_->manifest_value(),
-                                 *whitelisted_manifest)) {
+  // Now check if there's a WhitelistEntry.
+  scoped_ptr<CrxInstaller::WhitelistEntry> entry(
+      RemoveWhitelistEntry(extension_->id()));
+  if (is_gallery_install_ && entry.get() && original_manifest_.get()) {
+    if (!(original_manifest_->Equals(entry->parsed_manifest.get()))) {

[Matt Perry, 2011/05/25 01:46:06]

isn't the Extension::manifest_value supposed to be the original_manifest as
contained in the .crx? I don't see why we need to keep this separate copy.

[asargent_no_longer_on_chrome, 2011/05/25 04:42:00]

The Extension::manifest_value has already had a couple of transformations
applied to it: localization and insertion of the public key field. (See
SandboxedExtensionUnpacker::OnUnpackExtensionSucceeded).

What I was doing before, just comparing the extension's manifest_value but
ignoring the public key field, worked for non-localized extensions but would
cause comparison failures with any localized fields like name, description, etc.

[Matt Perry, 2011/05/25 19:03:24]

Should we also check to ensure that the localized names are the same?

       ReportFailureFromUIThread(
           l10n_util::GetStringUTF8(IDS_EXTENSION_MANIFEST_INVALID));
       return;
     }
     whitelisted = true;
   }
 
   if (client_ &&
       (!allow_silent_install_ || !whitelisted)) {
     AddRef();  // Balanced in Proceed() and Abort().
@@ skipping 130 matching lines @@
     client_->OnInstallSuccess(extension_.get(), install_icon_.get());
 
   // Tell the frontend about the installation and hand off ownership of
   // extension_ to it.
   frontend_weak_->OnExtensionInstalled(extension_);
   extension_ = NULL;
 
   // We're done. We don't post any more tasks to ourselves so we are deleted
   // soon.
 }