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

Implement token-based sessions #4463

Merged
merged 26 commits into from
Sep 3, 2024
Merged

Implement token-based sessions #4463

merged 26 commits into from
Sep 3, 2024

Conversation

zoldar
Copy link
Contributor

@zoldar zoldar commented Aug 22, 2024

Changes

Follow-up to #4452 implementing token-based sessions.

  • A new table is added for storing sessions called user_sessions and used by Auth.UserSession schema
  • The session struct stores user reference, token (randomly generated on creation), last_used_at and timeout_at timestamps
  • Inactive session timeout is kept at 14 days, like before, just managed server-side
  • New user sessions refresh is managed by a new plug, UserSessionTouch which also converts existing legacy sessions to new ones on the go
  • There's a CleanUserSessions background job cleaning timed out sessions with an additional grace period of 7 days, just in case (sessions, when retrieved are always filtered by timeout, so that's not an issue)
  • Retrievals of legacy sessions is logged with a warning. Logs will be scanned for occurrences to determine whether rate of legacy sessions goes down towards zero during/after 14 day period as expected
  • Device name is detected using logic largely copied from Ingestion.Event pipeline. I have decided to not DRY these as they serve very different purposes.

TODO

  • properly refresh valid and dispose of expired sessions
  • instrument retrievals of legacy session cookie (accomplished with simple logging)
  • more tests
  • investigate and fix borked "remember 2fa" cookie mechanism false alarm, it's fine
  • extract migration to a different PR (may be done post-review)

Tests

  • Automated tests have been added

Changelog

  • Entry has been added to changelog

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@zoldar zoldar changed the title Token based sessions Implement token-based sessions Aug 22, 2024
@zoldar zoldar force-pushed the token-based-sessions branch from ebdbfa9 to 6a098c0 Compare August 22, 2024 21:46
@zoldar zoldar force-pushed the session-cookie-consolidation branch from 5c4445b to fbe1cdb Compare August 23, 2024 08:48
Base automatically changed from session-cookie-consolidation to master August 23, 2024 08:53
@zoldar zoldar force-pushed the token-based-sessions branch from 6a098c0 to 45361dd Compare August 23, 2024 08:56
@zoldar zoldar force-pushed the token-based-sessions branch 5 times, most recently from 5da8238 to f090002 Compare August 26, 2024 11:09
@zoldar zoldar added the preview label Aug 26, 2024
Copy link

Preview environment👷🏼‍♀️🏗️
PR-4463

@zoldar zoldar force-pushed the token-based-sessions branch from 1798576 to 9ff4fad Compare August 26, 2024 12:57
@zoldar zoldar requested a review from a team August 26, 2024 13:06
@zoldar zoldar marked this pull request as ready for review August 26, 2024 13:08
Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

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

Overall the implementation looks good to me.

I would like to review the DB queries we fire off during authentication. I tried loading a random settings screen with this PR and in the logs I counted:
4 SELECT requests from user_sessions
1 UPDATE to user_sessions
1 SELECT from users

I feel like we should be able to get by with
1 SELECT from user_sessions JOIN users
1 UPDATE to user_sessions

lib/plausible_web/user_auth.ex Outdated Show resolved Hide resolved
@zoldar zoldar force-pushed the token-based-sessions branch from defddfb to 1b66868 Compare August 29, 2024 13:01
@zoldar
Copy link
Contributor Author

zoldar commented Aug 29, 2024

@ukutaht I have revised session retrieval and modified logic so that session, user and last subscription are always fetched in a single query. I have also modified plug pipelines interacting with session to ensure they don't refetch the session or user unless necessary.

By the way, I have noticed that our external API endpoints (/api/event included) have :fetch_session and AuthPlug in their pipeline - I have removed both as none of those endpoints need the session (I'll do some double-checking for that still).

This limits the number of session related queries to minimum. When I open funnel settings, I can see 2 SELECTs of user session (one for dead view and one for live view, we can't get around that), and one UPDATE for user_session (via UserSessionTouch). All else is related to validating site access permission (user role, site membership etc.). If we want to address those, I'd opt for doing that in a separate PR.

Copy link
Member

@aerosol aerosol left a comment

Choose a reason for hiding this comment

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

Good work! Keeping legacy sessions alive is a really nice touch.

lib/plausible_web/plugs/user_session_touch.ex Show resolved Hide resolved
lib/plausible_web/router.ex Outdated Show resolved Hide resolved
lib/plausible_web/user_auth.ex Outdated Show resolved Hide resolved
lib/plausible_web/user_auth.ex Show resolved Hide resolved
@zoldar zoldar force-pushed the token-based-sessions branch from 2e856a7 to 2eced93 Compare September 2, 2024 10:37
@zoldar
Copy link
Contributor Author

zoldar commented Sep 2, 2024

@aerosol @ukutaht all feedback so far applied and PR ready for re-review.

@zoldar zoldar force-pushed the token-based-sessions branch from 2eced93 to a023574 Compare September 2, 2024 10:57
@zoldar zoldar force-pushed the token-based-sessions branch from a023574 to bbe81dc Compare September 3, 2024 08:04
@zoldar zoldar merged commit 373d4dd into master Sep 3, 2024
10 checks passed
@zoldar zoldar deleted the token-based-sessions branch September 3, 2024 09:34
zoldar added a commit that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants