Identity API: web-based scope approval dialogs for getAuthToken

Identity API: web-based scope approval dialogs for getAuthToken

When getAuthToken needs to prompt a user for permissions, it now uses
a web-based authorization flow instead of native dialogs.

BUG=228908
(step #4 of the bug description)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200683

[Michael Courage, 2013-05-16 05:29:12]

Please review,
asargent: chrome/browser/extensions/* & gypi files
rogerta: google_apis/gaia/gaia_urls.*

[Roger Tawa OOO till Jul 10th, 2013-05-16 16:06:15]

google_apis/gaia lgtm

A few nits in the rest.

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc (right):

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc:103: std::string
token_key(kOAuth2RedirectAccessTokenKey);
Better to use:

  std::vector<std::pair<std::string, std::string> > pairs;
  base::SplitStringIntoKeyValuePairs(fragment, '=', '&', &pairs);

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.h (right):

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:51: virtual void
OnGaiaFlowCompleted(const std::string& token,
Change name to |access_token| to make it clearer?

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:76: // Creates an
interactive flow. Can override for testing.
Add description of |url| arg.

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_api.h:137:
scoped_ptr<GaiaWebAuthFlow> web_auth_flow_;
Maybe call this |gaia_web_auth_flow_| instead to differentiate from WebAuthFlow
class?

[Michael Courage, 2013-05-16 20:39:02]

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc (right):

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc:103: std::string
token_key(kOAuth2RedirectAccessTokenKey);
On 2013/05/16 16:06:15, Roger Tawa wrote:
> Better to use:
> 
>   std::vector<std::pair<std::string, std::string> > pairs;
>   base::SplitStringIntoKeyValuePairs(fragment, '=', '&', &pairs);

Done. That's so much nicer, thanks!

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.h (right):

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:51: virtual void
OnGaiaFlowCompleted(const std::string& token,
On 2013/05/16 16:06:15, Roger Tawa wrote:
> Change name to |access_token| to make it clearer?

Done.

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:76: // Creates an
interactive flow. Can override for testing.
On 2013/05/16 16:06:15, Roger Tawa wrote:
> Add description of |url| arg.

Done.

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/15148007/diff/9001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_api.h:137:
scoped_ptr<GaiaWebAuthFlow> web_auth_flow_;
On 2013/05/16 16:06:15, Roger Tawa wrote:
> Maybe call this |gaia_web_auth_flow_| instead to differentiate from
WebAuthFlow
> class?

Done.

[asargent_no_longer_on_chrome, 2013-05-16 21:39:19]

lgtm

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.h (right):

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:53: const
std::string& error) = 0;
nit: It seems slightly unexpected that there are separate "Failure" and
"Completed" callbacks, but the "Completed" one contains an |error| parameter
that needs to be checked. I guess there are different kinds of failure? Perhaps
worth a comment here.

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:85: std::string
auth_url_;
Any reason you aren't using a GURL here for |auth_url_|? This doesn't end up
containing the special invalid url mentioned in the overview comments up top
does it?

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.h:126: // Maps OAuth2
protocol errors to an error message.
nit: it might be helpful to mention who the audience is for the message - the
user (a dialog, etc.)? The developer (eg inside a failure callback)?

[Michael Courage, 2013-05-16 22:44:08]

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/gaia_web_auth_flow.h (right):

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:53: const
std::string& error) = 0;
On 2013/05/16 21:39:20, Antony Sargent wrote:
> nit: It seems slightly unexpected that there are separate "Failure" and
> "Completed" callbacks, but the "Completed" one contains an |error| parameter
> that needs to be checked. I guess there are different kinds of failure?
Perhaps
> worth a comment here. 

I was thinking of these errors as special because they don't involve protocol or
network failures, but from the caller POV they aren't really that special, so I
moved them to the FlowFailure call.

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/gaia_web_auth_flow.h:85: std::string
auth_url_;
On 2013/05/16 21:39:20, Antony Sargent wrote:
> Any reason you aren't using a GURL here for |auth_url_|? This doesn't end up
> containing the special invalid url mentioned in the overview comments up top
> does it?

Done. No crazy URLs here.

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/15148007/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/identity/identity_api.h:126: // Maps OAuth2
protocol errors to an error message.
On 2013/05/16 21:39:20, Antony Sargent wrote:
> nit: it might be helpful to mention who the audience is for the message - the
> user (a dialog, etc.)? The developer (eg inside a failure callback)? 

Done.

[commit-bot: I haz the power, 2013-05-16 22:54:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/courage@chromium.org/15148007/25002

[commit-bot: I haz the power, 2013-05-17 01:12:56]

Change committed as 200683