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

Refresh active JWT tokens #1904

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Soxasora
Copy link
Member

@Soxasora Soxasora commented Feb 13, 2025

Description

Addresses #1596
Refreshes the logged-in multi-auth account JWT token together with session

Screenshots

Additional Context

By passing response we trigger setMultiAuthCookies that we used to trigger only on login/sign-up.

We could also specialize the token Set-Cookie to avoid accessing db for user data if we just want the token to be refreshed:

res.appendHeader('Set-Cookie', cookie.serialize(`multi_auth.${id}`, jwt, cookieOptions))

But by refreshing both the multi-auth current user cookie and the user list, we'll probably have less points of failure

Checklist

Are your changes backwards compatible? Please answer below:
todo

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
5, normal multi-auth between 3 users, refreshes the multi-auth token together with session, no hiccups

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
n/a

Did you introduce any new environment variables? If so, call them out explicitly here:
todo

@Soxasora Soxasora force-pushed the refresh_expiring_jwt branch from 825430e to 025c646 Compare February 13, 2025 18:54
@Soxasora Soxasora changed the title wip Refresh JWT token Refresh active JWT tokens Feb 13, 2025
@Soxasora Soxasora force-pushed the refresh_expiring_jwt branch from 3309f11 to abed173 Compare February 15, 2025 12:56
@Soxasora Soxasora force-pushed the refresh_expiring_jwt branch from abed173 to fc05422 Compare February 15, 2025 12:56
@Soxasora Soxasora added the auth label Feb 17, 2025
@Soxasora Soxasora marked this pull request as ready for review February 18, 2025 16:11
@huumn huumn requested a review from ekzyis February 21, 2025 18:43
Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

This looks very close to what we want!

However, two things:

  1. As you mentioned in chat, this does a db call to fetch the current user which we don't want since this runs on every request
  2. This only updates the multi_auth.<id> token of the current user, but not the other ones

I imagine we can solve both via a new function refreshMultiAuthCookies that looks at the user list stored in the multi_auth cookie to loop over every cookie with multi_auth.<id> and then issues a new JWT given the existing JWT (decodeJWT + encodeJWT).

Since we now always need req and res in the jwt callback, I noticed that instead of checking only for req && res to distinguish new logins and conditionally run setMultiAuthCookies, we can additionally check for user.

So this means if user && req && res is true, we run setMultiAuthCookies else we run refreshMultiAuthCookies.

Does this make sense?

@Soxasora
Copy link
Member Author

Soxasora commented Mar 3, 2025

  1. This only updates the multi_auth. token of the current user, but not the other ones

I was conflicted on this before, I just thought that if the user is not using their other accounts, might as well let them expire naturally.1 So it was also convenient that setMultiAuthCookies was doing just that! On the other hand refreshing for other logged-in accounts would be really convenient, too... Your solution to this fits perfectly, what do you think?

So this means if user && req && res is true, we run setMultiAuthCookies else we run refreshMultiAuthCookies.
Does this make sense?

It does! I thought just that.
Thanks a lot! ^^ Coming right up!

Footnotes

  1. with a graceful fail

@Soxasora
Copy link
Member Author

Soxasora commented Mar 4, 2025

I tested with 5 accounts and got a flawless experience in account switching for the scenarios I picked!
Cookies are always refreshed for every logged-in account and I got the multi_auth user list cookie to update too to maintain refresh functionality.

Edit: adjusted to #1941

@ekzyis ekzyis self-requested a review March 4, 2025 00:49
Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

This does refresh all JWTs of other users but I noticed following things:

  1. it does not refresh multi_auth.user-id which might be important to do since else, the cookie pointer might not get sent to the server anymore 🤔

  2. when I switch between users, I get ERR_HTTP_HEADERS_SENT in the server logs but the switch seems to work as expected, see video:

2025-03-05.14-23-20.mp4
  1. this also happens randomly while idle:
2025-03-05.14-50-21.mp4
  1. I noticed that a signout can lead to multi_auth.<id> and multi_auth.user-id getting deleted but not multi_auth so the code in refreshMultiAuthCookies would try to decode undefined, see screenshot:

2025-03-05-145229_1914x1049_scrot

This isn't caused by your code, but we need to handle it until it's fixed. I think we can just check if the token exists before trying to decode it.

I'll debug pages/api/signout.js today. I also noticed that once you switch between users, multi_auth.user-id expires after the session unlike the other cookies for authentication, including next-auth.session-token, which expire after 30 days. Maybe that's related to the issues we're seeing with account switching. 🙏

@Soxasora
Copy link
Member Author

Soxasora commented Mar 6, 2025

Awesome! Thanks for catching all the stuff I didn't see!

ERR_HTTP_HEADERS_SENT

I realized that headers were being sent prematurely which meant that a practical way to solve this would be to group all the cookies to be set and then push all of them together, I didn't get any error after doing this and switching works for me ^^

  1. I noticed that a signout can lead to multi_auth.<id> and multi_auth.user-id getting deleted but not multi_auth so the code in refreshMultiAuthCookies would try to decode undefined

This is something I didn't encounter though, in my case signout do edit multi_auth cookie
Before signout:

[
  { id: 21858, name: 'soxa_dev', photoId: null },
  { id: 21859, name: 'soxa_dev2', photoId: null }
]

After signout:

[ { id: 21859, name: 'soxa_dev2', photoId: null } ]

Nonetheless to avoid every kind of weird scenarios in which an incorrect multi_auth edit happens, now there's a condition to refresh the token if it's actually present.

multi_auth.user-id expires after the session unlike the other cookies for authentication

Another great catch that makes a lot of sense, now it gets refreshed!

Super grateful of the work you do with me ^^
I'll continue monitoring the code and its effects to iron out other kinks that may appear.

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.

3 participants