Only allow installation of extensions/apps with gallery update url via download from gallery

Only allow installation of extensions/apps with gallery update url via download from gallery

BUG=45542
TEST=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50333

[rafaelw, 2010-06-17 18:06:35]

Here's the main chunk of this. Questions:

1) Do we want to either wait on this or have it disabled by default until we
ship to avoid annoyance for devs during development?

2) Failure results in the standard modal dialog box with non-i18n text. Do we
need slicker UI or localized text?

[Aaron Boodman, 2010-06-17 21:41:25]

On 2010/06/17 18:06:35, rafaelw wrote:
> Here's the main chunk of this. Questions:
> 
> 1) Do we want to either wait on this or have it disabled by default until we
> ship to avoid annoyance for devs during development?

It doesn't seem like we'd hit this very frequently during development. Am I
missing something?

> 2) Failure results in the standard modal dialog box with non-i18n text. Do we
> need slicker UI or localized text?

The crappy dialog is fine. Normal people shouldn't see this. It seems
responsible to localize the text though.

[rafaelw, 2010-06-18 01:23:26]

done.

[Aaron Boodman, 2010-06-18 01:36:13]

LGTM

http://codereview.chromium.org/2855009/diff/2002/4001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2855009/diff/2002/4001#newcode3376
chrome/app/generated_resources.grd:3376: <message
name="IDS_EXTENSION_DISALLOW_NON_DOWNLOADED_GALLERY_INSTALLS" desc="Error
displayed when an extension that has an update URL used by the gallery is
installed when not directly downloaded from the gallery.">
Nit: how about "This package can only be installed from the Google Web Store."
where "Google Web Store" is the same constant that is used in the launcher.

[rafaelw1, 2010-06-18 02:37:08]

On Thu, Jun 17, 2010 at 6:36 PM,  <aa@chromium.org> wrote:
> LGTM
>
>
> http://codereview.chromium.org/2855009/diff/2002/4001
> File chrome/app/generated_resources.grd (right):
>
> http://codereview.chromium.org/2855009/diff/2002/4001#newcode3376
> chrome/app/generated_resources.grd:3376: <message
> name="IDS_EXTENSION_DISALLOW_NON_DOWNLOADED_GALLERY_INSTALLS"
> desc="Error displayed when an extension that has an update URL used by
> the gallery is installed when not directly downloaded from the
> gallery.">
> Nit: how about "This package can only be installed from the Google Web
> Store." where "Google Web Store" is the same constant that is used in
> the launcher.

Sorry to be lame. I can't find the "Google Web Store" constant. Can
you point where it's used or defined?

>
> http://codereview.chromium.org/2855009/show
>

[Aaron Boodman, 2010-06-18 03:42:32]

On Thu, Jun 17, 2010 at 7:37 PM, Rafael Weinstein <rafaelw@google.com> wrote
> Sorry to be lame. I can't find the "Google Web Store" constant. Can
> you point where it's used or defined?

My fault for not being more specific. This constant isn't defined in
Chrome, it comes from a switch, here:

http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/com...

However, now that we've announced the web store, we can move it into
Chrome and no longer need the switch. Can you remove it and defined
the constant as a resource, so that it can be translated and reused in
other places?

Also the name is actually supposed to be "Chrome Web Store", sorry.

- a

[rafaelw, 2010-06-18 21:03:54]

done

[Aaron Boodman, 2010-06-18 21:10:38]

Sorry for all the nits.

http://codereview.chromium.org/2855009/diff/13001/14002
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/2855009/diff/13001/14002#newcode104
chrome/browser/dom_ui/app_launcher_handler.cc:104: std::string gallery_url =
extension_urls::kGalleryBrowsePrefix;
I think we use the gallery URL in a one or two other places. How about putting a
static helper function in extension.h that contains the logic here.

