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

Also check for user before setting multi auth cookies #1941

Merged
merged 2 commits into from
Mar 4, 2025

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Mar 3, 2025

While reviewing #1904, I noticed checking req && res to detect if it's a login is weird. Not sure why I did that since next-auth already gives us a way to detect logins:

The arguments user, account, profile and isNewUser are only passed the first time this callback is called on a new session, after the user signs in. In subsequent calls, only token will be available.

-- https://next-auth.js.org/configuration/callbacks#jwt-callback

So I moved the code into the existing if (user) block and tested that we can still:

  1. login
  2. add a new account
  3. logout (until full logout)

Authentication is critical piece of code so to keep the if (req && res) check might make sense but req is definitely set (we also access it above) so I only added if (res). res should also always be set ... but only afaict.

Update

Changed my mind regarding this PR, see #1941 (comment)

@ekzyis ekzyis requested a review from Soxasora March 3, 2025 23:15
@Soxasora
Copy link
Member

Soxasora commented Mar 3, 2025

It's the right place and the right logic, every action of authentication with Next Auth brings with itself res

export default async (req, res) => {
await NextAuth(req, res, getAuthOptions(req, res))
}

so that's always available at login/signup for sure, still think that if (res) doesn't hurt.

By adding res to graphql's polling of getServerSession/getAuthOptions we would take this even further and implement a cleaner way to refresh tokens for logged-in users since we wouldn't have user (no authentication action), actually super cool!

@ekzyis ekzyis changed the title Move multi auth init into existing if (user) Also check for user before setting multi auth cookies Mar 4, 2025
@huumn huumn merged commit bf54044 into master Mar 4, 2025
6 checks passed
@huumn huumn deleted the move-multi-auth-init branch March 4, 2025 14:58
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.

3 participants