Principal Service: Add support for multiple user accounts

Principal Service: Add support for multiple user accounts

This Cl enhances the Mojo Principal Service so that it
supports multiple user identities for each app. It offers
two new methods
- GetUsers to get all the user identities for the app
- SetCurrentUser to set the current user identity for the app

R=ashankar@google.com, gauthamt@google.com, ukode@google.com

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

[ataly, 2015-10-22 01:47:05]

Description was changed from

==========
Principal Service: Add support for multiple user accounts

This Cl enhances the Mojo Principal Service so that it
supports multiple user identities for each app. It offers
two new methods
- GetUsers to get all the user identities for the app
- SetCurrentUser to set the current user identity for the app
==========

to

==========
Principal Service: Add support for multiple user accounts

This Cl enhances the Mojo Principal Service so that it
supports multiple user identities for each app. It offers
two new methods
- GetUsers to get all the user identities for the app
- SetCurrentUser to set the current user identity for the app
==========

[ataly, 2015-10-22 01:47:05]

ataly@google.com changed reviewers:
+ gauthamt@google.com, ukode@google.com

[ukode, 2015-10-22 20:19:01]

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:18: struct UserId {
Is UserInfo/UserData or "User" a better option, given this could be extended in
the future?

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:63: // obtained by
invoking GetUserBlessing().
nit: Good to add a TODO here on how/when the blessing becomes invalid during
account change events such as change password or user being disabled or deleted
in the future for abuse reasons.

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:67: Login() =>
(UserId? user_id);
On second thought, wondering if we can set the current User identity bit in this
call as well, as the app is requesting for a fresh login anyways. Thoughts? and
setCurrentUser provides the calling app additional support for switching users,
if needed.

Otherwise in normal cases, each app needs to make two auth calls (Login()
followed by setCurrentUser()) which is not really required for a majority of
usecases.

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:83: // Logout sets
the current user identity of the calling application instance
Does logout also delete the entry in Principal Service DB or disabling the cache
entry in the underlying platform?

https://codereview.chromium.org/1418013004/diff/120001/services/vanadium/secu...
File services/vanadium/security/principal_service.go (right):

https://codereview.chromium.org/1418013004/diff/120001/services/vanadium/secu...
services/vanadium/security/principal_service.go:30: tokenInfoURL =
"https://www.googleapis.com/oauth2/v1/tokeninfo"
Is this URL right. I thought they moved to v2 or v3
(https://www.googleapis.com/oauth2/v3/tokeninfo) for the latest iteration.

[ataly, 2015-10-23 21:14:53]

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:18: struct UserId {
On 2015/10/22 20:19:01, ukode wrote:
> Is UserInfo/UserData or "User" a better option, given this could be extended
in
> the future?

Done, went with 'User'

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:63: // obtained by
invoking GetUserBlessing().
Actually this particular blessing would not invalidate when the user disables
the account, changes passwords etc. The reason is that these blessings do not
carry any caveats. Any caveats obtained on the blessing received from the
Vanadium IDP are stripped out before exposing them to Mojo applications.

Having said that, I plan to change this code soon to make mojo blessings "real",
i.e., usable within Syncbase and Vanadium RPCs in general. I added a TODO on the
blessings type to that effect. We had this simplified definition in the initial
principal service implementaiton mainly because at the time there was no real
use-case for blessings and they were simply used as user identifiers.

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:67: Login() =>
(UserId? user_id);
Login does set the new identity as the current identity for the app. I mention
this in the last sentence of the first paragraph in the comment. Did you want
Login to take a bool indicating whether the new identity should be made current
or not?

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:83: // Logout sets
the current user identity of the calling application instance
No. Logout does not delete the user entry. It simply clears the "current user
identity" field for the app. A call to GetUsers before and after a Logout call
would only differ in current user identity returned. After Logout the current
user would be null.

https://codereview.chromium.org/1418013004/diff/120001/services/vanadium/secu...
File services/vanadium/security/principal_service.go (right):

https://codereview.chromium.org/1418013004/diff/120001/services/vanadium/secu...
services/vanadium/security/principal_service.go:30: tokenInfoURL =
"https://www.googleapis.com/oauth2/v1/tokeninfo"
Oh yes, thanks for pointing this out. At the moment, I am not invoking tokenInfo
so I got rid of this const. I obtain the email address from the blessing
obtained from the Vanadium IDP.

[ataly, 2015-10-23 22:40:47]

ataly@google.com changed reviewers:
+ ashankar@google.com

[ukode, 2015-10-23 22:45:44]

LGTM.

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:63: // obtained by
invoking GetUserBlessing().
On 2015/10/23 21:14:53, ataly wrote:
> Actually this particular blessing would not invalidate when the user disables
> the account, changes passwords etc. The reason is that these blessings do not
> carry any caveats. Any caveats obtained on the blessing received from the
> Vanadium IDP are stripped out before exposing them to Mojo applications.
> 
> Having said that, I plan to change this code soon to make mojo blessings
"real",
> i.e., usable within Syncbase and Vanadium RPCs in general. I added a TODO on
the
> blessings type to that effect. We had this simplified definition in the
initial
> principal service implementaiton mainly because at the time there was no real
> use-case for blessings and they were simply used as user identifiers.
> 

Acknowledged. Thanks for the clarification.

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:67: Login() =>
(UserId? user_id);
On 2015/10/23 21:14:53, ataly wrote:
> Login does set the new identity as the current identity for the app. I mention
> this in the last sentence of the first paragraph in the comment. Did you want
> Login to take a bool indicating whether the new identity should be made
current
> or not?

This looks good. We don't need a bool again.

https://codereview.chromium.org/1418013004/diff/120001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:83: // Logout sets
the current user identity of the calling application instance
On 2015/10/23 21:14:53, ataly wrote:
> No. Logout does not delete the user entry. It simply clears the "current user
> identity" field for the app. A call to GetUsers before and after a Logout call
> would only differ in current user identity returned. After Logout the current
> user would be null.

For now this looks ok, but in the future, we should separate out the account
credential management (add account, update account, delete account) with the
downscoping or signing into app calls as discussed offline.

https://codereview.chromium.org/1418013004/diff/120001/services/vanadium/secu...
File services/vanadium/security/principal_service.go (right):

https://codereview.chromium.org/1418013004/diff/120001/services/vanadium/secu...
services/vanadium/security/principal_service.go:30: tokenInfoURL =
"https://www.googleapis.com/oauth2/v1/tokeninfo"
On 2015/10/23 21:14:53, ataly wrote:
> Oh yes, thanks for pointing this out. At the moment, I am not invoking
tokenInfo
> so I got rid of this const. I obtain the email address from the blessing
> obtained from the Vanadium IDP.

Thanks for the clean up.

[ukode, 2015-10-23 22:46:21]

lgtm

[gautham, 2015-10-26 20:52:01]

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:62: // obtained by
invoking GetUserBlessing().
remove ()

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:80:
SetCurrentUser(User user) => (string? error);
I know we discussed this. I'm wondering whether we could fold this into the
Login user interface, for eg: similar to the android account selector we pop up
a set of users and ask the client to select one or add a new user.

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:98:
GetUserBlessing(AppInstanceName? app) => (Blessing? user_blessing);
Any reason why User, Blessing are not part of the same struct?. I'd rather not
have separate interfaces for GetUser/GetUserBlessing and GetUsers/GetBlessings.

https://codereview.chromium.org/1418013004/diff/140001/services/vanadium/secu...
File services/vanadium/security/principal_service.go (right):

https://codereview.chromium.org/1418013004/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.go:129: principals
map[vpkg.AppInstanceName]*principal // GUARDED_BY(mu)
maybe call it appPrincipalMap

[ataly, 2015-10-28 21:32:25]

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:62: // obtained by
invoking GetUserBlessing().
On 2015/10/26 20:52:01, gautham wrote:
> remove ()

Done.

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:80:
SetCurrentUser(User user) => (string? error);
Yes, we can definitely go in this direction (i.e., have some standard account
selection UIs implemented by this service.)

I think the current goal is to experiment with the programmatic interface a bit
and see what common UI and usage patterns evolve.

https://codereview.chromium.org/1418013004/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:98:
GetUserBlessing(AppInstanceName? app) => (Blessing? user_blessing);
On 2015/10/26 20:52:01, gautham wrote:
> Any reason why User, Blessing are not part of the same struct?. I'd rather not
> have separate interfaces for GetUser/GetUserBlessing and
GetUsers/GetBlessings. 

Done.

https://codereview.chromium.org/1418013004/diff/140001/services/vanadium/secu...
File services/vanadium/security/principal_service.go (right):

https://codereview.chromium.org/1418013004/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.go:129: principals
map[vpkg.AppInstanceName]*principal // GUARDED_BY(mu)
I called it 'appPrincipals'. The 'map' is kinda redundant.

[ataly, 2015-10-28 21:33:43]

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

[jamesr, 2015-10-28 21:45:51]

https://codereview.chromium.org/1418013004/diff/160001/examples/bank_app/bank.cc
File examples/bank_app/bank.cc (right):

https://codereview.chromium.org/1418013004/diff/160001/examples/bank_app/bank...
examples/bank_app/bank.cc:36: void Run(const vanadium::UserPtr& u) const {
our style guide says to avoid one-letter variable names except for loop
counters. 'u' is also a valid modifier in C++ which is extra confusing. i'd say
'user' here

(C++ needs to be more verbose than a language like Go in cases like this, in
part because the language grammar is far less clean)

[ataly, 2015-10-28 21:59:14]

https://codereview.chromium.org/1418013004/diff/160001/examples/bank_app/bank.cc
File examples/bank_app/bank.cc (right):

https://codereview.chromium.org/1418013004/diff/160001/examples/bank_app/bank...
examples/bank_app/bank.cc:36: void Run(const vanadium::UserPtr& u) const {
On 2015/10/28 21:45:51, jamesr wrote:
> our style guide says to avoid one-letter variable names except for loop
> counters. 'u' is also a valid modifier in C++ which is extra confusing. i'd
say
> 'user' here
> 
> (C++ needs to be more verbose than a language like Go in cases like this, in
> part because the language grammar is far less clean)

Ah, I see. Thanks for pointing that out. Done.

[gautham, 2015-10-28 23:02:45]

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:57: // as the current
user identity for the instance.
application instance.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:60: // email address
of the user to the unique public/private key-pair of the instance.
application instance.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:61: // This key pair
is generated and persisted by this service on the first Login
This seems like an implementation detail. Not sure its needed in API level
comments.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:74: GetUsers() =>
(array<User> ids, User? current);
maybe call this GetAuthenticatedUsers or GetLoggedInUsers

Also, given that we have a GetUser() below do we need to return current here as
part of this interface?

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:76: // SetCurrentUser
sets the provided identity as the current user identity of
SetCurrentUser sets the current user identity of the calling application
instance....

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:87: // If a null
application instance is provided then the current user identifier
identifier -> identity

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:90: // Returns null
if the application instance has not invoked Login.
Returns null if the application instance is in logged out state (see 'Logout').

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:91:
GetUser(AppInstanceName? app) => (User? user);
GetUser -> GetCurrentUser given that we have SetCurrentUser.

[ashankar, 2015-10-30 02:24:14]

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/bank.cc
File examples/bank_app/bank.cc (right):

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/bank...
examples/bank_app/bank.cc:39: user_->append(user->email);
Why "append"?
Just:
*user_ = user->email
?

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/cust...
File examples/bank_app/customer.cc (right):

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/cust...
examples/bank_app/customer.cc:34: MOJO_LOG(INFO) << "Login() to the principal
service failed\n";
Remove trailing "\n", shouldn't be necessary with the LOG macros

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:18: string email;
Must it be "email" - would saying "username" be more appropriate?

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:19: Blessing
user_blessing;
Just "blessing" - it's part of the "User" struct, so don't need the "_user"
suffix?

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:39: // to make it
match the WireBlessing type.
Would it make sense to link to the security concepts page:
https://github.com/vanadium/docs/blob/master/concepts/security.md
as a form of documentation?

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:74: GetUsers() =>
(array<User> ids, User? current);
On 2015/10/28 23:02:45, gautham wrote:
> maybe call this GetAuthenticatedUsers or GetLoggedInUsers
> 
> Also, given that we have a GetUser() below do we need to return current here
as
> part of this interface?

+1.
Also, is mojo style to have the "Get" prefix or do they generally do away with
it?
Basically, let's just stick with their convention (and maybe you are, I'm jus'
sayin' :)

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:84: Logout();
Nit: Would re-order things so that related methods are together:

Login();
Logout()
GetLoggedInUsers()

SetCurrentUser()
GetCurrentUser()

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
File services/vanadium/security/conventions.go (right):

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:12: const chainSeparator = "/"
Can't we use the constant defined in the mojom file?

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:15: // function from the Vanadium API.
Not for this CL, but now that the Vanadium code is public, can we consider
importing it and not duplicating these things?
And in the mean time, maybe just have a comment about how this is related to:
godoc.org/v.io/v23/conventions
?

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:36: if len(parts) != 4 {
We don't want "==4", but >=4, right?
So:
if len(parts) < 4 {
  ...
}

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:40: if (parts[0] != "dev.v.io") ||
(parts[1] != "u") {
Should we have a TODO about the notion of an identity provider? Or maybe make
this function return (identityProvider, username string, err error) instead?

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
File services/vanadium/security/principal.go (right):

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:29: func (p *principal) getUsers()
([]vpkg.User, *vpkg.User) {
Just "users()"
(the "get" prefix is not idiomatic in go:
https://golang.org/doc/effective_go.html#Getters)

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:33: for _, user := range p.users {
How about:
users := make([]vpkg.User, len(p.users))
copy(users, p.users)
return users, p.curr

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:46: func (p *principal)
setCurrentUser(user vpkg.User) (err *string) {
Why (err *string) instead of "err error"?

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:59: func (p *principal)
unsetCurrentUser() {
clearCurrentUser?

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
File services/vanadium/security/principal_service.go (right):

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal_service.go:74: return &str, nil
Shouldn't we be returning an error instead of setting the string value to the
error string?

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal_service.go:85: if app == nil {
Why is this needed? Who would call this with nil and expect a reasonable answer?

[ataly, 2015-11-04 00:24:30]

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/bank.cc
File examples/bank_app/bank.cc (right):

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/bank...
examples/bank_app/bank.cc:39: user_->append(user->email);
On 2015/10/30 02:24:13, ashankar wrote:
> Why "append"?
> Just:
> *user_ = user->email
> ?

Done.

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/cust...
File examples/bank_app/customer.cc (right):

https://codereview.chromium.org/1418013004/diff/180001/examples/bank_app/cust...
examples/bank_app/customer.cc:34: MOJO_LOG(INFO) << "Login() to the principal
service failed\n";
On 2015/10/30 02:24:13, ashankar wrote:
> Remove trailing "\n", shouldn't be necessary with the LOG macros

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:18: string email;
I have been going back and forth on this. The username is not necessarily the
email address. For instance, based on what I understood from Usha, on Android
the username can also be the phone number.

Therefore for now I though we should just promise the email address in the User
object as that is what our Vanadium IDP blessings promise. The other minor
reason is that an email address stands on its own where a username string may
require the identity provider name as well.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:19: Blessing
user_blessing;
On 2015/10/30 02:24:13, ashankar wrote:
> Just "blessing" - it's part of the "User" struct, so don't need the "_user"
> suffix?

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:39: // to make it
match the WireBlessing type.
On 2015/10/30 02:24:13, ashankar wrote:
> Would it make sense to link to the security concepts page:
> https://github.com/vanadium/docs/blob/master/concepts/security.md
> as a form of documentation?

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:57: // as the current
user identity for the instance.
On 2015/10/28 23:02:45, gautham wrote:
> application instance. 

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:60: // email address
of the user to the unique public/private key-pair of the instance.
On 2015/10/28 23:02:45, gautham wrote:
> application instance. 

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:61: // This key pair
is generated and persisted by this service on the first Login
On 2015/10/28 23:02:45, gautham wrote:
> This seems like an implementation detail. Not sure its needed in API level
> comments.

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:74: GetUsers() =>
(array<User> ids, User? current);
On 2015/10/28 23:02:45, gautham wrote:
> maybe call this GetAuthenticatedUsers or GetLoggedInUsers
> 
> Also, given that we have a GetUser() below do we need to return current here
as
> part of this interface?

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:76: // SetCurrentUser
sets the provided identity as the current user identity of
On 2015/10/28 23:02:45, gautham wrote:
> SetCurrentUser sets the current user identity of the calling application
> instance....

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:84: Logout();
On 2015/10/30 02:24:13, ashankar wrote:
> Nit: Would re-order things so that related methods are together:
> 
> Login();
> Logout()
> GetLoggedInUsers()
> 
> SetCurrentUser()
> GetCurrentUser()

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:87: // If a null
application instance is provided then the current user identifier
On 2015/10/28 23:02:45, gautham wrote:
> identifier -> identity

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:90: // Returns null
if the application instance has not invoked Login.
On 2015/10/28 23:02:45, gautham wrote:
> Returns null if the application instance is in logged out state (see
'Logout').

Done.

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:91:
GetUser(AppInstanceName? app) => (User? user);
Since we renamed GetUsers to GetLoggedInUsers, I renamed SetCurrentUser to
SetUser.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
File services/vanadium/security/conventions.go (right):

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:12: const chainSeparator = "/"
I tried to do that but for some reason the constant was not in the go generated
code. There may be a bug in the go code generation.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:15: // function from the Vanadium API.
Yes this is one of the things we need to do. Will get this done in a subsequent
CL.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:36: if len(parts) != 4 {
On 2015/10/30 02:24:13, ashankar wrote:
> We don't want "==4", but >=4, right?
> So:
> if len(parts) < 4 {
>   ...
> }

Done.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:40: if (parts[0] != "dev.v.io") ||
(parts[1] != "u") {
I have a TODO about specifying the identity provider in the user struct in the
Mojom. As you suggested the right thing is to use the v23/conventions library
instead. I added a TODO to do that.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
File services/vanadium/security/principal.go (right):

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:29: func (p *principal) getUsers()
([]vpkg.User, *vpkg.User) {
On 2015/10/30 02:24:13, ashankar wrote:
> Just "users()"
> (the "get" prefix is not idiomatic in go:
> https://golang.org/doc/effective_go.html#Getters)

Done.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:33: for _, user := range p.users {
On 2015/10/30 02:24:13, ashankar wrote:
> How about:
> users := make([]vpkg.User, len(p.users))
> copy(users, p.users)
> return users, p.curr
> 

Done.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:46: func (p *principal)
setCurrentUser(user vpkg.User) (err *string) {
Mojom does not support an error type. My understanding is that whenever a
service wants to return an error, it should stringify it and return it as an
explicit return value. See the "SelectAccount" method in this Mojom interface
for example:
https://github.com/domokit/mojo/blob/15a59a82d9e5e7ee4157f082e53ca92026ee8d1a...

(The error return values that we see in this file are only in the Go generated
code. The errors are simply logged and not returned to the client.) 

In this particular case, I feel that it is important to send a reason for
failure to the client so I added an explicit error return value.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/principal.go:59: func (p *principal)
unsetCurrentUser() {
On 2015/10/30 02:24:13, ashankar wrote:
> clearCurrentUser?

Done.

[ashankar, 2015-11-04 00:37:10]

LGTM

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:18: string email;
On 2015/11/04 00:24:29, ataly wrote:
> I have been going back and forth on this. The username is not necessarily the
> email address. For instance, based on what I understood from Usha, on Android
> the username can also be the phone number.
> 
> Therefore for now I though we should just promise the email address in the
User
> object as that is what our Vanadium IDP blessings promise. The other minor
> reason is that an email address stands on its own where a username string may
> require the identity provider name as well.

On the other hand, we don't want to end up in a position where an email is
required, right?
Anyway, this is fine for now but as this evolves I do think that we might want
to make this username - and email can be one form of username

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
File services/vanadium/security/conventions.go (right):

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:12: const chainSeparator = "/"
On 2015/11/04 00:24:30, ataly wrote:
> I tried to do that but for some reason the constant was not in the go
generated
> code. There may be a bug in the go code generation.

Add a TODO about this - we should be able to remove this at some point :)

https://codereview.chromium.org/1418013004/diff/200001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/200001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:77: // Returns null
if the application instance has not invoked Login or if the instance
Are .mojom files supposed to follow an 80-char line limit or something?
If so, please fix.
If not, nevermind :)

[ataly, 2015-11-04 21:22:51]

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/180001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:18: string email;
On 2015/11/04 00:37:10, ashankar wrote:
> On 2015/11/04 00:24:29, ataly wrote:
> > I have been going back and forth on this. The username is not necessarily
the
> > email address. For instance, based on what I understood from Usha, on
Android
> > the username can also be the phone number.
> > 
> > Therefore for now I though we should just promise the email address in the
> User
> > object as that is what our Vanadium IDP blessings promise. The other minor
> > reason is that an email address stands on its own where a username string
may
> > require the identity provider name as well.
> 
> On the other hand, we don't want to end up in a position where an email is
> required, right?
> Anyway, this is fine for now but as this evolves I do think that we might want
> to make this username - and email can be one form of username

Acknowledged.

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
File services/vanadium/security/conventions.go (right):

https://codereview.chromium.org/1418013004/diff/180001/services/vanadium/secu...
services/vanadium/security/conventions.go:12: const chainSeparator = "/"
On 2015/11/04 00:37:10, ashankar wrote:
> On 2015/11/04 00:24:30, ataly wrote:
> > I tried to do that but for some reason the constant was not in the go
> generated
> > code. There may be a bug in the go code generation.
> 
> Add a TODO about this - we should be able to remove this at some point :)

Done.

https://codereview.chromium.org/1418013004/diff/200001/mojo/services/vanadium...
File mojo/services/vanadium/security/interfaces/principal.mojom (right):

https://codereview.chromium.org/1418013004/diff/200001/mojo/services/vanadium...
mojo/services/vanadium/security/interfaces/principal.mojom:77: // Returns null
if the application instance has not invoked Login or if the instance
On 2015/11/04 00:37:10, ashankar wrote:
> Are .mojom files supposed to follow an 80-char line limit or something?
> If so, please fix.
> If not, nevermind :)

Done.

[gautham, 2015-11-04 22:46:51]

lgtm

[ataly, 2015-11-05 01:15:30]

Committed patchset #13 (id:240001) manually as
9c1db641158f7677fed64024c9c718475593c37e (presubmit successful).