Introduce authentication service.

Introduce authentication service.

This service allows to request oauth2 token. The only current
implementation is for Android.

Also the implementation is currently using a single project for
authentication.

R=ppi@chromium.org, tonyg@chromium.org
BUG=https://github.com/domokit/mojo/issues/94

Committed: https://chromium.googlesource.com/external/mojo/+/9b79e2c6ea8ecf19fe7904eeea1e331ef30c0ff1

[ppi, 2015-04-29 13:32:24]

https://codereview.chromium.org/1116653002/diff/1/examples/dart/console_examp...
File examples/dart/console_example/BUILD.gn (right):

https://codereview.chromium.org/1116653002/diff/1/examples/dart/console_examp...
examples/dart/console_example/BUILD.gn:14: "//services/authentication:bindings",
Is this intentional? There are no changed .dart files here.

https://codereview.chromium.org/1116653002/diff/1/mojo/public/java/applicatio...
File
mojo/public/java/application/src/org/chromium/mojo/application/ApplicationDelegate.java
(left):

https://codereview.chromium.org/1116653002/diff/1/mojo/public/java/applicatio...
mojo/public/java/application/src/org/chromium/mojo/application/ApplicationDelegate.java:32:
String requestorUrl, ApplicationConnection connection);
Why this change?

https://codereview.chromium.org/1116653002/diff/1/services/android/intent_rec...
File services/android/intent_receiver.mojom (right):

