server/config: Service-side project access checks.

server/config: Service-side project access checks.

Add the ability for services to mirror the project access checks that
the "luci-config" service performs. This targets caching and testing
support, enabling non-service authorities to use cached service entries
by pruning the results in the service.

BUG=chromium:674378
TEST=unit

Review-Url: https://codereview.chromium.org/2576993002
Committed: https://github.com/luci/luci-go/commit/99375447064d421783741c2e11ef4bf736839c9d

[dnj, 2016-12-15 05:10:40]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2016-12-15 05:10:41]

PTAL. Full stack is:
https://codereview.chromium.org/2580713002 (base)
https://codereview.chromium.org/2578893002 (textproto)
https://codereview.chromium.org/2576993002 <-- this
https://codereview.chromium.org/2576343002 (erroring)
https://codereview.chromium.org/2582433002 (test)
https://codereview.chromium.org/2573403002 (caching)
https://codereview.chromium.org/2575383002 (gaeconfig)
https://codereview.chromium.org/2576923003 (datastore cache)

[iannucci, 2017-01-07 20:29:22]

iannucci@chromium.org changed reviewers:
+ vadimsh@chromium.org

[iannucci, 2017-01-07 20:29:23]

looks ok, but vadim should comment on the comments

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access.go
File server/config/access/access.go (right):

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access...
server/config/access/access.go:5: // Package access implements a software
config.Authority access check against a
not sure `software` is the right adjective. maybe 'application local'. 'best
effort'?

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access...
server/config/access/access.go:64: // If "group" is "all", then short-circuit to
permitted.
!!! but `all` is a valid group name!

can't we make this `group:*` instead, since * is not a valid group name?

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access...
server/config/access/access.go:72: // If there is no ":" in the access string,
this is a user ACL.
bummer that we couldn't have made the access string format the same as the
identity format (e.g. require `user:something@something.somewhere`)

[Vadim Sh., 2017-01-07 21:04:23]

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access.go
File server/config/access/access.go (right):

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access...
server/config/access/access.go:64: // If "group" is "all", then short-circuit to
permitted.
On 2017/01/07 20:29:23, iannucci wrote:
> !!! but `all` is a valid group name!
> 
> can't we make this `group:*` instead, since * is not a valid group name?

We should just remove this short-circuit (if luci-py does it, there too). "all"
is valid group, it exists for real, and it is basically "*:*" -
https://chrome-infra-auth.appspot.com/auth/groups#all

(memberships checks are ~= free in terms of RPC cost, no need for short
circuits).

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access...
server/config/access/access.go:72: // If there is no ":" in the access string,
this is a user ACL.
On 2017/01/07 20:29:23, iannucci wrote:
> bummer that we couldn't have made the access string format the same as the
> identity format (e.g. require `user:something@something.somewhere`)

It is the same. "user:" is just default prefix.

So one can use require: "abc@example.com" AND require: "user:abc@example.com"
(or whatever)

[dnj, 2017-01-10 03:27:13]

Updated, PTAL.

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access.go
File server/config/access/access.go (right):

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access...
server/config/access/access.go:5: // Package access implements a software
config.Authority access check against a
On 2017/01/07 20:29:23, iannucci wrote:
> not sure `software` is the right adjective. maybe 'application local'. 'best
> effort'?

Done.

https://codereview.chromium.org/2576993002/diff/1/server/config/access/access...
server/config/access/access.go:64: // If "group" is "all", then short-circuit to
permitted.
On 2017/01/07 21:04:23, Vadim Sh. wrote:
> On 2017/01/07 20:29:23, iannucci wrote:
> > !!! but `all` is a valid group name!
> > 
> > can't we make this `group:*` instead, since * is not a valid group name?
> 
> We should just remove this short-circuit (if luci-py does it, there too).
"all"
> is valid group, it exists for real, and it is basically "*:*" -
> https://chrome-infra-auth.appspot.com/auth/groups#all
> 
> (memberships checks are ~= free in terms of RPC cost, no need for short
> circuits).

Done.

[dnj, 2017-01-10 23:15:43]

(Ping)

[iannucci, 2017-01-11 03:02:53]

lgtm

[iannucci, 2017-01-11 03:03:48]

On 2017/01/11 03:02:53, iannucci wrote:
> lgtm

Oops I hit the button by accident. Reviewing for real right now.

[iannucci, 2017-01-11 03:06:51]

actually lgtm now

[dnj, 2017-01-11 03:26:34]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2017-01-11 03:26:37]

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, 2017-01-11 03:32:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484105194162250,
"parent_rev": "18514e2d34000278bc46539c92a08c95692f2dd4", "commit_rev":
"99375447064d421783741c2e11ef4bf736839c9d"}

[commit-bot: I haz the power, 2017-01-11 03:32:44]

Description was changed from

==========
server/config: Service-side project access checks.

Add the ability for services to mirror the project access checks that
the "luci-config" service performs. This targets caching and testing
support, enabling non-service authorities to use cached service entries
by pruning the results in the service.

BUG=chromium:674378
TEST=unit
==========

to

==========
server/config: Service-side project access checks.

Add the ability for services to mirror the project access checks that
the "luci-config" service performs. This targets caching and testing
support, enabling non-service authorities to use cached service entries
by pruning the results in the service.

BUG=chromium:674378
TEST=unit

Review-Url: https://codereview.chromium.org/2576993002
Committed:
https://github.com/luci/luci-go/commit/99375447064d421783741c2e11ef4bf736839c9d
==========

[commit-bot: I haz the power, 2017-01-11 03:32:45]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-go/commit/99375447064d421783741c2e11ef4bf736839c9d