Initial skeletal implementation of the PrincipalService. Also, use the Login()/GetUserBlessing()

Initial skeletal implementation of the PrincipalService. Also, use the Login()/GetUserBlessing()
service from echo_client/echo_server.

R=jamesr@chromium.org, ataly@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/658bb42c0c386630ee6257a4f7cde4aa138d50a8

[gautham, 2015-07-31 17:09:33]

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

[gautham, 2015-07-31 17:09:50]

gauthamt@google.com changed reviewers:
- ashankar@google.com

[ataly, 2015-07-31 19:04:04]

Overall, looks good to me. I added a few minor comments on the mojom
definitions.

https://codereview.chromium.org/1261403003/diff/40001/examples/echo/echo_serv...
File examples/echo/echo_server.cc (right):

https://codereview.chromium.org/1261403003/diff/40001/examples/echo/echo_serv...
examples/echo/echo_server.cc:57: // EchoClient user available in
b.get()->chain[0]->extension
MInor: Should we just say "user identity available in b.get()->chain?"

(To be precise, the user identity is concatenation of
b.get()->chain[i]->extension for all i.)

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
File mojo/services/vanadium/security/public/interfaces/principal.mojom (right):

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:10: struct
AppName {
Should this be called "AppInstanceName"?
(In the comments inside the PrincipalService interface we use AppName to refer
to an application instance.)

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:35: //
Certificate represents a human-readable name/public-key pair. The private-key
Nit: we should say human-readable name and public-key pair.

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:43: //
Blessing represents a user identity.
How about changing the comment to:

// Blessing is a credential binding a (human-readable) user identity to a public
key. The corresponding private key is only available for signing operations
within the PrincipalService application.

(This last sentence currently appears in the comment for Certificate but I think
we can remove it from there)

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:48: // A
service that binds that user identities to an application instance running
extra "that"

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:51: // Login
is called by an application instance (requestor_url/qualifier) that
Nit: We use "this" application instance in the comments below, should we change
it to "the calling" application instance? (I am not sure what the Mojo
convention is.)

[gauthamt, 2015-07-31 20:24:16]

gautham.dorai@gmail.com changed reviewers:
+ gautham.dorai@gmail.com

[gauthamt, 2015-07-31 20:24:17]

https://codereview.chromium.org/1261403003/diff/40001/examples/echo/echo_serv...
File examples/echo/echo_server.cc (right):

https://codereview.chromium.org/1261403003/diff/40001/examples/echo/echo_serv...
examples/echo/echo_server.cc:57: // EchoClient user available in
b.get()->chain[0]->extension
On 2015/07/31 19:04:04, ataly wrote:
> MInor: Should we just say "user identity available in b.get()->chain?"
> 
> (To be precise, the user identity is concatenation of
> b.get()->chain[i]->extension for all i.)
> 

Done.

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
File mojo/services/vanadium/security/public/interfaces/principal.mojom (right):

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:10: struct
AppName {
On 2015/07/31 19:04:04, ataly wrote:
> Should this be called "AppInstanceName"?
> (In the comments inside the PrincipalService interface we use AppName to refer
> to an application instance.)

Done.

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:35: //
Certificate represents a human-readable name/public-key pair. The private-key
On 2015/07/31 19:04:04, ataly wrote:
> Nit: we should say human-readable name and public-key pair.

Done.

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:43: //
Blessing represents a user identity.
On 2015/07/31 19:04:04, ataly wrote:
> How about changing the comment to:
> 
> // Blessing is a credential binding a (human-readable) user identity to a
public
> key. The corresponding private key is only available for signing operations
> within the PrincipalService application.
> 
> (This last sentence currently appears in the comment for Certificate but I
think
> we can remove it from there)

Done.

I'd left out binding before as we don't really sign it.

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:48: // A
service that binds that user identities to an application instance running
On 2015/07/31 19:04:04, ataly wrote:
> extra "that"

Done.

https://codereview.chromium.org/1261403003/diff/40001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:51: // Login
is called by an application instance (requestor_url/qualifier) that
On 2015/07/31 19:04:04, ataly wrote:
> Nit: We use "this" application instance in the comments below, should we
change
> it to "the calling" application instance? (I am not sure what the Mojo
> convention is.)

Changed it to "Removes the user blessing for the application instance that
invokes the Logout method"

[gautham, 2015-07-31 21:11:58]

gauthamt@google.com changed reviewers:
+ jamesr@chromium.org, viettrungluu@chromium.org
- gautham.dorai@gmail.com

[jamesr, 2015-08-03 18:26:39]

Could you make a copy of the existing echo example that uses this functionality
so that the existing example continues to work in environments that aren't able
to run Go mojo apps?

https://codereview.chromium.org/1261403003/diff/60001/examples/echo/echo_clie...
File examples/echo/echo_client.cc (right):

https://codereview.chromium.org/1261403003/diff/60001/examples/echo/echo_clie...
examples/echo/echo_client.cc:32: login_service_->Login(Callback<void(const
BlessingPtr)>());
the C++ code generator will generate a typedef for this called LoginCallback so
you can say simply

login_service_->Login(LoginCallback());

https://codereview.chromium.org/1261403003/diff/60001/mojo/services/vanadium/...
File mojo/services/vanadium/security/public/interfaces/principal.mojom (right):

https://codereview.chromium.org/1261403003/diff/60001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:32:
array<uint8> s;
are |r| and |s| of a predictable size? how are they encoded?

https://codereview.chromium.org/1261403003/diff/60001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:56: Login() =>
(Blessing? user_blessing);
if the login fails for whatever reason, what happens? does this call result in a
null Blessing or a Blessing with an empty list of certificates?

[gautham, 2015-08-03 21:45:04]

Thanks for the review. 

I added a c++ implementation of PrincipalService that gets used when go is not
available. So, I left the echo_client/echo_server changes in there.

https://codereview.chromium.org/1261403003/diff/60001/examples/echo/echo_clie...
File examples/echo/echo_client.cc (right):

https://codereview.chromium.org/1261403003/diff/60001/examples/echo/echo_clie...
examples/echo/echo_client.cc:32: login_service_->Login(Callback<void(const
BlessingPtr)>());
On 2015/08/03 18:26:39, jamesr wrote:
> the C++ code generator will generate a typedef for this called LoginCallback
so
> you can say simply
> 
> login_service_->Login(LoginCallback());

Done.

https://codereview.chromium.org/1261403003/diff/60001/mojo/services/vanadium/...
File mojo/services/vanadium/security/public/interfaces/principal.mojom (right):

https://codereview.chromium.org/1261403003/diff/60001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:32:
array<uint8> s;
On 2015/08/03 18:26:39, jamesr wrote:
> are |r| and |s| of a predictable size? how are they encoded?

we'll probably end up using the openssl libraries for this. The size of the
signature depends on the curve params we choose. 

https://www.openssl.org/docs/crypto/ECDSA_sign_ex.html

https://codereview.chromium.org/1261403003/diff/60001/mojo/services/vanadium/...
mojo/services/vanadium/security/public/interfaces/principal.mojom:56: Login() =>
(Blessing? user_blessing);
On 2015/08/03 18:26:39, jamesr wrote:
> if the login fails for whatever reason, what happens? does this call result in
a
> null Blessing or a Blessing with an empty list of certificates?

// Returns null if login fails.

[jamesr, 2015-08-03 21:53:19]

I'd still prefer leaving echo_server/client alone and adding a new example that
shows how to use the principal service. You can clone the existing code to use
as a starting point.

[gautham, 2015-08-04 02:57:01]

On 2015/08/03 21:53:19, jamesr wrote:
> I'd still prefer leaving echo_server/client alone and adding a new example
that
> shows how to use the principal service. You can clone the existing code to use
> as a starting point.

I reverted the echo example and added another example called Bank which shows
similar user Login/GetUserBlessing flow. 

Thanks
Gautham

[gautham, 2015-08-06 20:58:26]

Please take another look when you get a chance. 

Gautham

[jamesr, 2015-08-11 21:55:08]

I'm still not sure how this system is supposed to integrate with information
available in the system.  For instance, is the description of an instance of an
app supposed to come from the app or the shell or some other service?  How are
different actors in the system supposed to take advantage of this information?

https://codereview.chromium.org/1261403003/diff/100001/mojo/services/vanadium...
File mojo/services/vanadium/security/public/interfaces/principal.mojom (right):

https://codereview.chromium.org/1261403003/diff/100001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:10: struct
AppInstanceName {
this doesn't appear to be used anywhere.  Where is the information about which
app is running supposed to come from?  The shell has an idea of the identity of
each piece of code in the system and provides it in the Connect..() call, but
I'm not sure if that is exactly what this wants to get.

[gauthamt, 2015-08-11 22:21:20]

gautham.dorai@gmail.com changed reviewers:
+ gautham.dorai@gmail.com

[gauthamt, 2015-08-11 22:21:20]

https://codereview.chromium.org/1261403003/diff/100001/mojo/services/vanadium...
File mojo/services/vanadium/security/public/interfaces/principal.mojom (right):

https://codereview.chromium.org/1261403003/diff/100001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:10: struct
AppInstanceName {
On 2015/08/11 21:55:08, jamesr wrote:
> this doesn't appear to be used anywhere.  Where is the information about which
> app is running supposed to come from?  The shell has an idea of the identity
of
> each piece of code in the system and provides it in the Connect..() call, but
> I'm not sure if that is exactly what this wants to get.

This is used to in the bank example. A service would use this to identify the
app that is requesting a connection and can ask the PrincipalService to return
the user identity associated with the AppInstance (if that app had called
Login).

[gautham, 2015-08-12 02:31:05]

This includes an implementation of the PrincipalService table and updates to the
Bank/Customer app. 

Thanks
Gautham

[gautham, 2015-08-14 16:33:27]

Reminder about this CL.

Gautham

[jamesr, 2015-08-17 21:38:22]

Left some comments about the C++ code and interface.  I'm less familiar with the
go code (and this code review tool appears to have mangled the indentation) but
from a glance it appears to differ in a number of ways from the C++ version. 
Could you describe in the patch description what the two impls are expected to
do?

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

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
2015

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:23: void Deposit(const int32_t usd) override {
balance_ += usd; }
drop the 'const' - the body is trivial enough that you can tell you aren't
modifying it in the body of the function

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:34: BankUser(std::string* user) : user_(user) { }
add explicit keyword to one-argument constructor

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:46: mutable std::string *user_;
you aren't modifying the pointer value, just the pointed-to value, so you should
be able to drop the mutable

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:62: if (url.length() > 0) {
what is this branch for?

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:67: login_service_.WaitForIncomingResponse();
why is this blocking? you should probably check the return value explicitly and
do something if it returns false (meaning the pipe closed before you got a
response value)

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:70: if (user.length() <= 0) {
std::string::length() returns a size_t, which is unsigned, so this is equivalent
to == 0.  use .empty() instead

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

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
2015

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:22: if (b.get()) {
StructPtr<> (and the rest of our ptr wrapper types) can be bool-tested in the
reasonable way, so you can just write

if (b) {
...
}

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:23: for (size_t i = 0; i < b.get()->chain.size();
i++) {
b defines operator->, so you can just say

b->chain...

instead of b.get()->

same in the rest

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:24: user += "/";
this is a pretty inefficient way to build a string, although maybe that's
intentional to keep the example code small

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:25: user += b.get()->chain[i].get()->extension;
same for chain[i]

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:28: if (user.length() > 0) {
!empty()

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:54: bank->GetBalance(GetBalanceResponse());
seems like a good place to use a lambda instead of a full class for the functor

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

https://codereview.chromium.org/1261403003/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:5: module
mojo;
i think this should go in a 'vanadium' module or some other clarifying name.
it's premature (imo) to grab the namespace mojo.Signature / mojo.AppInstanceName
/ mojo.Certificate, etc

https://codereview.chromium.org/1261403003/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:15: //
Signature represents a digital signature of a message.
could you leave out the Signature and other concepts that aren't being used yet
from this patch? it'll be a lot easier to review the APIs with an example caller
and callee to look at and (from what I can see) nothing in this patch uses
Signature in any way

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
File services/vanadium/security/BUILD.gn (right):

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/BUILD.gn:19:
mojo_native_application("principal_service") {
instead of having two ways to build the same .mojo i think it'd be better to
have both targets listed out with different names, possibly with the go one
guarded, so that a developer can build both. with this setup it's impossible in
one checkout to build the go and C++ versions for a particular build (without
mucking around with the go_build_tool variable between builds).

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

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:5: #include <cstdlib>
we generally use the <blahblah.h> headers instead of the <cblahblah> ones. the
only differences are some minor ones around the global namespace

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:15: namespace mojo {
no "mojo". namespace should match directory structure and the path to this
doesn't include 'mojo'

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:21: // interface for Linux.
what about this is Linux-specific?

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:51:
connection->AddService<mojo::PrincipalService>(this);
you don't need to spell out the template parameter here, |this| inherits only
from one InterfaceFactory base type. connection->AddService(this) should work

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:62:
mojo::BindingSet<mojo::PrincipalService> binding_;
this means every connection to the PrincipalService sees the same blessing, i.e.
there's no way to preserve information about which app is which. that seems
pretty important to the design of the service, isn't it?

[gautham, 2015-08-18 01:54:22]

Thanks for the review. 

I've removed the C++ version of the principal_service. I've made the necessary
modifications to the BUILD.gn (android && go). The authentication service works
only with android anyway. 

Gautham

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

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
On 2015/08/17 21:38:21, jamesr wrote:
> 2015

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:23: void Deposit(const int32_t usd) override {
balance_ += usd; }
On 2015/08/17 21:38:21, jamesr wrote:
> drop the 'const' - the body is trivial enough that you can tell you aren't
> modifying it in the body of the function

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:34: BankUser(std::string* user) : user_(user) { }
On 2015/08/17 21:38:21, jamesr wrote:
> add explicit keyword to one-argument constructor

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:46: mutable std::string *user_;
On 2015/08/17 21:38:21, jamesr wrote:
> you aren't modifying the pointer value, just the pointed-to value, so you
should
> be able to drop the mutable

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:62: if (url.length() > 0) {
I reject connection requests from customers that have not logged in. Doesn't the
shell also connect to the app through this call and it won't have a user
identity?

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:67: login_service_.WaitForIncomingResponse();
> why is this blocking? you should probably check the return value explicitly
and
> do something if it returns false (meaning the pipe closed before you got a
> response value)

I wanted to reject connection requests from customers that don't have a user
identity (ie: have not called Login()). Is there a better way to do this?

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/bank...
examples/bank_app/bank.cc:70: if (user.length() <= 0) {
On 2015/08/17 21:38:21, jamesr wrote:
> std::string::length() returns a size_t, which is unsigned, so this is
equivalent
> to == 0.  use .empty() instead

Done.

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

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2015/08/17 21:38:21, jamesr wrote:
> 2015

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:22: if (b.get()) {
On 2015/08/17 21:38:21, jamesr wrote:
> StructPtr<> (and the rest of our ptr wrapper types) can be bool-tested in the
> reasonable way, so you can just write
> 
> if (b) {
> ...
> }

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:23: for (size_t i = 0; i < b.get()->chain.size();
i++) {
On 2015/08/17 21:38:21, jamesr wrote:
> b defines operator->, so you can just say
> 
> b->chain...
> 
> instead of b.get()->
> 
> same in the rest

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:24: user += "/";
On 2015/08/17 21:38:21, jamesr wrote:
> this is a pretty inefficient way to build a string, although maybe that's
> intentional to keep the example code small

Acknowledged.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:25: user += b.get()->chain[i].get()->extension;
On 2015/08/17 21:38:21, jamesr wrote:
> same for chain[i]

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:28: if (user.length() > 0) {
On 2015/08/17 21:38:21, jamesr wrote:
> !empty()

Done.

https://codereview.chromium.org/1261403003/diff/140001/examples/bank_app/cust...
examples/bank_app/customer.cc:54: bank->GetBalance(GetBalanceResponse());
On 2015/08/17 21:38:21, jamesr wrote:
> seems like a good place to use a lambda instead of a full class for the
functor

Done.

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

https://codereview.chromium.org/1261403003/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:5: module
mojo;
On 2015/08/17 21:38:22, jamesr wrote:
> i think this should go in a 'vanadium' module or some other clarifying name.
> it's premature (imo) to grab the namespace mojo.Signature /
mojo.AppInstanceName
> / mojo.Certificate, etc

Done.

https://codereview.chromium.org/1261403003/diff/140001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:15: //
Signature represents a digital signature of a message.
On 2015/08/17 21:38:22, jamesr wrote:
> could you leave out the Signature and other concepts that aren't being used
yet
> from this patch? it'll be a lot easier to review the APIs with an example
caller
> and callee to look at and (from what I can see) nothing in this patch uses
> Signature in any way

Done.

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
File services/vanadium/security/BUILD.gn (right):

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/BUILD.gn:19:
mojo_native_application("principal_service") {
On 2015/08/17 21:38:22, jamesr wrote:
> instead of having two ways to build the same .mojo i think it'd be better to
> have both targets listed out with different names, possibly with the go one
> guarded, so that a developer can build both. with this setup it's impossible
in
> one checkout to build the go and C++ versions for a particular build (without
> mucking around with the go_build_tool variable between builds).

I removed the C++ version and deleted principal_service.cc. The bank example and
principal-service only work with go/android. Made the necessary BUILD.gn
modifications.

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

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:5: #include <cstdlib>
On 2015/08/17 21:38:22, jamesr wrote:
> we generally use the <blahblah.h> headers instead of the <cblahblah> ones. the
> only differences are some minor ones around the global namespace

Removed file.

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:15: namespace mojo {
On 2015/08/17 21:38:22, jamesr wrote:
> no "mojo". namespace should match directory structure and the path to this
> doesn't include 'mojo'

Removed file.

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:21: // interface for Linux.
On 2015/08/17 21:38:22, jamesr wrote:
> what about this is Linux-specific?

Removed file.

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:51:
connection->AddService<mojo::PrincipalService>(this);
On 2015/08/17 21:38:22, jamesr wrote:
> you don't need to spell out the template parameter here, |this| inherits only
> from one InterfaceFactory base type. connection->AddService(this) should work

Removed file.

https://codereview.chromium.org/1261403003/diff/140001/services/vanadium/secu...
services/vanadium/security/principal_service.cc:62:
mojo::BindingSet<mojo::PrincipalService> binding_;
On 2015/08/17 21:38:22, jamesr wrote:
> this means every connection to the PrincipalService sees the same blessing,
i.e.
> there's no way to preserve information about which app is which. that seems
> pretty important to the design of the service, isn't it?

Removed file.

[jamesr, 2015-08-19 05:30:55]

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

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:17: namespace mojo {
drop the 'mojo::' part

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:20: using namespace vanadium;
google's C++ style guide (which we use) bans using-directives:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces. 
you can use using statements to pull specific types into the namespace

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:68: login_service_.WaitForIncomingResponse();
hmm - our c++ bindings let you choose whether to register services on a
connection at any time (i.e. you could curry the |connection| param along into a
callback and decide to call AddService<> based on that) but not a clean way to
close down a connection later.  we should probably fix that.

at a minimum you should check the return value of WaitForIncomingResponse(). if
it's false, that means the pipe closed before a reply was read which you should
treat as a failure to get a blessing (checking for an empty string is too
indirect imo)

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:77: connection->AddService<Bank>(this);
nit: i don't think you need the <Bank> part of this call, the template parameter
should be unambiguously deduced in this context

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

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:8: 
nit: no blank line needed here

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:15: namespace mojo {
drop the mojo namespace

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:18: using namespace vanadium;
ditto re using-declarations

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:33: RunLoop::current()->Quit();  // All done!
should the login callback quit? feels like this should be at the end of
initialize() or the callback to GetBalance()

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:43: login_service_.WaitForIncomingResponse();
check for failure

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:52: bank->GetBalance(Callback<void(const
int32_t&)>(gb_callback));
does just GetBalance(gb_callback) not work? the type that |auto| deduces here is
some unutterable lambda type but that should convert to the callback type just
fine.  if it doesn't compile without the Callback<void..> bit could you paste
the compile error you see?

[ashankar, 2015-08-19 05:50:38]

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

[ashankar, 2015-08-19 05:50:39]

Just some random notes, I by no means know what the Mojo way is yet :)

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/BUIL...
File examples/bank_app/BUILD.gn (right):

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/BUIL...
examples/bank_app/BUILD.gn:3: # found in the LICENSE file.
Would it make sense to have a README file in this directory explaining the
example?

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

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:41: user_->append("/");
Should this separator "/" be defined as a mojom constant?
https://docs.google.com/document/d/1r7yCseBktlDEN9CKp_JWD0ZYxMi4GCsLXMvSN5sI0...

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:73: return false;
When would url.length be 0, and in that case why is it okay to connect to the
service unauthenticated?

(It might all be fine - my mojo mojo just isn't up to snuff to know that)

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

https://codereview.chromium.org/1261403003/diff/200001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:20:
array<uint8>? publickey;
Should we mention that this is the DER representation of the public key

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

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:33: cert :=
[]principal.Certificate{principal.Certificate{Extension: *name}}
What about the public key? It seems that we're returning a blessing with a
certificate but the public key field in that is empty?
Seems like this first patch doesn't intend to implement key management?

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:48: b :=
pImpl.psd.GetUserBlessing(app)
Optional:
return pImpl.psd.GetUserBlessing(app), nil

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:69: sync.Mutex
Instead of embedding a Mutex I would recommend making it a field.
Otherwise, the Mutex methods (Lock/Unlock) become "exported" methods on a
PrincipalServiceDelegate object making it appear that clients are supposed to
use it - which is not the case - you don't want clients of
PrincipalServiceDelegate to call Lock/Unlock - or for these methods to show up
in godoc or the like.

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:85:
connection.ProvideServices(&principal.PrincipalService_ServiceFactory{&PrincipalServiceImpl{app,
psd}})
gofmt?

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:94: func (psd
*PrincipalServiceDelegate) AddUserBlessing(app principal.AppInstanceName, b
*principal.Blessing) {
Do these methods need to be exported?

[gautham, 2015-08-19 17:45:51]

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/BUIL...
File examples/bank_app/BUILD.gn (right):

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/BUIL...
examples/bank_app/BUILD.gn:3: # found in the LICENSE file.
On 2015/08/19 05:50:38, ashankar wrote:
> Would it make sense to have a README file in this directory explaining the
> example?

Done.

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

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:17: namespace mojo {
On 2015/08/19 05:30:54, jamesr wrote:
> drop the 'mojo::' part

Done.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:20: using namespace vanadium;
On 2015/08/19 05:30:54, jamesr wrote:
> google's C++ style guide (which we use) bans using-directives:
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces. 
> you can use using statements to pull specific types into the namespace

Done.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:41: user_->append("/");
On 2015/08/19 05:50:38, ashankar wrote:
> Should this separator "/" be defined as a mojom constant?
>
https://docs.google.com/document/d/1r7yCseBktlDEN9CKp_JWD0ZYxMi4GCsLXMvSN5sI0...

Done.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:68: login_service_.WaitForIncomingResponse();
On 2015/08/19 05:30:54, jamesr wrote:
> hmm - our c++ bindings let you choose whether to register services on a
> connection at any time (i.e. you could curry the |connection| param along into
a
> callback and decide to call AddService<> based on that) but not a clean way to
> close down a connection later.  we should probably fix that.
> 
> at a minimum you should check the return value of WaitForIncomingResponse().
if
> it's false, that means the pipe closed before a reply was read which you
should
> treat as a failure to get a blessing (checking for an empty string is too
> indirect imo)

Checked the return value and logged a failure for now.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:73: return false;
On 2015/08/19 05:50:38, ashankar wrote:
> When would url.length be 0, and in that case why is it okay to connect to the
> service unauthenticated?
> 
> (It might all be fine - my mojo mojo just isn't up to snuff to know that)

This happens I launch both mojo:customer and mojo:bank from the command line and
the shell connects to the bank? ie: the shell has an empty application url.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/bank...
examples/bank_app/bank.cc:77: connection->AddService<Bank>(this);
On 2015/08/19 05:30:54, jamesr wrote:
> nit: i don't think you need the <Bank> part of this call, the template
parameter
> should be unambiguously deduced in this context

Done.

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

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:8: 
On 2015/08/19 05:30:55, jamesr wrote:
> nit: no blank line needed here

Done.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:15: namespace mojo {
On 2015/08/19 05:30:55, jamesr wrote:
> drop the mojo namespace

Done.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:18: using namespace vanadium;
On 2015/08/19 05:30:55, jamesr wrote:
> ditto re using-declarations

Done.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:33: RunLoop::current()->Quit();  // All done!
On 2015/08/19 05:30:55, jamesr wrote:
> should the login callback quit? feels like this should be at the end of
> initialize() or the callback to GetBalance()

Your right. This should not quit. I thought I'd removed it.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:43: login_service_.WaitForIncomingResponse();
On 2015/08/19 05:30:55, jamesr wrote:
> check for failure

Done.

https://codereview.chromium.org/1261403003/diff/200001/examples/bank_app/cust...
examples/bank_app/customer.cc:52: bank->GetBalance(Callback<void(const
int32_t&)>(gb_callback));
On 2015/08/19 05:30:54, jamesr wrote:
> does just GetBalance(gb_callback) not work? the type that |auto| deduces here
is
> some unutterable lambda type but that should convert to the callback type just
> fine.  if it doesn't compile without the Callback<void..> bit could you paste
> the compile error you see?

../../mojo/public/cpp/bindings/callback.h: In instantiation of 'void
mojo::Callback<void(Args ...)>::RunnableAdapter<Sink>::Run(typename
mojo::internal::Callback_ParamTraits<Args>::ForwardType ...) const [with Sink =
examples::BankCustomer::Initialize(mojo::ApplicationImpl*)::<lambda(const
int32_t&)>; Args = {int}]':
../../examples/bank_app/customer.cc:65:1:   required from here
../../mojo/public/cpp/bindings/callback.h:76:7: error: 'const struct
examples::BankCustomer::Initialize(mojo::ApplicationImpl*)::<lambda(const
int32_t&)>' has no member named 'Run'
       sink.Run(internal::Forward(args)...);
       ^

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

https://codereview.chromium.org/1261403003/diff/200001/mojo/services/vanadium...
mojo/services/vanadium/security/public/interfaces/principal.mojom:20:
array<uint8>? publickey;
On 2015/08/19 05:50:38, ashankar wrote:
> Should we mention that this is the DER representation of the public key

Done.

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

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:33: cert :=
[]principal.Certificate{principal.Certificate{Extension: *name}}
On 2015/08/19 05:50:38, ashankar wrote:
> What about the public key? It seems that we're returning a blessing with a
> certificate but the public key field in that is empty?
> Seems like this first patch doesn't intend to implement key management?

Yes. Key management to follow.

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:48: b :=
pImpl.psd.GetUserBlessing(app)
On 2015/08/19 05:50:38, ashankar wrote:
> Optional:
> return pImpl.psd.GetUserBlessing(app), nil

Done.

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:69: sync.Mutex
On 2015/08/19 05:50:38, ashankar wrote:
> Instead of embedding a Mutex I would recommend making it a field.
> Otherwise, the Mutex methods (Lock/Unlock) become "exported" methods on a
> PrincipalServiceDelegate object making it appear that clients are supposed to
> use it - which is not the case - you don't want clients of
> PrincipalServiceDelegate to call Lock/Unlock - or for these methods to show up
> in godoc or the like.

Done.

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:85:
connection.ProvideServices(&principal.PrincipalService_ServiceFactory{&PrincipalServiceImpl{app,
psd}})
On 2015/08/19 05:50:38, ashankar wrote:
> gofmt?

Done.

https://codereview.chromium.org/1261403003/diff/200001/services/vanadium/secu...
services/vanadium/security/principal_service.go:94: func (psd
*PrincipalServiceDelegate) AddUserBlessing(app principal.AppInstanceName, b
*principal.Blessing) {
On 2015/08/19 05:50:38, ashankar wrote:
> Do these methods need to be exported?

Done.

[ashankar, 2015-08-19 18:28:35]

Looks good to me, thanks

[jamesr, 2015-08-19 19:08:00]

lgtm

[gautham, 2015-08-19 19:40:49]

Committed patchset #12 (id:220001) manually as
658bb42c0c386630ee6257a4f7cde4aa138d50a8 (presubmit successful).