Milo: ACL support

Milo: ACL support

This just includes a IsAllowed() helper function, that gets called by each
module (buildbot, buildbucket, etc)

BUG=637375
Committed: https://github.com/luci/luci-go/commit/c6255395674c45b38e0cebd6c391169125745a89

[Ryan Tseng, 2016-08-12 18:56:04]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 18:56:13]

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

[Ryan Tseng, 2016-08-12 18:56:49]

Remove debugging

[Ryan Tseng, 2016-08-12 18:56:50]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 18:56:53]

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

[Ryan Tseng, 2016-08-12 18:59:50]

hinoka@google.com changed reviewers:
+ nodir@chromium.org, vadimsh@chromium.org

[Ryan Tseng, 2016-08-12 18:59:51]

ptal, this will be used later to add acls to build views, console views, etc

[Ryan Tseng, 2016-08-12 18:59:59]

Description was changed from

==========
Milo: Acl support

This just includes a IsAllowed() helper function, that gets called by each
module (buildbot, buildbucket, etc)

BUG=637375
==========

to

==========
Milo: ACL support

This just includes a IsAllowed() helper function, that gets called by each
module (buildbot, buildbucket, etc)

BUG=637375
==========

[commit-bot: I haz the power, 2016-08-12 19:03:23]

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

[commit-bot: I haz the power, 2016-08-12 19:03:23]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-08-15 22:04:52]

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
File milo/appengine/settings/acl.go (right):

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
milo/appengine/settings/acl.go:19: func IsAllowed(c context.Context, project
string, write bool) bool {
is 'write' used anywhere currently (or going to be used very soon)? If not,
let's keep it out for now. I have a feeling it may end up as a dead code.

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
milo/appengine/settings/acl.go:24: return false
the function should return (bool, error) tuple and return (false, error) in this
case.

"No access" is not the same as "failed to check access". When access check
fails, the user should see HTTP 500 (not HTTP 403 as in the case of "no
access").

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
milo/appengine/settings/acl.go:46: if _, ok := users["public"]; ok {
no need for this:
1) We have group "all", that includes anonymous users. Here's logdog using it:
https://chromium.googlesource.com/infra/infra/+/infra/config/luci-logdog.cfg#9
2) The check, as it is now, will prevent logged in users to view public entries.
(Because they won't pass check on line 45).

[Ryan Tseng, 2016-08-16 00:00:23]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 00:00:34]

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

[Ryan Tseng, 2016-08-16 00:03:19]

Simplified

[Ryan Tseng, 2016-08-16 00:03:20]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 00:03:29]

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

[Ryan Tseng, 2016-08-16 00:03:56]

Removed write ACLs, using chrome-infra-auth for public read acls, it's a lot
shorter now

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
File milo/appengine/settings/acl.go (right):

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
milo/appengine/settings/acl.go:19: func IsAllowed(c context.Context, project
string, write bool) bool {
On 2016/08/15 22:04:52, Vadim Sh. wrote:
> is 'write' used anywhere currently (or going to be used very soon)? If not,
> let's keep it out for now. I have a feeling it may end up as a dead code.

Removing for now.  It's not currently used anywhere.  Will add back in if it's
required somehow.

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
milo/appengine/settings/acl.go:24: return false
On 2016/08/15 22:04:52, Vadim Sh. wrote:
> the function should return (bool, error) tuple and return (false, error) in
this
> case.
> 
> "No access" is not the same as "failed to check access". When access check
> fails, the user should see HTTP 500 (not HTTP 403 as in the case of "no
> access").

Done.

https://codereview.chromium.org/2241853002/diff/20001/milo/appengine/settings...
milo/appengine/settings/acl.go:46: if _, ok := users["public"]; ok {
On 2016/08/15 22:04:52, Vadim Sh. wrote:
> no need for this:
> 1) We have group "all", that includes anonymous users. Here's logdog using it:
> https://chromium.googlesource.com/infra/infra/+/infra/config/luci-logdog.cfg#9
> 2) The check, as it is now, will prevent logged in users to view public
entries.
> (Because they won't pass check on line 45).

Done.

[commit-bot: I haz the power, 2016-08-16 00:09:59]

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

[commit-bot: I haz the power, 2016-08-16 00:10:00]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-08-16 00:21:22]

https://codereview.chromium.org/2241853002/diff/60001/milo/appengine/settings...
File milo/appengine/settings/acl_test.go (right):

https://codereview.chromium.org/2241853002/diff/60001/milo/appengine/settings...
milo/appengine/settings/acl_test.go:77: type testingAuthDB struct{}
you can probably avoid most of this mocks if you use stuff from
https://godoc.org/github.com/luci/luci-go/server/auth/authtest

in particular,
https://godoc.org/github.com/luci/luci-go/server/auth/authtest#FakeDB and
https://godoc.org/github.com/luci/luci-go/server/auth/authtest#FakeState

[Ryan Tseng, 2016-08-16 00:37:20]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 00:37:29]

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

[Ryan Tseng, 2016-08-16 00:38:07]

https://codereview.chromium.org/2241853002/diff/60001/milo/appengine/settings...
File milo/appengine/settings/acl_test.go (right):

https://codereview.chromium.org/2241853002/diff/60001/milo/appengine/settings...
milo/appengine/settings/acl_test.go:77: type testingAuthDB struct{}
On 2016/08/16 00:21:22, Vadim Sh. wrote:
> you can probably avoid most of this mocks if you use stuff from
> https://godoc.org/github.com/luci/luci-go/server/auth/authtest
> 
> in particular,
> https://godoc.org/github.com/luci/luci-go/server/auth/authtest#FakeDB and
> https://godoc.org/github.com/luci/luci-go/server/auth/authtest#FakeState

Done.

[Vadim Sh., 2016-08-16 00:40:38]

lgtm

[commit-bot: I haz the power, 2016-08-16 00:44:20]

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

[commit-bot: I haz the power, 2016-08-16 00:44:21]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2016-08-19 00:03:09]

rebase

[Ryan Tseng, 2016-08-19 00:03:10]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 00:03:12]

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

[Ryan Tseng, 2016-08-19 00:03:20]

The CQ bit was unchecked by hinoka@google.com

[Ryan Tseng, 2016-08-19 00:03:23]

The CQ bit was checked by hinoka@google.com

[Ryan Tseng, 2016-08-19 00:03:24]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2241853002/#ps90001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-19 00:03:32]

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

[commit-bot: I haz the power, 2016-08-19 00:13:59]

Description was changed from

==========
Milo: ACL support

This just includes a IsAllowed() helper function, that gets called by each
module (buildbot, buildbucket, etc)

BUG=637375
==========

to

==========
Milo: ACL support

This just includes a IsAllowed() helper function, that gets called by each
module (buildbot, buildbucket, etc)

BUG=637375

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

[commit-bot: I haz the power, 2016-08-19 00:14:00]

Committed patchset #6 (id:90001) as
https://github.com/luci/luci-go/commit/c6255395674c45b38e0cebd6c391169125745a89