Change the web store private install API to accept a localized extension name.
In our initial design of the beginInstallWithManifest function, we forgot that
some extensions have a name which needs token substitution for i18n/l10n. This
change renames beginInstallWithManifest to beginInstallWithManifest2, and
modifies the signature to take optional localizedName and locale parameters.
I also refactored how we compare the passed in manifest used in the
pre-download confirmation dialog - we now keep a copy of the source from the
.crx file to compare against, since i18n substututions can happen on the
manifest we use to construct the Extension object.
BUG=75821
TEST=Covered by browser tests for now; will require web store server-side
changes before it can be manually tested.
Review URL: http://codereview.chromium.org/6992047
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@86780 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index 8f13497c0..d289c1c 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -41,63 +41,65 @@
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);
@@ -185,7 +187,7 @@
return;
}
- OnUnpackSuccess(extension->path(), extension->path(), extension);
+ OnUnpackSuccess(extension->path(), extension->path(), NULL, extension);
}
void CrxInstaller::InstallWebApp(const WebApplicationInfo& web_app) {
@@ -209,7 +211,7 @@
// 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,
@@ -311,6 +313,7 @@
void CrxInstaller::OnUnpackSuccess(const FilePath& temp_dir,
const FilePath& extension_dir,
+ const DictionaryValue* original_manifest,
const Extension* extension) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -326,6 +329,9 @@
extension_ = extension;
temp_dir_ = temp_dir;
+ if (original_manifest)
+ 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;
@@ -347,17 +353,6 @@
NOTREACHED();
}
-// 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(
- 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);
-}
-
void CrxInstaller::ConfirmInstall() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
if (!frontend_weak_.get())
@@ -398,12 +393,11 @@
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()))) {
ReportFailureFromUIThread(
l10n_util::GetStringUTF8(IDS_EXTENSION_MANIFEST_INVALID));
return;
diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h
index 0cc9dc7..937fa4f 100644
--- a/chrome/browser/extensions/crx_installer.h
+++ b/chrome/browser/extensions/crx_installer.h
@@ -55,21 +55,25 @@
// crbug.com/54916
static void SetWhitelistedInstallId(const std::string& id);
+ struct WhitelistEntry {
+ scoped_ptr<DictionaryValue> parsed_manifest;
+ std::string localized_name;
+ };
+
// Exempt the next extension install with |id| from displaying a confirmation
// prompt, since the user already agreed to the install via
// beginInstallWithManifest. We require that the extension manifest matches
- // |parsed_manifest| which is what was used to prompt with. Ownership of
- // |parsed_manifest| is transferred here.
- static void SetWhitelistedManifest(const std::string& id,
- DictionaryValue* parsed_manifest);
+ // the manifest in |entry|, which is what was used to prompt with. Ownership
+ // of |entry| is transferred here.
+ static void SetWhitelistEntry(const std::string& id, WhitelistEntry* entry);
- // Returns the previously stored manifest from a call to
- // SetWhitelistedManifest.
- static const DictionaryValue* GetWhitelistedManifest(const std::string& id);
+ // Returns the previously stored manifest from a call to SetWhitelistEntry.
+ static const CrxInstaller::WhitelistEntry* GetWhitelistEntry(
+ const std::string& id);
- // Removes any whitelisted manifest for |id| and returns it. The caller owns
+ // Removes any whitelist data for |id| and returns it. The caller owns
// the return value and is responsible for deleting it.
- static DictionaryValue* RemoveWhitelistedManifest(const std::string& id);
+ static WhitelistEntry* RemoveWhitelistEntry(const std::string& id);
// Returns whether |id| is whitelisted - only call this on the UI thread.
static bool IsIdWhitelisted(const std::string& id);
@@ -159,6 +163,7 @@
virtual void OnUnpackFailure(const std::string& error_message);
virtual void OnUnpackSuccess(const FilePath& temp_dir,
const FilePath& extension_dir,
+ const DictionaryValue* original_manifest,
const Extension* extension);
// Returns true if we can skip confirmation because the install was
@@ -225,6 +230,10 @@
// ExtensionService on success, or delete it on failure.
scoped_refptr<const Extension> extension_;
+ // A parsed copy of the unmodified original manifest, before any
+ // transformations like localization have taken place.
+ scoped_ptr<DictionaryValue> original_manifest_;
+
// If non-empty, contains the current version of the extension we're
// installing (for upgrades).
std::string current_version_;
diff --git a/chrome/browser/extensions/extension_webstore_private_api.cc b/chrome/browser/extensions/extension_webstore_private_api.cc
index 1dead6c..a38d867e 100644
--- a/chrome/browser/extensions/extension_webstore_private_api.cc
+++ b/chrome/browser/extensions/extension_webstore_private_api.cc
@@ -21,6 +21,7 @@
#include "chrome/common/chrome_switches.h"
#include "chrome/common/extensions/extension_constants.h"
#include "chrome/common/extensions/extension_error_utils.h"
+#include "chrome/common/extensions/extension_l10n_util.h"
#include "chrome/common/net/gaia/gaia_constants.h"
#include "content/browser/tab_contents/tab_contents.h"
#include "content/common/notification_details.h"
@@ -33,8 +34,13 @@
namespace {
+const char kIconDataKey[] = "iconData";
+const char kIdKey[] = "id";
+const char kLocalizedNameKey[] = "localizedName";
const char kLoginKey[] = "login";
+const char kManifestKey[] = "manifest";
const char kTokenKey[] = "token";
+
const char kImageDecodeError[] = "Image decode failed";
const char kInvalidIdError[] = "Invalid id";
const char kInvalidManifestError[] = "Invalid manifest";
@@ -283,15 +289,25 @@
return false;
}
- EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &id_));
+ DictionaryValue* details = NULL;
+ EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
+ CHECK(details);
+
+ EXTENSION_FUNCTION_VALIDATE(details->GetString(kIdKey, &id_));
if (!Extension::IdIsValid(id_)) {
SetResult(INVALID_ID);
error_ = kInvalidIdError;
return false;
}
- EXTENSION_FUNCTION_VALIDATE(args_->GetString(1, &icon_data_));
- EXTENSION_FUNCTION_VALIDATE(args_->GetString(2, &manifest_));
+ EXTENSION_FUNCTION_VALIDATE(details->GetString(kManifestKey, &manifest_));
+
+ if (details->HasKey(kIconDataKey))
+ EXTENSION_FUNCTION_VALIDATE(details->GetString(kIconDataKey, &icon_data_));
+
+ if (details->HasKey(kLocalizedNameKey))
+ EXTENSION_FUNCTION_VALIDATE(details->GetString(kLocalizedNameKey,
+ &localized_name_));
scoped_refptr<SafeBeginInstallHelper> helper =
new SafeBeginInstallHelper(this, icon_data_, manifest_);
@@ -354,19 +370,29 @@
icon_ = icon;
parsed_manifest_.reset(parsed_manifest);
+ // If we were passed a localized name to use in the dialog, create a copy
+ // of the original manifest and replace the name in it.
+ scoped_ptr<DictionaryValue> localized_manifest;
+ if (!localized_name_.empty()) {
+ localized_manifest.reset(parsed_manifest->DeepCopy());
+ localized_manifest->SetString(extension_manifest_keys::kName,
+ localized_name_);
+ }
+
// Create a dummy extension and show the extension install confirmation
// dialog.
std::string init_errors;
dummy_extension_ = Extension::Create(
FilePath(),
Extension::INTERNAL,
- *static_cast<DictionaryValue*>(parsed_manifest_.get()),
+ localized_manifest.get() ? *localized_manifest.get() : *parsed_manifest,
Extension::NO_FLAGS,
&init_errors);
if (!dummy_extension_.get()) {
OnParseFailure(MANIFEST_ERROR, std::string(kInvalidManifestError));
return;
}
+
if (icon_.empty())
icon_ = Extension::GetDefaultIcon(dummy_extension_->is_app());
@@ -401,7 +427,10 @@
}
void BeginInstallWithManifestFunction::InstallUIProceed() {
- CrxInstaller::SetWhitelistedManifest(id_, parsed_manifest_.release());
+ CrxInstaller::WhitelistEntry* entry = new CrxInstaller::WhitelistEntry;
+ entry->parsed_manifest.reset(parsed_manifest_.release());
+ entry->localized_name = localized_name_;
+ CrxInstaller::SetWhitelistEntry(id_, entry);
SetResult(ERROR_NONE);
SendResponse(true);
@@ -430,7 +459,7 @@
}
if (!CrxInstaller::IsIdWhitelisted(id) &&
- !CrxInstaller::GetWhitelistedManifest(id)) {
+ !CrxInstaller::GetWhitelistEntry(id)) {
error_ = ExtensionErrorUtils::FormatErrorMessage(
kNoPreviousBeginInstallError, id);
return false;
diff --git a/chrome/browser/extensions/extension_webstore_private_api.h b/chrome/browser/extensions/extension_webstore_private_api.h
index 4acc7f6..3c909e1 100644
--- a/chrome/browser/extensions/extension_webstore_private_api.h
+++ b/chrome/browser/extensions/extension_webstore_private_api.h
@@ -106,6 +106,7 @@
std::string id_;
std::string manifest_;
std::string icon_data_;
+ std::string localized_name_;
// The results of parsing manifest_ and icon_data_ go into these two.
scoped_ptr<DictionaryValue> parsed_manifest_;
@@ -114,7 +115,7 @@
// A dummy Extension object we create for the purposes of using
// ExtensionInstallUI to prompt for confirmation of the install.
scoped_refptr<Extension> dummy_extension_;
- DECLARE_EXTENSION_FUNCTION_NAME("webstorePrivate.beginInstallWithManifest");
+ DECLARE_EXTENSION_FUNCTION_NAME("webstorePrivate.beginInstallWithManifest2");
};
class CompleteInstallFunction : public SyncExtensionFunction {
diff --git a/chrome/browser/extensions/extension_webstore_private_apitest.cc b/chrome/browser/extensions/extension_webstore_private_apitest.cc
index cdd465e..2638beb 100644
--- a/chrome/browser/extensions/extension_webstore_private_apitest.cc
+++ b/chrome/browser/extensions/extension_webstore_private_apitest.cc
@@ -51,9 +51,10 @@
return url.ReplaceComponents(replace_host);
}
- // Navigates to |page| and runs the Extension API test there.
- bool RunInstallTest(const std::string& page) {
- GURL crx_url = GetTestServerURL("extension.crx");
+ // Navigates to |page| and runs the Extension API test there. Any downloads
+ // of extensions will return the contents of |crx_file|.
+ bool RunInstallTest(const std::string& page, const std::string& crx_file) {
+ GURL crx_url = GetTestServerURL(crx_file);
CommandLine::ForCurrentProcess()->AppendSwitchASCII(
switches::kAppsGalleryUpdateURL, crx_url.spec());
@@ -69,18 +70,23 @@
// TODO(asargent) - flaky; see crbug.com/80606.
// Test cases where the user accepts the install confirmation dialog.
IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiTest, FLAKY_InstallAccepted) {
- ASSERT_TRUE(RunInstallTest("accepted.html"));
+ ASSERT_TRUE(RunInstallTest("accepted.html", "extension.crx"));
+}
+
+// Tests passing a localized name.
+IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiTest, InstallLocalized) {
+ ASSERT_TRUE(RunInstallTest("localized.html", "localized_extension.crx"));
}
// Now test the case where the user cancels the confirmation dialog.
IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiTest, InstallCancelled) {
BeginInstallWithManifestFunction::SetAutoConfirmForTests(false);
- ASSERT_TRUE(RunInstallTest("cancelled.html"));
+ ASSERT_TRUE(RunInstallTest("cancelled.html", "extension.crx"));
}
IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiTest, InstallNoGesture) {
BeginInstallFunction::SetIgnoreUserGestureForTests(false);
- ASSERT_TRUE(RunInstallTest("no_user_gesture.html"));
+ ASSERT_TRUE(RunInstallTest("no_user_gesture.html", "extension.crx"));
}
IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiTest,
@@ -88,7 +94,7 @@
ui_test_utils::WindowedNotificationObserver observer(
NotificationType::EXTENSION_INSTALL_ERROR,
NotificationService::AllSources());
- ASSERT_TRUE(RunInstallTest("incorrect_manifest1.html"));
+ ASSERT_TRUE(RunInstallTest("incorrect_manifest1.html", "extension.crx"));
observer.Wait();
}
@@ -97,6 +103,6 @@
ui_test_utils::WindowedNotificationObserver observer(
NotificationType::EXTENSION_INSTALL_ERROR,
NotificationService::AllSources());
- ASSERT_TRUE(RunInstallTest("incorrect_manifest2.html"));
+ ASSERT_TRUE(RunInstallTest("incorrect_manifest2.html", "extension.crx"));
observer.Wait();
}
diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.cc b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
index 8e31ddf..b8eb88c 100644
--- a/chrome/browser/extensions/sandboxed_extension_unpacker.cc
+++ b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
@@ -284,7 +284,7 @@
if (!RewriteCatalogFiles())
return;
- ReportSuccess();
+ ReportSuccess(manifest);
}
void SandboxedExtensionUnpacker::OnUnpackExtensionFailed(
@@ -480,14 +480,18 @@
client_->OnUnpackFailure(error);
}
-void SandboxedExtensionUnpacker::ReportSuccess() {
+void SandboxedExtensionUnpacker::ReportSuccess(
+ const DictionaryValue& original_manifest) {
UMA_HISTOGRAM_COUNTS("Extensions.SandboxUnpackSuccess", 1);
RecordSuccessfulUnpackTimeHistograms(
crx_path_, base::TimeTicks::Now() - unpack_start_time_);
// Client takes ownership of temporary directory and extension.
- client_->OnUnpackSuccess(temp_dir_.Take(), extension_root_, extension_);
+ client_->OnUnpackSuccess(temp_dir_.Take(),
+ extension_root_,
+ &original_manifest,
+ extension_);
extension_ = NULL;
}
diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.h b/chrome/browser/extensions/sandboxed_extension_unpacker.h
index 7294b39..9c10886 100644
--- a/chrome/browser/extensions/sandboxed_extension_unpacker.h
+++ b/chrome/browser/extensions/sandboxed_extension_unpacker.h
@@ -25,10 +25,14 @@
//
// extension_root - The path to the extension root inside of temp_dir.
//
+ // original_manifest - The parsed but unmodified version of the manifest,
+ // with no modifications such as localization, etc.
+ //
// extension - The extension that was unpacked. The client is responsible
// for deleting this memory.
virtual void OnUnpackSuccess(const FilePath& temp_dir,
const FilePath& extension_root,
+ const DictionaryValue* original_manifest,
const Extension* extension) = 0;
virtual void OnUnpackFailure(const std::string& error) = 0;
@@ -189,7 +193,7 @@
virtual void OnProcessCrashed(int exit_code);
void ReportFailure(FailureReason reason, const std::string& message);
- void ReportSuccess();
+ void ReportSuccess(const DictionaryValue& original_manifest);
// Overwrites original manifest with safe result from utility process.
// Returns NULL on error. Caller owns the returned object.
diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc b/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
index bc31c45..165f571 100644
--- a/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
+++ b/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
@@ -27,6 +27,7 @@
void OnUnpackSuccess(const FilePath& temp_dir,
const FilePath& extension_root,
+ const DictionaryValue* original_manifest,
const Extension* extension) {
// Don't delete temp_dir here, we need to do some post op checking.
}
@@ -38,16 +39,17 @@
public:
virtual ~MockSandboxedExtensionUnpackerClient() {}
- MOCK_METHOD3(OnUnpackSuccess,
+ MOCK_METHOD4(OnUnpackSuccess,
void(const FilePath& temp_dir,
const FilePath& extension_root,
+ const DictionaryValue* original_manifest,
const Extension* extension));
MOCK_METHOD1(OnUnpackFailure,
void(const std::string& error));
void DelegateToFake() {
- ON_CALL(*this, OnUnpackSuccess(_, _, _))
+ ON_CALL(*this, OnUnpackSuccess(_, _, _, _))
.WillByDefault(Invoke(::OnUnpackSuccess));
}
};
@@ -157,7 +159,7 @@
};
TEST_F(SandboxedExtensionUnpackerTest, NoCatalogsSuccess) {
- EXPECT_CALL(*client_, OnUnpackSuccess(_, _, _));
+ EXPECT_CALL(*client_, OnUnpackSuccess(_, _, _, _));
EXPECT_CALL(*client_, OnUnpackFailure(_)).Times(0);
SetupUnpacker("no_l10n.crx");
@@ -179,7 +181,7 @@
}
TEST_F(SandboxedExtensionUnpackerTest, WithCatalogsSuccess) {
- EXPECT_CALL(*client_, OnUnpackSuccess(_, _, _));
+ EXPECT_CALL(*client_, OnUnpackSuccess(_, _, _, _));
EXPECT_CALL(*client_, OnUnpackFailure(_)).Times(0);
SetupUnpacker("good_l10n.crx");
