Implement gallery install API

Implement gallery install API

This patch implements chrome.experimental.management.install() which is only available for use by the web store. Calling with an extensionId, causes the download url to be constructed internally and downloaded and then cause the installation to bypass the normal permissions dialog.

BUG=27431, 54148
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58956

[rafaelw, 2010-09-07 21:57:04]

Pawel: please review changes to testserver.py. Note that in addition to adding
the crx mime-type, I made it so that mime-type mapping can ignore query-params.

Carl, please look at the construction of the download URL in
extension_management_api.cc (InstallFunction). I considered merging this code
with that in ExtensionUpdater, but it didn't seem common enough to be gaining
much.

Erik, please review the rest.

[Carl Nygaard, 2010-09-08 00:01:20]

LGTM

Nothing big.  The URL generation seems fine to me.  I'll leave it to Erik to
double check the rest.

http://codereview.chromium.org/3353015/diff/1/2
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/3353015/diff/1/2#newcode103
chrome/browser/extensions/crx_installer.cc:103:
FROM_HERE,NewRunnableFunction(CrxInstaller::ClearWhitelistedInstallId));
Want to add a new line before "NewRunnableFunction"?

http://codereview.chromium.org/3353015/diff/1/6
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/3353015/diff/1/6#newcode150
chrome/browser/extensions/extension_management_api.cc:150:
params.push_back("id=" + id);
Do we validate that the id looks "normal" (e.g. 26 char ascii)?  Just in case
the webstore falls victim to XSS, it would be good to avoid letting people try
to get creative with the exploit -- the worst that should happen is that a
publish item in the gallery is installed.

http://codereview.chromium.org/3353015/diff/1/6#newcode159
chrome/browser/extensions/extension_management_api.cc:159:
CrxInstaller::SetWhitelistedInstallId(id);
Just being paranoid, but this doesn't somehow bypass the blacklisting
functionality, does it?  (I assume that happens at some other layer).

[Erik does not do reviews, 2010-09-08 00:42:24]

http://codereview.chromium.org/3353015/diff/1/2
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/3353015/diff/1/2#newcode103
chrome/browser/extensions/crx_installer.cc:103:
FROM_HERE,NewRunnableFunction(CrxInstaller::ClearWhitelistedInstallId));
nit: space after comma

http://codereview.chromium.org/3353015/diff/1/3
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/3353015/diff/1/3#newcode53
chrome/browser/extensions/crx_installer.h:53: static void
SetWhitelistedInstallId(const std::string& id);
I think it's possible to have two installs queued up at once, so I don't think
static will work here.

http://codereview.chromium.org/3353015/diff/1/4
File chrome/browser/extensions/extension_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/1/4#newcode89
chrome/browser/extensions/extension_apitest.cc:89: // Load an extension and wait
for it to notify of PASSED or FAILED.
Since these variables can be called with interesting permutations (empty vs not,
fragment vs full URL), it seems like a comment is warranted explaining this. 
The existing comment is wrong when extension name is an empty string for
example.

