Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auth): Store credentials in sessionStorage #261

Merged
merged 37 commits into from
Oct 6, 2021

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Aug 30, 2021

Related #257

Creates a user session that will remember credentials after the browser tab is refreshed and will only forget the credentials when the user logs out or closes the tab. If Cryostat was configured with Basic authentication, the user menu will display the username and display a user icon otherwise.

Since this PR only changes the frontend, backend support would likely be required for further security improvements.

@jan-law jan-law changed the title feat(auth): Create user session feat(auth): Store credentials in sessionStorage Aug 30, 2021
src/app/index.tsx Outdated Show resolved Hide resolved
src/app/Login/Login.tsx Outdated Show resolved Hide resolved
src/app/Login/Login.tsx Outdated Show resolved Hide resolved
@jan-law jan-law force-pushed the user-session branch 5 times, most recently from 2a23bb8 to 1880d06 Compare September 7, 2021 21:59
@jan-law
Copy link
Contributor Author

jan-law commented Sep 8, 2021

When the Login component renders, it sends an auth request to check for the auth method, the messaging server connects to the client and then disconnects when it notices an auth failure.

Sep 08, 2021 2:50:39 PM io.cryostat.core.log.Logger info
INFO: Outgoing WS message: {"meta":{"category":"WS_CLIENT_ACTIVITY","type":{"type":"application","subType":"json"},"serverTime":1631112639},"message":{"10.0.2.100:46278":"connected"}}
Sep 08, 2021 2:50:39 PM io.cryostat.core.log.Logger info
INFO: Disconnected remote client 10.0.2.100:46278 due to authentication failure
Sep 08, 2021 2:50:39 PM io.cryostat.core.log.Logger info
INFO: Outgoing WS message: {"meta":{"category":"WS_CLIENT_ACTIVITY","type":{"type":"application","subType":"json"},"serverTime":1631112639},"message":{"10.0.2.100:46278":"auth failure"}}
Sep 08, 2021 2:50:39 PM io.cryostat.core.log.Logger info
INFO: Disconnected remote client 10.0.2.100:46278
Sep 08, 2021 2:50:39 PM io.cryostat.core.log.Logger info
INFO: Outgoing WS message: {"meta":{"category":"WS_CLIENT_ACTIVITY","type":{"type":"application","subType":"json"},"serverTime":1631112639},"message":{"10.0.2.100:46278":"disconnected"}}
Sep 08, 2021 2:50:40 PM io.cryostat.core.log.Logger warn
WARNING: Exception thrown
io.vertx.ext.web.handler.impl.HttpStatusException: Unauthorized

If I quickly login, then logout, then login again, then logout again, the Cryostat web server will send a web socket error saying INFO: Dropping remote client 10.0.2.100:46294 due to too many concurrent connections. Logging in one more time will trigger the server to repeatedly drop the new connection, causing the web client to repeatedly auto-retry the login /auth request.

Since there may be a slight delay in between processing the auth request to disconnecting clients, and Max concurrent WebSocket connections: 2, is there anything I can do on the client side to properly disconnect the web client before logging in again?

Sep 08, 2021 2:50:46 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:46216): POST /api/v1/auth 401 1ms
Sep 08, 2021 2:50:54 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:46218): OPTIONS /api/v1/auth 200 0ms
Sep 08, 2021 2:50:54 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:46216): POST /api/v1/auth 200 0ms
Sep 08, 2021 2:50:54 PM io.cryostat.core.log.Logger info
INFO: Dropping remote client 10.0.2.100:46286 due to too many concurrent connections
Sep 08, 2021 2:50:54 PM io.cryostat.core.log.Logger info
INFO: Outgoing WS message: {"meta":{"category":"WS_CLIENT_ACTIVITY","type":{"type":"application","subType":"json"},"serverTime":1631112654},"message":{"10.0.2.100:46286":"dropped"}}
Sep 08, 2021 2:50:55 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:46216): POST /api/v1/auth 200 0ms
Sep 08, 2021 2:50:55 PM io.cryostat.core.log.Logger info
INFO: Dropping remote client 10.0.2.100:46288 due to too many concurrent connections
Sep 08, 2021 2:50:55 PM io.cryostat.core.log.Logger info
INFO: Outgoing WS message: {"meta":{"category":"WS_CLIENT_ACTIVITY","type":{"type":"application","subType":"json"},"serverTime":1631112655},"message":{"10.0.2.100:46288":"dropped"}}
Sep 08, 2021 2:50:57 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:46216): POST /api/v1/auth 200 0ms
Sep 08, 2021 2:50:57 PM io.cryostat.core.log.Logger info
INFO: Dropping remote client 10.0.2.100:46290 due to too many concurrent connections
Sep 08, 2021 2:50:57 PM io.cryostat.core.log.Logger info
INFO: Outgoing WS message: {"meta":{"category":"WS_CLIENT_ACTIVITY","type":{"type":"application","subType":"json"},"serverTime":1631112657},"message":{"10.0.2.100:46290":"dropped"}}

@andrewazores
Copy link
Member

It sounds like you might need to add some logic to the NotificationChannel.service so that it can handle disconnect/reconnect nicely. Right now it attempts to open a WebSocket connection with combineLatest(notificationsUrl, this.login.getToken(), this.login.getAuthMethod()), so if the token and/or authMethod change it will cause a new connection attempt to be made. With this new user session flow these things can change much more often over the lifecycle of the application, so some better websocket handling is needed.

Specifically, you'll probably want to close the websocket (this.ws.complete() in the service code) on connection failures and probably on logout as well, and open a new connection again once there is a new login request from the user and the authMethod/token are non-empty, etc.

@jan-law jan-law marked this pull request as ready for review September 8, 2021 20:21
@jan-law
Copy link
Contributor Author

jan-law commented Sep 8, 2021

@andrewazores Thanks for your help so far, this PR is ready for review

@andrewazores
Copy link
Member

Seems to behave as expected when using the BasicAuthManager. With NoopAuthManager, the login screen appears but there is also a blank user avatar icon in the masthead. This seems okay for now, since the backend doesn't really emulate an actual auth challenge login flow when using the NoopAuthManager. Once the backend does properly support user sessions we can do something better here.

I did twice see websocket repeated websocket connection failures with error notifications appearing after a page refresh, but I'm not sure how to reproduce that. I'll see if I can figure it out.

@andrewazores
Copy link
Member

When I use the BasicAuthManager and first log in to the web-client, then go to Recordings and select a target, only the Active Recording List is shown (Archives are disabled). If I refresh the page, the Archived recordings tab appears. I can reproduce this by logging out, closing the tab, re-opening the tab, and then signing back in and going to Recordings.

andrewazores
andrewazores previously approved these changes Sep 9, 2021
@andrewazores
Copy link
Member

Just tested it with the typical smoketest/webpack-dev-server setup and it looks and works great for both Basic auth and no auth. I'll try it out soon in OpenShift with Bearer auth.

One note I have is that the new Login.service holds state for both a username and a token, but only one of these will ever be in use at a time depending on the auth configuration of the server. Could you find a way to refactor the service and the components that access it so that there is just one piece of stored state that is interpreted as either a username or a token depending on the detected auth method? I'm fine with this being done as a follow-up PR, in fact I would prefer that since it will make the review a bit easier.

@jan-law
Copy link
Contributor Author

jan-law commented Oct 6, 2021

Sounds good. I was also wondering if there's a way to display a username from the OpenShift auth implementation. We can continue this discussion in the follow up PR

@andrewazores
Copy link
Member

I do think we can add something in the backend to query the OpenShift auth server and get the username back from the Bearer token:

https://kubernetes.io/docs/reference/access-authn-authz/authentication/#tokenreview-request-0

We already use TokenReviews to check if the user's supplied token is valid in cases where the user does not require any additional resource permissions (ex. create recordings) - so we could create ex. POST /api/v2/auth that returns information about the requesting user when given their token, with just a couple of small hooks into the auth manager implementations to supply this info.

@andrewazores andrewazores merged commit 21a940f into cryostatio:main Oct 6, 2021
@jan-law jan-law deleted the user-session branch October 6, 2021 20:06
@ebaron ebaron added the feat New feature or request label May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants