server, ui: login: template logged in user into index.html #25195

vilterp · 2018-05-01T00:18:18Z

...instead of serving a static index.html generated at build time via a webpack plugin. This allows the UI to decide whether it needs to show a login UI (and show the logged in username if there is one) without making an additional request to the backend.

Fixes #25171

TODO:

make sure this doesn't break make buildshort
favicon not showing up
tests
- insecure: can get assets, index.html doesn't have user
- secure
  - logged out: can get assets, index.html doesn't have user
  - logged in: can get assets, index.html has user
fix race condition where health check endpoint crashes server before it's fully started up

Release note: None

cockroach-teamcity · 2018-05-01T00:18:24Z

This change is

couchand · 2018-05-01T16:47:59Z

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

pkg/server/authentication.go, line 309 at r1 (raw file):

	inner  http.Handler

	rejectIfUnauthenticated bool

suggest reversing the logic so the zero value is the safe one... allowAnonymous maybe?

pkg/server/authentication.go, line 313 at r1 (raw file):

func newAuthenticationMux(
	s *authenticationServer, inner http.Handler, rejectIfUnauthenticated bool,

suggest exposing two different constructors rather than passing in the bool

pkg/server/authentication.go, line 327 at r1 (raw file):

	username, httpCode, err := am.getSession(w, req)
	if am.rejectIfUnauthenticated && err != nil {
		http.Error(w, err.Error(), httpCode)

i think i made this comment on the original pr... i don't think we should be returning the error. we should log it, but the actor attempting to log in shouldn't be able to see details beyond the status code.

pkg/server/authentication.go, line 332 at r1 (raw file):

	newCtx := context.WithValue(req.Context(), loggedInUserKey{}, username)
	newReq := req.WithContext(newCtx)

probably worth updating tests to reflect this

pkg/server/server.go, line 1450 at r1 (raw file):

	log.Event(ctx, "added http endpoints")

	// Also throw the landing page in there. It won't work well, but it's better than a 404.

are you sure you want to move this all the way down here? if so, the comment isn't really relevant anymore...

pkg/server/server.go, line 1459 at r1 (raw file):

	})

	assetsHandler := func(writer http.ResponseWriter, request *http.Request) {

suggest moving this to a new file and adding tests for the various cases.

pkg/server/server.go, line 1478 at r1 (raw file):

			wrappedErr := errors.Wrap(err, "templating index.html")
			log.Error(ctx, wrappedErr)
			writer.WriteHeader(500)

http.Error?

pkg/ui/ui.go, line 101 at r1 (raw file):

		</script>

		<script src="protos.dll.js" type="text/javascript"></script>

if we can get these to be build-time rather than code-time, that would be ideal, but if that's too complicated hard-coding isn't that bad.

Comments from Reviewable

mrtracy · 2018-05-01T17:04:48Z

It's my opinion that, rather than having index.html be generated with "loggedInUser" set, we should have the authenticationMux set a second, non-httponly cookie on the outgoing response which contains the logged in user name. I think that the index.html generation here is a bit heavy-handed for this particular enhancement.

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

pkg/server/authentication.go, line 313 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

suggest exposing two different constructors rather than passing in the bool

+1

pkg/server/authentication.go, line 327 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

i think i made this comment on the original pr... i don't think we should be returning the error. we should log it, but the actor attempting to log in shouldn't be able to see details beyond the status code.

Agreed, this could unintentionally leak details to an unauthenticated client.

Comments from Reviewable

couchand · 2018-05-01T17:19:03Z

set a second, non-httponly cookie on the outgoing response ... the index.html generation here is a bit heavy-handed for this particular enhancement

a reasonable perspective, however, this also seems like a good strategy for addressing #19024

vilterp · 2018-05-01T18:37:37Z

Thanks @mrtracy; I hadn't really considered passing the username as a cookie accessible from JS.

In past projects, a server-templated JS object has proven useful for a lot of things (version, auth, data loading, feature flags, etc) so I figured it was worth introducing, despite maybe not being the most expedient way to do login.

vilterp · 2018-05-02T19:00:20Z

Review status: 1 of 4 files reviewed at latest revision, 8 unresolved discussions.

pkg/server/authentication.go, line 309 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

suggest reversing the logic so the zero value is the safe one... allowAnonymous maybe?

Done.

pkg/server/authentication.go, line 313 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

+1

Done, although the names are clunky. Lmk if you have better ideas.

pkg/server/server.go, line 1478 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

http.Error?

Done. Thanks; this didn't seem idiomatic and somehow I forgot about http.Error.

pkg/ui/ui.go, line 101 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

if we can get these to be build-time rather than code-time, that would be ideal, but if that's too complicated hard-coding isn't that bad.

Yeah, I couldn't think of a way of passing them that was worth the complexity. The names of the three bundles were hardcoded in the webpack config anyway, so we haven't really increased the amount of hardcoding.

Comments from Reviewable

vilterp · 2018-05-04T22:29:01Z

Update: after some debugging with Andrei, looks like the crash should be fixed by #24945. The problem was that in server.Start the healthcheck endpoint is exposed before the SQL executor is initialized. This was never a problem until authorization was turned on (with the env var), which causes it to run SQL to check the session. Once we exempt it from auth, it won't run SQL, as before.

couchand · 2018-05-15T20:40:54Z

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.

pkg/server/authentication.go, line 313 at r1 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done, although the names are clunky. Lmk if you have better ideas.

I just removed "DisallowAnonymous" since that's considered the default.

pkg/server/authentication.go, line 327 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Agreed, this could unintentionally leak details to an unauthenticated client.

Updated to log the real error and then respond with a generic error.

Comments from Reviewable

couchand · 2018-05-15T20:44:19Z

@mrtracy would you please take one more look at this when you get a chance?

mrtracy · 2018-05-15T21:58:51Z

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/server/authentication.go, line 327 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Updated to log the real error and then respond with a generic error.

Isn't this going to log "Web Session Error" when you load up index.html the first time, without yet having logged in?

Comments from Reviewable

mrtracy · 2018-05-15T22:11:31Z

Had a comment on the log message for unauthenticated requests, otherwise this is

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

couchand · 2018-05-16T17:30:40Z

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/server/authentication.go, line 327 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Isn't this going to log "Web Session Error" when you load up index.html the first time, without yet having logged in?

Indeed. I think we still want to log if it hits any but the first conditional in getSession, or maybe this should just be moved into the if !am.allowAnonymous block?

Actually, I think getSession should just take into account am.allowAnonymous.

Comments from Reviewable

couchand · 2018-05-16T17:48:50Z

Reviewed 4 of 8 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

pkg/server/authentication.go, line 327 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Indeed. I think we still want to log if it hits any but the first conditional in getSession, or maybe this should just be moved into the if !am.allowAnonymous block?

Actually, I think getSession should just take into account am.allowAnonymous.

Nevermind on that last point, it's too much work for too little benefit. I just pushed it inside the if !am.allowAnonymous block.

Comments from Reviewable

...instead of serving a static index.html generated at build time via a webpack plugin. This allows the UI to decide whether it needs to show a login UI (and show the logged in username if there is one) without making an additional request to the backend. Release note: None

Release note: None

couchand · 2018-05-16T20:29:38Z

bors r+

25195: server, ui: login: template logged in user into index.html r=couchand a=vilterp ...instead of serving a static index.html generated at build time via a webpack plugin. This allows the UI to decide whether it needs to show a login UI (and show the logged in username if there is one) without making an additional request to the backend. Fixes #25171 TODO: - [x] make sure this doesn't break `make buildshort` - [x] favicon not showing up - tests - [x] insecure: can get assets, index.html doesn't have user - secure - [x] logged out: can get assets, index.html doesn't have user - [x] logged in: can get assets, index.html has user - [x] fix race condition where health check endpoint crashes server before it's fully started up Release note: None Co-authored-by: Pete Vilter <[email protected]> Co-authored-by: Andrew Couch <[email protected]>

craig · 2018-05-16T20:50:20Z

Build succeeded

GitHub CI (Cockroach)

By telling Webpack to proxy the path `/` through to the Cockroach process. Webpack used to serve `/` itself from a static file, but as of cockroachdb#25195 we generate `/` dynamically in Go, so we can use it to pass login state to the UI. Fixes cockroachdb#25858 Release note: None

25868: sql: Log event for TRUNCATE TABLE r=a-robinson a=a-robinson Fixes #25867 Release note (sql change): TRUNCATE TABLE commands are now logged in the event log. 25871: ui: unbreak `make watch` r=vilterp a=vilterp By telling Webpack to proxy the path `/` through to the Cockroach process. Webpack used to serve `/` itself from a static file, but as of #25195 we generate `/` dynamically in Go, so we can use it to pass login state to the UI. Fixes #25858 Release note: None 25875: ui: fix action names broken by #25823 r=couchand a=couchand In that PR, I mechanically replaced identifiers when there were new warnings about shadowing, but in two places I accidentally changed semantics due to the shorthand form of object literals. 🤦‍♂️ Release note: None Co-authored-by: Alex Robinson <[email protected]> Co-authored-by: Pete Vilter <[email protected]> Co-authored-by: Andrew Couch <[email protected]>

Better separate concerns by making package ui responsible for providing an HTTP handler that can serve its assets. As a side effect of the refactor, fix rendering of the "no UI installed" page in short binaries, which was broken in cockroachdb#25195. This is not pure code movement, so ui.Handler should be reviewed as if it were new code.

Better separate concerns by making package ui responsible for providing an HTTP handler that can serve its assets. As a side effect of the refactor, fix rendering of the "no UI installed" page in short binaries, which was broken in cockroachdb#25195. This is not pure code movement, so ui.Handler should be reviewed as if it were new code. Release note: None

vilterp requested review from a team May 1, 2018 00:18

vilterp mentioned this pull request May 1, 2018

ui: add top-level LoginContainer: require login before rendering anything #25005

Merged

5 tasks

vilterp force-pushed the go-template-index-html branch from 18e200f to 4099f7d Compare May 1, 2018 00:20

vilterp force-pushed the go-template-index-html branch from 4099f7d to 0dab315 Compare May 1, 2018 21:56

vilterp force-pushed the go-template-index-html branch 6 times, most recently from eec27f0 to 30a03be Compare May 4, 2018 22:00

couchand mentioned this pull request May 15, 2018

Web UI Authn & Authz (Tracking Issue) #25529

Closed

couchand force-pushed the go-template-index-html branch from 30a03be to 9112cb6 Compare May 15, 2018 20:38

couchand force-pushed the go-template-index-html branch from 9112cb6 to e9d50cb Compare May 16, 2018 17:46

couchand force-pushed the go-template-index-html branch from e9d50cb to 6f7e4d7 Compare May 16, 2018 18:27

Pete Vilter and others added 2 commits May 16, 2018 16:04

server: make authn failures always return the same error

a49e590

Release note: None

couchand added 2 commits May 16, 2018 16:04

server: rename authMuxDisallowAnonymous to authMux

32b4e00

Release note: None

ui: remove html-webpack-plugin

fbc5c5a

Release note: None

couchand force-pushed the go-template-index-html branch from 6f7e4d7 to fbc5c5a Compare May 16, 2018 20:04

couchand mentioned this pull request May 16, 2018

server: exempt healthcheck endpoint from authentication check #24945

Merged

craig bot merged commit fbc5c5a into cockroachdb:master May 16, 2018

vilterp mentioned this pull request May 23, 2018

ui: make watch broken #25858

Closed

vilterp mentioned this pull request May 23, 2018

ui: unbreak make watch #25871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server, ui: login: template logged in user into index.html #25195

server, ui: login: template logged in user into index.html #25195

vilterp commented May 1, 2018 •

edited by couchand

Loading

cockroach-teamcity commented May 1, 2018

couchand commented May 1, 2018

mrtracy commented May 1, 2018

couchand commented May 1, 2018

vilterp commented May 1, 2018

vilterp commented May 2, 2018

vilterp commented May 4, 2018

couchand commented May 15, 2018

couchand commented May 15, 2018

mrtracy commented May 15, 2018

mrtracy commented May 15, 2018

couchand commented May 16, 2018

couchand commented May 16, 2018

couchand commented May 16, 2018

craig bot commented May 16, 2018

server, ui: login: template logged in user into index.html #25195

server, ui: login: template logged in user into index.html #25195

Conversation

vilterp commented May 1, 2018 • edited by couchand Loading

cockroach-teamcity commented May 1, 2018

couchand commented May 1, 2018

mrtracy commented May 1, 2018

couchand commented May 1, 2018

vilterp commented May 1, 2018

vilterp commented May 2, 2018

vilterp commented May 4, 2018

couchand commented May 15, 2018

couchand commented May 15, 2018

mrtracy commented May 15, 2018

mrtracy commented May 15, 2018

couchand commented May 16, 2018

couchand commented May 16, 2018

couchand commented May 16, 2018

craig bot commented May 16, 2018

Build succeeded

vilterp commented May 1, 2018 •

edited by couchand

Loading