-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: update protection to accept array with multiples options #1565
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nextauthjs/next-auth/9PLdXwXQpGFdTgrAe2J3MzaKZ5BW |
I have been thinking about this, and I am curious what do you think if we implement it such that protection could take an array as well? so In any way, have you tried/verified that this resolves your Okta problem? |
Using the array to pass multiples validation does sounds good. I see in the future it could be I've just found out that nonce is not required with So another solution would be a flag/config on the provider to indicate that the provider wants a state when is pkce. I've noticed that currently the response_type is enforced to be always 'code' and it looks like to be working for me, but I don’t know if we will find scenarios where it must be 'id_token' or 'token'. This changes that I've made worked, I was able to successfully authenticate on okta, but because the way i've tested (using yarn link , I've got some issues related to duplicate reacts, that happens when using yarn link) I was unable to got much further on the callback. Anyways, it should works, since the only diference is that the state callback will validade the state param. |
Any update here? |
Sorry if I haven't been explicit about it, but if you could change the implementation to be able to provide an array instead of |
No worries, it was an easy change. |
Looks good to me |
@iaincollins @balazsorban44 |
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.
Thanks for your patience! I finally had the chance to check this out locally, and it seems to work nicely! Tested with Auth0.
🎉 This PR is included in version 3.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…nisms (nextauthjs#1565) * fix: add protection both option * feat: update docs with new protection value * fix: lint files * refactor: change protection from string to array * chore: reverting unespected change * chore: lint files Co-authored-by: Balázs Orbán <[email protected]>
🎉 This PR is included in version 4.0.0-next.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…nisms (nextauthjs#1565) * fix: add protection both option * feat: update docs with new protection value * fix: lint files * refactor: change protection from string to array * chore: reverting unespected change * chore: lint files Co-authored-by: Balázs Orbán <[email protected]>
What:
Add new protection value: "both", to use both handlers (state and pkce).
Why:
Some provider require the state param when using the pkce protection as reported on this issues:
#1530
#1367
How:
Just editing the handlers validations to handle in case of "both" is set on protection.
Checklist:
It's note ready, actually its more a suggestion than a PR.
Note: I called it both, but it could have other names, such as: pkce-state or pkce-with-state