OAuth2 support for Webstore downloads.

OAuth2 support for Webstore downloads.

This patch does three related things:

1. Plumbs an optional IdentityProvider through 
ExtensionUpdater / ExtensionDownloader.

2. Adds support in ExtensionDownloader for OAuth2 token-
based authentication to protected Webstore download URLs,
using the optional IdentityProvider as its account ID and 
token source. Bonus points for general cleanup of the 
credential iteration code in ExtensionDownloader.

3. Wires up Chrome's ExtensionDownloader (via
ExtensionService) with a ProfileIdentityProvider so that
it may authenticate as the Chrome-signed-in user via OAuth2.

BUG=376553, 122763
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288261

[Ken Rockot(use gerrit already), 2014-08-05 19:38:00]

Antony, could you please take a look?

[asargent_no_longer_on_chrome, 2014-08-06 00:26:29]

https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:201: }
I think this function pushes ExtensionFetch over the line of "passive carrier of
data" vs "object with  behavior", so it's appropriate to switch it from a struct
to a class. 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...

"Methods should not provide behavior but should only be used to set up the data
members"

https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:757: bool is_https =
fetch->url.SchemeIs("https");
can you switch this to url::kHttpsScheme while you're in here?

(I suppose we could also use SchemeIsSecure() instead of SchemeIs())

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS
File extensions/browser/DEPS (right):

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS...
extensions/browser/DEPS:5: "+google_apis/gaia",
are you sure we want to take a dependency on this, instead of defining some
abstract interface that gets wired up inside chrome/browser/extensions/ ?

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/webs...
File extensions/browser/webstore_oauth2_token_provider.cc (right):

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/webs...
extensions/browser/webstore_oauth2_token_provider.cc:30:
scopes.insert(kChromeWebstoreOAuth2Scope);
nit: can the above 2 lines be pushed inside the body of the if statement?

[Ken Rockot(use gerrit already), 2014-08-06 16:49:56]

+rogerta for google_apis/gaia dependency; please see inline discussion as well

https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:201: }
On 2014/08/06 00:26:29, Antony Sargent wrote:
> I think this function pushes ExtensionFetch over the line of "passive carrier
of
> data" vs "object with  behavior", so it's appropriate to switch it from a
struct
> to a class. 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...
> 
> "Methods should not provide behavior but should only be used to set up the
data
> members"

Actually, I feel like the requirement for this specific behavior is set by
ExtensionDownloader anyway and ExtensionFetch can be left as a struct, so I've
moved the implementation into ExtensionDownloader.

https://codereview.chromium.org/434493002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:757: bool is_https =
fetch->url.SchemeIs("https");
On 2014/08/06 00:26:29, Antony Sargent wrote:
> can you switch this to url::kHttpsScheme while you're in here?
> 
> (I suppose we could also use SchemeIsSecure() instead of SchemeIs())

Done.

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS
File extensions/browser/DEPS (right):

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS...
extensions/browser/DEPS:5: "+google_apis/gaia",
On 2014/08/06 00:26:29, Antony Sargent wrote:
> are you sure we want to take a dependency on this, instead of defining some
> abstract interface that gets wired up inside chrome/browser/extensions/ ? 

Even if I abstract away this dependency now, a time will soon come when we want
device identity support in app_shell, and that's going to end up depending on
gaia as well. So there's going to be a gaia-dependent component shared between
//extensions/shell and //chrome. It could live in its own //components
component, or maybe in a new //google_apis/webstore, but it will still depend on
gaia.

Alternatively, I don't see why OAuth2TokenService and/or IdentityProvider
themselves couldn't be abstracted away from google_apis into something more
generic like //components/oauth2. Then I would feel less bad about duplicating
specific google_apis dependencies within each extensions embedder rather than in
general-purpose extensions code.

+rogerta as OWNER of //google_apis/gaia - Any thoughts on this?

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/webs...
File extensions/browser/webstore_oauth2_token_provider.cc (right):

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/webs...
extensions/browser/webstore_oauth2_token_provider.cc:30:
scopes.insert(kChromeWebstoreOAuth2Scope);
On 2014/08/06 00:26:29, Antony Sargent wrote:
> nit: can the above 2 lines be pushed inside the body of the if statement?

most definitely. done

[asargent_no_longer_on_chrome, 2014-08-06 19:38:48]

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS
File extensions/browser/DEPS (right):

