-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add force_pkce option #1705
Add force_pkce option #1705
Conversation
The force_pkce option is disabled by default. When enabled, it requires non-confidential clients to use PKCE when requesting an access_token using an authorization code.
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.
👍
@nbulaj Any feedback on this? We've now implemented it as monkeypatches in my company's app, but it would be cool to get it into the gem. I'm happy to make changes to the design or the implementation if you have any issues with the current code. Thanks! |
hey @mattmanning , I find it great, thanks! Let me check the implementation details |
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.
Just a minor comment, otherwise LGTM 👍
doorkeeper.gemspec
Outdated
@@ -52,6 +52,7 @@ Gem::Specification.new do |gem| | |||
gem.add_development_dependency "factory_bot", "~> 6.0" | |||
gem.add_development_dependency "generator_spec", "~> 0.10.0" | |||
gem.add_development_dependency "grape" | |||
gem.add_development_dependency "pry-byebug" |
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.
I don't find pry / byebug useful, can we avoid adding it there please?
@nbulaj Thank you for looking! I have removed pry-byebug. Please let me know if you would like any other changes. |
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.
LGTM 🙇♂️
The force_pkce option is disabled by default. When enabled, it requires non-confidential clients to use PKCE when requesting an access_token using an authorization code.
Summary
PKCE should be used for all non-confidential clients (those who can't protect their client_secret). However, since the OAuth 2.0 spec doesn't require it, it should be disabled by default and only used if desired by the server. https://www.rfc-editor.org/rfc/rfc7636#section-5
Other Information
This implementation is slightly different than what was discussed in #1654. A more stringent option that also requires PKCE for confidential clients could later be added to a bundled OAuth 2.1 option.