implement sign in dialog

Show login dialog in a popup if the user is not signed in
and getAuthToken API is called. Plumb interactive flag
appropriately.

Add tests for various possible paths get auth token API can
take.

BUG=131235, 124493
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148017

[Munjal (Google), 2012-07-11 21:15:13]

James, this is the patch that uses the observers I added to LoginUIService.

[Mihai Parparita -not on Chrome, 2012-07-12 00:39:21]

This fixes https://code.google.com/p/chromium/issues/detail?id=131235, right?
Can you add it to the CL description.

It seems like there are enough states (interactive, not interactive, signed in,
not signed in) that tests would be good to have (LoginUIService appears to be
design to be mockable).

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:59: if
(StartFlow(GetTokenFlowMode())) {
Rather than having to always call StartToken with GetTokenFlowMode, can you just
make it parameter-less and call GetTokenFlowMode() inside of it, when creating
the parameters?

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:127: if
(oauth2_info.client_id.empty()) {
Seems like this check (and the scope size one) could be moved to
GetAuthTokenFunction::RunImpl, so that you don't even try to get the user to
sign in if the extension is not configured correctly.

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:168: // Focus existing
UI.
Nit: This comment (and the one a couple of lines below) are just stating what
the code does, and so don't really seem necessary.

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:173:
GURL(chrome::kChromeUISyncPromoURL),
Can knowledge of this page and its URL be encapsulated into LoginUIService? It
already has ShowLoginUI, perhaps it have an overloaded variant that does't
require a Browser which opens the UI in a popup.

[Mihai Parparita -not on Chrome, 2012-07-12 00:39:44]

+Evan, since he's looked at identity API stuff recently too.

[Munjal (Google), 2012-07-12 18:41:56]

I am working on adding tests. Replied to all other comments.

I also added bug to the description, thanks for the reminder.

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:59: if
(StartFlow(GetTokenFlowMode())) {
On 2012/07/12 00:39:21, Mihai Parparita wrote:
> Rather than having to always call StartToken with GetTokenFlowMode, can you
just
> make it parameter-less and call GetTokenFlowMode() inside of it, when creating
> the parameters?

Actually ,that is what I first tried until I realized that StartFlow is
sometimes called with a specific parameter also. It is not always determined via
a command line flag.

So I either do this or I overload StartFlow. But I thought latter was
inappropriate since the only reason I would add an overload is to not duplicate
the code of determining the mode from command line flag, which I can do via a
helper instead of overloading StartFlow.

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:127: if
(oauth2_info.client_id.empty()) {
On 2012/07/12 00:39:21, Mihai Parparita wrote:
> Seems like this check (and the scope size one) could be moved to
> GetAuthTokenFunction::RunImpl, so that you don't even try to get the user to
> sign in if the extension is not configured correctly.

Done.

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:168: // Focus existing
UI.
On 2012/07/12 00:39:21, Mihai Parparita wrote:
> Nit: This comment (and the one a couple of lines below) are just stating what
> the code does, and so don't really seem necessary.

Done.

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:173:
GURL(chrome::kChromeUISyncPromoURL),
On 2012/07/12 00:39:21, Mihai Parparita wrote:
> Can knowledge of this page and its URL be encapsulated into LoginUIService? It
> already has ShowLoginUI, perhaps it have an overloaded variant that does't
> require a Browser which opens the UI in a popup.

Actually I had it exactly that way. But in talking to Andrew (who wrote
LoginUIService), we agreed that it is better to do i here because:
- He is going to remove ShowLoginUI method in LoginUIService
- This is the only place that needs to show login UI popup.

So we agreed that it is better to not add another method to LoginUIService.

[Mihai Parparita -not on Chrome, 2012-07-12 23:31:32]

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:59: if
(StartFlow(GetTokenFlowMode())) {
On 2012/07/12 18:41:56, munjal wrote:
> On 2012/07/12 00:39:21, Mihai Parparita wrote:
> > Rather than having to always call StartToken with GetTokenFlowMode, can you
> just
> > make it parameter-less and call GetTokenFlowMode() inside of it, when
creating
> > the parameters?
> 
> Actually ,that is what I first tried until I realized that StartFlow is
> sometimes called with a specific parameter also. It is not always determined
via
> a command line flag.
> 
> So I either do this or I overload StartFlow. But I thought latter was
> inappropriate since the only reason I would add an overload is to not
duplicate
> the code of determining the mode from command line flag, which I can do via a
> helper instead of overloading StartFlow.

Ah, I'd missed the StartFlow(OAuth2MintTokenFlow::MODE_MINT_TOKEN_FORCE)
callsite. Leaving it as-is is fine.

https://chromiumcodereview.appspot.com/10701041/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/identity/identity_api.cc:173:
GURL(chrome::kChromeUISyncPromoURL),
On 2012/07/12 18:41:56, munjal wrote:
> On 2012/07/12 00:39:21, Mihai Parparita wrote:
> > Can knowledge of this page and its URL be encapsulated into LoginUIService?
It
> > already has ShowLoginUI, perhaps it have an overloaded variant that does't
> > require a Browser which opens the UI in a popup.
> 
> Actually I had it exactly that way. But in talking to Andrew (who wrote
> LoginUIService), we agreed that it is better to do i here because:
> - He is going to remove ShowLoginUI method in LoginUIService
> - This is the only place that needs to show login UI popup.
> 
> So we agreed that it is better to not add another method to LoginUIService.

OK

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:58: // Checks if there is
a master login token to mint token for the extension.
"to mint tokens" sounds more correct.

[Evan Stade, 2012-07-13 08:57:33]

just style nits, otherwise lgtm

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:192:
OAuth2MintTokenFlow::MODE_MINT_TOKEN_FORCE :
this indent should be 4 spaces relative to 'return'

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:46: virtual void
OnLoginUIShown(LoginUIService::LoginUI* ui) OVERRIDE;
I can't find this interface in code search, was it added recently? I'd say that
neither of them should be pure virtual. You shouldn't have to define an empty
function here. However if you don't want to change it away from pure virtual
then you can just {} inline here instead of in the cc.

[Munjal (Google), 2012-07-16 20:13:27]

Okay, finally added tests for all code paths that get auth token can take. It
took a bit of time to try out a few things to come at a reasonable approach.

Note that I am not very fond of the approach I finally took, but it seemed the
best alternative and allowed me to exercise all cases in tests.

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:192:
OAuth2MintTokenFlow::MODE_MINT_TOKEN_FORCE :
On 2012/07/13 08:57:33, Evan Stade wrote:
> this indent should be 4 spaces relative to 'return'

Done.

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:46: virtual void
OnLoginUIShown(LoginUIService::LoginUI* ui) OVERRIDE;
On 2012/07/13 08:57:33, Evan Stade wrote:
> I can't find this interface in code search, was it added recently? I'd say
that
> neither of them should be pure virtual. You shouldn't have to define an empty
> function here. However if you don't want to change it away from pure virtual
> then you can just {} inline here instead of in the cc.

Yeah, that change was landed recently. But now it has been a few days, so you
will probably see in code search.

I made it empty inline for now. I will do another patch since atwilson@ might
want to weigh in on pure virtual versus not.

https://chromiumcodereview.appspot.com/10701041/diff/13002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:58: // Checks if there is
a master login token to mint token for the extension.
On 2012/07/12 23:31:32, Mihai Parparita wrote:
> "to mint tokens" sounds more correct.

Done.

[Mihai Parparita -not on Chrome, 2012-07-17 01:01:00]

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:185: return
GetExtension()->id();
You can override the Extension instance used in a RunFunctionAndReturnResult
call by calling function->set_extension()

Rather than having to pull out these simple getters into standalone methods that
you can override, you could just have (in your test) a helper method like
CreateGetAuthTokenFunction(OAuth2Info) that makes an Extension with the right
OAuth2Info, creates the GetAuthTokenFunction, sets the extension for it (and any
other shared setup work) and returns it, ready to called.

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:34: static const char
kInvalidClientId[];
I don't think it's worth exposing these constants in the header file just for
the sake of tests. You can leave them in an anonymous namespace in the .cc file,
and hardcode the strings in the test.

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_apitest.cc (left):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:15: class
IdentityInterceptor : public OAuth2MintTokenFlow::InterceptorForTests {
Can you remove the interceptor support from OAuth2MinTokenFlow and WebAuthFlow,
since these tests were the only uses?

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:148: // Without a
callback the function will not generate a result.
Instead of having to repeat this every time, can you make
RunFunctionAndReturnResult always do this (I don't think it would affect any
current uses).

[Munjal (Google), 2012-07-19 22:35:41]

Addressed all comments. Please take another look.

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:185: return
GetExtension()->id();
On 2012/07/17 01:01:00, Mihai Parparita wrote:
> You can override the Extension instance used in a RunFunctionAndReturnResult
> call by calling function->set_extension()
> 
> Rather than having to pull out these simple getters into standalone methods
that
> you can override, you could just have (in your test) a helper method like
> CreateGetAuthTokenFunction(OAuth2Info) that makes an Extension with the right
> OAuth2Info, creates the GetAuthTokenFunction, sets the extension for it (and
any
> other shared setup work) and returns it, ready to called.

Done.

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:34: static const char
kInvalidClientId[];
On 2012/07/17 01:01:00, Mihai Parparita wrote:
> I don't think it's worth exposing these constants in the header file just for
> the sake of tests. You can leave them in an anonymous namespace in the .cc
file,
> and hardcode the strings in the test.

That is what I did first. But then I thought that it is silly to have to change
tests every time someone makes a tiny change to the error messages. So given
that error messages are const char* const, I thought it makes sense to make them
public.

If you feel strongly, let me know.

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_apitest.cc (left):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:15: class
IdentityInterceptor : public OAuth2MintTokenFlow::InterceptorForTests {
Let me do it in a separate patch.

On 2012/07/17 01:01:00, Mihai Parparita wrote:
> Can you remove the interceptor support from OAuth2MinTokenFlow and
WebAuthFlow,
> since these tests were the only uses?

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:148: // Without a
callback the function will not generate a result.
On 2012/07/17 01:01:00, Mihai Parparita wrote:
> Instead of having to repeat this every time, can you make
> RunFunctionAndReturnResult always do this (I don't think it would affect any
> current uses).

Done.

[Mihai Parparita -not on Chrome, 2012-07-20 18:02:32]

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:34: static const char
kInvalidClientId[];
On 2012/07/19 22:35:42, munjal wrote:
> On 2012/07/17 01:01:00, Mihai Parparita wrote:
> > I don't think it's worth exposing these constants in the header file just
for
> > the sake of tests. You can leave them in an anonymous namespace in the .cc
> file,
> > and hardcode the strings in the test.
> 
> That is what I did first. But then I thought that it is silly to have to
change
> tests every time someone makes a tiny change to the error messages. So given
> that error messages are const char* const, I thought it makes sense to make
them
> public.
> 
> If you feel strongly, let me know.

In that case, to be consistent with other exported constants, they should be in
an identity_constants namespace. See for example:

http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
or
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_apitest.cc (left):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:15: class
IdentityInterceptor : public OAuth2MintTokenFlow::InterceptorForTests {
On 2012/07/19 22:35:42, munjal wrote:
> Let me do it in a separate patch.

OK, filed http://crbug.com/138279

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:135: const Extension*
CreateExtension(bool with_client_id, bool with_scopes) {
Two boolean parameters side-by-side like this make code hard to read. Can you
turn this into a bit field with enum constants?

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:146:
ext->set_oauth2_info(oauth2_info);
It looks like you're adding this setter, but I'm not seeing extension.h in the
CL.

Also, if you're going to be casting away the constness of the extension, can you
just also do that for the struct returned by oauth2_info(), so that you can
mutate it?

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/test/data/e...
File chrome/test/data/extensions/platform_apps/minimal/manifest.json (left):

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/minimal/manifest.json:6:
"experimental",
This is still necessary, the minimal app listens for the launch event, which is
experimental:

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/p...

You can either add the experimental flag to the command-line when running your
test, or create a new minimal app for your test that is even simpler (doesn't
actually create windows).

[Munjal (Google), 2012-07-20 18:54:14]

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10701041/diff/19002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:34: static const char
kInvalidClientId[];
On 2012/07/20 18:02:32, Mihai Parparita wrote:
> On 2012/07/19 22:35:42, munjal wrote:
> > On 2012/07/17 01:01:00, Mihai Parparita wrote:
> > > I don't think it's worth exposing these constants in the header file just
> for
> > > the sake of tests. You can leave them in an anonymous namespace in the .cc
> > file,
> > > and hardcode the strings in the test.
> > 
> > That is what I did first. But then I thought that it is silly to have to
> change
> > tests every time someone makes a tiny change to the error messages. So given
> > that error messages are const char* const, I thought it makes sense to make
> them
> > public.
> > 
> > If you feel strongly, let me know.
> 
> In that case, to be consistent with other exported constants, they should be
in
> an identity_constants namespace. See for example:
> 
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
> or
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

Done.

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:135: const Extension*
CreateExtension(bool with_client_id, bool with_scopes) {
On 2012/07/20 18:02:32, Mihai Parparita wrote:
> Two boolean parameters side-by-side like this make code hard to read. Can you
> turn this into a bit field with enum constants?

Done.

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_apitest.cc:146:
ext->set_oauth2_info(oauth2_info);
On 2012/07/20 18:02:32, Mihai Parparita wrote:
> It looks like you're adding this setter, but I'm not seeing extension.h in the
> CL.
> 

Good catch. I realized it when the try bots failed. Added the file to the patch.

> Also, if you're going to be casting away the constness of the extension, can
you
> just also do that for the struct returned by oauth2_info(), so that you can
> mutate it?

Good idea. Done. And I don't need the setter anymore either. Cool.

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/test/data/e...
File chrome/test/data/extensions/platform_apps/minimal/manifest.json (left):

https://chromiumcodereview.appspot.com/10701041/diff/30004/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/minimal/manifest.json:6:
"experimental",
On 2012/07/20 18:02:32, Mihai Parparita wrote:
> This is still necessary, the minimal app listens for the launch event, which
is
> experimental:
> 
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/p...
> 
> You can either add the experimental flag to the command-line when running your
> test, or create a new minimal app for your test that is even simpler (doesn't
> actually create windows).

Done. I added a new minimal app for oauth2 tests.

[Mihai Parparita -not on Chrome, 2012-07-23 17:53:30]

LGTM

[commit-bot: I haz the power, 2012-07-23 18:03:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/munjal@chromium.org/10701041/33003

[commit-bot: I haz the power, 2012-07-23 18:55:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/munjal@chromium.org/10701041/41003

[commit-bot: I haz the power, 2012-07-23 19:40:51]

Try job failure for 10701041-41003 (retry) (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-23 23:47:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/munjal@chromium.org/10701041/36013

[commit-bot: I haz the power, 2012-07-24 01:12:27]

Change committed as 148017