Skip to content

feat: add isNewUser arg on the signIn callback #8478

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
Closed
wants to merge 12 commits into from
Closed

feat: add isNewUser arg on the signIn callback #8478

wants to merge 12 commits into from

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?

@balazsorban44 balazsorban44 force-pushed the main branch 3 times, most recently from fa96b45 to 65aa467 Compare October 24, 2023 00:35
@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
Contributor

Another bump here! Thanks

@stale Stale
Copy link

stale bot commented Jan 21, 2025

It looks like this issue did not receive any activity for 60 days. It will be closed in 7 days if no further activity occurs. If you think your issue is still relevant, commenting will keep it open. Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label Jan 21, 2025
@stale Stale
Copy link

stale bot commented Feb 1, 2025

To keep things tidy, we are closing this issue for now. If you think your issue is still relevant, leave a comment and we might reopen it. Thanks!

@stale stale bot closed this Feb 1, 2025
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

@mcnaveen mcnaveen mcnaveen approved these changes

@siddharthd0 siddharthd0 siddharthd0 approved these changes

@0ubbe 0ubbe Awaiting requested review from 0ubbe

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

Assignees
No one assigned
Labels
legacy Refers to `next-auth` v4. Minimal maintenance. stale Did not receive any activity for 60 days
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