-
Notifications
You must be signed in to change notification settings - Fork 4k
feat: update account on login #9721
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
balazsorban44
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks interesting, and probably something we would like to consider! However the correct flow is to create a feature request first:
https://github.com/nextauthjs/next-auth/discussions/new?category=ideas
I also recommend splitting this to multiple PRs so the adapter can be addressed separately.
|
@balazsorban44 thanks for the quick reply! I've added the discussion as requested. Should I delete this pull request and create a new one for each change? |
|
We can keep the PR for now! I think if we introduce this behind an Some code that might be useful. To throw a proper error if the flag is missing, something like: next-auth/packages/next-auth/src/lib/future/index.ts Lines 60 to 65 in 30fb90f
Then you an extend the next-auth/packages/core/src/types.ts Line 589 in 62f24f5
|
|
Awesome, sounds great! Is the next-auth/packages/next-auth/src/lib/future/index.ts file available on main? Do I need to fork from your branch instead and point my PR there? |
|
It's just to showcase a possible implementation. you can copy paste it and adjust the naming accordingly for this PR |
|
@balazsorban44 I added the experimental error handling function for updateAccount 😄 I went ahead and updated the documentation as well |
|
Hi @balazsorban44! Is there anything else that should be done before we can proceed with merging these changes? 😄 |
|
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! |
☕️ Reasoning
What needed fixing:
Hi!
Currently, when signing in using a provider, we're populating the Account with refresh and access tokens. These are left static for the entire lifetime of the Account, which I think shouldn't be the case as that will turn at some point on revoked access if the user clicks “sign me out of all my devices” on their provider account, their admin triggers a password loss procedure or if we change the scopes we request on our token.
This is why in this PR I aim to update the Account with newly issued refresh and access tokens with each subsequent login from the user.
This is a PoC (tested locally, works) and I'd love to get some insights on how feasible this solution is to be implemented in Next Auth, both on
coreand onv4since we need it in several of our projects.Changes:
I created a new adapter function called
updateAccount()(on prisma adapter only as a PoC) which takes anAccountand aUserIDas props. This function will search for that account on the database and update it with the updated account information on login. In case that Account does not exist, it will create a new entry, which means this function could potentially be used as well for linking an account as well.The function is optional to make sure no other adapters are being broken after merging.
🧢 Checklist
🎫 Affected issues
📌 Resources