auth_service: Add '/memberships/list' and '/memberships/check' API.

auth_service: Add '/memberships/list' and '/memberships/check' API.

BUG=671456, 671451
R=tandrii@chromium.org

Review-Url: https://codereview.chromium.org/2550143003
Committed: https://github.com/luci/luci-py/commit/7067fd8f75d5eae5d849cc6695b422469a7b7063

[Vadim Sh., 2016-12-06 05:09:03]

PTAL

Adding tests now.

This CL is mostly about formats of requests and response. Read the doc here:
https://chrome-infra-auth-dev.appspot.com/auth/api

Example response:
https://chrome-infra-auth-dev.appspot.com/auth/api/v1/memberships/list?identi...

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/rest_api.py (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:933: 'verb': 'POST',
batched queries and REST API don't mix well :( I decided to abuse POST body for
submitting batched requests. It gives at least some kind of consistency.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:1002: {'group': g} for g in
sorted(auth_db.fetch_groups_with_member(ident))
It is {'memberships': ...} dict and {'group': ...} dicts (vs naked list of dicts
or naked list of groups) to be forward compatible: if we suddenly need to add a
new response field (e.g. date of membership or something), there's a place where
to add it.

[tandrii(chromium), 2016-12-06 12:56:47]

thanks, you are very fast :)

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/rest_api.py (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:933: 'verb': 'POST',
On 2016/12/06 05:09:03, Vadim Sh. wrote:
> batched queries and REST API don't mix well :( I decided to abuse POST body
for
> submitting batched requests. It gives at least some kind of consistency.

Acknowledged.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:970: if not
isinstance(per_identity, dict) or not per_identity:
why is per_identity a dict and not a list?

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:988: # Params are not used
currently, but they must be dict in any case.
ah, i see -> that's why dict, would be nice to say so upfront :)

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:1002: {'group': g} for g in
sorted(auth_db.fetch_groups_with_member(ident))
On 2016/12/06 05:09:03, Vadim Sh. wrote:
> It is {'memberships': ...} dict and {'group': ...} dicts (vs naked list of
dicts
> or naked list of groups) to be forward compatible: if we suddenly need to add
a
> new response field (e.g. date of membership or something), there's a place
where
> to add it.

Acknowledged.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:1025: 'response_type':
'Memberships',
IMO, this should be two distinct API calls. For Gerrit, AFAIU, we only need one
of them, so maybe just have that one?

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:1082: for ident_str, params
in per_identity.iteritems():
this seems like a lot of copy-pasta from above.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/templates/api.html (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/templates/api.html:42: <th style="width:
260px;">Path</th>
my way of doing UI :D

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/ui.py (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/ui.py:492: 'parallel. Dict keys match
query parameters or the non-batched call '
s/or/of ? if not, please rephrase - i didn't get it.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/ui.py:493: 'and have same defaults.',
the same

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/ui.py:549: path = simplified
why this change?

[Vadim Sh., 2016-12-07 01:10:00]

Description was changed from

==========
auth_service: Add '/memberships/list' and '/memberships/query' API.

BUG=671456, 671451
R=tandrii@chromium.org
==========

to

==========
auth_service: Add '/memberships/list' and '/memberships/check' API.

BUG=671456, 671451
R=tandrii@chromium.org
==========

[Vadim Sh., 2016-12-07 01:18:40]

If you really insist, I can split documentation related stuff into a separate
CL.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/rest_api.py (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:970: if not
isinstance(per_identity, dict) or not per_identity:
On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> why is per_identity a dict and not a list?

Done.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:988: # Params are not used
currently, but they must be dict in any case.
On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> ah, i see -> that's why dict, would be nice to say so upfront :)

Done.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:1025: 'response_type':
'Memberships',
On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> IMO, this should be two distinct API calls. For Gerrit, AFAIU, we only need
one
> of them, so maybe just have that one?

Done. There's /memberships/list and /memberships/check call now.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:1082: for ident_str, params
in per_identity.iteritems():
On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> this seems like a lot of copy-pasta from above.

Extracted some into a separate base class.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/templates/api.html (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/templates/api.html:42: <th style="width:
260px;">Path</th>
On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> my way of doing UI :D

It looks OK on my monitor :)

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/ui.py (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/ui.py:492: 'parallel. Dict keys match
query parameters or the non-batched call '
On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> s/or/of ? if not, please rephrase - i didn't get it.

Done.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/ui.py:549: path = simplified
On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> why this change?

To show "?identity=..." on API doc page:
https://chrome-infra-auth-dev.appspot.com/auth/api (here specifically:
https://screenshot.googleplex.com/cgoxmiz3E0n.png

https://codereview.chromium.org/2550143003/diff/20001/appengine/components/co...
File appengine/components/components/auth/ui/ui.py (right):

https://codereview.chromium.org/2550143003/diff/20001/appengine/components/co...
appengine/components/components/auth/ui/ui.py:475: def get(self):
this stuff renders this page: https://chrome-infra-auth-dev.appspot.com/auth/api

It has a notion of "JSON document type". Some of such types (like "Group" and
"Status") are used by many requests, and so they are specified centrally in
doc_types above.

Before this CL, it was the only place to document a doc type. 

New handlers use very specific request and response types that unlikely to be
reused. I've modified this code to allow documenting such "unique" types closer
to where they are used, because splitting the doc between two files is not very
good.

[Vadim Sh., 2016-12-09 23:33:11]

ping

[tandrii(chromium), 2016-12-12 17:07:12]

On 2016/12/07 01:18:40, Vadim Sh. wrote:
> If you really insist, I can split documentation related stuff into a separate
> CL.
I don't think I meant to insist on that, and I definitely don't now.

[tandrii(chromium), 2016-12-12 17:07:17]

lgtm

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/rest_api.py (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/rest_api.py:1082: for ident_str, params
in per_identity.iteritems():
On 2016/12/07 01:18:40, Vadim Sh. wrote:
> On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> > this seems like a lot of copy-pasta from above.
> 
> Extracted some into a separate base class.

Nice.

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
File appengine/components/components/auth/ui/ui.py (right):

https://codereview.chromium.org/2550143003/diff/1/appengine/components/compon...
appengine/components/components/auth/ui/ui.py:549: path = simplified
On 2016/12/07 01:18:40, Vadim Sh. wrote:
> On 2016/12/06 12:56:46, tandrii(chromium) wrote:
> > why this change?
> 
> To show "?identity=..." on API doc page:
> https://chrome-infra-auth-dev.appspot.com/auth/api (here specifically:
> https://screenshot.googleplex.com/cgoxmiz3E0n.png

Acknowledged.

https://codereview.chromium.org/2550143003/diff/20001/appengine/components/co...
File appengine/components/components/auth/ui/rest_api.py (right):

https://codereview.chromium.org/2550143003/diff/20001/appengine/components/co...
appengine/components/components/auth/ui/rest_api.py:1066: '(in a same format as
non-batched version).',
s/a same/the same

[Vadim Sh., 2016-12-13 00:24:11]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2016-12-13 00:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org
Link to the patchset: https://codereview.chromium.org/2550143003/#ps40001
(title: "add tests and more docs")

[commit-bot: I haz the power, 2016-12-13 00:24:14]

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-12-13 00:27:29]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481588651916350,
"parent_rev": "2ebd542bd0771108214f27634ac8c17a26a43808", "commit_rev":
"7067fd8f75d5eae5d849cc6695b422469a7b7063"}

[commit-bot: I haz the power, 2016-12-13 00:27:31]

Description was changed from

==========
auth_service: Add '/memberships/list' and '/memberships/check' API.

BUG=671456, 671451
R=tandrii@chromium.org
==========

to

==========
auth_service: Add '/memberships/list' and '/memberships/check' API.

BUG=671456, 671451
R=tandrii@chromium.org

Review-Url: https://codereview.chromium.org/2550143003
Committed:
https://github.com/luci/luci-py/commit/7067fd8f75d5eae5d849cc6695b422469a7b7063
==========

[commit-bot: I haz the power, 2016-12-13 00:27:32]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-py/commit/7067fd8f75d5eae5d849cc6695b422469a7b7063