https://codereview.chromium.org/1116653002/diff/1/services/android/intent_rec...
services/android/intent_receiver.mojom:27: RegisterActivityResult(IntentReceiver
receiver) => (array<uint8>? intent);
Should this be called RegisterStartActivityForResult or
RegisterStartActivityForResultReceiver ?

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
File services/authentication/authentication.mojom (right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:9: // Request an account to use.
s/Request/Requests/

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:11: // Request an oauth2 token for
the given account with the given scopes.
s/Request/Requests/

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:13: // Request to clear a
previously acquired token. This should be called when a
s/Request/Requests/

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
File
services/authentication/src/org/chromium/mojo/authentication/Authentication.java
(right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/Authentication.java:20:
public class Authentication implements ApplicationDelegate {
Can we call this AuthenticationServiceApp or AuthenticationApp ?
"Authentication" seems overly general as an entity name.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:38:
private final class TrampolineResponse implements RegisterActivityResultResponse
{
Should this be called RegisterStartActivityForResultCallback?

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:40:
* The intent the shell need to send.
s/need/needs/

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:45:
this.mIntent = intent;
nit: "this." is probably not needed.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:88:
for (int i = 0; i < scopes.length; ++i) {
Can we use
http://developer.android.com/reference/android/text/TextUtils.html#join(java....,
java.lang.Iterable) instead?

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:97:
} catch (final UserRecoverableAuthException e) {
Could you add a comment explaining when we would enter this case?

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:130:
// Unrecoverable error. A null token will be send to the client.
Should we log something informative here?

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:182:
// Nothing to do.
Should we log something informative here?

[tonyg, 2015-04-29 13:34:18]

tonyg@chromium.org changed reviewers:
+ alhaad@chromium.org

[tonyg, 2015-04-29 13:47:43]

This looks great. A couple of comments, but I defer to ppi on the lg2m.

https://codereview.chromium.org/1116653002/diff/1/services/android/intent_rec...
File services/android/intent_receiver.mojom (right):

https://codereview.chromium.org/1116653002/diff/1/services/android/intent_rec...
services/android/intent_receiver.mojom:27: RegisterActivityResult(IntentReceiver
receiver) => (array<uint8>? intent);
[+alhaad]

Just curious if this new API also helps w/ alhaad's desired use case of using
the system camera?

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
File services/authentication/authentication.mojom (right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:9: // Request an account to use.
This interface could probably use a little more robust documentation. For
instance, I assume that a null username means it failed or the user cancelled,
but the comment should clarify how to treat null.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:140:
String[] accountTypes = new String[] {"com.google"};
The mojom doesn't specify that this only produces google oauth2 tokens. We
should probably either note that there (perhaps w/ a TOOD?) or allow this param
to be passed in.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:141:
String message = "Select an account to use with application: " + mConsumerURL;
I wonder whether we should make this message a little more scary initially.
Something that let's folks understand the current state of security of this
token in mojo shell.

[qsr, 2015-04-29 14:22:20]

https://codereview.chromium.org/1116653002/diff/1/examples/dart/console_examp...
File examples/dart/console_example/BUILD.gn (right):

https://codereview.chromium.org/1116653002/diff/1/examples/dart/console_examp...
examples/dart/console_example/BUILD.gn:14: "//services/authentication:bindings",
On 2015/04/29 13:32:23, ppi wrote:
> Is this intentional? There are no changed .dart files here.

 Was my tests. Sorry. Thanks.

https://codereview.chromium.org/1116653002/diff/1/mojo/public/java/applicatio...
File
mojo/public/java/application/src/org/chromium/mojo/application/ApplicationDelegate.java
(left):

https://codereview.chromium.org/1116653002/diff/1/mojo/public/java/applicatio...
mojo/public/java/application/src/org/chromium/mojo/application/ApplicationDelegate.java:32:
String requestorUrl, ApplicationConnection connection);
On 2015/04/29 13:32:23, ppi wrote:
> Why this change?

 Because now that I use this requestorUrl, I realized that this is part of the
connection object. So this is just confusing.

https://codereview.chromium.org/1116653002/diff/1/services/android/intent_rec...
File services/android/intent_receiver.mojom (right):

https://codereview.chromium.org/1116653002/diff/1/services/android/intent_rec...
services/android/intent_receiver.mojom:27: RegisterActivityResult(IntentReceiver
receiver) => (array<uint8>? intent);
On 2015/04/29 13:32:23, ppi wrote:
> Should this be called RegisterStartActivityForResult or
> RegisterStartActivityForResultReceiver ?

Renamed: RegisterActivityResultReceiver

https://codereview.chromium.org/1116653002/diff/1/services/android/intent_rec...
services/android/intent_receiver.mojom:27: RegisterActivityResult(IntentReceiver
receiver) => (array<uint8>? intent);
On 2015/04/29 13:47:43, tonyg wrote:
> [+alhaad]
> 
> Just curious if this new API also helps w/ alhaad's desired use case of using
> the system camera?

 I don't know for sure. It depends a lot on which API he wants to use. We still
cannot receive intent that have active binders in them.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
File services/authentication/authentication.mojom (right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:9: // Request an account to use.
On 2015/04/29 13:32:23, ppi wrote:
> s/Request/Requests/

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:9: // Request an account to use.
On 2015/04/29 13:47:43, tonyg wrote:
> This interface could probably use a little more robust documentation. For
> instance, I assume that a null username means it failed or the user cancelled,
> but the comment should clarify how to treat null.

 Added documentation, and human readable error messages.

 We will probably want some error code some day, but this is not something I
feel we are ready for yet.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:11: // Request an oauth2 token for
the given account with the given scopes.
On 2015/04/29 13:32:23, ppi wrote:
> s/Request/Requests/

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/aut...
services/authentication/authentication.mojom:13: // Request to clear a
previously acquired token. This should be called when a
On 2015/04/29 13:32:23, ppi wrote:
> s/Request/Requests/

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
File
services/authentication/src/org/chromium/mojo/authentication/Authentication.java
(right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/Authentication.java:20:
public class Authentication implements ApplicationDelegate {
On 2015/04/29 13:32:23, ppi wrote:
> Can we call this AuthenticationServiceApp or AuthenticationApp ?
> "Authentication" seems overly general as an entity name.

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/04/29 13:32:23, ppi wrote:
> 2015

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:38:
private final class TrampolineResponse implements RegisterActivityResultResponse
{
On 2015/04/29 13:32:24, ppi wrote:
> Should this be called RegisterStartActivityForResultCallback?

 Hum, I have no strong opinion, so done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:40:
* The intent the shell need to send.
On 2015/04/29 13:32:24, ppi wrote:
> s/need/needs/

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:45:
this.mIntent = intent;
On 2015/04/29 13:32:23, ppi wrote:
> nit: "this." is probably not needed.

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:88:
for (int i = 0; i < scopes.length; ++i) {
On 2015/04/29 13:32:23, ppi wrote:
> Can we use
>
http://developer.android.com/reference/android/text/TextUtils.html#join(java....,
> java.lang.Iterable) instead?

 We could, and I will if you insist, but this is a little bit less efficient as
it creates an intermediary string that I do not need.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:97:
} catch (final UserRecoverableAuthException e) {
On 2015/04/29 13:32:23, ppi wrote:
> Could you add a comment explaining when we would enter this case?

Done.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:130:
// Unrecoverable error. A null token will be send to the client.
On 2015/04/29 13:32:24, ppi wrote:
> Should we log something informative here?

Added an error to the protocol -> we are sending the error to the consumer.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:140:
String[] accountTypes = new String[] {"com.google"};
On 2015/04/29 13:47:43, tonyg wrote:
> The mojom doesn't specify that this only produces google oauth2 tokens. We
> should probably either note that there (perhaps w/ a TOOD?) or allow this
param
> to be passed in.

This param is android specific, so passing it in seems not the right solution.
We might want to have a notion of account type etc., but I don't think we are
ready at this point. I'll just comment on this in the mojom.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:141:
String message = "Select an account to use with application: " + mConsumerURL;
On 2015/04/29 13:47:43, tonyg wrote:
> I wonder whether we should make this message a little more scary initially.
> Something that let's folks understand the current state of security of this
> token in mojo shell.

 This part is only sending the identity of an account to the shell, not getting
any token. The token are gotten from the other method, and at this point there
is no message at all. I want the shell to display a dialog on the other method,
but I need to create a dialog service for this (dialog can only be used from the
main thread) before I can do this. When I do, it will be clear which application
get tokens.

https://codereview.chromium.org/1116653002/diff/1/services/authentication/src...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:182:
// Nothing to do.
On 2015/04/29 13:32:24, ppi wrote:
> Should we log something informative here?

 Not really. On a failure here, there is absolutely nothing you can do.

[ppi, 2015-04-29 15:13:44]

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
File services/authentication/authentication.mojom (right):

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/authentication.mojom:9: // TODO(qsr): This API only
handles google account at this time. It will need
s/account/accounts/

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:38:
private final class RegisterStartActivityForResultCallback
nit: After the rename in mojom, perhaps the straightforward
RegisterActivityResultReceiverCallback would work better here?

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:41:
* The intent the shell needs to send.
Maybe let's clarify even further:

"The intent that the requesting application needs to be run by shell on its
behalf."

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:77:
/**
Could you comment here that when an user-recoverable error occurs, we store
|callback| for later and return without calling it?

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:101:
GetOAuth2TokenResponse mCurrentCallback = callback;
nit: maybe mPendingCallback ?

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:105:
call(null, "User denied request.");
s/request/the request/

https://codereview.chromium.org/1116653002/diff/20001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/IntentReceiverRegistry.java
(right):

https://codereview.chromium.org/1116653002/diff/20001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/IntentReceiverRegistry.java:106:
String uuid;
I'm probably missing something obvious, but why not String uuid =
UUID.randomUUID().toString(); as we do above?

https://codereview.chromium.org/1116653002/diff/20001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/IntentReceiverRegistry.java:109:
if (registry.mLastRequestCode < 1) {
Why this would ever be lower than 1 if we start at 0 and increment before
getting here?

[qsr, 2015-04-29 15:29:58]

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
File services/authentication/authentication.mojom (right):

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/authentication.mojom:9: // TODO(qsr): This API only
handles google account at this time. It will need
On 2015/04/29 15:13:43, ppi wrote:
> s/account/accounts/

Done.

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:38:
private final class RegisterStartActivityForResultCallback
On 2015/04/29 15:13:44, ppi wrote:
> nit: After the rename in mojom, perhaps the straightforward
> RegisterActivityResultReceiverCallback would work better here?

Done.

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:41:
* The intent the shell needs to send.
On 2015/04/29 15:13:44, ppi wrote:
> Maybe let's clarify even further:
> 
> "The intent that the requesting application needs to be run by shell on its
> behalf."

Done.

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:77:
/**
On 2015/04/29 15:13:44, ppi wrote:
> Could you comment here that when an user-recoverable error occurs, we store
> |callback| for later and return without calling it?

As discussed offline, passing a callback around doesn't seem unusual enough to
have a special comment.

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:101:
GetOAuth2TokenResponse mCurrentCallback = callback;
On 2015/04/29 15:13:44, ppi wrote:
> nit: maybe mPendingCallback ?

Done.

https://codereview.chromium.org/1116653002/diff/20001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:105:
call(null, "User denied request.");
On 2015/04/29 15:13:43, ppi wrote:
> s/request/the request/

Done.

https://codereview.chromium.org/1116653002/diff/20001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/IntentReceiverRegistry.java
(right):

https://codereview.chromium.org/1116653002/diff/20001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/IntentReceiverRegistry.java:106:
String uuid;
On 2015/04/29 15:13:44, ppi wrote:
> I'm probably missing something obvious, but why not String uuid =
> UUID.randomUUID().toString(); as we do above?

 Because the startActivityForResult use an int to match request and response, so
I need to index by integers.

https://codereview.chromium.org/1116653002/diff/20001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/IntentReceiverRegistry.java:109:
if (registry.mLastRequestCode < 1) {
Added comment.

[ppi, 2015-04-29 15:31:39]

lgtm

[qsr, 2015-04-29 15:33:15]

Committed patchset #3 (id:40001) manually as
9b79e2c6ea8ecf19fe7904eeea1e331ef30c0ff1 (presubmit successful).