config_service: Add authentication to the UI.

config_service: Add authentication to the UI.

Users can now sign in to view all projects and services.

This change involved creating a modified version of the google-signin-aware element that
works with Polymer 2.0. This modified file is in a third party folder.

Staging version can be found here: https://2941-71c3c10-tainted-cwpayton-dot-luci-config.appspot.com/newui

BUG=730832
Review-Url: https://codereview.chromium.org/2962703003
Committed: https://github.com/luci/luci-py/commit/160ef2c6cacca9617e67c94b758ff4303e3fe839

[cwpayton, 2017-06-28 00:10:21]

Description was changed from

==========
Added authentication to the UI. Users can now sign in to view all projects and
services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

BUG=730832
==========

to

==========
config_service: Added authentication to the UI. Users can now sign in to view
all projects and services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

BUG=730832
==========

[cwpayton, 2017-06-28 00:11:28]

cwpayton@google.com changed reviewers:
+ hinoka@chromium.org, sergeyberezin@chromium.org

[cwpayton, 2017-06-28 00:11:29]

PTAL

[cwpayton, 2017-06-28 16:28:39]

I added a few patches to make viewing my modifications easier. If you want to
see the changes I made to google-signin-aware.html, you can easily do so by
clicking the 2 in the "Delta from patch set" column. This will show the
differences between the original file and the file after my brief changes.

[Sergey Berezin, 2017-06-28 19:53:03]

Most of the code looks good, however this change is bigger than one CL can
allow, in particular due to the client_id (it requires changes in more than one
repo).

Also, when you're ready, please provide screenshot links and/or a link to the
staging version. Thanks!

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html
(right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html:2:
Copyright 2014 Google Inc
As discussed offline, let's commit the original version of this file in a
separate CL first, then rebase this CL on top of that with just your local
changes. This way, your changes will be cleanly preserved in the history.

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html:18:
<link rel="import"
href="../../../bower_components/google-apis/google-js-api.html">
This is probably fine for the our vendored copy, but will not be portable if we
try to submit this change upstream.

It appears this component expects to live in the same "bower_components"
subfolder together with its dependencies. One way to simulate this without
modifying the component is to symlink polymer.html and google-js-api.html from
the appropriate subfolders in third_party to the above paths. This way, this
change won't be necessary.

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html:244:
var that = this;
Is this renaming necessary? There is no function closure in the call below.

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-ui.html (right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-ui.html:88: <div
hidden$="[[!frontPageActive]]">
nit: would it also work if we wrap the subpages in <template is="dom-if"
if="[[frontPageActive]]"> ?

IIUC, this would remove inactive content from the DOM, and in particular,
disable all AJAX calls from the hidden elements. WDYT?

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File appengine/config_service/ui/static/index.html (right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/static/index.html:23:
client_id="247108661754-svmo17vmk1j5hlt388gb45qblgvg2h98.apps.googleusercontent.com">
I'd rather not bake a specific client ID into the code. One way to supply a
client_id is to generate index.html dynamically by the server, where the
client_id is injected into the html e.g. through jinja template.

Ayana had it setup earlier here:
https://codereview.chromium.org/2936423002/diff/60001/appengine/config_servic...
but it wasn't necessary at the time. Maybe it's time to revive it.

Another way is to add an endpoint to serve client_id and request it after
loading the page, but this will slow down overall page load, and may be
unnecessary complicated in Polymer.

We'd also need to add client_id entry to the config proto:
https://github.com/luci/luci-py/blob/master/appengine/components/components/c...
and then add the actual value in the specific configs in the internal repo (will
follow up offline for those). This is how the server would know its specific
client_id without baking it into the code.

Proto modifications should be done in a separate CL, and this CL will depend on
it.

[Sergey Berezin, 2017-06-28 19:54:09]

Description was changed from

==========
config_service: Added authentication to the UI. Users can now sign in to view
all projects and services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

BUG=730832
==========

to

==========
config_service: Add authentication to the UI.

Users can now sign in to view all projects and services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

BUG=730832
==========

[Sergey Berezin, 2017-06-28 19:55:16]

(I also updated the CL title and description)

[cwpayton, 2017-06-29 22:28:15]

PTAL

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html
(right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html:2:
Copyright 2014 Google Inc
On 2017/06/28 19:53:02, Sergey Berezin wrote:
> As discussed offline, let's commit the original version of this file in a
> separate CL first, then rebase this CL on top of that with just your local
> changes. This way, your changes will be cleanly preserved in the history.

Done.

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html:18:
<link rel="import"
href="../../../bower_components/google-apis/google-js-api.html">
On 2017/06/28 19:53:02, Sergey Berezin wrote:
> This is probably fine for the our vendored copy, but will not be portable if
we
> try to submit this change upstream.
> 
> It appears this component expects to live in the same "bower_components"
> subfolder together with its dependencies. One way to simulate this without
> modifying the component is to symlink polymer.html and google-js-api.html from
> the appropriate subfolders in third_party to the above paths. This way, this
> change won't be necessary.

Acknowledged.

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-ui.html (right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-ui.html:88: <div
hidden$="[[!frontPageActive]]">
On 2017/06/28 19:53:02, Sergey Berezin wrote:
> nit: would it also work if we wrap the subpages in <template is="dom-if"
> if="[[frontPageActive]]"> ?
> 
> IIUC, this would remove inactive content from the DOM, and in particular,
> disable all AJAX calls from the hidden elements. WDYT?

This sounds like it would work a bit better for what we're trying to do. I'll
integrate this change now.

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File appengine/config_service/ui/static/index.html (right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/static/index.html:23:
client_id="247108661754-svmo17vmk1j5hlt388gb45qblgvg2h98.apps.googleusercontent.com">
On 2017/06/28 19:53:02, Sergey Berezin wrote:
> I'd rather not bake a specific client ID into the code. One way to supply a
> client_id is to generate index.html dynamically by the server, where the
> client_id is injected into the html e.g. through jinja template.
> 
> Ayana had it setup earlier here:
>
https://codereview.chromium.org/2936423002/diff/60001/appengine/config_servic...
> but it wasn't necessary at the time. Maybe it's time to revive it.
> 
> Another way is to add an endpoint to serve client_id and request it after
> loading the page, but this will slow down overall page load, and may be
> unnecessary complicated in Polymer.
> 
> We'd also need to add client_id entry to the config proto:
>
https://github.com/luci/luci-py/blob/master/appengine/components/components/c...
> and then add the actual value in the specific configs in the internal repo
(will
> follow up offline for those). This is how the server would know its specific
> client_id without baking it into the code.
> 
> Proto modifications should be done in a separate CL, and this CL will depend
on
> it.

I fixed this by serving the client_id through a jinja template for now. We can
discuss proto modifications more offline.

[cwpayton, 2017-06-29 23:04:35]

Description was changed from

==========
config_service: Add authentication to the UI.

Users can now sign in to view all projects and services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

BUG=730832
==========

to

==========
config_service: Add authentication to the UI.

Users can now sign in to view all projects and services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

Staging version can be found here:
https://2941-71c3c10-tainted-cwpayton-dot-luci-config.appspot.com/newui

BUG=730832
==========

[Sergey Berezin, 2017-06-29 23:52:54]

LGTM with a few more comments. Thanks!

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html
(right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html:244:
var that = this;
On 2017/06/28 19:53:02, Sergey Berezin wrote:
> Is this renaming necessary? There is no function closure in the call below.

What about this line?

https://codereview.chromium.org/2962703003/diff/80001/appengine/config_servic...
File appengine/config_service/handlers.py (right):

https://codereview.chromium.org/2962703003/diff/80001/appengine/config_servic...
appengine/config_service/handlers.py:39:
'247108661754-svmo17vmk1j5hlt388gb45qblgvg2h98.apps.googleusercontent.com'
nit: let's add a TODO to make this configurable.

https://codereview.chromium.org/2962703003/diff/80001/appengine/config_servic...
File appengine/config_service/ui/bower.json (right):

https://codereview.chromium.org/2962703003/diff/80001/appengine/config_servic...
appengine/config_service/ui/bower.json:25: "iron-a11y-keys-behavior": "1 - 2",
Let's resolve this and the one below to more specific versions (check which ones
your bower actually installed).

[cwpayton, 2017-06-30 00:12:48]

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
File
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html
(right):

https://codereview.chromium.org/2962703003/diff/40001/appengine/config_servic...
appengine/config_service/ui/common/third_party/google-signin/google-signin-aware.html:244:
var that = this;
On 2017/06/29 23:52:53, Sergey Berezin wrote:
> On 2017/06/28 19:53:02, Sergey Berezin wrote:
> > Is this renaming necessary? There is no function closure in the call below.
> 
> What about this line?

This line is in here because of the way the "this" keyword works in JavaScript.
I'm pretty sure this is standard across most languages, but at least in JS the
"this" keyword belongs to the object that called the method that "this" was used
in. So in this case, if I don't set "var that = this", the line that says
"this.initAuth2.bind(this)" won't work because the "this" that it is referring
to is the gapi object because it is the gapi object that called the "load"
method, NOT the AuthEngine object. So in order to use AuthEngine's methods, I
need to preserve the "this" that represents the AuthEngine.

https://codereview.chromium.org/2962703003/diff/80001/appengine/config_servic...
File appengine/config_service/handlers.py (right):

https://codereview.chromium.org/2962703003/diff/80001/appengine/config_servic...
appengine/config_service/handlers.py:39:
'247108661754-svmo17vmk1j5hlt388gb45qblgvg2h98.apps.googleusercontent.com'
On 2017/06/29 23:52:53, Sergey Berezin wrote:
> nit: let's add a TODO to make this configurable.

Done.

[cwpayton, 2017-06-30 00:12:59]

The CQ bit was checked by cwpayton@google.com

[cwpayton, 2017-06-30 00:13:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/2962703003/#ps100001
(title: "Added TODO to handlers.py and confirmed versions in bower.json")

[commit-bot: I haz the power, 2017-06-30 00:13:08]

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-06-30 00:15:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498781579611110,
"parent_rev": "33233c3d9841007f0e3a46f8b0300982b0bc3a53", "commit_rev":
"160ef2c6cacca9617e67c94b758ff4303e3fe839"}

[commit-bot: I haz the power, 2017-06-30 00:15:42]

Description was changed from

==========
config_service: Add authentication to the UI.

Users can now sign in to view all projects and services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

Staging version can be found here:
https://2941-71c3c10-tainted-cwpayton-dot-luci-config.appspot.com/newui

BUG=730832
==========

to

==========
config_service: Add authentication to the UI.

Users can now sign in to view all projects and services.

This change involved creating a modified version of the google-signin-aware
element that
works with Polymer 2.0. This modified file is in a third party folder.

Staging version can be found here:
https://2941-71c3c10-tainted-cwpayton-dot-luci-config.appspot.com/newui

BUG=730832

Review-Url: https://codereview.chromium.org/2962703003
Committed:
https://github.com/luci/luci-py/commit/160ef2c6cacca9617e67c94b758ff4303e3fe839
==========

[commit-bot: I haz the power, 2017-06-30 00:15:43]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-py/commit/160ef2c6cacca9617e67c94b758ff4303e3fe839