http://codereview.chromium.org/2855009/diff/13001/14003
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/2855009/diff/13001/14003#newcode143
chrome/browser/extensions/crx_installer.cc:143:
extension_urls::kGalleryDownloadPrefix, false)) {
Instead of having this separate kGalleryDownloadPrefix, it is safe (at least for
now) to just use the gallery URL. It is always a prefix of the detail pages.
Then you could use the helper function I proposed here.

[rafaelw, 2010-06-18 22:06:11]

no worries. done.

http://codereview.chromium.org/2855009/diff/13001/14002
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/2855009/diff/13001/14002#newcode104
chrome/browser/dom_ui/app_launcher_handler.cc:104: std::string gallery_url =
extension_urls::kGalleryBrowsePrefix;
On 2010/06/18 21:10:38, Aaron Boodman wrote:
> I think we use the gallery URL in a one or two other places. How about putting
a
> static helper function in extension.h that contains the logic here.

Done.

http://codereview.chromium.org/2855009/diff/13001/14003
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/2855009/diff/13001/14003#newcode143
chrome/browser/extensions/crx_installer.cc:143:
extension_urls::kGalleryDownloadPrefix, false)) {
This is a different url (downloads not browse).

On 2010/06/18 21:10:38, Aaron Boodman wrote:
> Instead of having this separate kGalleryDownloadPrefix, it is safe (at least
for
> now) to just use the gallery URL. It is always a prefix of the detail pages.
> Then you could use the helper function I proposed here.

[Aaron Boodman, 2010-06-18 22:14:48]

LGTM w/ final nit below.

http://codereview.chromium.org/2855009/diff/20001/9008
File chrome/common/chrome_switches.cc (left):

http://codereview.chromium.org/2855009/diff/20001/9008#oldcode1000
chrome/common/chrome_switches.cc:1000: // DO NOT ADD YOUR CRAP TO THE BOTTOM OF
THIS FILE.
lols. never seen this before.

http://codereview.chromium.org/2855009/diff/20001/9011
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/2855009/diff/20001/9011#newcode218
chrome/common/extensions/extension.h:218: static std::string
AppsGalleryBrowsePrefix();
Final nit: I think a better name would be ChromeStoreURL. That avoid confusion
w/ "extension" vs "app", and "prefix".

[rafaelw, 2010-06-18 22:22:56]

http://codereview.chromium.org/2855009/diff/20001/9011
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/2855009/diff/20001/9011#newcode218
chrome/common/extensions/extension.h:218: static std::string
AppsGalleryBrowsePrefix();
On 2010/06/18 22:14:49, Aaron Boodman wrote:
> Final nit: I think a better name would be ChromeStoreURL. That avoid confusion
> w/ "extension" vs "app", and "prefix".

Done.

[tkent, 2010-06-21 05:00:02]

rafaelw committed this as r50333.
I rolled it out by r50334 because of multiple test failures.  Please apply try
server.

XP Tests (dbg)(1) : unit_tests did not complete failed 6 Flakiness dashboard . [
stdio GetButtonLabel GetMessageText PreviewSummaryString DeleteURL
PreviewSummaryAndObfuscatedNumberStrings GetText ]
Interactive Tests (dbg) : interactive_ui_tests 3 flaky did not complete failed 7
Flakiness dashboard . [ stdio BackspaceInKeywordMode DesiredTLD
EnterToOpenHistoryPage EscapeToDefaultMatch MenuStaysVisibleAfterDelete
TestStarButtonAccObj FLAKY_Tab1Tab3Escape ]
Vista Tests (dbg)(1) : unit_tests did not complete failed 6 Flakiness dashboard
. [ stdio GetButtonLabel GetMessageText PreviewSummaryString DeleteURL
PreviewSummaryAndObfuscatedNumberStrings GetText ]

[Aaron Boodman, 2010-06-21 05:10:52]

Those failures are due to grd changes. It would have gone green on
next build. Rafael, see:

http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC...

for the recommended way to land a grd change. I used this last week
and it worked.

- a

On Sun, Jun 20, 2010 at 10:00 PM,  <tkent@chromium.org> wrote:
> rafaelw committed this as r50333.
> I rolled it out by r50334 because of multiple test failures.  Please apply
> try
> server.
>
> XP Tests (dbg)(1) : unit_tests did not complete failed 6 Flakiness dashboard
> . [
> stdio GetButtonLabel GetMessageText PreviewSummaryString DeleteURL
> PreviewSummaryAndObfuscatedNumberStrings GetText ]
> Interactive Tests (dbg) : interactive_ui_tests 3 flaky did not complete
> failed 7
> Flakiness dashboard . [ stdio BackspaceInKeywordMode DesiredTLD
> EnterToOpenHistoryPage EscapeToDefaultMatch MenuStaysVisibleAfterDelete
> TestStarButtonAccObj FLAKY_Tab1Tab3Escape ]
> Vista Tests (dbg)(1) : unit_tests did not complete failed 6 Flakiness
> dashboard
> . [ stdio GetButtonLabel GetMessageText PreviewSummaryString DeleteURL
> PreviewSummaryAndObfuscatedNumberStrings GetText ]
>
>
>
> http://codereview.chromium.org/2855009/show
>

[akalin, 2010-06-24 01:09:57]

drive-by (I ran into a crash caused by this)

http://codereview.chromium.org/2855009/diff/22001/23003
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/2855009/diff/22001/23003#newcode144
chrome/browser/extensions/crx_installer.cc:144:
ReportFailureFromUIThread(l10n_util::GetStringFUTF8(
This should be ReportFailureFromFileThread()

[akalin, 2010-06-24 01:23:03]

Also this breaks extensions sync (and maybe auto-updating?) because the
auto-updater has an http URL.  Here's what it looks like from OnUnpackSuccess:


(gdb) print original_url_.spec().c_str()$9 = 0x216c4ecc
"http://clients2.googleusercontent.com/crx/download/OgAAANyrWI_IYsSnRTDSS8cm-9ZtWsOx_YfZsG-5iwJoRBBTP2AFOhm_RdugiNFvvbk9-Lo38OCBN4j4RrC-HbV6y6sAxlKa5XYTMMeeMoEqVxPzuBBvrIz37gJF/extension_0_7_3.crx"
(gdb) print extension_urls::kGalleryDownloadPrefix$10 = 0x9a31b0c
"https://clients2.googleusercontent.com/crx/download"
(gdb)

[akalin, 2010-06-24 01:27:46]

Also worth noting that a similar check occurs in
ExtensionsService::IsDownloadFromGallery

On 2010/06/24 01:23:03, akalin wrote:
> Also this breaks extensions sync (and maybe auto-updating?) because the
> auto-updater has an http URL.  Here's what it looks like from OnUnpackSuccess:
> 
> 
> (gdb) print original_url_.spec().c_str()$9 = 0x216c4ecc
>
"http://clients2.googleusercontent.com/crx/download/OgAAANyrWI_IYsSnRTDSS8cm-9ZtWsOx_YfZsG-5iwJoRBBTP2AFOhm_RdugiNFvvbk9-Lo38OCBN4j4RrC-HbV6y6sAxlKa5XYTMMeeMoEqVxPzuBBvrIz37gJF/extension_0_7_3.crx"
> (gdb) print extension_urls::kGalleryDownloadPrefix$10 = 0x9a31b0c
> "https://clients2.googleusercontent.com/crx/download"
> (gdb)

[rafaelw, 2010-06-24 02:27:50]

http://codereview.chromium.org/2855009/diff/22001/23003
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/2855009/diff/22001/23003#newcode144
chrome/browser/extensions/crx_installer.cc:144:
ReportFailureFromUIThread(l10n_util::GetStringFUTF8(
Thanks for the catch. Done here: http://codereview.chromium.org/2867023

On 2010/06/24 01:09:58, akalin wrote:
> This should be ReportFailureFromFileThread()