Account linking does not work correctly when using JWT

[RobertCraigie]

Describe the bug
Signing in with email and then signing in with an OAuth provider creates two separate users when using JWT sessions.

It should be noted that I only tested this issue with an OAuth provider that does not grant access to the user's email address

Steps to reproduce
https://github.com/RobertCraigie/next-auth-multiple-user-bug

• Sign in with email
• Sign in with Bungie

Expected behavior
One user is created and the OAuth account is linked to said user.

Screenshots or error logs
https://imgur.com/a/sI2pNzp

Additional context
I have traced the root cause of this issue to

next-auth/src/server/lib/callback-handler.js

Line 55 in 73d21e6

if (session && session.user) {

This block of code expects the decoded JWT to have a user property when it does not.

A simple fix is to add the user property in the JWT callback

...
  callbacks: {
    jwt: async (token, user, account, profile, isNewUser) => {
      const isSignIn = user ? true : false;

      if (isSignIn) {
        token.user = { id: user.id }
      }

      return Promise.resolve(token);
    }
  },
...

Feedback

• Found the documentation helpful
• Found documentation but was incomplete
• Could not find relevant documentation
• Found the example project helpful
• Did not find the example project helpful

[iaincollins]

Thanks for the bug reprot @RobertCraigie I think this makes sense!

In v3 release the JWT contents got refactored to use commonly supported JWT claims instead of a user object (e.g. it used to have .user object with .user.name, .user.email, etc but now the property names are just .name, .email).

It looks like the bug was introduced then.

That's for tracing the bug down! Line 55 and 56 in src/server/lib/callback-handler.js do seem to be the problem!

I'm adding some extra detail in below in case I forget - or if anyone else is able to pick this up!

Context

As part of the changes v3 we also removed exporting the User ID from the token by default.

This was done as we switched from encrypted by default (using a signed token that was then encrypted with AES) to only signed by default (with optional JWE compatible encryption able to be enabled by setting jwt.encryption: true) and I wanted to avoid exposing a User ID to the client if we didn't need to to enhance security and to reduce the default JWT payload size.

Both being encrypted by default and the JWT being too large by default were proving to be issues for users (as total cookie size per browser is 4096 bytes per domain there is much room to play with).

Fix

I don't think we want to back to encrypted tokens by default - as they are much larger and they introduce interoperability problems for people where they are passing on tokens to other services on the same domain.

To fix this bug we need to change both those lines and also add the User ID to the JWT if there is one. Using the sub claim seems to make the most sense.

If we update the contents of defaultJwtPayload in src/server/routes/callback.js (lines 87 and 177) for both the oauth and email flows to add the ID as the sub property and update lines 55 and 56 in src/server/lib/callback-handler.js according that should resolve this issue.

Note defaultJwtPayload is also defined in line 263 of src/server/routes/callback.js but for the Credentials flow, which is only supported without a database so we can ignore that instance.

Update: I meant to add what you've suggested is a great workaround until this is addressed!

[RobertCraigie]

Thanks for your really quick and informative reply!

After creating this issue I also noticed that two users are created if the end user signs in with an OAuth provider that does not grant access to an email address before signing in with email.

As far as I can tell this is because next-auth does not check if the user is already signed in, instead it just checks if the email address already belongs to a user and if it doesn't then a new user is created.

next-auth/src/server/lib/callback-handler.js

Lines 73 to 96 in 73d21e6

	const userByEmail = profile.email ? await getUserByEmail(profile.email) : null
	if (userByEmail) {
	// If they are not already signed in as the same user, this flow will
	// sign them out of the current session and sign them in as the new user
	if (isSignedIn) {
	if (user.id !== userByEmail.id && !useJwtSession) {
	// Delete existing session if they are currently signed in as another user.
	// This will switch user accounts for the session in cases where the user was
	// already logged in with a different account.
	await deleteSession(sessionToken)
	}
	}
	// Update emailVerified property on the user object
	const currentDate = new Date()
	user = await updateUser({ ...userByEmail, emailVerified: currentDate })
	await dispatchEvent(events.updateUser, user)
	} else {
	// Create user account if there isn't one for the email address already
	const currentDate = new Date()
	user = await createUser({ ...profile, emailVerified: currentDate })
	await dispatchEvent(events.createUser, user)
	isNewUser = true
	}

[iaincollins]

Thanks Robert!

Hmm, so - for context - things that are going on here:

1. Currently users and email address are 1:1 and we do a curtsey check to help users avoid creating a second account by mistake. We don't automatically link them when they are the same, as it's not secure to do so across providers (as some providers let you sign in without verifying an email address, it can be used to hijack someone else's account).

   Originally NextAuth.js ONLY supported providers that returned email addresses to avoid this can of worms, but this was relaxed to allow support for providers that did not always return an email address.

   If a provider doesn't return an email address (or is associated with another email address the user has) then there isn't anything we can do. While the current behaviour is working as intended, ultimately we'd like to handle this more gracefully - e.g. with much better error handling if an email provider is configured.

2. The built-in sign in page currently lets you 'link' accounts, but it shouldn't really. If you are signed in, it should either redirect to the homepage, or display a link/unlink page.

   Effectively it's acting as a 'link accounts' flow. I left in as an undocumented feature, until linking and unlinking and deleting accounts is properly restored (these were features in v1 but were not included in the re-write for v2).

   I thought I'd have already gotten around to doing this by now as it's very simple to add, but I've gotten overwhelmed by issues and pull requests. I think having this be explicit would address this more obvious though.

I think addressing point 2 - and address point 1 better at some point in the future - would both help resolve confusion relating to this.

We could also want to allow people to merge accounts, but that's probably quite far down the road - if 2 is implemented users will be able to do that themselves by unlinking and re-linking, which is probably sufficient in most cases (as automatically allowing folks merging accounts could introduce also sorts of other issues for people).

Supporting multiple email address per account (each with its own verified state) is still something I'd like to have, though is not yet on the roadmap. If we did that we'd want to give consideration as to how to do it in a backwards compatible way, with email address management - in practice, a single email address per user as practical limitation is fairly typical, so alternate / recover addresses are currently a nice-to-have.

[iaincollins]

Addendum: I didn't fully address what you'd raised in your comment, sorry.

The behaviour in Lines 71 to 96 should be reviewed.

My hunch is it's correct (in that if someone tries to sign in with an email address that is not associated with a user account, we should sign them in as a different user) and that we actually probably need to address this by providing a specific flow to add or change an email address on account.

I imagine we'd add that flow to the Link / Unlink page, and the most elegant way to do that is something I've been thinking of recently (with various pros/cons of having users just be able to change it but having the verification flag removed if they do, to requiring users to verify it before it's changed).

I would guess we would address that when we add Linking / Unlinking / Deleting accounts.

Tagging in #228 and #230

[RobertCraigie]

That makes sense, thank you.

However in the meantime (until a specific flow for adding/changing email addresses is added), is a solution like this safe/valid?

if (providerAccount.type === 'email') {
  // If signing in with an email, check if an account with the same email address exists already
  const userByEmail = profile.email ? await getUserByEmail(profile.email) : null
  if (userByEmail) {
    ...
  } else if (isSignedIn) {
    // user is signed in with an account that is not linked to an email address
    // as they have just verified their email address it is safe to update the signed in user's email address
    const currentDate = new Date()
    user = await updateUser({ ...user, ...profile, emailVerified: currentDate })
    await dispatchEvent(events.updateUser, user)
  }
  ...
}

[iaincollins]

Hmm I'm too tired to give you a good answer, I'll try and remember to look at this tomorrow, but the intent from the comments seems reasonable. :-)

I appreciate what is good behaviour here is a bit fuzzy, without a one-to-many user<->email relationship, but saving an address when linking if you don't already have one seems reasonable.

I think ideally we'd want to cherry pick the email field from the profile to add to the user object, rather than merging in the profile object, as you'd not want to override the name (if one was already specified). I think version 1.x (pre Serverless) did this too, for both email and the name field (only updating them when linking if they were not set).

We've just made some good progress on automating CD/CI tests today, so things should start moving quickly again soon - the main reason for not jumping on stuff like this sooner has been the pain of manual regression testing, but the OP is a great example of less common regression bug we can avoid in future by having a test case for it.