Enable OAuth2 PKCE extension in Doorkeeper #2312
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisroos
force-pushed
the
add-pkce
branch
8 times, most recently
from
9130b04
to
982c440
Compare
floehopper
approved these changes
Aug 21, 2023
There was a problem hiding this comment.
This looks good to me 👍 I have a few thoughts...
- I don't feel strongly about keeping CSRF enabled for the tests. Since
Capybara::DSLis mixed intoActionDispatch::IntegrationTestwould it be feasible/sensible to use those for the user bit of the interaction (i.e. signing in) thus coping with CSRF...? Or is there a problem that Capybara will follow redirects you don't want to...? - I suppose ideally you'd use different sessions, although I have to admit I'm only 99% sure that's actually how things work! Having said that, I'm not sure it's worth putting a lot of effort into it. Would using a mixture of Capybara & Rails request helpers (as described in my previous point) achieve separate sessions? If not, could you not just call
ActionDispatch::Integration::Session#reset!in the test after signing in and before requesting the authorization code...? - These Doorkeeper docs mention checking a couple of things when enabling PKCE - it would be nice to mention somewhere that you have checked them even if they don't apply.
I'm happy to leave all the above to your discretion - essentially it'd be fine to merge as is!
Lastly, don't forget to remove the TODO points from the commit note before you merge!
This test demonstrates/exercises the OAuth2 interactions involved in the Authorization Code Grant type[1]. This is the "happy path" that's followed when the user has the signin permission in the application they're authorizing to make requests on their behalf. I'm adding this to give me confidence that I'm not breaking this behaviour when I enable the PKCE extension. Note that some of our "integration" tests are using Rails' RequestHelpers[2] (e.g `get` and `post`) while others are using the Capybara methods (e.g. `visit`). I'm using the Rails RequestHelpers as I need to simulate requests that happen outside of the browser (e.g. the request for the Access Token). Note that CSRF protection was enabled in our integration tests in c69288d. It's not particularly useful in these tests so I'm disabling it rather than working out how to get/set the authenticity token. I'm using `ActionDispatch::Integration::Runner#reset!`[3] to reset the session between the user approving the Authorization Grant (in the form of the Authorization Code) and the client using that Authorization Grant to request an Access Token. My hope is that this makes it slightly clearer that the Authorization Code request is performed by the user's User Agent and the Access Token request, and subsequent request for the protected resource, is performed by the client. [1]: https://oauth.net/2/grant-types/authorization-code/ [2]: https://api.rubyonrails.org/v7.0.7/classes/ActionDispatch/Integration/RequestHelpers.html [3]: https://api.rubyonrails.org/v7.0.7/classes/ActionDispatch/Integration/Runner.html#method-i-reset-21
I ran `bundle exec rails generate doorkeeper:pkce` to enable the PKCE extension[1] in Doorkeeper[2]. Rubocop complained about the migration that was generated so I changed it to use `change_table` instead of two calls to `add_column`. The PKCE extension isn't mandatory at this point (as demonstrated by the "oauth2 authorization code grant type" test continuing to pass) but it will be used if the client supplies a `code_challenge_method` and `code_challenge` when requesting the Authorization Code. It's our intention to mandate the use of PKCE once we can be sure all clients of Signon are using it. The Doorkeeper docs[2] include the following 2 points to consider: 1. If you overrode the doorkeeper/authorizations/new.html.erb view, make sure that you have the code_challenge and code_challenge_method hidden form fields. We never render this template and haven't overridden it so this is of no concern to us. 2. Also, in case your client is public (e.g. mobile app, single page app) note that the Doorkeeper::Application for the client should have confidential: false. We only support confidential clients at the moment so this is of no concern to us. Note that we set `Doorkeeper::Application#confidential` to true in schema.rb[3] and don't allow it to be overridden in the UI. [1]: https://datatracker.ietf.org/doc/html/rfc7636 [2]: https://doorkeeper.gitbook.io/guides/ruby-on-rails/pkce-flow [3]: https://github.com/alphagov/signon/blob/78ebf57fe0a787d1e462b39f2475d3b74f085511/db/schema.rb#L114
|
Thanks, @floehopper 👍
I haven't tried but I worry that it might be confusing to mix the two styles in a single test. Especially as I'd prefer to more explicitly split the integration-style and the system-style tests that already make up our "integration tests".
I've updated the commit to use
I've updated the commit note to explain why these don't concern us.
Thank you!
Done. |
chrisroos
added a commit
to alphagov/gds-sso
that referenced
this pull request
Aug 23, 2023
https://trello.com/c/59EBweBx In alphagov/signon#2312 we enabled the [OAuth2 PKCE extension](https://datatracker.ietf.org/doc/html/rfc7636) in Signon. In this commit we update our GDS OAuth2 OmniAuth Strategy to make use of the PKCE extension. This means that any of our apps using this Gem will benefit from the additional protection offered by the PKCE extension.
chrisroos
added a commit
to alphagov/gds-sso
that referenced
this pull request
Aug 23, 2023
https://trello.com/c/59EBweBx In alphagov/signon#2312 we enabled the OAuth2 PKCE extension[1] in Signon. In this commit we update our GDS OAuth2 OmniAuth Strategy to make use of the PKCE extension. This means that any of our apps using this Gem will benefit from the additional protection offered by the PKCE extension. [1]: https://datatracker.ietf.org/doc/html/rfc7636
chrisroos
added a commit
to alphagov/gds-sso
that referenced
this pull request
Aug 23, 2023
https://trello.com/c/59EBweBx In alphagov/signon#2312 we enabled the OAuth2 PKCE extension[1] in Signon. In this commit we update our GDS OAuth2 OmniAuth Strategy to make use of the PKCE extension. This means that any of our apps using this Gem will benefit from the additional protection offered by the PKCE extension. [1]: https://datatracker.ietf.org/doc/html/rfc7636
chrisroos
added a commit
to alphagov/gds-sso
that referenced
this pull request
Aug 24, 2023
https://trello.com/c/59EBweBx In alphagov/signon#2312 we enabled the OAuth2 PKCE extension[1] in Signon. In this commit we update our GDS OAuth2 OmniAuth Strategy to make use of the PKCE extension. This means that any of our apps using this Gem will benefit from the additional protection offered by the PKCE extension. [1]: https://datatracker.ietf.org/doc/html/rfc7636
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://trello.com/c/59EBweBx
Signon utilises the OAuth 2.0 Authorization Code Grant type to enable users to authorize client applications to act on their behalf. That page includes:
This PR enables the PKCE extension in Doorkeeper. We're not yet mandating Signon clients to use PKCE but it will be used if the client supplies a
code_challengeandcode_challenge_methodin the request for the Authorization Code.We hope to mandate the use of PKCE once we're confident all Signon clients are using it.