A network partition can make it impossible for a new user to sign up

[abeluck]

Describe the bug

There is a transactional bug in the callback handler, specifically here:

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

Lines 194 to 205 in 64084d6

	user = await createUser(profile)
	await dispatchEvent(events.createUser, user)
	await linkAccount(
	user.id,
	providerAccount.provider,
	providerAccount.type,
	providerAccount.id,
	providerAccount.refreshToken,
	providerAccount.accessToken,
	providerAccount.accessTokenExpires
	)

If createUser succeeds, but then linkAccount fails due to a networking issue for example, then it becomes impossible for a user to later login/sign up.

The user is created, but no linked accounts exist.

On later runs through the flow isSignedIn will be false, but a user with the email address will exist resulting in:

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

Lines 177 to 186 in 64084d6

	const userByEmail = profile.email ? await getUserByEmail(profile.email) : null
	if (userByEmail) {
	// We end up here when we don't have an account with the same [provider].id *BUT*
	// we do already have an account with the same email address as the one in the
	// oAuth profile the user has just tried to sign in with.
	//
	// We don't want to have two accounts with the same email address, and we don't
	// want to link them in case it's not safe to do so, so instead we prompt the user
	// to sign in via email to verify their identity and then link the accounts.
	throw new AccountNotLinkedError()

Steps to reproduce

You can reproduce this by providing a signIn event that uses setTimeout to "sleep" for a long time. Long enough for you to stop the test database to simulate a network partition.

After the flow fails the first time, start the database again and attempt to sign up again.

Expected behavior

A network partition event shouldn't result in making it impossible for a user to sign up.

Screenshots or error logs

n/a

Additional context

n/a

Feedback
Documentation refers to searching through online documentation, code comments and issue history. The example project refers to next-auth-example.

• Found the documentation helpful

The code comments are very complete, which made understanding this bug easier.

[abeluck]

I think the correct solution is for there to be a new adapter function that creates the user and links the account all at once. This will allow the underlying implementation to use a transaction.

[iaincollins]

I suggest configuring an email adaptor, not least as users can lose access to accounts at third party services. Supporting email sign in prevents people from not being able to sign up in this scenario - and in the event their provider account is suspended.

I'm not particularly concerned about the scenario of handling rolling back in the unlikely event of a network problem mid way through sign up; if that is actually happening frequently enough to a be problem then I suggest investigating that issue with your service.

I'm happy to accept a patch if someone is willing to do it and provide tests for it though.

PS: You should be able to log errors during sign up using the existing events - and even handle rollback that way for orphaned accounts, if you want to (though from memory I'm not sure what the message passed to the error function would be in this scenario).

[stale[bot]]

Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks!

[abeluck]

I'm not particularly concerned about the scenario of handling rolling back in the unlikely event of a network problem mid way through sign up; if that is actually happening frequently enough to a be problem then I suggest investigating that issue with your service.

I understand your position, though I respectfully disagree. Good distributed systems design necessitates taking things like transactions into account. But I get that it's not a large concern for the project now.

I'm happy to accept a patch if someone is willing to do it and provide tests for it though.

What would an acceptable patch look like to you? Would you accept breaking API changes to the adapter? Could we provide a new adapter interface (V2 or something) that mostly wraps the current one but fixes this problem? That seems much more palatable than breaking the API.

[balazsorban44]

@abeluck I am rewriting the adapter API and problems like this is high on my list!

#2361

Open for any suggestions, maybe as comments on the PR! Please address any concerns. I think database adapters should take transactions into consideration for the underlying database to be able to use its full potential!

One particular case (not yet in the PR) is the session handling where currently we do 3 subsequent queries to the database at some point, where 1 or 2 should be enough by batching those. Not just faster, but also a less network error-prone solution.