New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AccountManagerService API #864

Merged
merged 11 commits into from Dec 7, 2017

Conversation

Projects
None yet
4 participants
@gdbelvin
Collaborator

gdbelvin commented Nov 24, 2017

This PR creates the AccountManagerService definition.
This supports delegated account management by a trusted third party.
All actions on accounts are logged in the those accounts using distinguished keys and committed to in the master append-only log.

This PR also experiments with bringing the proto directory futher inline with the style guide

  • New directory structure
    • core/api
      • v1/ketransparency_proto
      • subapi/
        • v1/subapi_proto
  • Generated code will have a _proto ending in the package name
  • Service definitions and message definitions will live side by side

@gdbelvin gdbelvin requested review from phad and codingllama Nov 24, 2017

@codingllama

This turned into a bit of an impromptu API review, which is not that great for you because 1. I don't think I have enough context about your API and 2. Github is probably not the best place to have this discussion. So, take my comments with a grain of salt. I'm happy to have this discussion in an "informal" review, outside of Github, if you like.

I really like your documentation. It answered all the questions I had.

@gdbelvin gdbelvin changed the title from AccountResetService API to AccountManagerService API Dec 5, 2017

@google google deleted a comment from codecov-io Dec 5, 2017

Show outdated Hide outdated core/proto/accountmanager/v1/accountmanager.proto Outdated
Show outdated Hide outdated core/proto/accountmanager/v1/accountmanager.proto Outdated
Show outdated Hide outdated core/proto/accountmanager/v1/accountmanager.proto Outdated
Show outdated Hide outdated core/proto/accountmanager/v1/accountmanager.proto Outdated
Show outdated Hide outdated core/proto/accountmanager/v1/accountmanager.proto Outdated
Show outdated Hide outdated core/proto/accountmanager/v1/accountmanager.proto Outdated
// authorized_keys is the set of keys allowed to sign updates for this entry.
repeated keyspb.PublicKey authorized_keys = 5;
// status is set when account is part of a batch operation.
google.rpc.Status status = 6;

This comment has been minimized.

@cesarghali

cesarghali Dec 5, 2017

Collaborator

Just out of curiosity, how is this status gonna be used? Can't we use the returned error to report the status?

@cesarghali

cesarghali Dec 5, 2017

Collaborator

Just out of curiosity, how is this status gonna be used? Can't we use the returned error to report the status?

This comment has been minimized.

@gdbelvin

gdbelvin Dec 5, 2017

Collaborator

Typically, when you create or modify an object, you return the same object as a result. This works great when creating or modifying a single User, but gets a little muddy when we're talking about a batch of users. How should we associate the particular status with the particular user? Here we've just put it inside the User object. It will only be filled in the response to a batch message.

@gdbelvin

gdbelvin Dec 5, 2017

Collaborator

Typically, when you create or modify an object, you return the same object as a result. This works great when creating or modifying a single User, but gets a little muddy when we're talking about a batch of users. How should we associate the particular status with the particular user? Here we've just put it inside the User object. It will only be filled in the response to a batch message.

This comment has been minimized.

@cesarghali

cesarghali Dec 5, 2017

Collaborator

SG, thanks for the explanation!

@cesarghali

cesarghali Dec 5, 2017

Collaborator

SG, thanks for the explanation!

@gdbelvin

This comment has been minimized.

Show comment
Hide comment
@gdbelvin

gdbelvin Dec 5, 2017

Collaborator

Updated PTAL

Collaborator

gdbelvin commented Dec 5, 2017

Updated PTAL

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 5, 2017

Codecov Report

Merging #864 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
+ Coverage   48.78%   48.83%   +0.04%     
==========================================
  Files          34       34              
  Lines        2353     2351       -2     
==========================================
  Hits         1148     1148              
+ Misses       1001      999       -2     
  Partials      204      204
Impacted Files Coverage Δ
core/adminserver/admin_server.go 51.61% <ø> (+1.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4d8e41...8625066. Read the comment docs.

codecov-io commented Dec 5, 2017

Codecov Report

Merging #864 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
+ Coverage   48.78%   48.83%   +0.04%     
==========================================
  Files          34       34              
  Lines        2353     2351       -2     
==========================================
  Hits         1148     1148              
+ Misses       1001      999       -2     
  Partials      204      204
Impacted Files Coverage Δ
core/adminserver/admin_server.go 51.61% <ø> (+1.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4d8e41...8625066. Read the comment docs.

@codingllama

I've added a few API comments / questions. I don't consider any of my comments blocking, so feel free to proceed if you like. Consider this a drive-by review ;)

Show outdated Hide outdated core/managementserver/server.go Outdated
Show outdated Hide outdated core/api/usermanager/v1/usermanager_proto/usermanager.proto Outdated
Show outdated Hide outdated core/api/usermanager/v1/usermanager_proto/usermanager.proto Outdated
Show outdated Hide outdated core/api/usermanager/v1/usermanager_proto/usermanager.proto Outdated
Show outdated Hide outdated core/api/usermanager/v1/usermanager_proto/usermanager.proto Outdated
body: "user"
};
}

This comment has been minimized.

@codingllama

codingllama Dec 7, 2017

Do you need to include Get / ListUsers here, or are those covered by another API? If the latter, it may be worth mentioned that somewhere in the docs (ie, "to read users use $otherAPI.$someMethod" or something like it.)

@codingllama

codingllama Dec 7, 2017

Do you need to include Get / ListUsers here, or are those covered by another API? If the latter, it may be worth mentioned that somewhere in the docs (ie, "to read users use $otherAPI.$someMethod" or something like it.)

type Server struct{}
// New creates a new managementserver
func New() *Server {

This comment has been minimized.

@codingllama

codingllama Dec 7, 2017

Do you need the indirection here, or could users just do s := &Server{} ?

@codingllama

codingllama Dec 7, 2017

Do you need the indirection here, or could users just do s := &Server{} ?

This comment has been minimized.

@gdbelvin

gdbelvin Dec 7, 2017

Collaborator

This function will be expanding to take arguments in the future :-)
I quite like calling New to create objects. Is that not canonical?

@gdbelvin

gdbelvin Dec 7, 2017

Collaborator

This function will be expanding to take arguments in the future :-)
I quite like calling New to create objects. Is that not canonical?

This comment has been minimized.

@codingllama

codingllama Dec 7, 2017

A New() func is OK, but since Server is exported, a trivial New might get you worse readability than a Server literal (because of field labels vs method params).

This is just my 2c though.

@codingllama

codingllama Dec 7, 2017

A New() func is OK, but since Server is exported, a trivial New might get you worse readability than a Server literal (because of field labels vs method params).

This is just my 2c though.

// this service has for a given domain and app.
rpc GetKeySet (GetKeySetRequest) returns (keymaster.proto.KeySet) {
option (google.api.http) = {
get: "/usermanager/v1/domains/{domain_id}/apps/{app_id}/keyset"

This comment has been minimized.

@codingllama

codingllama Dec 7, 2017

Since you have an implied 1:1 relationship between KeySets and Apps would it make sense to return them as part of the App entity? That would mean you have a "GetApp" method instead of "GetKeySet".

"No" is perfectly fine answer here, specially if there are motives for the design that I might be missing.

@codingllama

codingllama Dec 7, 2017

Since you have an implied 1:1 relationship between KeySets and Apps would it make sense to return them as part of the App entity? That would mean you have a "GetApp" method instead of "GetKeySet".

"No" is perfectly fine answer here, specially if there are motives for the design that I might be missing.

This comment has been minimized.

@gdbelvin

gdbelvin Dec 7, 2017

Collaborator

Interesting idea.
If the App type defined in usermanager is allowed to be different than the main API, then yes, we could do that. It is possible, however, that we may want to make the keyset end point more broadly available than the other info we might want to put in GetApp

@gdbelvin

gdbelvin Dec 7, 2017

Collaborator

Interesting idea.
If the App type defined in usermanager is allowed to be different than the main API, then yes, we could do that. It is possible, however, that we may want to make the keyset end point more broadly available than the other info we might want to put in GetApp

// by everyone before they are used to avoid service disruption.
//
// Resources:
// - domains/apps/keyset

This comment has been minimized.

@codingllama

codingllama Dec 7, 2017

Is it worth mentioning how Domains, Apps and KeySets are created? (IIRC, you have a pre-defined number of them in config files, right?)

@codingllama

codingllama Dec 7, 2017

Is it worth mentioning how Domains, Apps and KeySets are created? (IIRC, you have a pre-defined number of them in config files, right?)

This comment has been minimized.

@gdbelvin

gdbelvin Dec 7, 2017

Collaborator

I'll add a line about it. Thanks for thinking of it.

@gdbelvin

gdbelvin Dec 7, 2017

Collaborator

I'll add a line about it. Thanks for thinking of it.

@gdbelvin gdbelvin merged commit 37e5cae into google:master Dec 7, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment