ui: add top-level LoginContainer: require login before rendering anything by vilterp · Pull Request #25005 · cockroachdb/cockroach

vilterp · 2018-04-23T19:55:36Z

Depends on #25057
Touches #24939

Shown above:

go to admin UI; see login screen
error message when you type in the wrong password
you can't hit an authenticated endpoint because you don't have a valid session (this checking is turned on by [DNM] server: require web login by default in secure mode #24944, only in secure mode)
once you login with the right password, you can see the UI (the temporary "connection lost" banner shouldn't be there)
now you (and the UI itself) can hit endpoints, because you have a valid session

Todos:

redirect to login page instead of current wrapper component switching thing
logout
- logout button (make API call; redirect to login & blow away redux store)
- log out other tabs (if they get a 403, redirect to login & blow away redux store)
styling

Release note: None

cockroach-teamcity · 2018-04-23T19:55:41Z

This change is

vilterp · 2018-04-23T21:27:17Z

pkg/ui/src/views/login/loginContainer.tsx

TODO: move this into redux store

mrtracy · 2018-04-23T21:51:06Z

pkg/ui/src/views/login/loginContainer.tsx

Local storage might be an answer, with a fallback on authorization failure. A quick outline:

This needs to be in the redux state anyway - move this there.

After creating the store for the actual page, initialize it with one-time code that checks local storaage and, if it finds the logged-in flat, updates the redux state before the initial rendering.

When the login method actually succeeds, write to this local state after updating the redux state.

If a user method gets an authorization error, we need to clear this logged-in state if it exists.

You might see if there is some blessed method of synchronizing pieces of the redux state with local storage, but I doubt it's especially tricky. #4 seems like the hard part.

Didn't think of local storage, but it makes sense. My plan had been to make index.html a Go template into which I would dump window.loggedInUser = {{ loggedInUser }} or similar. However, index.html is not a nice little static file I can make into a template; it's generated through webpack (ultimately from this: https://github.com/jaketrent/html-webpack-template/blob/master/index.ejs)

So, local storage could work nicely. Will probably move forward with that unless @couchand has objections.

mrtracy · 2018-04-23T21:57:45Z

pkg/ui/src/views/login/loginContainer.tsx

The general idea here is sound to me; with this method, we also don't need to remember any query params or URL paths, since this just renders in place regardless of the path. I think it's a good place to start.

@couchand said IRL he thinks this should redirect to /login, not just display the login page on whatever URL you're trying to go to. I think the argument is that with the redirect, we see /login in metrics.

Then we do have to remember where you're trying to go somehow, with a GET parameter or something. (maybe it can be stuffed in the state of LoginContainer somehow, if that's still there).

vilterp · 2018-04-30T23:51:49Z

K, changed to use Redux. This also now depends on #25195, which sets window.loggedInUser in a script tag in index.html, generated client-side in a Go template

Also, need to figure out how to test this.

couchand · 2018-05-01T17:09:41Z

Reviewed 7 of 7 files at r6.
Review status: 7 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

pkg/ui/src/redux/login.ts, line 14 at r6 (raw file):

declare global {
  interface Window {
    loggedInUser: string;

probably loggedInUser?

pkg/ui/src/redux/login.ts, line 19 at r6 (raw file):

export interface LoginState {
  loggedInUser: string;

likewise

pkg/ui/src/redux/login.ts, line 21 at r6 (raw file):

  loggedInUser: string;
  error: string;
  inProgress: boolean;

suggest keeping the names of these props consistent with CachedDataReducer

pkg/ui/src/redux/login.ts, line 99 at r6 (raw file):

        dispatch(loginSuccess(username));
      })
      .catch((err) => {

suggest passing two functions to then to avoid catching anything thrown in the dispatch(loginSuccess...

pkg/ui/src/views/login/loginContainer.tsx, line 30 at r1 (raw file):

Previously, vilterp (Pete Vilter) wrote…

@couchand said IRL he thinks this should redirect to /login, not just display the login page on whatever URL you're trying to go to. I think the argument is that with the redirect, we see /login in metrics.

Then we do have to remember where you're trying to go somehow, with a GET parameter or something. (maybe it can be stuffed in the state of LoginContainer somehow, if that's still there).

The usual method is a query parameter with the URL to return to. Need to consider the impact of URL redactions as well.

Comments from Reviewable

vilterp · 2018-05-01T19:18:32Z

Review status: 7 of 11 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/ui/src/views/login/loginContainer.tsx, line 30 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

The usual method is a query parameter with the URL to return to. Need to consider the impact of URL redactions as well.

Hm yes, I guess we'd need to make sure to run the value of the query param through redaction before sending to segment.

Comments from Reviewable

vilterp · 2018-05-01T19:19:44Z

Review status: 7 of 11 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/ui/src/redux/login.ts, line 99 at r6 (raw file):

Previously, couchand (Andrew Couch) wrote…

suggest passing two functions to then to avoid catching anything thrown in the dispatch(loginSuccess...

Oh good idea, I didn't know you could do that. I think we do this wrong other places (e.g. CachedDataReducer), resulting in swallowing JS errors as if they're API call errors

Comments from Reviewable

vilterp · 2018-05-02T21:41:43Z

Added a login indicator on the side. Says "insecure mode" if login is not enabled. (currently, enabled is secure && COCKROACH_EXPERIMENTAL_REQUIRE_WEB_LOGIN env var; see backend PR)

couchand · 2018-05-21T20:45:11Z

Updated to redirect. Things remaining here:

redacting the redirect url parameter,
addressing inline feedback above
logout & styling

vilterp · 2018-05-21T22:26:57Z

Tested redirect; works nicely! Working on logout.

…hing Release note: None

This allows us to have one object representing both "is login enabled?" and "does this user have access?" state questions. Release note: None

Previously, the login page was swapped in for the content of a page if a user was not logged in. This changes the behavior to instead redirect to a free standing login page. The original URL accessed is stashed in the query parameter redirectTo, and after authentication the user is returned to that page. Release note: None.

Prevent URLs from slipping though the analytics redactions if the are encoded in a query parameter, as with the login redirect param. Release note: None

couchand · 2018-05-22T16:59:31Z

Ok I think everything else mentioned has an issue filed, so this is probably ready to go, PTAL @vilterp / @mrtracy

vilterp · 2018-05-22T17:47:46Z

Logout coming up next.

Reviewed 1 of 7 files at r6, 4 of 8 files at r7, 3 of 5 files at r8, 6 of 6 files at r9, 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

pkg/ui/src/redux/login.ts, line 14 at r6 (raw file):

Previously, couchand (Andrew Couch) wrote…

probably loggedInUser?

Done.

pkg/ui/src/redux/login.ts, line 19 at r6 (raw file):

Previously, couchand (Andrew Couch) wrote…

likewise

Done.

pkg/ui/src/redux/login.ts, line 21 at r6 (raw file):

Previously, couchand (Andrew Couch) wrote…

suggest keeping the names of these props consistent with CachedDataReducer

I guess this would change to inFlight? (I don't really care tho)

pkg/ui/src/redux/login.ts, line 99 at r6 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Oh good idea, I didn't know you could do that. I think we do this wrong other places (e.g. CachedDataReducer), resulting in swallowing JS errors as if they're API call errors

Done.

pkg/ui/src/views/login/loginContainer.tsx, line 8 at r1 (raw file):

Previously, vilterp (Pete Vilter) wrote…

TODO: move this into redux store

Done.

pkg/ui/src/views/login/loginContainer.tsx, line 15 at r1 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Didn't think of local storage, but it makes sense. My plan had been to make index.html a Go template into which I would dump window.loggedInUser = {{ loggedInUser }} or similar. However, index.html is not a nice little static file I can make into a template; it's generated through webpack (ultimately from this: https://github.com/jaketrent/html-webpack-template/blob/master/index.ejs)

So, local storage could work nicely. Will probably move forward with that unless @couchand has objections.

Done.

pkg/ui/src/views/login/loginContainer.tsx, line 30 at r1 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Hm yes, I guess we'd need to make sure to run the value of the query param through redaction before sending to segment.

Done.

Comments from Reviewable

couchand · 2018-05-22T17:48:46Z

bors r+

25005: ui: add top-level LoginContainer: require login before rendering anything r=couchand a=vilterp Depends on #25057 Touches #24939 ![](https://user-images.githubusercontent.com/7341/39150096-38b4cd28-470f-11e8-9d67-e1832d35a211.gif) Shown above: 1. go to admin UI; see login screen 2. error message when you type in the wrong password 3. you can't hit an authenticated endpoint because you don't have a valid session (this checking is turned on by #24944, only in secure mode) 4. once you login with the right password, you can see the UI (the temporary "connection lost" banner shouldn't be there) 5. now you (and the UI itself) can hit endpoints, because you have a valid session Todos: - [ ] redirect to login page instead of current wrapper component switching thing - [ ] logout - [ ] logout button (make API call; redirect to login & blow away redux store) - [ ] log out other tabs (if they get a 403, redirect to login & blow away redux store) - [ ] styling Release note: None 25628: distsql: support lookup join on secondary index r=solongordon a=solongordon joinReader now supports lookup joins on secondary indexes. This was a trivial change for queries where all the output columns are included in the secondary index. I just modified the physical planner to specify the secondary index in the JoinReaderSpec and removed checks which prevented secondary indexes from being used. The more complicated situation is when we want to do a lookup join against a non-covering index. In this case, the logical planner plans an index join before the inner join, but we want to perform the lookup join first. We now handle this by only planning the lookup join during physical planning, not the index join. During execution, the joinReader detects that there are output columns not covered by the secondary index, and it performs primary index lookups as necessary to retrieve the additional columns. Fixes #25431 Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com> Co-authored-by: Andrew Couch <andrew@cockroachlabs.com> Co-authored-by: Solon Gordon <solon@cockroachlabs.com>

craig · 2018-05-22T18:07:51Z

Build succeeded

GitHub CI (Cockroach)

In cockroachdb#25005 I accidentally let the login indicator leak even if the environment variable wasn't set. This corrects that issue. Release note: None

In cockroachdb#25005 I accidentally let the login indicator leak even if the environment variable wasn't set. This corrects that issue. Release note: None Fixes: cockroachdb#25843 Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com>

25676: storage: Fix flaky TestReplicaNotLeaseHolderError r=a-robinson a=a-robinson Caused by the proactive renewal of expiration-based leases (see #25322 and #25625 for more detail). Fixes #25731 Release note: None I'm not sure how many more of these flakes to expect over the next couple weeks. I'm currently running `stressrace` on all of `pkg/storage` on a gceworker to try and ferret them out if there are any more, but there's no guarantee that'll find all of them. 25816: ui: don't show login indicator unless login is enabled r=couchand a=couchand In #25005 I accidentally let the login indicator leak even if the environment variable wasn't set. This corrects that issue. Release note: None Fixes: #25843 25853: dep: Bump grpc-go to include perf improvements and data race fix r=a-robinson a=a-robinson Un-reverts #24883 to include grpc/grpc-go#2074 This also removed hdrhistogram-writer because it apparently isn't used anymore. Release note: None Maybe we want to wait until they put this into a more formal release SHA, though. Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com> Co-authored-by: Andrew Couch <andrew@cockroachlabs.com>

vilterp requested a review from a team April 23, 2018 19:55

vilterp force-pushed the login-page branch from a941d26 to cbe95d8 Compare April 23, 2018 20:21

vilterp commented Apr 23, 2018

View reviewed changes

pkg/ui/src/views/login/loginContainer.tsx Outdated

Copy link
Copy Markdown

Contributor Author

vilterp Apr 23, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: move this into redux store

mrtracy reviewed Apr 23, 2018

View reviewed changes

vilterp force-pushed the login-page branch 2 times, most recently from 5c6dd3e to 85000c2 Compare April 30, 2018 23:48

vilterp requested a review from a team April 30, 2018 23:48

vilterp force-pushed the login-page branch 2 times, most recently from 213d712 to 8060345 Compare May 2, 2018 21:40

vilterp force-pushed the login-page branch 3 times, most recently from fcd015c to 99687e2 Compare May 4, 2018 19:03

couchand mentioned this pull request May 15, 2018

Web UI Authn & Authz (Tracking Issue) #25529

Closed

Amruta-Ranade mentioned this pull request May 17, 2018

User authentication in the Admin UI cockroachdb/docs#1582

Closed

couchand force-pushed the login-page branch from 99687e2 to 61b6c3c Compare May 21, 2018 20:42

vilterp requested a review from a team as a code owner May 21, 2018 20:42

couchand removed request for a team May 21, 2018 20:43

Pete Vilter and others added 4 commits May 22, 2018 12:58

ui: add top-level LoginContainer: require login before rendering anyt…

c138ab7

…hing Release note: None

ui: refactor LoginState to manage enabled/disabled

c840d73

This allows us to have one object representing both "is login enabled?" and "does this user have access?" state questions. Release note: None

ui: redact URLs encoded in query parameters

c0ee127

Prevent URLs from slipping though the analytics redactions if the are encoded in a query parameter, as with the login redirect param. Release note: None

couchand force-pushed the login-page branch from 61b6c3c to c0ee127 Compare May 22, 2018 16:58

vilterp changed the title ~~[WIP] ui: add top-level LoginContainer: require login before rendering anything~~ ui: add top-level LoginContainer: require login before rendering anything May 22, 2018

craig bot merged commit c0ee127 into cockroachdb:master May 22, 2018

couchand mentioned this pull request May 22, 2018

ui: don't show login indicator unless login is enabled #25816

Merged

Conversation

vilterp commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Apr 23, 2018

Uh oh!

vilterp Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrtracy Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

vilterp Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

mrtracy Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

vilterp Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

vilterp commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

couchand commented May 1, 2018

Uh oh!

vilterp commented May 1, 2018

Uh oh!

vilterp commented May 1, 2018

Uh oh!

vilterp commented May 2, 2018

Uh oh!

couchand commented May 21, 2018

Uh oh!

vilterp commented May 21, 2018

Uh oh!

couchand commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vilterp commented May 22, 2018

Uh oh!

couchand commented May 22, 2018

Uh oh!

craig bot commented May 22, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vilterp commented Apr 23, 2018 •

edited

Loading

vilterp Apr 23, 2018 •

edited

Loading

vilterp commented Apr 30, 2018 •

edited

Loading

couchand commented May 22, 2018 •

edited

Loading