http://codereview.chromium.org/3353015/diff/1/4#newcode95
chrome/browser/extensions/extension_apitest.cc:95: if
(!std::string(extension_name).empty() &&
Given that this can now be empty, please add a new variation of
RunExtensionSubtest (maybe RunComponentExtensionSubtest?) that only takes the
subtest_page and comment that as appropriate so that callers don't need to know
the internals of this.

http://codereview.chromium.org/3353015/diff/1/4#newcode102
chrome/browser/extensions/extension_apitest.cc:102: if (!subtest_page.empty()) {
maybe we now need to also verify that at least one of the two arguments has data
in it?

http://codereview.chromium.org/3353015/diff/1/4#newcode104
chrome/browser/extensions/extension_apitest.cc:104: if (!url.is_valid()) {
is_valid's purpose here is really checking to see if this is a fragment vs. a
fully-qualified URL, right?  I've always found is_valid a bit confusing in this
way.  Maybe a comment would help.

http://codereview.chromium.org/3353015/diff/1/5
File chrome/browser/extensions/extension_gallery_install_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/1/5#newcode28
chrome/browser/extensions/extension_gallery_install_apitest.cc:28:
"http://www.a.com");
in general, it's good to use example.com, example.net, etc. when writing test
code (even when we're not reaching out to the net for real).

http://codereview.chromium.org/3353015/diff/1/6
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/3353015/diff/1/6#newcode155
chrome/browser/extensions/extension_management_api.cc:155: GURL url(url_string +
"?response=redirect&x=" +
doesn't this append id= directly after the x=?  that seems strange.

http://codereview.chromium.org/3353015/diff/1/6#newcode161
chrome/browser/extensions/extension_management_api.cc:161: NavigationController&
controller =
Given that this bit is a bit weird, it's probably worth explaining with a
comment.

http://codereview.chromium.org/3353015/diff/1/7
File chrome/browser/extensions/extension_management_api.h (right):

http://codereview.chromium.org/3353015/diff/1/7#newcode35
chrome/browser/extensions/extension_management_api.h:35: static void
SetTestingInstallBaseUrl(const char* testing_install_base_url);
nit: newline before protected

http://codereview.chromium.org/3353015/diff/1/13
File chrome/test/data/extensions/api_test/extension_gallery_install/test.html
(right):

http://codereview.chromium.org/3353015/diff/1/13#newcode48
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:48:
chrome.experimental.management.uninstall(id);
check for error in callback

http://codereview.chromium.org/3353015/diff/1/13#newcode51
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:51:
chrome.experimental.management.install(id);
add callback here to assert if there's an error

[rafaelw, 2010-09-08 01:25:05]

http://codereview.chromium.org/3353015/diff/1/2
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/3353015/diff/1/2#newcode103
chrome/browser/extensions/crx_installer.cc:103:
FROM_HERE,NewRunnableFunction(CrxInstaller::ClearWhitelistedInstallId));
On 2010/09/08 00:01:20, Carl Nygaard wrote:
> Want to add a new line before "NewRunnableFunction"?

Done.

http://codereview.chromium.org/3353015/diff/1/2#newcode103
chrome/browser/extensions/crx_installer.cc:103:
FROM_HERE,NewRunnableFunction(CrxInstaller::ClearWhitelistedInstallId));
On 2010/09/08 00:42:24, Erik Kay wrote:
> nit: space after comma

Done.

http://codereview.chromium.org/3353015/diff/1/3
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/3353015/diff/1/3#newcode53
chrome/browser/extensions/crx_installer.h:53: static void
SetWhitelistedInstallId(const std::string& id);
Do you mean that a scalar won't work. I.e. you want a long-lived set of
whitelisted ids?

The downside is that you have to let that set just build up over time.

I had assumed that this degenerate case would be hard to come by in practice.
The failure mode is that only the last installed crx gets to bypass the
permissions dialog.

Do you feel like this case will be more common?

On 2010/09/08 00:42:24, Erik Kay wrote:
> I think it's possible to have two installs queued up at once, so I don't think
> static will work here.

http://codereview.chromium.org/3353015/diff/1/4
File chrome/browser/extensions/extension_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/1/4#newcode89
chrome/browser/extensions/extension_apitest.cc:89: // Load an extension and wait
for it to notify of PASSED or FAILED.
I changed the comment in the header. Missed this one. Done.

On 2010/09/08 00:42:24, Erik Kay wrote:
> Since these variables can be called with interesting permutations (empty vs
not,
> fragment vs full URL), it seems like a comment is warranted explaining this. 
> The existing comment is wrong when extension name is an empty string for
> example.

http://codereview.chromium.org/3353015/diff/1/4#newcode95
chrome/browser/extensions/extension_apitest.cc:95: if
(!std::string(extension_name).empty() &&
On 2010/09/08 00:42:24, Erik Kay wrote:
> Given that this can now be empty, please add a new variation of
> RunExtensionSubtest (maybe RunComponentExtensionSubtest?) that only takes the
> subtest_page and comment that as appropriate so that callers don't need to
know
> the internals of this.

Done.

http://codereview.chromium.org/3353015/diff/1/4#newcode102
chrome/browser/extensions/extension_apitest.cc:102: if (!subtest_page.empty()) {
On 2010/09/08 00:42:24, Erik Kay wrote:
> maybe we now need to also verify that at least one of the two arguments has
data
> in it?

Done.

http://codereview.chromium.org/3353015/diff/1/4#newcode104
chrome/browser/extensions/extension_apitest.cc:104: if (!url.is_valid()) {
On 2010/09/08 00:42:24, Erik Kay wrote:
> is_valid's purpose here is really checking to see if this is a fragment vs. a
> fully-qualified URL, right?  I've always found is_valid a bit confusing in
this
> way.  Maybe a comment would help.

Done.

http://codereview.chromium.org/3353015/diff/1/5
File chrome/browser/extensions/extension_gallery_install_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/1/5#newcode28
chrome/browser/extensions/extension_gallery_install_apitest.cc:28:
"http://www.a.com");
On 2010/09/08 00:42:24, Erik Kay wrote:
> in general, it's good to use http://example.com, http://example.net, etc. when
writing test
> code (even when we're not reaching out to the net for real).

Done.

http://codereview.chromium.org/3353015/diff/1/6
File chrome/browser/extensions/extension_management_api.cc (right):

http://codereview.chromium.org/3353015/diff/1/6#newcode150
chrome/browser/extensions/extension_management_api.cc:150:
params.push_back("id=" + id);
Done. Good idea.

On 2010/09/08 00:01:20, Carl Nygaard wrote:
> Do we validate that the id looks "normal" (e.g. 26 char ascii)?  Just in case
> the webstore falls victim to XSS, it would be good to avoid letting people try
> to get creative with the exploit -- the worst that should happen is that a
> publish item in the gallery is installed.

http://codereview.chromium.org/3353015/diff/1/6#newcode155
chrome/browser/extensions/extension_management_api.cc:155: GURL url(url_string +
"?response=redirect&x=" +
No, it's escaped. See ExtensionUpdater, line 71 for an explanation.

On 2010/09/08 00:42:24, Erik Kay wrote:
> doesn't this append id= directly after the x=?  that seems strange.

http://codereview.chromium.org/3353015/diff/1/6#newcode159
chrome/browser/extensions/extension_management_api.cc:159:
CrxInstaller::SetWhitelistedInstallId(id);
Yes, this is orthogonal. It will only result in an installable extension not
getting the permissions dialog. It won't bypass anything else.

On 2010/09/08 00:01:20, Carl Nygaard wrote:
> Just being paranoid, but this doesn't somehow bypass the blacklisting
> functionality, does it?  (I assume that happens at some other layer).

http://codereview.chromium.org/3353015/diff/1/6#newcode161
chrome/browser/extensions/extension_management_api.cc:161: NavigationController&
controller =
On 2010/09/08 00:42:24, Erik Kay wrote:
> Given that this bit is a bit weird, it's probably worth explaining with a
> comment.

Done.

http://codereview.chromium.org/3353015/diff/1/7
File chrome/browser/extensions/extension_management_api.h (right):

http://codereview.chromium.org/3353015/diff/1/7#newcode35
chrome/browser/extensions/extension_management_api.h:35: static void
SetTestingInstallBaseUrl(const char* testing_install_base_url);
On 2010/09/08 00:42:24, Erik Kay wrote:
> nit: newline before protected

Done.

http://codereview.chromium.org/3353015/diff/1/13
File chrome/test/data/extensions/api_test/extension_gallery_install/test.html
(right):

http://codereview.chromium.org/3353015/diff/1/13#newcode48
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:48:
chrome.experimental.management.uninstall(id);
On 2010/09/08 00:42:24, Erik Kay wrote:
> check for error in callback

Done.

http://codereview.chromium.org/3353015/diff/1/13#newcode51
chrome/test/data/extensions/api_test/extension_gallery_install/test.html:51:
chrome.experimental.management.install(id);
On 2010/09/08 00:42:24, Erik Kay wrote:
> add callback here to assert if there's an error

Done.

[Paweł Hajdan Jr., 2010-09-08 17:56:27]

http://codereview.chromium.org/3353015/diff/17001/18005
File chrome/browser/extensions/extension_gallery_install_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/17001/18005#newcode15
chrome/browser/extensions/extension_gallery_install_apitest.cc:15: CommandLine*
command_line) {
nit: Why weird indentation?

http://codereview.chromium.org/3353015/diff/17001/18005#newcode24
chrome/browser/extensions/extension_gallery_install_apitest.cc:24:
"http://www.example.com:1337/files/extensions/good.crx");
Do not hardcode the port number, get it from the test server.

http://codereview.chromium.org/3353015/diff/17001/18005#newcode25
chrome/browser/extensions/extension_gallery_install_apitest.cc:25:
host_resolver()->AddRule("www.example.com", "127.0.0.1");
Why not just use localhost instead of example.com?

http://codereview.chromium.org/3353015/diff/17001/18014
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/3353015/diff/17001/18014#newcode158
net/tools/testserver/testserver.py:158: 'crx' : 'application/x-chrome-extension'
nit: Please keep the alphabetical ordering.

http://codereview.chromium.org/3353015/diff/17001/18014#newcode184
net/tools/testserver/testserver.py:184: (shortname, extension) =
os.path.splitext(file_name.split("?")[0])
Why is this needed?

[rafaelw, 2010-09-08 19:37:42]

Addressed Pawel's comments.

Also moved install to webstorePrivate per offline discussion with erikkay & aa.

All: PTAL

http://codereview.chromium.org/3353015/diff/17001/18005
File chrome/browser/extensions/extension_gallery_install_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/17001/18005#newcode15
chrome/browser/extensions/extension_gallery_install_apitest.cc:15: CommandLine*
command_line) {
On 2010/09/08 17:56:27, Paweł Hajdan Jr. wrote:
> nit: Why weird indentation?

Done.

http://codereview.chromium.org/3353015/diff/17001/18005#newcode24
chrome/browser/extensions/extension_gallery_install_apitest.cc:24:
"http://www.example.com:1337/files/extensions/good.crx");
On 2010/09/08 17:56:27, Paweł Hajdan Jr. wrote:
> Do not hardcode the port number, get it from the test server.

Done.

http://codereview.chromium.org/3353015/diff/17001/18005#newcode25
chrome/browser/extensions/extension_gallery_install_apitest.cc:25:
host_resolver()->AddRule("www.example.com", "127.0.0.1");
Not done per offline chat.

On 2010/09/08 17:56:27, Paweł Hajdan Jr. wrote:
> Why not just use localhost instead of example.com?

http://codereview.chromium.org/3353015/diff/17001/18014
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/3353015/diff/17001/18014#newcode158
net/tools/testserver/testserver.py:158: 'crx' : 'application/x-chrome-extension'
On 2010/09/08 17:56:27, Paweł Hajdan Jr. wrote:
> nit: Please keep the alphabetical ordering.

Done.

http://codereview.chromium.org/3353015/diff/17001/18014#newcode184
net/tools/testserver/testserver.py:184: (shortname, extension) =
os.path.splitext(file_name.split("?")[0])
In this test, the url will contain query-params. This comes through to the
|file_name| variable.

On 2010/09/08 17:56:27, Paweł Hajdan Jr. wrote:
> Why is this needed?

[Paweł Hajdan Jr., 2010-09-08 20:03:21]

Test code LGTM.

[Erik does not do reviews, 2010-09-08 23:52:02]

http://codereview.chromium.org/3353015/diff/1/3
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/3353015/diff/1/3#newcode53
chrome/browser/extensions/crx_installer.h:53: static void
SetWhitelistedInstallId(const std::string& id);
I mean that you probably need a list and then remove the installed item from the
list.

The scenario I'm imagining is two 10MB installs on a slow link.  This will
become more of a problem when we allow larger downloads from the store.  I agree
that the failure mode isn't horrible, but it's still a problem.  It doesn't seem
too hard to fix this, but if you'd like to punt it to another CL, that's fine. 
If so, make sure you file a bug.

On 2010/09/08 01:25:06, rafaelw wrote:
> Do you mean that a scalar won't work. I.e. you want a long-lived set of
> whitelisted ids?
> 
> The downside is that you have to let that set just build up over time.
> 
> I had assumed that this degenerate case would be hard to come by in practice.
> The failure mode is that only the last installed crx gets to bypass the
> permissions dialog.
> 
> Do you feel like this case will be more common?
> 
> On 2010/09/08 00:42:24, Erik Kay wrote:
> > I think it's possible to have two installs queued up at once, so I don't
think
> > static will work here.
> 
>

http://codereview.chromium.org/3353015/diff/17001/18004
File chrome/browser/extensions/extension_apitest.h (right):

http://codereview.chromium.org/3353015/diff/17001/18004#newcode73
chrome/browser/extensions/extension_apitest.h:73: // |page_url| under the
extension, and wait for pass / fail notification.
"under the extension" isn't always accurate right?

http://codereview.chromium.org/3353015/diff/17001/18004#newcode78
chrome/browser/extensions/extension_apitest.h:78: // Load |page_url| and wait
for pass / fail notification.
...from extension API on the page.

http://codereview.chromium.org/3353015/diff/21001/22003
File chrome/browser/extensions/extension_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/21001/22003#newcode98
chrome/browser/extensions/extension_apitest.cc:98: LOG(INFO) << "Running
ExtensionApiTest with: " << extension_name;
update or remove this log

[rafaelw, 2010-09-09 01:39:49]

http://codereview.chromium.org/3353015/diff/1/3
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/3353015/diff/1/3#newcode53
chrome/browser/extensions/crx_installer.h:53: static void
SetWhitelistedInstallId(const std::string& id);
On 2010/09/08 23:52:02, Erik Kay wrote:
> I mean that you probably need a list and then remove the installed item from
the
> list.
> 
> The scenario I'm imagining is two 10MB installs on a slow link.  This will
> become more of a problem when we allow larger downloads from the store.  I
agree
> that the failure mode isn't horrible, but it's still a problem.  It doesn't
seem
> too hard to fix this, but if you'd like to punt it to another CL, that's fine.

> If so, make sure you file a bug.
> 
> On 2010/09/08 01:25:06, rafaelw wrote:
> > Do you mean that a scalar won't work. I.e. you want a long-lived set of
> > whitelisted ids?
> > 
> > The downside is that you have to let that set just build up over time.
> > 
> > I had assumed that this degenerate case would be hard to come by in
practice.
> > The failure mode is that only the last installed crx gets to bypass the
> > permissions dialog.
> > 
> > Do you feel like this case will be more common?
> > 
> > On 2010/09/08 00:42:24, Erik Kay wrote:
> > > I think it's possible to have two installs queued up at once, so I don't
> think
> > > static will work here.
> > 
> > 
> 
> 

Done.

http://codereview.chromium.org/3353015/diff/17001/18004
File chrome/browser/extensions/extension_apitest.h (right):

http://codereview.chromium.org/3353015/diff/17001/18004#newcode73
chrome/browser/extensions/extension_apitest.h:73: // |page_url| under the
extension, and wait for pass / fail notification.
On 2010/09/08 23:52:02, Erik Kay wrote:
> "under the extension" isn't always accurate right?

Done.

http://codereview.chromium.org/3353015/diff/17001/18004#newcode78
chrome/browser/extensions/extension_apitest.h:78: // Load |page_url| and wait
for pass / fail notification.
On 2010/09/08 23:52:02, Erik Kay wrote:
> ...from extension API on the page.

Done.

http://codereview.chromium.org/3353015/diff/21001/22003
File chrome/browser/extensions/extension_apitest.cc (right):

http://codereview.chromium.org/3353015/diff/21001/22003#newcode98
chrome/browser/extensions/extension_apitest.cc:98: LOG(INFO) << "Running
ExtensionApiTest with: " << extension_name;
On 2010/09/08 23:52:02, Erik Kay wrote:
> update or remove this log

Done.

[Erik does not do reviews, 2010-09-09 17:22:21]

LGTM - thanks!