Google OAuth Device Flow support for FNL

Added support for adding Google accounts on platforms other than Android such as FNL or Goobuntu. It also persists the refresh tokens using files service, which are later used for fetching access tokens without a re-auth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.

Bug# https://github.com/domokit/mojo/issues/600

Related design doc:
https://docs.google.com/document/d/1-LWlQl-9iInkeyuP0EoPhehDvyz68FCK0w_pfzZaZwU/edit

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/1195c66530da45464688fb251a1610155e5e089d

[ukode, 2015-11-20 18:39:46]

Description was changed from

==========
.
Google OAuth Device Flow support for FNL

.
Added support for adding Google accounts on platforms other than Android
such as FNL or Goobuntu. It also persists the refresh tokens using files
service, which are later used for fetching access tokens without a reauth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based
idtokens.


.


.


.


.


.


.


.


.


.


.


.


.

BUG=
==========

to

==========
Added support for adding Google accounts on platforms other than Android such as
FNL or Goobuntu. It also persists the refresh tokens using files service, which
are later used for fetching access tokens without a reauth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.

BUG=
==========

[ukode, 2015-11-20 18:39:46]

ukode@google.com changed reviewers:
+ jln@chromium.org, tonyg@chromium.org

[ukode, 2015-11-30 23:07:50]

Description was changed from

==========
Added support for adding Google accounts on platforms other than Android such as
FNL or Goobuntu. It also persists the refresh tokens using files service, which
are later used for fetching access tokens without a reauth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.

BUG=
==========

to

==========
Added support for adding Google accounts on platforms other than Android such as
FNL or Goobuntu. It also persists the refresh tokens using files service, which
are later used for fetching access tokens without a reauth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.
==========

[ukode, 2015-11-30 23:07:50]

ukode@google.com changed reviewers:
+ ataly@chromium.org

[jln (very slow on Chromium), 2015-12-01 00:08:44]

Hey Usha,

Just two quick things before I start taking a look that you should fix first:

1. Make sure you CL goes through "git cl format" so that clang-format corrects a
few style nits (like spacing).

In particular, be careful that "wrapped" lines should have four spaces indent,
not 2.

2. The /org/chromium/mojo/.... style paths are for Java only. Could you move
this?

https://codereview.chromium.org/1466733002/diff/40001/examples/authentication...
File examples/authentication_demo/google_authentication_demo.cc (right):

https://codereview.chromium.org/1466733002/diff/40001/examples/authentication...
examples/authentication_demo/google_authentication_demo.cc:9: 
Nit: no blank line here.

https://codereview.chromium.org/1466733002/diff/40001/services/authentication...
File
services/authentication/src/org/chromium/mojo/authentication/accounts_db_manager.h
(right):

https://codereview.chromium.org/1466733002/diff/40001/services/authentication...
services/authentication/src/org/chromium/mojo/authentication/accounts_db_manager.h:5:
#ifndef ACCOUNTS_DB_MANAGER_H_
1. Always have the full path reflected in the header guard.
2. Complicated full paths like this should only be for Java, not C++. You can
just have this in services/authentication/

[ukode, 2015-12-04 19:45:42]

Thanks Julien. Pls see the comments inline.

On 2015/12/01 00:08:44, jln (slow on Chromium) wrote:

> Hey Usha,
> 
> Just two quick things before I start taking a look that you should fix first:
> 
> 1. Make sure you CL goes through "git cl format" so that clang-format corrects
a
> few style nits (like spacing).
Done. 

> 
> In particular, be careful that "wrapped" lines should have four spaces indent,
> not 2.
Ack.

> 
> 2. The /org/chromium/mojo/.... style paths are for Java only. Could you move
> this?
> 
>
https://codereview.chromium.org/1466733002/diff/40001/examples/authentication...
> File examples/authentication_demo/google_authentication_demo.cc (right):
> 
>
https://codereview.chromium.org/1466733002/diff/40001/examples/authentication...
> examples/authentication_demo/google_authentication_demo.cc:9: 
> Nit: no blank line here.

Done.
> 
>
https://codereview.chromium.org/1466733002/diff/40001/services/authentication...
> File
>
services/authentication/src/org/chromium/mojo/authentication/accounts_db_manager.h
> (right):
> 
>
https://codereview.chromium.org/1466733002/diff/40001/services/authentication...
>
services/authentication/src/org/chromium/mojo/authentication/accounts_db_manager.h:5:
> #ifndef ACCOUNTS_DB_MANAGER_H_
> 1. Always have the full path reflected in the header guard.
> 2. Complicated full paths like this should only be for Java, not C++. You can
> just have this in services/authentication/

Ack.

[jln (very slow on Chromium), 2015-12-08 22:27:29]

Sorry I'm being so slow to review this. I just took a quick look but need to go
to a meeting. Here are a few quick remarks.

- Note that this doesn't seem to compile on the try bots

- I'm taking a quick general look, but ultimately we need someone more familiar
with Mojo than I am to review this.
rudominer@, cc-ed offered to help. Thanks Mitch!

- In general, more class comments or a general explanation of what you're doing
in the commit message would be helpful.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/mojo/services/a...
File mojo/services/authentication/interfaces/authentication.mojom (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/mojo/services/a...
mojo/services/authentication/interfaces/authentication.mojom:23:
GetOAuth2Token(string username, array<string> scopes) =>
Meta-remark (not for this CL): should the scopes really be strings? I wonder if
we shouldn't give them their own type with some checking etc..

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/accounts_db_manager.cc (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:42: std::string
new_contents(AccountsDbManager::contents_);
No need for AccountsDbManager:: qualifier.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:46: if
(existing_user_data.is_null()) {  // new account to be added
I don't quite understand what's happening here yet, but it looks like something
that should be done in a separate utility function, possibly in the anonymous
namespace, no?

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:68:
new_contents.assign(buffer.c_str(), buffer.size());
Why are new_contents and buffer both needed. It looks like you could do with
just one or am I mistaken?

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:90: return false;
Looks like the opened file is not getting closed here or in the next "return
false". Need to check how scoping is typically done in Mojo.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:177: const std::vector<uint8_t>
vec = bytes_read.storage();
I'm surprised if Mojo doesn't offer a better way to do this.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/accounts_db_manager.h (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:15: // TODO(ukode): Stuff copied
from mojo/public/cpp/bindings/lib/template_util.h.
This should definitely not be in this .h file.

If this is really needed, it could either be somewhere in a shared header file
that you include, or copied in the .cc file. (But copying call is a last-resort
evil).

Talk to vtl maybe?

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:58: class AccountsDbManager {
Please, document your class and methods as per style guide.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:71: bool checkIfUserExists(const
std::string& user_data);
nit: Capital C in CheckIfUserExists)

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:71: bool checkIfUserExists(const
std::string& user_data);
This method looks unimplemented, no?

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:72: mojo::files::DirectoryPtr
directory_;
Nit: vertical space

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/auth_data.h (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/auth_data.h:15: AuthData();
Fine to put trivial constructor/destructor inline in header file.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/auth_data.h:34: std::string GetAuthDataAsString(const
AuthData& auth_data);
Should these be static method in an AuthData class instead? Or perhaps in their
own AuthData namespace. Depends on prevailing style (need to ask someone in
Mojo)

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/google_authentication_service.cc (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/google_authentication_service.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
Did not look yet.

[rudominer, 2015-12-14 23:46:50]

rudominer@chromium.org changed reviewers:
+ rudominer@chromium.org

[rudominer, 2015-12-14 23:46:50]

Please make the tests pass on the trybots.

[rudominer, 2015-12-14 23:48:38]

Please associate this patch with a bug and in either this patch description or
the bug (or both) give a link to your design doc.

[ukode, 2015-12-16 19:24:12]

ukode@google.com changed reviewers:
+ vtl@google.com

[ukode, 2015-12-16 19:24:13]

PTAL.

https://chromiumcodereview.appspot.com/1466733002/diff/40001/examples/authent...
File examples/authentication_demo/google_authentication_demo.cc (right):

https://chromiumcodereview.appspot.com/1466733002/diff/40001/examples/authent...
examples/authentication_demo/google_authentication_demo.cc:9: 
On 2015/12/01 00:08:44, jln (slow on Chromium) wrote:
> Nit: no blank line here.

Done.

https://chromiumcodereview.appspot.com/1466733002/diff/40001/services/authent...
File
services/authentication/src/org/chromium/mojo/authentication/accounts_db_manager.h
(right):

https://chromiumcodereview.appspot.com/1466733002/diff/40001/services/authent...
services/authentication/src/org/chromium/mojo/authentication/accounts_db_manager.h:5:
#ifndef ACCOUNTS_DB_MANAGER_H_
On 2015/12/01 00:08:44, jln (slow on Chromium) wrote:
> 1. Always have the full path reflected in the header guard.
> 2. Complicated full paths like this should only be for Java, not C++. You can
> just have this in services/authentication/

Acknowledged.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/mojo/services/a...
File mojo/services/authentication/interfaces/authentication.mojom (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/mojo/services/a...
mojo/services/authentication/interfaces/authentication.mojom:23:
GetOAuth2Token(string username, array<string> scopes) =>
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Meta-remark (not for this CL): should the scopes really be strings? I wonder
if
> we shouldn't give them their own type with some checking etc..

OAuth scopes are in general strings, so most of Google3 and Android code
directly takes a list of scope strings, (optional) normalizes them and then
passes it to Google OAuth endpoints.Scope checking is already done at the LSO
backends, not sure how much we would like to validate these in our frontends
other than nullchecks.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/accounts_db_manager.cc (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:42: std::string
new_contents(AccountsDbManager::contents_);
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> No need for AccountsDbManager:: qualifier.

Done.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:46: if
(existing_user_data.is_null()) {  // new account to be added
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> I don't quite understand what's happening here yet, but it looks like
something
> that should be done in a separate utility function, possibly in the anonymous
> namespace, no?

This is very specific to account db functionality. Refactored this into a
separate helper method. Issue here is mojo::String is missing some append
capabilities. So I need to format the updated db contents first and then use
that to update the current DB contents. This method does two things (also
clarified in comments above):

1. Updates the user credential if it already exists. (Using File Write mode)
2. Or else adds a new record to the Db file. (Using File Append mode)

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:68:
new_contents.assign(buffer.c_str(), buffer.size());
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Why are new_contents and buffer both needed. It looks like you could do with
> just one or am I mistaken?

Fixed it as part of refactoring.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:90: return false;
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Looks like the opened file is not getting closed here or in the next "return
> false". Need to check how scoping is typically done in Mojo.

Good catch. Fixed it.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.cc:177: const std::vector<uint8_t>
vec = bytes_read.storage();
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> I'm surprised if Mojo doesn't offer a better way to do this.

+vtl/mitch - do you have any other suggestion here?

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/accounts_db_manager.h (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:15: // TODO(ukode): Stuff copied
from mojo/public/cpp/bindings/lib/template_util.h.
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> This should definitely not be in this .h file.
> 
> If this is really needed, it could either be somewhere in a shared header file
> that you include, or copied in the .cc file. (But copying call is a
last-resort
> evil).
> 
> Talk to vtl maybe?

Acknowledged. +Vtl

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:58: class AccountsDbManager {
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Please, document your class and methods as per style guide.

Done.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:71: bool checkIfUserExists(const
std::string& user_data);
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> nit: Capital C in CheckIfUserExists)

Done.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:71: bool checkIfUserExists(const
std::string& user_data);
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> This method looks unimplemented, no?

Good catch, Had it before, later removed it as it wasn't needed. Forgot to
update the header file. Fixed it now.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/accounts_db_manager.h:72: mojo::files::DirectoryPtr
directory_;
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Nit: vertical space

Done.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/auth_data.h (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/auth_data.h:15: AuthData();
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Fine to put trivial constructor/destructor inline in header file.

Acknowledged. But inlining led to compile time errors.

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/auth_data.h:34: std::string GetAuthDataAsString(const
AuthData& auth_data);
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Should these be static method in an AuthData class instead? Or perhaps in
their
> own AuthData namespace. Depends on prevailing style (need to ask someone in
> Mojo)

+Mitch - thoughts?

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
File services/authentication/google_authentication_service.cc (right):

https://chromiumcodereview.appspot.com/1466733002/diff/100001/services/authen...
services/authentication/google_authentication_service.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2015/12/08 22:27:29, jln (very slow on Chromium) wrote:
> Did not look yet.

Acknowledged.

[ukode, 2015-12-17 00:58:32]

Description was changed from

==========
Added support for adding Google accounts on platforms other than Android such as
FNL or Goobuntu. It also persists the refresh tokens using files service, which
are later used for fetching access tokens without a reauth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.
==========

to

==========
Added support for adding Google accounts on platforms other than Android such as
FNL or Goobuntu. It also persists the refresh tokens using files service, which
are later used for fetching access tokens without a re-auth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.

Bug# https://github.com/domokit/mojo/issues/600

Related design doc: 
https://docs.google.com/document/d/1-LWlQl-9iInkeyuP0EoPhehDvyz68FCK0w_pfzZaZ...
==========

[rudominer, 2015-12-17 07:28:30]

I'm not done reviewing but here are a few comments.

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
File mojo/services/authentication/interfaces/authentication.mojom (right):

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
mojo/services/authentication/interfaces/authentication.mojom:31: // Requests an
oauth2 device code response for the given set of scopes. In
I think you need more comments that explain how to use these methods. If I
understand correctly, the combination of GetOAuth2DeviceCode() followed by
AddAccount() is to be used instead of using SelectAccount() when running on FNL.
SelectAccount() should only be used when running on Android and these two
methods should only be used when not running on Android. Is that right? If that
is not right please explain the relationship between the methods.

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
mojo/services/authentication/interfaces/authentication.mojom:32: // case of
error, response will be null and error will contain a description
s/response will be null/verifcation_url, device_code and user_code will be null/
or "all response parameters other than error will be null"

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
mojo/services/authentication/interfaces/authentication.mojom:34:
GetOAuth2DeviceCode(array<string> scopes) => (string? verification_url, string?
device_code, string? user_code, string? error);
break long line please.

[ukode, 2015-12-17 21:53:24]

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
File mojo/services/authentication/interfaces/authentication.mojom (right):

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
mojo/services/authentication/interfaces/authentication.mojom:31: // Requests an
oauth2 device code response for the given set of scopes. In
On 2015/12/17 07:28:30, rudominer wrote:
> I think you need more comments that explain how to use these methods. If I
> understand correctly, the combination of GetOAuth2DeviceCode() followed by
> AddAccount() is to be used instead of using SelectAccount() when running on
FNL.
> SelectAccount() should only be used when running on Android and these two
> methods should only be used when not running on Android. Is that right? If
that
> is not right please explain the relationship between the methods.

Thats right. Fixed the comments.

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
mojo/services/authentication/interfaces/authentication.mojom:32: // case of
error, response will be null and error will contain a description
On 2015/12/17 07:28:30, rudominer wrote:
> s/response will be null/verifcation_url, device_code and user_code will be
null/
> or "all response parameters other than error will be null"

Done.

https://codereview.chromium.org/1466733002/diff/180001/mojo/services/authenti...
mojo/services/authentication/interfaces/authentication.mojom:34:
GetOAuth2DeviceCode(array<string> scopes) => (string? verification_url, string?
device_code, string? user_code, string? error);
On 2015/12/17 07:28:30, rudominer wrote:
> break long line please.

Done.

[viettrungluu, 2015-12-18 20:59:48]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[viettrungluu, 2015-12-18 20:59:49]

(Note: Please send code reviews to viettrungluu@chromium.org -- I'm more likely
to notice them that way!)

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
File mojo/data_pipe_utils/data_pipe_utils.cc (right):

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:123: std::min(source.size(),
max_buffer_size)};
nit: Probably should explicitly static_cast<uint32_t>(...) this.

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:124: mojo::CreateDataPipe(&options,
&producer, &consumer);
FWIW, you can use the mojo::DataPipe helper class, which will save a few lines
of code.

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:126: return consumer.Pass();
The .Pass() shouldn't be necessary, I think?

(Though it might become necessary again if you use mojo::DataPipe....)

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
File mojo/data_pipe_utils/data_pipe_utils.h (right):

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.h:58: mojo::ScopedDataPipeConsumerHandle
WriteStringToConsumerHandle(
nit (also everywhere in the .cc file): The mojo:: qualifier isn't needed, since
you're already in the mojo namespace.

This function also needs a descriptive comment.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatin...
File
services/authenticating_url_loader_interceptor/authenticating_url_loader_interceptor_apptest.cc
(right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatin...
services/authenticating_url_loader_interceptor/authenticating_url_loader_interceptor_apptest.cc:105:

So, the style in this file appears to be to omit the blank line between things
that fall under the "AuthenticationService implementation" comment above (to
group them together).

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File services/authentication/BUILD.gn (right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/BUILD.gn:41: # "dummy_authentication_app.cc",
delete?

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/BUILD.gn:56: "//mojo/public/cpp/application:standalone",
If you're going to use //base, *probably* you should also use //mojo/application
instead (and the misleadingly-named ApplicationRunnerChromium).

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/accounts_db_manager.cc:27:
files->OpenFileSystem(nullptr, GetProxy(&directory_), Capture(&error));
I don't think you should block the thread (and even more so in the ctor).

(For the ctor, it seems like it'd be reasonable for it to just take a
DirectoryPtr, and leave it for the caller to provide it.)

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/accounts_db_manager.cc:98: Capture(&error));
In general, you shouldn't do synchronous I/O (i.e., blocking and waiting for the
response).

I'm also not sure why you need to open the file on each call, rather than just
keeping it open.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File services/authentication/google_authentication_impl.h (right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/google_authentication_impl.h:18: /**
nit: We generally don't use this comment style in C++ code.

[qsr, 2015-12-21 14:22:41]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-12-21 14:22:42]

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:253:
callback.call(null, null, null, "Unsupported operation.");
It seems that this CL introduce a parallel flow.

How are user of this API supposed to know which calls to make to obtain a token?

[rudominer, 2015-12-21 16:51:49]

rudominer@chromium.org changed reviewers:
- rudominer@chromium.org

[rudominer, 2015-12-21 16:52:46]

rudominer@chromium.org changed reviewers:
+ rudominer@chromium.org

[rudominer, 2015-12-21 16:52:46]

Removing myself from the reviewers because I think you have plenty of Mojo
feedback from qsr and vtl.

[rudominer, 2016-01-04 21:03:44]

rudominer@chromium.org changed reviewers:
- rudominer@chromium.org

[ukode, 2016-01-06 23:52:59]

PTAL.

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
File mojo/data_pipe_utils/data_pipe_utils.cc (right):

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:123: std::min(source.size(),
max_buffer_size)};
On 2015/12/18 20:59:49, viettrungluu wrote:
> nit: Probably should explicitly static_cast<uint32_t>(...) this.

Done.

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:124: mojo::CreateDataPipe(&options,
&producer, &consumer);
On 2015/12/18 20:59:49, viettrungluu wrote:
> FWIW, you can use the mojo::DataPipe helper class, which will save a few lines
> of code.

Done.

https://codereview.chromium.org/1466733002/diff/220001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:126: return consumer.Pass();
On 2015/12/18 20:59:49, viettrungluu wrote:
> The .Pass() shouldn't be necessary, I think?
> 
> (Though it might become necessary again if you use mojo::DataPipe....)

Yes.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatin...
File
services/authenticating_url_loader_interceptor/authenticating_url_loader_interceptor_apptest.cc
(right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatin...
services/authenticating_url_loader_interceptor/authenticating_url_loader_interceptor_apptest.cc:105:

On 2015/12/18 20:59:49, viettrungluu wrote:
> So, the style in this file appears to be to omit the blank line between things
> that fall under the "AuthenticationService implementation" comment above (to
> group them together).

Acknowledged.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File services/authentication/BUILD.gn (right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/BUILD.gn:41: # "dummy_authentication_app.cc",
On 2015/12/18 20:59:49, viettrungluu wrote:
> delete?

Done.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/BUILD.gn:56: "//mojo/public/cpp/application:standalone",
On 2015/12/18 20:59:49, viettrungluu wrote:
> If you're going to use //base, *probably* you should also use
//mojo/application
> instead (and the misleadingly-named ApplicationRunnerChromium).

Done.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/accounts_db_manager.cc:27:
files->OpenFileSystem(nullptr, GetProxy(&directory_), Capture(&error));
On 2015/12/18 20:59:49, viettrungluu wrote:
> I don't think you should block the thread (and even more so in the ctor).
> 
> (For the ctor, it seems like it'd be reasonable for it to just take a
> DirectoryPtr, and leave it for the caller to provide it.)

Good point. Made the change.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/accounts_db_manager.cc:98: Capture(&error));
On 2015/12/18 20:59:49, viettrungluu wrote:
> In general, you shouldn't do synchronous I/O (i.e., blocking and waiting for
the
> response).
> 
> I'm also not sure why you need to open the file on each call, rather than just
> keeping it open.

Moved the opening and closing files to ctor and dtor. In the future, thinking of
moving to a  simple database like LevelDB in place of files. Thoughts?

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File services/authentication/google_authentication_impl.h (right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/google_authentication_impl.h:18: /**
On 2015/12/18 20:59:49, viettrungluu wrote:
> nit: We generally don't use this comment style in C++ code.

Acknowledged.

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1466733002/diff/220001/services/authenticatio...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:253:
callback.call(null, null, null, "Unsupported operation.");
On 2015/12/21 14:22:42, qsr wrote:
> It seems that this CL introduce a parallel flow.
> 
> How are user of this API supposed to know which calls to make to obtain a
token?

GetOAuth2DeviceCode and AddAccount are the account manager apis for
bootstrapping an account on the device and persists a refresh token for each
account on the device. With Android these are already taken care by the Android
Account Manager built in framework. These are used only on devices that don't
have this capability such as Linux/IOS or FNL platforms. Once the account is
added, tokens are still retrieved using the same GetOAuth2Token api that
converts the underlying refresh tokens to access tokens and returned back to the
calling app.

[ukode, 2016-01-06 23:53:31]

On 2015/12/21 16:52:46, rudominer wrote:
> Removing myself from the reviewers because I think you have plenty of Mojo
> feedback from qsr and vtl.

Thanks Mitch for the initial review.

[viettrungluu, 2016-01-11 21:23:45]

Note that my comments about doing things asynchronously instead of synchronously
apply everywhere.

(These comments would still apply even if you were to talk to some simpler
database service instead of the files service.)

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
File examples/authentication_demo/google_authentication_demo.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:21: class
GoogleAuthApp : public mojo::ApplicationDelegate {
nit: Here and below: You probably don't need "mojo::" in most places.

That said, I'm not sure this should be in the mojo namespace at all. (Also, feel
free to use |using| statements at the file level.)

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:24: 
nit: Probably you should also explicitly declare a destructor.

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:26: LOG(INFO) <<
"Connecting to authentication service...";
You may want to consider using DLOG(INFO), VLOG, or DVLOG (rather than
unconditionally logging stuff even for Release builds).

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:55: // TODO: Replace
this with polling logic or show some spinner in System UI
nit: "TODO(ukode)" :)

https://codereview.chromium.org/1466733002/diff/240001/mojo/data_pipe_utils/d...
File mojo/data_pipe_utils/data_pipe_utils.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:118:
DCHECK_LE(static_cast<uint32_t>(source.size()), max_buffer_size);
Maybe make this a CHECK_LE instead of a DCHECK_LE (since it'll hang if it
fails)?

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:17: const char*
kAccountsDbFileName = "accounts_db.txt";
nit: This isn't as const as you think it is. You probably want |char
kAccountsDbFileName[] = "accounts_db.txt";|.

(Otherwise, |const char* const kAccountsDbFileName = ...;| would also be OK.)

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:21:
AccountsDbManager::AccountsDbManager(mojo::files::DirectoryPtr& directory) {
Depending on how this is used, this should either take a |DirectoryPtr| (not a
reference) -- if the caller doesn't want to hang on to it -- or a
|DirectoryPtr*|.

(In general, the Google C++ style forbids non-const references.)

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:24:
directory.Pass()->OpenFile(kAccountsDbFileName, GetProxy(&file_),
No |.Pass()| here.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:29:
directory.WaitForIncomingResponse();
Instead of waiting for the response (esp. since you just die anyway on failure),
maybe you should actually provide a callback that verifies success?

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:39:
file_.WaitForIncomingResponse();
Ditto here. Note that closing the file is optional. The file is effectively
closed if |file_| is destroyed. (The only extra utility is that calling
|Close()| gives some opportunity of reporting error, but it's not a big deal
since you just die anyway on failure.)

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:49: mojo::String&
new_db_contents) {
For this method, you should probably just return a |mojo::String| (which may be
null on error).

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:56: base::StringTokenizer
lines(AccountsDbManager::contents_, "\n");
(Here and below....) |AccountsDbManager::| not needed.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:60: if
(user_record.find(username) == std::string::npos) {
What's the format of contents_? Does this properly handle the case when one
username is a substring of another?

(Is there other data for which this may be problematic?)

In general, I'm not sure why you don't store the data in memory in an
already-parsed way, and only serialize/deserialize when writing to/reading from
the file.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:67: buffer += "\n";
Why is this needed? (It results in |buffer| beginning with a newline.)

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:72: return;
nit: no |return| needed here

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:82: ? true
|? true : false| is unnecessary.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:106:
file_.WaitForIncomingResponse();
You shouldn't do a blocking write like this. In general, you should handle this
asynchronously, which obviously requires propagating the asynchronousness to the
callers of this method as well.

I.e., you should start the request here and then pass a callback.

Note that doing a write synchronously here will bring all request processing to
a halt (since all requests are processed on your main thread).

(There are alternatives to doing things asynchronously, but would require, e.g.,
handling requests on worker threads or something like that.)

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:113:
AccountsDbManager::contents_.assign(new_db_contents.data(),
Keeping contents_ in sync with the actual contents may become more difficult if
you make things async.

That said, depending on the failure mode of Write(), contents_ may already be
wrong.

So you could just optimistically update contents_ and assume that Write()
succeeds.

[qsr, 2016-01-13 12:01:01]

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
File examples/authentication_demo/google_authentication_demo.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:55: // TODO: Replace
this with polling logic or show some spinner in System UI
Should the polling part be handled in the authentication service somehow? All
user of this API will need this, no?

https://codereview.chromium.org/1466733002/diff/240001/mojo/data_pipe_utils/d...
File mojo/data_pipe_utils/data_pipe_utils.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:122:
static_cast<uint32_t>(std::min(source.size(), max_buffer_size))};
once you change to a check, just use source.size()

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.h:65: explicit
AccountsDbManager(mojo::files::DirectoryPtr& directory);
const mojo::files::DirectoryPtr& ? or use a pointer.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.h:72: mojo::String& user_data);
You should use a pointer for out values.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.h:84: mojo::String&
new_db_contents);
Same for out parameters.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/dummy_authentication_app.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/dummy_authentication_app.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
I think this file is not used anymore.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/google_authentication_impl.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/google_authentication_impl.cc:145: void
GoogleAuthenticationServiceImpl::SelectAccount(
I don't understand how we will be able to use a single code path for FNL and
android if you do not implement this method.

Could you at least implement it when returnLastSelected is true and return the
last account that was added?

[ukode, 2016-02-09 00:20:18]

Sorry this took a while between juggling various things. Changelog for the
latest patch includes:

1. SelectAccount method now works with a default impl of picking the first
account from the database to the given app specified by the origin url. UI for
account selection picker will be added later. This includes implementing the
authentication_impl_db.mojom interface using by serializing contents to an auth
db file under app persistent cache dir.

2. Moved all the file operations to async mode instead of blocking calls.

3. Implemented polling for user authorization inside AddAccount interface by
calling itself recursively. Poll requests are separated by 7s intervals for
honoring server-side rate limits. And a maximum of 15 attempts can be performed
before timing out. So that implies, user has close to 1.5 minutes to authorize
on the secondary device. We will adjust the final settings later on, once
depending on the real usage.

4. Other minor nits.

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
File examples/authentication_demo/google_authentication_demo.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:21: class
GoogleAuthApp : public mojo::ApplicationDelegate {
On 2016/01/11 21:23:44, viettrungluu wrote:
> nit: Here and below: You probably don't need "mojo::" in most places.
> 
> That said, I'm not sure this should be in the mojo namespace at all. (Also,
feel
> free to use |using| statements at the file level.)

Acknowledged. Removed it from mojo namespace. mojo:: is still needed here,
otherwise get this compiler error:
error: unknown class name 'ApplicationDelegate'; did you mean
'mojo::ApplicationDelegate'?

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:24: 
On 2016/01/11 21:23:44, viettrungluu wrote:
> nit: Probably you should also explicitly declare a destructor.

Done.

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:26: LOG(INFO) <<
"Connecting to authentication service...";
On 2016/01/11 21:23:44, viettrungluu wrote:
> You may want to consider using DLOG(INFO), VLOG, or DVLOG (rather than
> unconditionally logging stuff even for Release builds).

Acknowledged.

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:55: // TODO: Replace
this with polling logic or show some spinner in System UI
On 2016/01/11 21:23:44, viettrungluu wrote:
> nit: "TODO(ukode)" :)

Done.

https://codereview.chromium.org/1466733002/diff/240001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:55: // TODO: Replace
this with polling logic or show some spinner in System UI
On 2016/01/13 12:01:00, qsr wrote:
> Should the polling part be handled in the authentication service somehow? All
> user of this API will need this, no?

Moved the polling logic under AddAccount interface. I did this recursively. PTAl
and confirm if this convention is ok with mojo codebase.

https://codereview.chromium.org/1466733002/diff/240001/mojo/data_pipe_utils/d...
File mojo/data_pipe_utils/data_pipe_utils.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:118:
DCHECK_LE(static_cast<uint32_t>(source.size()), max_buffer_size);
On 2016/01/11 21:23:44, viettrungluu wrote:
> Maybe make this a CHECK_LE instead of a DCHECK_LE (since it'll hang if it
> fails)?

Done.

https://codereview.chromium.org/1466733002/diff/240001/mojo/data_pipe_utils/d...
mojo/data_pipe_utils/data_pipe_utils.cc:122:
static_cast<uint32_t>(std::min(source.size(), max_buffer_size))};
On 2016/01/13 12:01:00, qsr wrote:
> once you change to a check, just use source.size()

Acknowledged.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:17: const char*
kAccountsDbFileName = "accounts_db.txt";
On 2016/01/11 21:23:44, viettrungluu wrote:
> nit: This isn't as const as you think it is. You probably want |char
> kAccountsDbFileName[] = "accounts_db.txt";|.
> 
> (Otherwise, |const char* const kAccountsDbFileName = ...;| would also be OK.)

Thanks for the tip. Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:21:
AccountsDbManager::AccountsDbManager(mojo::files::DirectoryPtr& directory) {
On 2016/01/11 21:23:45, viettrungluu wrote:
> Depending on how this is used, this should either take a |DirectoryPtr| (not a
> reference) -- if the caller doesn't want to hang on to it -- or a
> |DirectoryPtr*|.
> 
> (In general, the Google C++ style forbids non-const references.)

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:24:
directory.Pass()->OpenFile(kAccountsDbFileName, GetProxy(&file_),
On 2016/01/11 21:23:44, viettrungluu wrote:
> No |.Pass()| here.

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:29:
directory.WaitForIncomingResponse();
On 2016/01/11 21:23:44, viettrungluu wrote:
> Instead of waiting for the response (esp. since you just die anyway on
failure),
> maybe you should actually provide a callback that verifies success?

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:39:
file_.WaitForIncomingResponse();
On 2016/01/11 21:23:45, viettrungluu wrote:
> Ditto here. Note that closing the file is optional. The file is effectively
> closed if |file_| is destroyed. (The only extra utility is that calling
> |Close()| gives some opportunity of reporting error, but it's not a big deal
> since you just die anyway on failure.)

Ok,removing the file close functionality for now.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:49: mojo::String&
new_db_contents) {
On 2016/01/11 21:23:44, viettrungluu wrote:
> For this method, you should probably just return a |mojo::String| (which may
be
> null on error).

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:56: base::StringTokenizer
lines(AccountsDbManager::contents_, "\n");
On 2016/01/11 21:23:44, viettrungluu wrote:
> (Here and below....) |AccountsDbManager::| not needed.

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:60: if
(user_record.find(username) == std::string::npos) {
On 2016/01/11 21:23:44, viettrungluu wrote:
> What's the format of contents_? Does this properly handle the case when one
> username is a substring of another?
> 
> (Is there other data for which this may be problematic?)
> 
> In general, I'm not sure why you don't store the data in memory in an
> already-parsed way, and only serialize/deserialize when writing to/reading
from
> the file.

Good catch. Added necessary checks and removed file operations for the most part
except during initialization and any new commits.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:67: buffer += "\n";
On 2016/01/11 21:23:45, viettrungluu wrote:
> Why is this needed? (It results in |buffer| beginning with a newline.)

This is the line separator between records in the file. If there are no contents
to begin with then a newline is omitted through this check (never added to
buffer).

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:72: return;
On 2016/01/11 21:23:44, viettrungluu wrote:
> nit: no |return| needed here

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:82: ? true
On 2016/01/11 21:23:44, viettrungluu wrote:
> |? true : false| is unnecessary.

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:106:
file_.WaitForIncomingResponse();
On 2016/01/11 21:23:44, viettrungluu wrote:
> You shouldn't do a blocking write like this. In general, you should handle
this
> asynchronously, which obviously requires propagating the asynchronousness to
the
> callers of this method as well.
> 
> I.e., you should start the request here and then pass a callback.
> 
> Note that doing a write synchronously here will bring all request processing
to
> a halt (since all requests are processed on your main thread).
> 
> (There are alternatives to doing things asynchronously, but would require,
e.g.,
> handling requests on worker threads or something like that.)

Acknowledged. MOved to async method.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.cc:113:
AccountsDbManager::contents_.assign(new_db_contents.data(),
On 2016/01/11 21:23:44, viettrungluu wrote:
> Keeping contents_ in sync with the actual contents may become more difficult
if
> you make things async.
> 
> That said, depending on the failure mode of Write(), contents_ may already be
> wrong.
> 
> So you could just optimistically update contents_ and assume that Write()
> succeeds.

Acknowledged.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.h:65: explicit
AccountsDbManager(mojo::files::DirectoryPtr& directory);
On 2016/01/13 12:01:00, qsr wrote:
> const mojo::files::DirectoryPtr& ? or use a pointer.

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.h:72: mojo::String& user_data);
On 2016/01/13 12:01:00, qsr wrote:
> You should use a pointer for out values.

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/accounts_db_manager.h:84: mojo::String&
new_db_contents);
On 2016/01/13 12:01:00, qsr wrote:
> Same for out parameters.

Done.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/dummy_authentication_app.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/dummy_authentication_app.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/01/13 12:01:00, qsr wrote:
> I think this file is not used anymore.

Acknowledged. Removed it.

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
File services/authentication/google_authentication_impl.cc (right):

https://codereview.chromium.org/1466733002/diff/240001/services/authenticatio...
services/authentication/google_authentication_impl.cc:145: void
GoogleAuthenticationServiceImpl::SelectAccount(
On 2016/01/13 12:01:00, qsr wrote:
> I don't understand how we will be able to use a single code path for FNL and
> android if you do not implement this method.
> 
> Could you at least implement it when returnLastSelected is true and return the
> last account that was added?

Done. Implemented a primary account selection (the first account added to the
db) as part of SelectAccount method.

[qsr, 2016-02-16 14:17:06]

https://codereview.chromium.org/1466733002/diff/320001/examples/authenticatio...
File examples/authentication_demo/google_authentication_demo.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1466733002/diff/320001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:123:
::authentication::AuthenticationServicePtr auth_service_;
authentication_service_
(https://google.github.io/styleguide/cppguide.html#General_Naming_Rules) ?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/BUILD.gn (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/BUILD.gn:89: ":authentication($default_toolchain)",
Any reason you need to specify the toolchain here?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:67: while (lines.GetNext()) {
Please comment that you are removing the entry for user if it is present.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:70: if ((pos ==
std::string::npos) || (pos != 0)) {
std::string::npos will not be 0, so pos != 0 is enough.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:89: bool user_exists =
!GetAccountDataForUser(username).is_null();
This is parsing the string.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:92:
GetUpdatedDbContents(username, new_account_data, user_exists);
And this is doing it again.

Any reason the in-memory format is not a map. Wouldn't it be better to just
tranform it as a string before saving? I have the impression you still need to
do all the string manipulation anyway, but at least, with a map, in memory
manipulation will be more efficient.

If you want to still be able to send partial file update, you might want to have
both representation.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:125: delete this;
Return after the delete. Doesn't matter that much here, but if you ever add
something after the if, you will crash.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:157: creds_db_file_->Read(
Don't you have a race in this class? What happen if your methods are called
before this call (and the next one) finishes.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:171: void
AccountsDbManager::OnReadResponse(const mojo::files::Error error,
Maybe rename this. Response is not that clear when you have 2 different reads.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:196: const char* p =
reinterpret_cast<const char*>(&bytes_read[0]);
s/p/data/g?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:226:
bytes_to_write.push_back('\0');
Why this?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:23: explicit
AccountsDbManager(const mojo::files::DirectoryPtr directory);
Can you add some comment about those 2 constructors. When should the first one
be used?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:31: mojo::String
GetAllUserAccounts();
What is the return value?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:65: // Stores the cached account
data for all users.
What is the format of this string?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:68: authentication::Db
auth_grants;
auth_grants_

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/auth_data.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/auth_data.cc:14: const char kAuthDataSeparator = ',';
Why is it safe? Do we have guarantees that it will never be used in any of the
data you need in this class?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/auth_data.h (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/auth_data.h:14: struct AuthData {
You use a mojo struct for the tokens, any reason not to use one for this?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/authentication_impl_db.mojom (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/authentication_impl_db.mojom:5:
[DartPackage="mojo_services", JavaPackage="org.chromium.mojo.authentication"]
You should not need any of those. You do not want java or dart version of those
bindings.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/google_authentication_impl.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:85:
*static_cast<base::DictionaryValue*>(root.get()));
What is this for?
It seems you already have a dictionary value, why don't you return it? Why do
you need to transform everything in string?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:90:
base::ReplaceChars(val, "\"", "", &val);
Why isn't this done in ValueToString, for the String case, if necessary. Also,
in fact, why is this necessary at all? When do we expect to get " ?

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:107: accounts_db_manager_
= new AccountsDbManager(directory.Pass());
That looks like a leak.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:132: message +=
kUrlQueryParamSeparator;
You should have some helper method to generate query. Some day, a & will be in
your data and it will not be escaped -> use a method that takes a map and
generate the query, and make sure to espace what needs to be escaped

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/google_authentication_impl.h (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.h:102: AccountsDbManager*
accounts_db_manager_;
Your account db manager have a tendency to delete itself on all kind of errors.
How to we prevent crashes if that happens?

[ukode, 2016-02-26 21:35:50]

PTAL. Major change is now everything is using mojom structs instead of regular
struct, and serializing them directly to a file instead of a string first.

https://codereview.chromium.org/1466733002/diff/320001/examples/authenticatio...
File examples/authentication_demo/google_authentication_demo.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2016/02/16 14:17:05, qsr wrote:
> 2016

Done.

https://codereview.chromium.org/1466733002/diff/320001/examples/authenticatio...
examples/authentication_demo/google_authentication_demo.cc:123:
::authentication::AuthenticationServicePtr auth_service_;
On 2016/02/16 14:17:06, qsr wrote:
> authentication_service_
> (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules) ?

Done.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/BUILD.gn (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/BUILD.gn:89: ":authentication($default_toolchain)",
On 2016/02/16 14:17:06, qsr wrote:
> Any reason you need to specify the toolchain here?

Its implemented this way for all services, dont know the reason. Will find out
from Trung.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:67: while (lines.GetNext()) {
On 2016/02/16 14:17:06, qsr wrote:
> Please comment that you are removing the entry for user if it is present.

Had the same comment two lines above. Moved it down here.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:70: if ((pos ==
std::string::npos) || (pos != 0)) {
On 2016/02/16 14:17:06, qsr wrote:
> std::string::npos will not be 0, so pos != 0 is enough.

Acknowledged.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:89: bool user_exists =
!GetAccountDataForUser(username).is_null();
On 2016/02/16 14:17:06, qsr wrote:
> This is parsing the string.

Acknowledged.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:92:
GetUpdatedDbContents(username, new_account_data, user_exists);
On 2016/02/16 14:17:06, qsr wrote:
> And this is doing it again.
> 
> Any reason the in-memory format is not a map. Wouldn't it be better to just
> tranform it as a string before saving? I have the impression you still need to
> do all the string manipulation anyway, but at least, with a map, in memory
> manipulation will be more efficient.
> 
> If you want to still be able to send partial file update, you might want to
have
> both representation.

Got rid of this extra parsing completely,and directly serializing the mojom
credentials map object. The file will get rewritten, but I assume in the interim
it will be hardly 1-10 accounts per device, which is not a lot of disk IO.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:125: delete this;
On 2016/02/16 14:17:06, qsr wrote:
> Return after the delete. Doesn't matter that much here, but if you ever add
> something after the if, you will crash.

Done.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:157: creds_db_file_->Read(
On 2016/02/16 14:17:06, qsr wrote:
> Don't you have a race in this class? What happen if your methods are called
> before this call (and the next one) finishes.

The async reads happen very quickly and have enough time to complete before the
first read or write happens as the thread has to wait on user authorization on a
secondary device. 

Just to future-proof the code for not running into a race condition, I added a
simple flag(db_init_option_) that keeps track of the init state. Tested this
approach and it works fine. PTAL.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:171: void
AccountsDbManager::OnReadResponse(const mojo::files::Error error,
On 2016/02/16 14:17:06, qsr wrote:
> Maybe rename this. Response is not that clear when you have 2 different reads.

Acknowledged.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:196: const char* p =
reinterpret_cast<const char*>(&bytes_read[0]);
On 2016/02/16 14:17:06, qsr wrote:
> s/p/data/g?

Done.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.cc:226:
bytes_to_write.push_back('\0');
On 2016/02/16 14:17:06, qsr wrote:
> Why this?

The very first version of files service needed this for some reason. Can't
recollect now. I tried it by removing and it works fine.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:23: explicit
AccountsDbManager(const mojo::files::DirectoryPtr directory);
On 2016/02/16 14:17:06, qsr wrote:
> Can you add some comment about those 2 constructors. When should the first one
> be used?

Got rid of one constructor.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:31: mojo::String
GetAllUserAccounts();
On 2016/02/16 14:17:06, qsr wrote:
> What is the return value?

Made this to return an array of usernames instead. Pls check teh refactored
code.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:65: // Stores the cached account
data for all users.
On 2016/02/16 14:17:06, qsr wrote:
> What is the format of this string?

removed this completely. Made it a credentials db struct.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/accounts_db_manager.h:68: authentication::Db
auth_grants;
On 2016/02/16 14:17:06, qsr wrote:
> auth_grants_

Done.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/auth_data.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/auth_data.cc:14: const char kAuthDataSeparator = ',';
On 2016/02/16 14:17:06, qsr wrote:
> Why is it safe? Do we have guarantees that it will never be used in any of the
> data you need in this class?

Got rid of this completely. Instead using a mojom struct.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/auth_data.h (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/auth_data.h:14: struct AuthData {
On 2016/02/16 14:17:06, qsr wrote:
> You use a mojo struct for the tokens, any reason not to use one for this?

Done.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/authentication_impl_db.mojom (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/authentication_impl_db.mojom:5:
[DartPackage="mojo_services", JavaPackage="org.chromium.mojo.authentication"]
On 2016/02/16 14:17:06, qsr wrote:
> You should not need any of those. You do not want java or dart version of
those
> bindings.

Java version is still used by Android auth impl. And I needed the Dart binding
as well, otherwise presubmit is throwing errors. Code compiles and runs fine
without this, but presubmit fails.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/google_authentication_impl.cc (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:85:
*static_cast<base::DictionaryValue*>(root.get()));
On 2016/02/16 14:17:06, qsr wrote:
> What is this for?
> It seems you already have a dictionary value, why don't you return it? Why do
> you need to transform everything in string?

Some of the response values are ints, doubles and strings. Doing the type
conversion everywhere in the code is cumbersome. Here I am converting everything
to string which gets used accordingly later in the code..

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:90:
base::ReplaceChars(val, "\"", "", &val);
On 2016/02/16 14:17:06, qsr wrote:
> Why isn't this done in ValueToString, for the String case, if necessary. Also,
> in fact, why is this necessary at all? When do we expect to get " ?

ValueToString is a very basic method that returns a string representation of
resultant vals. Overloading ValueToString with specific decoding details doesn't
feel right. I added a new EncodeParam and DecodeParam helper methods that does
escaping and unescaping keys as well. 

All Google OAuth endpoints return the response in the following format. So we
need to get rid of extra "" for string return values (its not automatically done
by Json parser :( :

{
  "device_code" : "4/4-GMMhmHCXhWEzkobqIHGG_EnNYYsAkukHspeYUk9E8",
  "user_code" : "GQVQ-JKEC",
  "verification_url" : "https://www.google.com/device",
  "expires_in" : 1800,
  "interval" : 5
}

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:107: accounts_db_manager_
= new AccountsDbManager(directory.Pass());
On 2016/02/16 14:17:06, qsr wrote:
> That looks like a leak.

Fixed it.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.cc:132: message +=
kUrlQueryParamSeparator;
On 2016/02/16 14:17:06, qsr wrote:
> You should have some helper method to generate query. Some day, a & will be in
> your data and it will not be escaped -> use a method that takes a map and
> generate the query, and make sure to espace what needs to be escaped

Acknowledged. Added one above.

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
File services/authentication/google_authentication_impl.h (right):

https://codereview.chromium.org/1466733002/diff/320001/services/authenticatio...
services/authentication/google_authentication_impl.h:102: AccountsDbManager*
accounts_db_manager_;
On 2016/02/16 14:17:06, qsr wrote:
> Your account db manager have a tendency to delete itself on all kind of
errors.
> How to we prevent crashes if that happens?

Made it as a weak ptr, and checking if it still exists before every operation.

[qsr, 2016-03-04 15:06:46]

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:88: it !=
creds_store_.credentials.end(); it++) {
Why not use a for each loop? Doesn't it work on mojo maps?

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:105: creds_store_.version =
kCredsDbVersion;
This should be done during init.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:153: auth_grants_.version =
kAuthDbVersion;
Why is this necessary? Shouldn't you do this when reading the file, if the
number of bytes is 0?

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:184:
CHECK_EQ(creds_store_.version, kCredsDbVersion);
Maybe put a comment that when we have multiple version, this is not a fatal
error, but a sign that we need to update (or reinitialize) the db.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:185:
CHECK(creds_store_.credentials.size() > 0);
Why do you have this check? Isn't it possible that you remove a credential and
still save the file?

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:203:
auth_grants_.Deserialize((void*)data);
You want to use Validate before calling Deserialize. See:
services/url_response_disk_cache/url_response_disk_cache_db.cc
for code that validate the data before trying the deserialization.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:232: char buf[buf_size];
Why not directly creating a mojo::Array<uint8_t> of the right size and writing
to it?

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.h:29:
base::WeakPtr<AccountsDbManager> GetWeakPtr();
This still seems kind of weird. Now the callers use this weak ptr because this
class will delete itself in case of error, but the callers cannot delete it
anymore, so the instance will leak.

All in all, I do not think this is the correct way to handle this. You might
want to have error output on your public method, and put yourself in a mode
where you only return errors in case of error, or something like that. This
makes handling the lifecycle of this class easier.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.h:57: void
OnCredFileReadResponse(const mojo::files::Error error,
Do not use abbreviation, here and everywhere.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/credentials_impl_db.mojom (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/credentials_impl_db.mojom:14:
DOWNSCOPED_OAUTH_REFRESH_TOKEN
Can you remove everything we do not use?

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/credentials_impl_db.mojom:20: TWITTER
Same here.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/google_authentication_impl.cc (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/google_authentication_impl.cc:124:
dict->SetString(DecodeParam(it.key()), DecodeParam(val));
I do not understand why you need this. Why not keep the dictionnary as it is and
only do the decoding you need when you need it.

Especially because you know what type you will get.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/google_authentication_impl.cc:401:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(7000));
You are blocking your thread there. Can there be parallel queries that you are
also blocking? Should you use some asynchronous waiting by posting delayed
tasks?

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:254:
return;
Could you remove the return. Having this as the last line of a method doesn't do
anything.

[ukode, 2016-03-11 22:48:52]

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:88: it !=
creds_store_.credentials.end(); it++) {
On 2016/03/04 15:06:45, qsr wrote:
> Why not use a for each loop? Doesn't it work on mojo maps?

I get this error on using for each: indirection requires pointer operand
('mojo::Map<mojo::String, mojo::InlinedStructPtr<authentication::Credentials>
>::InternalIterator<mojo::Map<mojo::String,
mojo::InlinedStructPtr<authentication::Credentials>
>::IteratorMutability::kMutable>' invalid)
  for (auto& pair : creds_store_.credentials) {
                  ^

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:105: creds_store_.version =
kCredsDbVersion;
On 2016/03/04 15:06:45, qsr wrote:
> This should be done during init.

Done.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:153: auth_grants_.version =
kAuthDbVersion;
On 2016/03/04 15:06:45, qsr wrote:
> Why is this necessary? Shouldn't you do this when reading the file, if the
> number of bytes is 0?

Done, moved it below inside file read operation.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:184:
CHECK_EQ(creds_store_.version, kCredsDbVersion);
On 2016/03/04 15:06:45, qsr wrote:
> Maybe put a comment that when we have multiple version, this is not a fatal
> error, but a sign that we need to update (or reinitialize) the db.

Acknowledged.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:185:
CHECK(creds_store_.credentials.size() > 0);
On 2016/03/04 15:06:45, qsr wrote:
> Why do you have this check? Isn't it possible that you remove a credential and
> still save the file?

Good catch. Yes, that's possible. Removed the check and instead added a validate
earlier.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:203:
auth_grants_.Deserialize((void*)data);
On 2016/03/04 15:06:45, qsr wrote:
> You want to use Validate before calling Deserialize. See:
> services/url_response_disk_cache/url_response_disk_cache_db.cc
> for code that validate the data before trying the deserialization.

Done.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.cc:232: char buf[buf_size];
On 2016/03/04 15:06:45, qsr wrote:
> Why not directly creating a mojo::Array<uint8_t> of the right size and writing
> to it?

Done.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.h:29:
base::WeakPtr<AccountsDbManager> GetWeakPtr();
On 2016/03/04 15:06:45, qsr wrote:
> This still seems kind of weird. Now the callers use this weak ptr because this
> class will delete itself in case of error, but the callers cannot delete it
> anymore, so the instance will leak.
> 
> All in all, I do not think this is the correct way to handle this. You might
> want to have error output on your public method, and put yourself in a mode
> where you only return errors in case of error, or something like that. This
> makes handling the lifecycle of this class easier.

Acknowledged. Removed the weak ptr and exposed a new Validate method that checks
for current errors. The callers would now be checking using validate method
before invoking any operations.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/accounts_db_manager.h:57: void
OnCredFileReadResponse(const mojo::files::Error error,
On 2016/03/04 15:06:45, qsr wrote:
> Do not use abbreviation, here and everywhere.

Done.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/credentials_impl_db.mojom (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/credentials_impl_db.mojom:14:
DOWNSCOPED_OAUTH_REFRESH_TOKEN
On 2016/03/04 15:06:46, qsr wrote:
> Can you remove everything we do not use?

Done.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/credentials_impl_db.mojom:20: TWITTER
On 2016/03/04 15:06:45, qsr wrote:
> Same here.

Done.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File services/authentication/google_authentication_impl.cc (right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/google_authentication_impl.cc:124:
dict->SetString(DecodeParam(it.key()), DecodeParam(val));
On 2016/03/04 15:06:46, qsr wrote:
> I do not understand why you need this. Why not keep the dictionnary as it is
and
> only do the decoding you need when you need it.
> 
> Especially because you know what type you will get.

Done. Agree, code looks a lot simpler. Earlier with manual conversions, I have
to decode more stuff but I thought everything is done at one place. Now, it is
upto the caller only when needed removing the extra overhead.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/google_authentication_impl.cc:401:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(7000));
On 2016/03/04 15:06:46, qsr wrote:
> You are blocking your thread there. Can there be parallel queries that you are
> also blocking? Should you use some asynchronous waiting by posting delayed
> tasks?

Done. Moved it to using PostDelayedTask.

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
File
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java
(right):

https://codereview.chromium.org/1466733002/diff/360001/services/authenticatio...
services/authentication/src/org/chromium/mojo/authentication/AuthenticationServiceImpl.java:254:
return;
On 2016/03/04 15:06:46, qsr wrote:
> Could you remove the return. Having this as the last line of a method doesn't
do
> anything.

Done.

[qsr, 2016-03-18 10:52:23]

lgtm

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
services/authentication/accounts_db_manager.cc:127: error_ =
CREDENTIALS_DB_WRITE_ERROR;
Maybe CHECK or DCHECK or error at the start of every function.

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
services/authentication/accounts_db_manager.h:29: bool Validate();
Maybe called this isValid() instead? Validate seems to imply some action.

[ukode, 2016-03-18 18:25:24]

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
File services/authentication/accounts_db_manager.cc (right):

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
services/authentication/accounts_db_manager.cc:127: error_ =
CREDENTIALS_DB_WRITE_ERROR;
On 2016/03/18 10:52:22, qsr wrote:
> Maybe CHECK or DCHECK or error at the start of every function.

Done.

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
File services/authentication/accounts_db_manager.h (right):

https://codereview.chromium.org/1466733002/diff/400001/services/authenticatio...
services/authentication/accounts_db_manager.h:29: bool Validate();
On 2016/03/18 10:52:22, qsr wrote:
> Maybe called this isValid() instead? Validate seems to imply some action.

Done.

[ukode, 2016-03-18 21:18:58]

Description was changed from

==========
Added support for adding Google accounts on platforms other than Android such as
FNL or Goobuntu. It also persists the refresh tokens using files service, which
are later used for fetching access tokens without a re-auth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.

Bug# https://github.com/domokit/mojo/issues/600

Related design doc: 
https://docs.google.com/document/d/1-LWlQl-9iInkeyuP0EoPhehDvyz68FCK0w_pfzZaZ...
==========

to

==========
Added support for adding Google accounts on platforms other than Android such as
FNL or Goobuntu. It also persists the refresh tokens using files service, which
are later used for fetching access tokens without a re-auth.

Also, retrieves both GaiaID and user's email using OpenIDConnect based idtokens.

Bug# https://github.com/domokit/mojo/issues/600

Related design doc:
https://docs.google.com/document/d/1-LWlQl-9iInkeyuP0EoPhehDvyz68FCK0w_pfzZaZ...

R=qsr@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/1195c66530da45464688fb251a1...
==========

[ukode, 2016-03-18 21:19:01]

Committed patchset #24 (id:450001) manually as
1195c66530da45464688fb251a1610155e5e089d (presubmit successful).