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.

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

Merged
merged 9 commits into from
Apr 6, 2021

Conversation

viniciuscr
Copy link
Contributor

@viniciuscr viniciuscr commented Mar 22, 2021

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:

  • Documentation
  • Tests
  • Ready to be merged

    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

@vercel
Copy link

vercel bot commented Mar 22, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextauthjs/next-auth/9PLdXwXQpGFdTgrAe2J3MzaKZ5BW
✅ Preview: https://next-auth-git-fork-viniciuscr-fix-pkce-state-nextauthjs.vercel.app

@vercel vercel bot temporarily deployed to Preview March 22, 2021 13:22 Inactive
@github-actions github-actions bot added core Refers to `@auth/core` documentation Relates to documentation providers labels Mar 22, 2021
@viniciuscr viniciuscr changed the base branch from main to canary March 22, 2021 13:23
@viniciuscr viniciuscr changed the base branch from canary to main March 22, 2021 13:23
@vercel vercel bot temporarily deployed to Preview March 22, 2021 13:27 Inactive
@viniciuscr viniciuscr mentioned this pull request Mar 22, 2021
5 tasks
@balazsorban44
Copy link
Member

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 protection: ["pkce", "state"] so it's room for the future.

In any way, have you tried/verified that this resolves your Okta problem?

@viniciuscr
Copy link
Contributor Author

viniciuscr commented Mar 22, 2021

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 protection: ["pkce", "state"] so it's room for the future.

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
protection: ["pkce", "state", "nonce"]
it would be highly configurable, but the user must know this kind of specifications from each provider and type of authentication used.

I've just found out that nonce is not required with response_type: "code" but it is when response_type: "id_token" or response_type: "token". this way one must be aware that one must use protection: ["pkce", "state", "nonce"] when `response_type: "id_token". What could be simplified somehow in the future.

So another solution would be a flag/config on the provider to indicate that the provider wants a state when is pkce.
it does looks like a few needed it but most of the providers does not.

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.

@viniciuscr
Copy link
Contributor Author

Any update here?
I see more people are getting stuck with the same problem and I kind of need it to use next-auth, otherwise I will have to jump to the okta sdk.

@balazsorban44
Copy link
Member

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 "both" for protection to have more room for the future, I could proceed to merge this.

@vercel vercel bot temporarily deployed to Preview March 30, 2021 23:43 Inactive
@vercel vercel bot temporarily deployed to Preview March 30, 2021 23:51 Inactive
@vercel vercel bot temporarily deployed to Preview March 30, 2021 23:59 Inactive
@viniciuscr
Copy link
Contributor Author

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 "both" for protection to have more room for the future, I could proceed to merge this.

No worries, it was an easy change.
Now it accepts an array but also keeps compatibility with string.

@LuisValgoi
Copy link

Looks good to me

@vercel vercel bot temporarily deployed to Preview March 31, 2021 14:57 Inactive
@viniciuscr viniciuscr changed the title fix: add protection both option fix: update protection to accept array with multiples options Apr 5, 2021
@vercel vercel bot temporarily deployed to Preview April 5, 2021 11:17 Inactive
@viniciuscr
Copy link
Contributor Author

@iaincollins @balazsorban44
any advance to merge this PR?

@balazsorban44 balazsorban44 self-requested a review April 6, 2021 17:11
@vercel vercel bot temporarily deployed to Preview April 6, 2021 17:11 Inactive
Copy link
Member

@balazsorban44 balazsorban44 left a 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.

@balazsorban44 balazsorban44 merged commit 5a3ee47 into nextauthjs:main Apr 6, 2021
@github-actions
Copy link

github-actions bot commented Apr 6, 2021

🎉 This PR is included in version 3.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

viniciuscr added a commit to viniciuscr/next-auth that referenced this pull request Apr 9, 2021
…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 <info@balazsorban.com>
@github-actions
Copy link

🎉 This PR is included in version 4.0.0-next.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Refers to `@auth/core` documentation Relates to documentation providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants