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

Implement PKCE #91

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement PKCE #91

wants to merge 5 commits into from

Conversation

Ross65536
Copy link

Implement PKCE for authorization code grant type, as per RFC-7637:

  • add string fields code_challenge and code_challenge_method to oauth_access_grants table which will contain the PKCE information. Add instructions for upgrading.
  • add config option use_pkce. If true when issuing grants the code_challenge and code_challenge_method query fields are mandatory and are saved to the grant model. If false these fields are ignored and the corresponding grant models are set to nil
  • when emitting an access token, the grant model is checking for having the code_challenge_method field set. If set to a value not nil then the code_verifier query field is mandatory and it's used to check against the grant's code_challenge field. If set to nil this query parameter is ignored and the grant acts as though PKCE is disabled.

Partially inspired by #61

@dagumak
Copy link

dagumak commented Dec 19, 2021

Aww man! I was going to implement this, but you beat me to it.

@dagumak
Copy link

dagumak commented Dec 19, 2021

I think it would be a good idea to add a test with the examples in Appendix A and Appendix B.

@Ross65536
Copy link
Author

Appendix B

I'll see If I have time later for this. But feel free to contribute with the tests if you need it right away :)

@dagumak
Copy link

dagumak commented Dec 31, 2021

Appendix B

I'll see If I have time later for this. But feel free to contribute with the tests if you need it right away :)

I'm still new to Elixir, but I may give it a try later :)

@lukyanov
Copy link

Waiting for this to be merged to upstream. A very good PR!

Just one note. use_pkce: true forces PKCE for all applications. There are use cases when you want to enable it for some applications only. For example, I want to force it for javascript apps, but don't for server-side apps. What if instead of using the global config parameter we do these things instead:

  1. If code_challenge is present in the authorization request, we assume the client wants PKCE check and enable it.
  2. We add a field to the oauth_applications table something like require_pkce, and if it's true, we fail the authorization if the parameters are not present.

What do you think?

@lukyanov
Copy link

@Ross65536 Made a PR to your repo :)
Ross65536#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants