auth: Refactor how authentication methods are passed to server/auth library.

auth: Refactor how authentication methods are passed to server/auth library.

This CL removes a bunch of highly confusing public functions and makes
server/auth library more difficult to misuse.

The changes:
 * Use auth.Authenticator{...} struct to represent an object that makes
   authentication, instead of aliasing []Method for this. Even though it makes
   instantiation more verbose, it is cleaner conceptually and has a room for
   extensions (like custom HTTP 401 handlers, etc).
 * Get rid of two separate auth.Use(...) and auth.Authenticate(...) funcs that
   almost always cargo-culted into very specific usage pattern. Now there's only
   one auth.Authenticate(Method) function that returns authenticating
   middleware.
 * Store *Authenticator in the context with the rest of authentication state.
 * Get rid of auth.Autologin. It was used in single place only, move it there.

Update Milo to use fakeauth package for mocking auth state. Unfortunately, it
caused non-essential expectation change that made this CL even scarier.

R=iannucci@chromium.org
BUG=712506
Review-Url: https://codereview.chromium.org/2830443003
Committed: https://github.com/luci/luci-go/commit/a539fb93ab9faa249f025c4fbbe75733ddbab6d1

[Vadim Sh., 2017-04-18 23:46:24]

https://codereview.chromium.org/2830443003/diff/1/appengine/gaeauth/server/pr...
File appengine/gaeauth/server/prpc.go (right):

https://codereview.chromium.org/2830443003/diff/1/appengine/gaeauth/server/pr...
appengine/gaeauth/server/prpc.go:14: Methods: []auth.Method{
This is more verbose, but I think it looks less magical, especially to people
new to go.

https://codereview.chromium.org/2830443003/diff/1/milo/appengine/frontend/mai...
File milo/appengine/frontend/main_test.go (right):

https://codereview.chromium.org/2830443003/diff/1/milo/appengine/frontend/mai...
milo/appengine/frontend/main_test.go:92: c = auth.WithState(c,
&authtest.FakeState{Identity: identity.AnonymousIdentity})
previous version of this test never actually called auth.Authenticate(...) (only
auth.Use), so the fake user and fake client_id it tried to setup weren't
actually used. That's why in updated version we set AnonymousIdentity.

We need to initialize auth.State to make LoginURL and LogoutURL work.

https://codereview.chromium.org/2830443003/diff/1/scheduler/appengine/fronten...
File scheduler/appengine/frontend/handler.go (left):

https://codereview.chromium.org/2830443003/diff/1/scheduler/appengine/fronten...
scheduler/appengine/frontend/handler.go:152: baseUI :=
base.Extend(auth.Use(auth.Authenticator{server.CookieAuth}))
moved to ui.InstallHandlers

[Vadim Sh., 2017-04-19 02:00:22]

Actually... I'll probably remove an ability to pass multiple Methods to
Authenticate(...) middleware. It's rarely a good idea.

[Vadim Sh., 2017-04-19 02:39:57]

Description was changed from

==========
auth: Refactor how authentication methods are passed to server/auth library.

This CL removes a bunch of highly confusing public functions and makes
server/auth library more difficult to misuse.

The changes:
 * Use auth.Authenticator{...} struct to represent an object that makes
   authentication, instead of aliasing []Method for this. Even though it makes
   instantiation more verbose, it is cleaner conceptually and has a room for
   extensions (like custom HTTP 401 handlers, etc).
 * Get rid of two separate auth.Use(...) and auth.Authenticate(...) funcs that
   almost always cargo-culted into very specific usage pattern. Now there's only
   one auth.Authenticate(*Authenticator) function that returns authenticating
   middleware.
 * Store *Authenticator in the context with the rest of authentication state.
 * Get rid of auth.Autologin. It was used in single place only, move it there.

Update Milo to use fakeauth package for mocking auth state. Unfortunately, it
caused non-essential expectation change that made this CL even scarier.

R=iannucci@chromium.org
BUG=712506
==========

to

==========
auth: Refactor how authentication methods are passed to server/auth library.

This CL removes a bunch of highly confusing public functions and makes
server/auth library more difficult to misuse.

The changes:
 * Use auth.Authenticator{...} struct to represent an object that makes
   authentication, instead of aliasing []Method for this. Even though it makes
   instantiation more verbose, it is cleaner conceptually and has a room for
   extensions (like custom HTTP 401 handlers, etc).
 * Get rid of two separate auth.Use(...) and auth.Authenticate(...) funcs that
   almost always cargo-culted into very specific usage pattern. Now there's only
   one auth.Authenticate(*Authenticator) function that returns authenticating
   middleware.
 * Store *Authenticator in the context with the rest of authentication state.
 * Get rid of auth.Autologin. It was used in single place only, move it there.

Update Milo to use fakeauth package for mocking auth state. Unfortunately, it
caused non-essential expectation change that made this CL even scarier.

R=iannucci@chromium.org
BUG=712506
==========

[Vadim Sh., 2017-04-19 02:39:57]

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

[Vadim Sh., 2017-04-19 02:40:12]

Ryan, FYI.

https://codereview.chromium.org/2830443003/diff/40001/milo/appengine/common/m...
File milo/appengine/common/middleware.go (right):

https://codereview.chromium.org/2830443003/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:60: auth.Authenticate(server.CookieAuth),
I think this should be fine:
1. I removed OAuth2Method. It means Milo HTML pages can't be accessed with
OAuth2 access token anymore. But this fine, because no one should be using
OAuth2 to fetch HTML pages. Note that pRPC server uses its own authentication
(which is always OAuth2, see gaeauth/server/prpc.go).
2. I removed InboundAppIDAuthMethod. It used for GAE<->GAE communication. We
should be using OAuth2 in this case (it will work even when we switch to Flex).

[hinoka, 2017-04-19 18:37:10]

https://codereview.chromium.org/2830443003/diff/40001/milo/appengine/common/m...
File milo/appengine/common/middleware.go (right):

https://codereview.chromium.org/2830443003/diff/40001/milo/appengine/common/m...
milo/appengine/common/middleware.go:60: auth.Authenticate(server.CookieAuth),
On 2017/04/19 02:40:12, Vadim Sh. wrote:
> I think this should be fine:
> 1. I removed OAuth2Method. It means Milo HTML pages can't be accessed with
> OAuth2 access token anymore. But this fine, because no one should be using
> OAuth2 to fetch HTML pages. Note that pRPC server uses its own authentication
> (which is always OAuth2, see gaeauth/server/prpc.go).
> 2. I removed InboundAppIDAuthMethod. It used for GAE<->GAE communication. We
> should be using OAuth2 in this case (it will work even when we switch to
Flex).

Acknowledged.  That sounds right.

[Vadim Sh., 2017-04-19 19:03:41]

Description was changed from

==========
auth: Refactor how authentication methods are passed to server/auth library.

This CL removes a bunch of highly confusing public functions and makes
server/auth library more difficult to misuse.

The changes:
 * Use auth.Authenticator{...} struct to represent an object that makes
   authentication, instead of aliasing []Method for this. Even though it makes
   instantiation more verbose, it is cleaner conceptually and has a room for
   extensions (like custom HTTP 401 handlers, etc).
 * Get rid of two separate auth.Use(...) and auth.Authenticate(...) funcs that
   almost always cargo-culted into very specific usage pattern. Now there's only
   one auth.Authenticate(*Authenticator) function that returns authenticating
   middleware.
 * Store *Authenticator in the context with the rest of authentication state.
 * Get rid of auth.Autologin. It was used in single place only, move it there.

Update Milo to use fakeauth package for mocking auth state. Unfortunately, it
caused non-essential expectation change that made this CL even scarier.

R=iannucci@chromium.org
BUG=712506
==========

to

==========
auth: Refactor how authentication methods are passed to server/auth library.

This CL removes a bunch of highly confusing public functions and makes
server/auth library more difficult to misuse.

The changes:
 * Use auth.Authenticator{...} struct to represent an object that makes
   authentication, instead of aliasing []Method for this. Even though it makes
   instantiation more verbose, it is cleaner conceptually and has a room for
   extensions (like custom HTTP 401 handlers, etc).
 * Get rid of two separate auth.Use(...) and auth.Authenticate(...) funcs that
   almost always cargo-culted into very specific usage pattern. Now there's only
   one auth.Authenticate(Method) function that returns authenticating
   middleware.
 * Store *Authenticator in the context with the rest of authentication state.
 * Get rid of auth.Autologin. It was used in single place only, move it there.

Update Milo to use fakeauth package for mocking auth state. Unfortunately, it
caused non-essential expectation change that made this CL even scarier.

R=iannucci@chromium.org
BUG=712506
==========

[iannucci, 2017-04-19 21:58:34]

lgtm

https://codereview.chromium.org/2830443003/diff/60001/milo/appengine/frontend...
File
milo/appengine/frontend/expectations/buildbot.build-Debug_page-_CrWinGoma_30608.html
(right):

https://codereview.chromium.org/2830443003/diff/60001/milo/appengine/frontend...
milo/appengine/frontend/expectations/buildbot.build-Debug_page-_CrWinGoma_30608.html:27:
<a href="http://fake/login?dest=%2Ffoobar">login</a>
nit: let's use example.com TLD (https://www.iana.org/domains/reserved)

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go
File server/auth/auth.go (right):

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:97: // Authenticator perform authentication of incoming
requests.
nit: performs

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:99: // It is stateless object configured with a list of
methods to try when
nit: It is a

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:102: // auth methods.
How often do we actually have more than one Method?

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:307: http.Error(rw, msg, code)
Can this leak some secret info from msg -> user?

https://codereview.chromium.org/2830443003/diff/60001/server/settings/admin/h...
File server/settings/admin/handlers.go (right):

https://codereview.chromium.org/2830443003/diff/60001/server/settings/admin/h...
server/settings/admin/handlers.go:150: templates.MustRender(c.Context, c.Writer,
"pages/access_denied.html", nil)
I assume this page already has a link to go to the GAE login page too?

[Vadim Sh., 2017-04-19 22:32:47]

https://codereview.chromium.org/2830443003/diff/60001/milo/appengine/frontend...
File
milo/appengine/frontend/expectations/buildbot.build-Debug_page-_CrWinGoma_30608.html
(right):

https://codereview.chromium.org/2830443003/diff/60001/milo/appengine/frontend...
milo/appengine/frontend/expectations/buildbot.build-Debug_page-_CrWinGoma_30608.html:27:
<a href="http://fake/login?dest=%2Ffoobar">login</a>
On 2017/04/19 21:58:34, iannucci wrote:
> nit: let's use http://example.com TLD (https://www.iana.org/domains/reserved)

Done.

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go
File server/auth/auth.go (right):

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:97: // Authenticator perform authentication of incoming
requests.
On 2017/04/19 21:58:34, iannucci wrote:
> nit: performs

Done.

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:99: // It is stateless object configured with a list of
methods to try when
On 2017/04/19 21:58:34, iannucci wrote:
> nit: It is a

Done.

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:102: // auth methods.
On 2017/04/19 21:58:34, iannucci wrote:
> How often do we actually have more than one Method?

Not often, that's why the recommended way to use this stuff is through
Authenticate(Method) function. I decided to keep Authenticator{...} public for
users that really need multiple methods. I'll add a clarifying comment that this
is "advanced API".

https://codereview.chromium.org/2830443003/diff/60001/server/auth/auth.go#new...
server/auth/auth.go:307: http.Error(rw, msg, code)
On 2017/04/19 21:58:34, iannucci wrote:
> Can this leak some secret info from msg -> user?

I don't think it can. But just to be sure, removed error propagation to the
client (keeping it in the log).

https://codereview.chromium.org/2830443003/diff/60001/server/settings/admin/h...
File server/settings/admin/handlers.go (right):

https://codereview.chromium.org/2830443003/diff/60001/server/settings/admin/h...
server/settings/admin/handlers.go:150: templates.MustRender(c.Context, c.Writer,
"pages/access_denied.html", nil)
On 2017/04/19 21:58:34, iannucci wrote:
> I assume this page already has a link to go to the GAE login page too?

Yes. It has "logout" button, so you can login with different account.

[Vadim Sh., 2017-04-19 22:33:31]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2017-04-19 22:33:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2830443003/#ps80001
(title: "replies")

[commit-bot: I haz the power, 2017-04-19 22:33:34]

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-04-19 22:37:42]

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

[commit-bot: I haz the power, 2017-04-19 22:37:43]

Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35a12b215fc7ce10)
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35a12b217e413c10)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35a12b215f2fc310)

[Vadim Sh., 2017-04-19 22:39:57]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2017-04-19 22:39:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2830443003/#ps100001
(title: "fix test")

[commit-bot: I haz the power, 2017-04-19 22:40:04]

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-04-19 22:53:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492641597123200,
"parent_rev": "d5fcb24b683c1561c2b86f112a464c7656074fb0", "commit_rev":
"a539fb93ab9faa249f025c4fbbe75733ddbab6d1"}

[commit-bot: I haz the power, 2017-04-19 22:53:43]

Description was changed from

==========
auth: Refactor how authentication methods are passed to server/auth library.

This CL removes a bunch of highly confusing public functions and makes
server/auth library more difficult to misuse.

The changes:
 * Use auth.Authenticator{...} struct to represent an object that makes
   authentication, instead of aliasing []Method for this. Even though it makes
   instantiation more verbose, it is cleaner conceptually and has a room for
   extensions (like custom HTTP 401 handlers, etc).
 * Get rid of two separate auth.Use(...) and auth.Authenticate(...) funcs that
   almost always cargo-culted into very specific usage pattern. Now there's only
   one auth.Authenticate(Method) function that returns authenticating
   middleware.
 * Store *Authenticator in the context with the rest of authentication state.
 * Get rid of auth.Autologin. It was used in single place only, move it there.

Update Milo to use fakeauth package for mocking auth state. Unfortunately, it
caused non-essential expectation change that made this CL even scarier.

R=iannucci@chromium.org
BUG=712506
==========

to

==========
auth: Refactor how authentication methods are passed to server/auth library.

This CL removes a bunch of highly confusing public functions and makes
server/auth library more difficult to misuse.

The changes:
 * Use auth.Authenticator{...} struct to represent an object that makes
   authentication, instead of aliasing []Method for this. Even though it makes
   instantiation more verbose, it is cleaner conceptually and has a room for
   extensions (like custom HTTP 401 handlers, etc).
 * Get rid of two separate auth.Use(...) and auth.Authenticate(...) funcs that
   almost always cargo-culted into very specific usage pattern. Now there's only
   one auth.Authenticate(Method) function that returns authenticating
   middleware.
 * Store *Authenticator in the context with the rest of authentication state.
 * Get rid of auth.Autologin. It was used in single place only, move it there.

Update Milo to use fakeauth package for mocking auth state. Unfortunately, it
caused non-essential expectation change that made this CL even scarier.

R=iannucci@chromium.org
BUG=712506

Review-Url: https://codereview.chromium.org/2830443003
Committed:
https://github.com/luci/luci-go/commit/a539fb93ab9faa249f025c4fbbe75733ddbab6d1
==========

[commit-bot: I haz the power, 2017-04-19 22:53:45]

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