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

[SDK-3557] Refactor session lifecycle #787

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Conversation

adamjmcgrath
Copy link
Contributor

@adamjmcgrath adamjmcgrath commented Aug 24, 2022

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

📋 Changes

Changed the cookie writing behaviour from lazy (at the end of the request) to eager (when it's first accessed and the session is rolling and every-time it's updated).

This has enabled us to:

  • remove on-headers dependency (which doesn't play well with Next.js server and doesn't work at all with Next.js middleware)
  • remove wrappers (getServerSideProps wrapper and removed the need for an API route wrapper)
  • removed the magic about implicit session updates by adding an explicit updateUser method
  • allowed users to prevent the cookie from being written on every request when the session is absolute (not rolling) - which can be an issue for specific implementations and platforms

At a cost of potentially higher overhead when updating the session multiple times in the request, because multiple calls to updateUser will cause multiple cookie serialisation (encryption) attempts (but multiple reads will stay the same).

In order to do this we've needed to:

  • Make a stateful Cookie class to manage/aggregate writes to the set-cookie header
  • Add an updateUser method
  • Update the session cache to eagerly write to the cookie, rather than use on-headers

📎 References

see #668 #425 #616 #524

🎯 Testing

https://github.com/auth0/nextjs-auth0/blob/54cd48f1e1cb78eb37b700f39f4da826a5683c1a/tests/session/update-user.test.ts

@adamjmcgrath adamjmcgrath added the review:medium Medium review label Aug 24, 2022
@adamjmcgrath adamjmcgrath requested a review from a team as a code owner August 24, 2022 15:59
@vercel
Copy link

vercel bot commented Aug 24, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
nextjs-auth0 ⬜️ Ignored (Inspect) Aug 24, 2022 at 3:59PM (UTC)

@@ -93,6 +93,7 @@
"next": "^12.1.6",
"nock": "^13.0.5",
"oidc-provider": "^7.6.0",
"on-headers": "^1.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still using it in some test fixtures, so I moved it to devDependencies

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Looks good! Nice solution. Just left a question regarding the on-headers dependency, which –according to the PR description– was removed, yet is still in the package.json.

@adamjmcgrath adamjmcgrath merged commit c5918db into vNext Aug 25, 2022
@adamjmcgrath adamjmcgrath deleted the refactor-session branch August 25, 2022 13:18
@adamjmcgrath adamjmcgrath mentioned this pull request Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:medium Medium review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants