Add a mojo interface for AccountId

Add a mojo interface for AccountId

And use it for SessionController interface.

BUG=648964
Review-Url: https://codereview.chromium.org/2617763003
Cr-Commit-Position: refs/heads/master@{#443356}
Committed: https://chromium.googlesource.com/chromium/src/+/215f82a62a105c92faa42f068a4cfecdb63d46d1

[xiyuan, 2017-01-05 23:29:15]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:29:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-01-05 23:36:27]

xiyuan@chromium.org changed reviewers:
+ jamescook@chromium.org, tsepez@chromium.org

[xiyuan, 2017-01-05 23:36:27]

jamescook@, please drive the review.

tsepez@, please review the mojo files
  components/signin/public/interfaces/*

Thanks.

[Tom Sepez, 2017-01-05 23:53:28]

https://codereview.chromium.org/2617763003/diff/1/components/signin/public/in...
File components/signin/public/interfaces/account_id_traits.h (right):

https://codereview.chromium.org/2617763003/diff/1/components/signin/public/in...
components/signin/public/interfaces/account_id_traits.h:25: if
(!AccountId::Deserialize(serialized, out))
Is this ever called on the browser side or in another privileged context?  If
so, it has to be bullet-proof, and right now this is using the base JSON parser,
which we generally don't trust.  I thought there was a safe JSON package that
shipped this off to a utility process.  Can we use that here?

[Tom Sepez, 2017-01-05 23:53:54]

Description was changed from

==========
Add a mojo interface for AccountId

And use it for SessionController interface.

BUG=648964
==========

to

==========
Add a mojo interface for AccountId

And use it for SessionController interface.

BUG=648964
==========

[Tom Sepez, 2017-01-05 23:53:54]

tsepez@chromium.org changed reviewers:
+ jschuh@chromium.org

[commit-bot: I haz the power, 2017-01-05 23:56:58]

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

[commit-bot: I haz the power, 2017-01-05 23:56:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2017-01-06 00:22:48]

https://codereview.chromium.org/2617763003/diff/1/components/signin/public/in...
File components/signin/public/interfaces/account_id_traits.h (right):

https://codereview.chromium.org/2617763003/diff/1/components/signin/public/in...
components/signin/public/interfaces/account_id_traits.h:25: if
(!AccountId::Deserialize(serialized, out))
On 2017/01/05 23:53:28, Tom Sepez wrote:
> Is this ever called on the browser side or in another privileged context?  If
> so, it has to be bullet-proof, and right now this is using the base JSON
parser,
> which we generally don't trust.  I thought there was a safe JSON package that
> shipped this off to a utility process.  Can we use that here?

This runs in browser and (ash process in mash). Currently we are passing
serialized account id as a string and call AccountId::Deserialize to translate
that back to an AccountId. This CL wraps up that logic into typemapping and
should not make it less safe. What is the main concern? Some one would inject
data into a mojo channel and cause us to crash? If that is the case, how could
passing a string or a SkBitmap through mojo is safe?

Not sure safe JSON is good to use here because of its asynchronous nature. The
parsed value is obtained via a callback and I don't know whether we should hold
the Read function to wait for the callback.

[xiyuan, 2017-01-06 16:41:57]

https://codereview.chromium.org/2617763003/diff/1/components/signin/public/in...
File components/signin/public/interfaces/account_id_traits.h (right):

https://codereview.chromium.org/2617763003/diff/1/components/signin/public/in...
components/signin/public/interfaces/account_id_traits.h:25: if
(!AccountId::Deserialize(serialized, out))
On 2017/01/06 00:22:47, xiyuan wrote:
> On 2017/01/05 23:53:28, Tom Sepez wrote:
> > Is this ever called on the browser side or in another privileged context? 
If
> > so, it has to be bullet-proof, and right now this is using the base JSON
> parser,
> > which we generally don't trust.  I thought there was a safe JSON package
that
> > shipped this off to a utility process.  Can we use that here?
> 
> This runs in browser and (ash process in mash). Currently we are passing
> serialized account id as a string and call AccountId::Deserialize to translate
> that back to an AccountId. This CL wraps up that logic into typemapping and
> should not make it less safe. What is the main concern? Some one would inject
> data into a mojo channel and cause us to crash? If that is the case, how could
> passing a string or a SkBitmap through mojo is safe?
> 
> Not sure safe JSON is good to use here because of its asynchronous nature. The
> parsed value is obtained via a callback and I don't know whether we should
hold
> the Read function to wait for the callback.

Let me change signin.mojom.AccountId to a similar struct of real AccountId. This
way we could rely on mojo's serialization and avoid the unsafe JSON parser.

[James Cook, 2017-01-06 17:06:56]

xiyuan, should I look now or wait?

[xiyuan, 2017-01-06 17:15:37]

On 2017/01/06 17:06:56, James Cook wrote:
> xiyuan, should I look now or wait?

Sorry. Please hold on review until I finish the mojo file change.

[xiyuan, 2017-01-06 18:05:17]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 18:05:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-01-06 18:09:20]

Changed to use mojo serialization instead of the JSON based
AccountID::Serialize/Deserialize.

Ready for review. Thanks.

[commit-bot: I haz the power, 2017-01-06 18:11:50]

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

[commit-bot: I haz the power, 2017-01-06 18:11:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[xiyuan, 2017-01-06 18:24:04]

Patchset #3 (id:40001) has been deleted

[xiyuan, 2017-01-06 18:42:14]

Sorry for uploading another patch to fix deps.

account_id_traits.h is changed to use existing public AccountId methods instead
of being a friend to fix deps.

[xiyuan, 2017-01-06 18:46:29]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 18:46:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2017-01-06 19:35:23]

lgtm

[commit-bot: I haz the power, 2017-01-06 20:30:19]

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

[commit-bot: I haz the power, 2017-01-06 20:30:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2017-01-06 20:54:32]

LGTM

https://codereview.chromium.org/2617763003/diff/60001/components/signin/publi...
File components/signin/public/interfaces/account_id_traits.h (right):

https://codereview.chromium.org/2617763003/diff/60001/components/signin/publi...
components/signin/public/interfaces/account_id_traits.h:61: break;
Question: So trying to get an id() of an unknown account type is an error (hence
NOTREACHED below)?

[xiyuan, 2017-01-06 21:49:35]

https://codereview.chromium.org/2617763003/diff/60001/components/signin/publi...
File components/signin/public/interfaces/account_id_traits.h (right):

https://codereview.chromium.org/2617763003/diff/60001/components/signin/publi...
components/signin/public/interfaces/account_id_traits.h:61: break;
On 2017/01/06 20:54:32, James Cook wrote:
> Question: So trying to get an id() of an unknown account type is an error
(hence
> NOTREACHED below)?

Good question. And we actually have use of such AccountIds (contrary to my
believing). The failed tests hit that. Those are users with only emails that we
use in tests. And we probably also use them for legacy users that has not
migrated to new code (that populate them with their gaia ids).

Changed this to return empty string for such users. And updated Read function
below to allow UNKNOWN type AccountId to be constructed with only user_email or
gaia id. Also added a comment to doc it.

[xiyuan, 2017-01-06 21:49:40]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 21:50:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-01-06 21:56:46]

xiyuan@chromium.org changed reviewers:
+ blundell@chromium.org, msarda@chromium.org

[xiyuan, 2017-01-06 21:56:47]

msarda@, please do owner's review for
  components/signin/*
since rogerta is ooo.

blundell@, please do owner's review for
  components/typemaps.gni

alemate@, fyi and you are welcome to review it as well.

Thanks.

[James Cook, 2017-01-06 21:57:41]

still lgtm

[commit-bot: I haz the power, 2017-01-06 22:45:34]

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

[commit-bot: I haz the power, 2017-01-06 22:45:35]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-01-09 10:28:50]

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
components/signin/core/account_id/account_id.h:27: AccountId();
Why is this change needed?

[blundell, 2017-01-09 12:34:52]

//components/typemaps.gni lgtm

[xiyuan, 2017-01-09 17:42:57]

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
components/signin/core/account_id/account_id.h:27: AccountId();
On 2017/01/09 10:28:50, msarda wrote:
> Why is this change needed?

This is needed for mojo deserialization. The code needs to create an AccoundId
instance using default ctor and pass it as output pointer to StructTraits::Read.

Think the original intention to make the default ctor private is to force using
one of the static FromXXXX method to construct an AccountId. This move does not
change that. The default ctor creates an invalid/emtpy AccountId (is_valid()
returns false for such instance). So any caller still has to fill it up with
proper data.

We probably can get rid of EmptyAccountId now after this move since the public
default ctor would provide the same capability. If you want, I can create a
follow up CL to get rid of that.

[msarda, 2017-01-10 15:04:22]

lgtm

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
components/signin/core/account_id/account_id.h:27: AccountId();
On 2017/01/09 17:42:57, xiyuan wrote:
> On 2017/01/09 10:28:50, msarda wrote:
> > Why is this change needed?
> 
> This is needed for mojo deserialization. The code needs to create an AccoundId
> instance using default ctor and pass it as output pointer to
StructTraits::Read.
> 
> Think the original intention to make the default ctor private is to force
using
> one of the static FromXXXX method to construct an AccountId. This move does
not
> change that. The default ctor creates an invalid/emtpy AccountId (is_valid()
> returns false for such instance). So any caller still has to fill it up with
> proper data.
> 
> We probably can get rid of EmptyAccountId now after this move since the public
> default ctor would provide the same capability. If you want, I can create a
> follow up CL to get rid of that.

Thank you for the explanation. I am fine with us adding this constructor - I
think having the empty account id is also interesting in the log run.

May I ask you to add a comment about this constructor like:
// Creates an empty account id.
//
// Note: This constructor is public as it is required for mojo
// serialization. To create an AccountId object, prefer using the
// static FromXXXX methods or the EmptyAccountId method when
// creating an empty account id.

[xiyuan, 2017-01-12 00:06:54]

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
File components/signin/core/account_id/account_id.h (right):

https://codereview.chromium.org/2617763003/diff/80001/components/signin/core/...
components/signin/core/account_id/account_id.h:27: AccountId();
On 2017/01/10 15:04:22, msarda wrote:
> On 2017/01/09 17:42:57, xiyuan wrote:
> > On 2017/01/09 10:28:50, msarda wrote:
> > > Why is this change needed?
> > 
> > This is needed for mojo deserialization. The code needs to create an
AccoundId
> > instance using default ctor and pass it as output pointer to
> StructTraits::Read.
> > 
> > Think the original intention to make the default ctor private is to force
> using
> > one of the static FromXXXX method to construct an AccountId. This move does
> not
> > change that. The default ctor creates an invalid/emtpy AccountId (is_valid()
> > returns false for such instance). So any caller still has to fill it up with
> > proper data.
> > 
> > We probably can get rid of EmptyAccountId now after this move since the
public
> > default ctor would provide the same capability. If you want, I can create a
> > follow up CL to get rid of that.
> 
> Thank you for the explanation. I am fine with us adding this constructor - I
> think having the empty account id is also interesting in the log run.
> 
> May I ask you to add a comment about this constructor like:
> // Creates an empty account id.
> //
> // Note: This constructor is public as it is required for mojo
> // serialization. To create an AccountId object, prefer using the
> // static FromXXXX methods or the EmptyAccountId method when
> // creating an empty account id.

Done.

[xiyuan, 2017-01-12 19:26:48]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-01-12 19:26:48]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jamescook@chromium.org, blundell@chromium.org, msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2617763003/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-12 19:27:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 21:00:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484249208343440,
"parent_rev": "ca6a69d41a70cf1fadb286ffe84321c2c90e2995", "commit_rev":
"215f82a62a105c92faa42f068a4cfecdb63d46d1"}

[commit-bot: I haz the power, 2017-01-12 21:01:11]

Description was changed from

==========
Add a mojo interface for AccountId

And use it for SessionController interface.

BUG=648964
==========

to

==========
Add a mojo interface for AccountId

And use it for SessionController interface.

BUG=648964

Review-Url: https://codereview.chromium.org/2617763003
Cr-Commit-Position: refs/heads/master@{#443356}
Committed:
https://chromium.googlesource.com/chromium/src/+/215f82a62a105c92faa42f068a4c...
==========

[commit-bot: I haz the power, 2017-01-12 21:01:12]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/215f82a62a105c92faa42f068a4c...