LogDog: Project READ access for user endpoints.

LogDog: Project READ access for user endpoints.

Update LogDog Coordinator to load per-project configurations and use their
READ ACLs to enforce log access.

Previously, any user can read any log stream. After this CL, they have to
be explicitly granted READ permission in the project's configuration.

This also redefines project access for the list endpoint. Previously, the
user could view the project if the top-level project ACL permitted them.
Now, they can view any project that they have READ access to.

BUG=

Committed: https://github.com/luci/luci-go/commit/f0242c6fc473f8af6acc9aa9b01af4a16680cfa9

[dnj, 2016-05-11 04:55:25]

dnj@chromium.org changed reviewers:
+ estaab@chromium.org, iannucci@chromium.org, vadimsh@chromium.org

[dnj, 2016-05-11 04:55:26]

PTAL, this adds ACLs :)

[dnj, 2016-05-17 20:33:47]

Description was changed from

==========
LogDog: Project READ access for user endpoints.

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

to

==========
LogDog: Project READ access for user endpoints.

Update LogDog Coordinator to load per-project configurations and use their
READ ACLs to enforce log access.

Previously, any user can read any log stream. After this CL, they have to
be explicitly granted READ permission in the project's configuration.

This also redefines project access for the list endpoint. Previously, the
user could view the project if the top-level project ACL permitted them.
Now, they can view any project that they have READ access to.

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

[nodir, 2016-05-19 00:38:57]

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

[nodir, 2016-05-19 00:38:59]

https://codereview.chromium.org/1971493003/diff/20001/appengine/logdog/coordi...
File appengine/logdog/coordinator/project.go (right):

https://codereview.chromium.org/1971493003/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:16: "golang.org/x/net/context"
should be in a separate import block

https://codereview.chromium.org/1971493003/diff/20001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:22: projectNamespacePrefix = "luci."
I'd definitely use "project." so namespaces are "project.chromium" and so on. It
makes more sense to me than luci.chromium

later: but it is too late

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/auth.go (right):

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:22: // If the user is not a member of any
groups, a MembershipError will be
which "any groups" you are referring to in the context of docstring? I think you
meant "If the user is not, a MembershipError will be returned"...

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:35: // If the user is not a member of any
groups, a MembershipError will be
same here

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:48: // If the user is not a member of any
groups, a MembershipError will be
english: "any group"?

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/context.go (right):

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:37: // invalid
period missing

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/registerStream.go (right):

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:45: if d :=
pcfg.MaxStreamAge.Duration(); d > 0 && d < delay {
if it is not set in global config, but set in  project config, this condition
will be false because delay will be zero

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/project.go (right):

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:74: projects, err := config.Projects(c)
use
https://godoc.org/github.com/luci/luci-go/common/config#Interface.GetProjectC...

[dnj (Google), 2016-05-19 16:12:33]

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

[dnj (Google), 2016-05-19 16:12:34]

I've updated a few places to use GetProjectConfigs (cool endpoint, didn't know
it existed until Vadim showed me yesterday actually). Much better now :)

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/auth.go (right):

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:22: // If the user is not a member of any
groups, a MembershipError will be
On 2016/05/19 00:38:58, nodir wrote:
> which "any groups" you are referring to in the context of docstring? I think
you
> meant "If the user is not, a MembershipError will be returned"...

Done.

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:35: // If the user is not a member of any
groups, a MembershipError will be
On 2016/05/19 00:38:58, nodir wrote:
> same here

Done.

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:48: // If the user is not a member of any
groups, a MembershipError will be
On 2016/05/19 00:38:58, nodir wrote:
> english: "any group"?

Yeah this comment has gone through a few stages of copy/paste and function
signature change lol

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/context.go (right):

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/context.go:37: // invalid
On 2016/05/19 00:38:58, nodir wrote:
> period missing

Done.

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/services/registerStream.go (right):

https://codereview.chromium.org/1971493003/diff/40001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/services/registerStream.go:45: if d :=
pcfg.MaxStreamAge.Duration(); d > 0 && d < delay {
On 2016/05/19 00:38:58, nodir wrote:
> if it is not set in global config, but set in  project config, this condition
> will be false because delay will be zero

Good catch. Since I use this same method to constrain prefix expiration, I'll
generalize it.

[nodir, 2016-05-19 16:48:32]

Yeah, I have concerns it won't scale with a number of projects though

[nodir, 2016-05-19 17:17:21]

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/auth.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:21: //
I think separating these two paragraphs is unnecessary now

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/config/projects.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/config/projects.go:68: // config set, which we
ignore, so passing an empty string is acceptable.
the alternative is to split the func into two and not write this comment.  There
is no common computation.
The first one should have been present in config module..

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/config/projects.go:70: configs, err :=
config.Get(c).GetProjectConfigs(path, false)
note that this endpoint is slow. Please mention in the AllProjectConfigs that
this function is slow and should be used in serving code, only cron jobs

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/config/projects.go:72: log.WithError(err).Errorf(c,
"Failed to list 'luci-config' projects.")
this err message is not accurate

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/util.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/util.go:17: func MinDuration(candidates
...*google.Duration) (exp time.Duration) {
I like that you called it Min. It is more concrete that resolve

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/project.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:68: // a LogDog project configurations
that the current user has READ access to.
s/a //

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:70: allPcfgs, err :=
config.AllProjectConfigs(c)
I'd strongly recommend calling this function in a cron job and persisting
results to datastore; and then read from datastore here.

Then you can update your other functions for config loading to load from
datastore.
I am sorry I writing this only now. We should probably write a package that does
this sort of replication because all apps need it. If you want to do that, I can
review the code

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:75: for project, pcfg := range allPcfgs
{
add a TODO to make acl check concurrent. Auth groups can be large, so checks may
take time and we may have a lot of projects

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:76: // Verify user access. This includes
loading the project's config.
it does not include that

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:78: delete(allPcfgs, project)
this makes me nervous. I'd rather create a separate map with restricted set and
add projects there explicitly. Anyway you will probably do the explicit addition
if you make this code parallel because you will send accessible projects to a
channel

[dnj (Google), 2016-05-19 20:10:46]

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/auth.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/auth.go:21: //
On 2016/05/19 17:17:20, nodir wrote:
> I think separating these two paragraphs is unnecessary now

I'm okay with it. I like to separate the purpose of the function from its
behavior details.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/config/projects.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/config/projects.go:68: // config set, which we
ignore, so passing an empty string is acceptable.
On 2016/05/19 17:17:21, nodir wrote:
> the alternative is to split the func into two and not write this comment. 
There
> is no common computation.
> The first one should have been present in config module..

Done.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/config/projects.go:70: configs, err :=
config.Get(c).GetProjectConfigs(path, false)
On 2016/05/19 17:17:20, nodir wrote:
> note that this endpoint is slow. Please mention in the AllProjectConfigs that
> this function is slow and should be used in serving code, only cron jobs

Done.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/config/projects.go:72: log.WithError(err).Errorf(c,
"Failed to list 'luci-config' projects.")
On 2016/05/19 17:17:20, nodir wrote:
> this err message is not accurate

Done.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/endpoints/util.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/endpoints/util.go:17: func MinDuration(candidates
...*google.Duration) (exp time.Duration) {
On 2016/05/19 17:17:21, nodir wrote:
> I like that you called it Min. It is more concrete that resolve

Agreed.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
File appengine/logdog/coordinator/project.go (right):

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:68: // a LogDog project configurations
that the current user has READ access to.
On 2016/05/19 17:17:21, nodir wrote:
> s/a //

Done.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:70: allPcfgs, err :=
config.AllProjectConfigs(c)
On 2016/05/19 17:17:21, nodir wrote:
> I'd strongly recommend calling this function in a cron job and persisting
> results to datastore; and then read from datastore here.
> 
> Then you can update your other functions for config loading to load from
> datastore.
> I am sorry I writing this only now. We should probably write a package that
does
> this sort of replication because all apps need it. If you want to do that, I
can
> review the code

Agreed, and I'll put a TODO in, but this is not a performance bottleneck ATM.
This is an infrequently-called function at best, and memcached either way.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:75: for project, pcfg := range allPcfgs
{
On 2016/05/19 17:17:21, nodir wrote:
> add a TODO to make acl check concurrent. Auth groups can be large, so checks
may
> take time and we may have a lot of projects

If this is actually a problem, I think we need to optimize this lookup. It is
the sort of thing that needs to happen fast.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:76: // Verify user access. This includes
loading the project's config.
On 2016/05/19 17:17:21, nodir wrote:
> it does not include that

Done.

https://codereview.chromium.org/1971493003/diff/80001/appengine/logdog/coordi...
appengine/logdog/coordinator/project.go:78: delete(allPcfgs, project)
On 2016/05/19 17:17:21, nodir wrote:
> this makes me nervous. I'd rather create a separate map with restricted set
and
> add projects there explicitly. Anyway you will probably do the explicit
addition
> if you make this code parallel because you will send accessible projects to a
> channel

Agree about parallel construction.

I thought about this in response to your comment, but I think this is fine.
Delete is a reliable Go operation, and this logic is really simple: if there is
an error, remove the thing from the map. I am comfortable relying on this. Any
reconstruction around building another map will use the same checks and have
equivalent code paths.

[nodir, 2016-05-19 20:26:49]

lgtm

[dnj (Google), 2016-05-19 23:14:40]

The CQ bit was checked by dnj@google.com

[dnj (Google), 2016-05-19 23:14:40]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/1971493003/#ps120001
(title: "Updated patchset dependency")

[commit-bot: I haz the power, 2016-05-19 23:14:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971493003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971493003/120001

[commit-bot: I haz the power, 2016-05-19 23:20:46]

Description was changed from

==========
LogDog: Project READ access for user endpoints.

Update LogDog Coordinator to load per-project configurations and use their
READ ACLs to enforce log access.

Previously, any user can read any log stream. After this CL, they have to
be explicitly granted READ permission in the project's configuration.

This also redefines project access for the list endpoint. Previously, the
user could view the project if the top-level project ACL permitted them.
Now, they can view any project that they have READ access to.

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

to

==========
LogDog: Project READ access for user endpoints.

Update LogDog Coordinator to load per-project configurations and use their
READ ACLs to enforce log access.

Previously, any user can read any log stream. After this CL, they have to
be explicitly granted READ permission in the project's configuration.

This also redefines project access for the list endpoint. Previously, the
user could view the project if the top-level project ACL permitted them.
Now, they can view any project that they have READ access to.

BUG=

Committed:
https://github.com/luci/luci-go/commit/f0242c6fc473f8af6acc9aa9b01af4a16680cfa9
==========

[commit-bot: I haz the power, 2016-05-19 23:20:47]

Committed patchset #7 (id:120001) as
https://github.com/luci/luci-go/commit/f0242c6fc473f8af6acc9aa9b01af4a16680cfa9