https://codereview.chromium.org/434493002/diff/180001/extensions/browser/DEPS...
extensions/browser/DEPS:5: "+google_apis/gaia",
On 2014/08/06 16:49:55, Ken Rockot wrote:
> On 2014/08/06 00:26:29, Antony Sargent wrote:
> > are you sure we want to take a dependency on this, instead of defining some
> > abstract interface that gets wired up inside chrome/browser/extensions/ ? 
> 
> Even if I abstract away this dependency now, a time will soon come when we
want
> device identity support in app_shell, and that's going to end up depending on
> gaia as well. So there's going to be a gaia-dependent component shared between
> //extensions/shell and //chrome. It could live in its own //components
> component, or maybe in a new //google_apis/webstore, but it will still depend
on
> gaia.
> 
> Alternatively, I don't see why OAuth2TokenService and/or IdentityProvider
> themselves couldn't be abstracted away from google_apis into something more
> generic like //components/oauth2. Then I would feel less bad about duplicating
> specific google_apis dependencies within each extensions embedder rather than
in
> general-purpose extensions code.
> 
> +rogerta as OWNER of //google_apis/gaia - Any thoughts on this?

BTW I'm not strongly opposed to the way you have it or anything - I just wanted
to make sure we thought it through before moving forward.

[Ken Rockot(use gerrit already), 2014-08-06 20:06:54]

> BTW I'm not strongly opposed to the way you have it or anything - I just
wanted
> to make sure we thought it through before moving forward.

Gotcha. But since you brought it up, I want to make sure I'm not missing a good
opportunity to refactor some low-hanging fruit!

[Ken Rockot(use gerrit already), 2014-08-06 20:19:43]

On 2014/08/06 20:06:54, Ken Rockot wrote:
> > BTW I'm not strongly opposed to the way you have it or anything - I just
> wanted
> > to make sure we thought it through before moving forward.
> 
> Gotcha. But since you brought it up, I want to make sure I'm not missing a
good
> opportunity to refactor some low-hanging fruit!

Upon second glance the fruit does not hang so low. There are some gaia details
(URLs and error codes) that would need to be abstracted if the core
oauth2/identity code were to move, and I'm not aware of any compelling reason to
do this yet.

So Roger, I think for now I'm only asking for approval to introduce
//google_apis/gaia into //extensions/browser/DEPS.

[Roger Tawa OOO till Jul 10th, 2014-08-07 01:35:16]

Hi guys,

I don't see a problem introducing a dep on google_apis/gaia.  See other comments
below.

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:870:
base::StringPrintf("%s:Bearer %s",
nit: space between : and Bearer

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.h (right):

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.h:220: // true| if the
fetch should be retried. Returns |false| if the failure was
true|  --> |true|

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.h:227: void
OnWebstoreOAuth2TokenReceived(bool success, const std::string& token);
Nit: rename |token| to |access_token|, so its not confused with refresh tokens
used by oauth2.

https://codereview.chromium.org/434493002/diff/220001/extensions/browser/webs...
File extensions/browser/webstore_oauth2_token_provider.cc (right):

https://codereview.chromium.org/434493002/diff/220001/extensions/browser/webs...
extensions/browser/webstore_oauth2_token_provider.cc:39: callback.Run(true,
access_token_);
There is no need to optimize the number of calls to StartRequest(), as that is
done already inside O2TS.  That is, multiple requests for the same
account_id/scopes will result in only one request to the server and the
resulting access token is cached.

It would be simpler to just have an |access_token| member in
ExtensionDownloader.  Fetch the token once and then reuse it with each fetcher. 
I'm not sure you need WebstoreOAuth2TokenProvider at all.

When fetching an extension fails due to an auth error, you should call
O2TS::InvalidateToken() and clear the |access_token| member.  Then request a new
access token right away or only when you need it next.  I suspect this should be
done in ExtensionDownloader::OnCRXFetchComplete() but don't know the code well
enough to say.  See web history code for an example:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...

[Ken Rockot(use gerrit already), 2014-08-07 16:49:09]

Thanks!

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:870:
base::StringPrintf("%s:Bearer %s",
On 2014/08/07 01:35:15, Roger Tawa (build sheriff) wrote:
> nit: space between : and Bearer

Done.

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.h (right):

https://codereview.chromium.org/434493002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.h:220: // true| if the
fetch should be retried. Returns |false| if the failure was
On 2014/08/07 01:35:15, Roger Tawa (build sheriff) wrote:
> true|  --> |true|

Done.

https://codereview.chromium.org/434493002/diff/220001/extensions/browser/webs...
File extensions/browser/webstore_oauth2_token_provider.cc (right):

https://codereview.chromium.org/434493002/diff/220001/extensions/browser/webs...
extensions/browser/webstore_oauth2_token_provider.cc:39: callback.Run(true,
access_token_);
On 2014/08/07 01:35:16, Roger Tawa (build sheriff) wrote:
> There is no need to optimize the number of calls to StartRequest(), as that is
> done already inside O2TS.  That is, multiple requests for the same
> account_id/scopes will result in only one request to the server and the
> resulting access token is cached.
> 
> It would be simpler to just have an |access_token| member in
> ExtensionDownloader.  Fetch the token once and then reuse it with each
fetcher. 
> I'm not sure you need WebstoreOAuth2TokenProvider at all.
> 
> When fetching an extension fails due to an auth error, you should call
> O2TS::InvalidateToken() and clear the |access_token| member.  Then request a
new
> access token right away or only when you need it next.  I suspect this should
be
> done in ExtensionDownloader::OnCRXFetchComplete() but don't know the code well
> enough to say.  See web history code for an example:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...

Aye, this is (sort of) what I was originally doing, but thought it might be
cleaner to separate the O2TS::Consumer implementation from ExtensionDownloader.
In retrospect this did end up being more complex than necessary.

WebstoreOauth2TokenProvider is gone now.

[asargent_no_longer_on_chrome, 2014-08-07 17:09:12]

lgtm w/ a few small suggestions

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:753: this);
It might be helpful to future readers to add a comment here noting that we'll be
called back via OnGetTokenSuccess/OnGetTokenFailure, and control flow will
contine from there.

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:759:
access_token_.c_str()));
Would it be worth adding a kAuthorizationBearer constant as well to
net::HttpRequestHeaders ?

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:876:
access_token_.clear();
Can a misbehaving server cause us to fall into an infinite loop of requests if
it keeps responding with HTTP_UNAUTHORIZED? If so maybe we should remember the
fact that we're doing a retry for this case and give up after some reasonable
number of attempts.

[Ken Rockot(use gerrit already), 2014-08-07 19:50:48]

Many thanks. Roger, could you please take another look?

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:753: this);
On 2014/08/07 17:09:12, Antony Sargent wrote:
> It might be helpful to future readers to add a comment here noting that we'll
be
> called back via OnGetTokenSuccess/OnGetTokenFailure, and control flow will
> contine from there.  
> 

Added some clarity to the existing comment

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:759:
access_token_.c_str()));
On 2014/08/07 17:09:12, Antony Sargent wrote:
> Would it be worth adding a kAuthorizationBearer constant as well to
> net::HttpRequestHeaders ? 
> 

Bearer tokens are (unlike the Authorization header itself) an OAuth2 concept
rather than an HTTP concept.

It might be justifiable to add a constant somewhere in the oauth support code,
or even a utility function that takes a bearer token string and generates a
well-formed Authorization header.

Opting to leave as-is for now though.

https://codereview.chromium.org/434493002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_downloader.cc:876:
access_token_.clear();
On 2014/08/07 17:09:12, Antony Sargent wrote:
> Can a misbehaving server cause us to fall into an infinite loop of requests if
> it keeps responding with HTTP_UNAUTHORIZED? If so maybe we should remember the
> fact that we're doing a retry for this case and give up after some reasonable
> number of attempts. 
> 

Ah yes. Good catch. Added an attempt counter to the fetch.

[Roger Tawa OOO till Jul 10th, 2014-08-07 20:09:32]

lgtm

Thanks Ken.

[Ken Rockot(use gerrit already), 2014-08-07 20:15:55]

James, could you please take a look for //e/b/DEPS and adding myself as an OWNER
of the same?

[Ken Rockot(use gerrit already), 2014-08-07 20:16:48]

Oops. Nevermind James. No changes to //e/b/DEPS in this CL now. :D

[Ken Rockot(use gerrit already), 2014-08-07 21:03:12]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-08-07 21:22:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/434493002/300001

[commit-bot: I haz the power, 2014-08-08 02:30:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-08 02:42:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-08 02:42:34]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2014-08-08 02:55:03]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-08-08 02:56:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/434493002/320001

[commit-bot: I haz the power, 2014-08-08 08:35:15]

Change committed as 288261