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.

Sign up for GitHub

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

Jump to bottom

feat: add isNewUser arg on the signIn callback #8478

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: add isNewUser arg on the signIn callback #8478

wants to merge 12 commits into from
+90 −7

Conversation

kahveciderin
Copy link

@kahveciderin kahveciderin commented Sep 3, 2023 •
edited
Loading

This PR implements the isNewUser field on the signIn callback, to allow for the rejection of a potential sign up.

☕️ Reasoning

Sometimes we might want to reject the signIn event if the user doesn't already have an account.

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Fixes #8272

@vercel
Copy link

vercel bot commented Sep 3, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 3, 2023 10:02am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Sep 3, 2023 10:02am

@vercel
Copy link

vercel bot commented Sep 3, 2023

@kahveciderin is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the legacy Refers to `next-auth` v4. Minimal maintenance. label Sep 3, 2023
@kahveciderin kahveciderin marked this pull request as draft September 3, 2023 00:27
@kahveciderin
Copy link
Author

kahveciderin commented Sep 3, 2023 •
edited
Loading

As far as I understand, the "Credentials" provider is intended to support use cases where one has an existing system one needs to authenticate users against, so a user cannot be a new user when using the Credentials provider, thus I've now added isNewUser: false to the signIn callback when signing in with the credentials provider to reflect that.

@kahveciderin
Export input validation on the account parameter on callbackHandler t…
fecd450 
…o reduce code duplication between callbackHandler and checkIfUserIsNew
@kahveciderin
Copy link
Author

I have exported the "input validation" lines to an external helper function, because the same thing with the callbackHandler was being validated in the new checkIfUserIsNew function. Also, I simplified the checkIfUserIsNew function's logic by first checking the email no matter what provider is used, and it checks if the account exists only after establishing that the email doesn't exist and the user is using oauth.

@kahveciderin kahveciderin marked this pull request as ready for review September 3, 2023 09:50
@kahveciderin
Copy link
Author

I think I forgot to stage my last changes, will be pushing them now.

@kahveciderin
Copy link
Author

This PR should now be ready to merge, would be happy to get some reviews.

tilgovi
tilgovi reviewed Oct 3, 2023
View reviewed changes
packages/next-auth/src/core/routes/callback.ts
@@ -90,10 +90,17 @@ export default async function callback(params: {
}

try {
const isNewUser = await checkIfUserIsNew({
Copy link

Choose a reason for hiding this comment

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

This could be stored as let isNewUser = true on line 81 and set to false on line 89, couldn't it?

@ndom91 ndom91 changed the title Add support for isNewUser on the signIn callback feat: add isNewUser arg on the signIn callback Dec 24, 2023
@april83c
Copy link
Contributor

What's going on with this? Will it be merged?

@bilalmohib
Copy link

Please merge this work

@ehenoma
Copy link

ehenoma commented Jun 5, 2024

push

@siddharthd0
Copy link

merge when

mcnaveen
mcnaveen approved these changes Sep 19, 2024
View reviewed changes
Copy link

@mcnaveen mcnaveen left a comment

Choose a reason for hiding this comment

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

LGTM 🫡

@garshythoel
Copy link

Another bump here! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tilgovi tilgovi tilgovi left review comments

@siddharthd0 siddharthd0 siddharthd0 approved these changes

@mcnaveen mcnaveen mcnaveen approved these changes

@ubbe-xyz ubbe-xyz Awaiting requested review from ubbe-xyz ubbe-xyz is a code owner

@ThangHuuVu ThangHuuVu Awaiting requested review from ThangHuuVu ThangHuuVu is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
legacy Refers to `next-auth` v4. Minimal maintenance.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to block authorization depending on whether it is a new user or not
8 participants
@kahveciderin @april83c @bilalmohib @ehenoma @siddharthd0 @garshythoel @tilgovi @mcnaveen