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: Keycloak token refresh #1028

Merged
merged 5 commits into from
Nov 13, 2024
Merged

feat: Keycloak token refresh #1028

merged 5 commits into from
Nov 13, 2024

Conversation

Whoops
Copy link
Collaborator

@Whoops Whoops commented Oct 30, 2024

Summary of changes

Asana Ticket: 🏹 Implement authentication token refresh

[Please include a brief description of what was changed]

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

@Whoops
Copy link
Collaborator Author

Whoops commented Oct 30, 2024

For review, I suggest validating in dev

  • When idle for less than 30 minutes, a fresh login is not required
  • When idle for more than 30 minutes, the user is forced to login again

@Whoops Whoops requested review from a team and bfauble and removed request for a team October 30, 2024 21:16
@@ -103,14 +103,26 @@ config :tailwind,
cd: Path.expand("../assets", __DIR__)
]

config :arrow, ArrowWeb.AuthManager, issuer: "arrow"
# 12 hours in seconds
max_session_time = 12 * 60 * 60
Copy link
Contributor

Choose a reason for hiding this comment

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

re: your comment in the channel, do we currently, or can we possibly retain the form state in the session or something on re-login redirect so that if people do inadvertently run into a timeout while working on a form, all that info isn't just lost?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not easily (at least not off the top of my head, but I'm open to suggestions). In rough order of difficulty, in my mind, the options to further mitigate the problem are:

  1. Increase the idle and/or max session time beyond the recommendations
  2. Implement a javascript based session refresh based on mouse movements
    • Say once a minute, if the user has moved their or pressed a key on the page, refresh the session
    • Doesn't solve the user walking away for 30 minutes, but keeps them active if it's taking that long to fill out the form
  3. Implement a modal-based authentication flow that doesn't take them away from the page
    • I think we'd have to do something with iFrames here, like complete the login in an iframe, then pass the new token back to the parent page? Not sure.
  4. Somehow serialize the request we were trying to perform when we detected the user was past the idle timeout and replay it on returning.
    • Serializing the request isn't that hard, but since we detect expiration on the server side, we can't store it in LocalStorage on the client. For small requests, we could probably store them in the redirect URL that keycloak sends the user back to, but for things like uploading a shape, I'd assume Keycloak has a limit to the length of the URL.

And actually, after writing this up, one other option that doesn't really fit with the spirit of the guidance but would work:
Currently, the stops page is a LiveView, but when the user clicks Save, it does a POST request with the form data rather than doing the usual LiveView thing, where clicking the Save button would be a LiveView event. Since the LiveView session doesn't time out with the token if we instead made clicking the Save button a LiveView event, the save would happen within the existing session, then we'd redirect the user to the /stops index page, and that request would trigger the need for authentication. So the work would be saved, then the user would be prompted to authenticate.

Copy link
Collaborator Author

@Whoops Whoops Oct 31, 2024

Choose a reason for hiding this comment

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

In fact, looking closer, it's even already a LiveView event, and we are going out of our way to turn it into a client-side POST. If we just handled the database save right there in the event, and then redirected, that'd solve the dataloss problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the stops page is a LiveView, but when the user clicks Save, it does a POST request with the form data rather than doing the usual LiveView thing

I want to note that we lose the ability to make API requests if we remove the controller piece of the stops page. Not that we couldn't support both though. I'd expect in some cases supporting an API in addition to a LiveView makes sense (which, with the HTML controller-based way, you get for free).

I added back the API bits when I was importing the stops from gtfs_creator into arrow. For the record, I think that hitting the API was the right way to do that, not a database script, because with the API, I wasn't by-passing any of the Elixir-based validations that we were already doing as part of it. I found and fixed some issues with the data that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point on losing the ability to make API calls. We'd also lose the ability for the page to function without JavaScript (I tested, it does!). The updated PR saves within the live session by default and falls back to POST/PUT without otherwise.

@bfauble bfauble self-requested a review November 1, 2024 14:04
@Whoops Whoops force-pushed the wh-keycloak-refresh branch from 09271d3 to 42121d3 Compare November 1, 2024 21:28
@Whoops Whoops force-pushed the wh-keycloak-refresh branch from 42121d3 to 6a33337 Compare November 12, 2024 15:12
@Whoops Whoops force-pushed the wh-keycloak-refresh branch from 6a33337 to 4f003c2 Compare November 13, 2024 21:15
@Whoops Whoops merged commit a9fbf94 into master Nov 13, 2024
10 checks passed
@Whoops Whoops deleted the wh-keycloak-refresh branch November 13, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants