feat: support PKCE code_verifier in exchange_auth_code_for_tokens - #791
feat: support PKCE code_verifier in exchange_auth_code_for_tokens#791gagalago wants to merge 2 commits into
Conversation
| headers: { "Content-Type" => "application/json" } | ||
| ) | ||
|
|
||
| result = @client_secret_instance.send( |
There was a problem hiding this comment.
Both new tests run through @client_secret_instance, so the case PKCE is actually built for, a public client with no secret, never gets exercised.
Could we add a third case on a no-secret instance that asserts code_verifier is present and client_secret is absent from the body?
| code: code, | ||
| redirect_uri: redirect_uri | ||
| } | ||
| request_params[:code_verifier] = code_verifier unless code_verifier.nil? |
There was a problem hiding this comment.
This adds the key only when it is set, with unless code_verifier.nil?, while the other optional inputs on this method (redirect_uri right above, and client_secret) go into the hash as plain nil values and get sent as null. So code_verifier is the one param here that stays out of the body entirely when unset.
It reads inconsistent with the sibling params on the same method, and someone will eventually question the intent. We can add a small comment.
Changes
Adds an optional
code_verifierkeyword toAuth0::Api::AuthenticationEndpoints#exchange_auth_code_for_tokens, so the Authorization Code Flow with PKCE can be completed through the SDK.The parameter is sent only when provided:
No endpoint is added, removed or changed.
POST /oauth/tokenis already the target; this only allows one more documented body parameter to reach it. The keyword is appended last and defaults tonil, so every existing call site behaves exactly as before — whencode_verifieris omitted the request body is byte-for-byte what it is today.Usage, completing the flow that
authorization_urlcan already start viaadditional_parameters:References
code_verifierto be passed toexchange_auth_code_for_tokensendpoint #305 — "Allowcode_verifierto be passed toexchange_auth_code_for_tokensendpoint"In #305 a maintainer wrote that "
code_verifierspecifically should be a first-class parameter on that method, and should be optional", which is the shape implemented here. That thread also documented a workaround that reachesrequest_with_retrydirectly; it still works, but it depends on a method outside the documented public API.code_verifierdoes not currently appear anywhere in the repository, so as of v6.1.0 there is no supported way to perform a PKCE exchange. For reference,node-auth0exposes this asauthorizationCodeGrantWithPKCE, which requirescodeandcode_verifier.Testing
Two cases added to
test/unit/authentication_endpoints_test.rb, next to the existingexchange_auth_code_for_tokenstests and using the same WebMock body-matching style:test_exchange_auth_code_for_tokens_with_code_verifier— assertscode_verifieris present in the request body when passedtest_exchange_auth_code_for_tokens_omits_code_verifier_when_not_provided— asserts the key is absent from the body when it is not, which is what protects existing callersTo run them:
I checked that the first test actually fails if the one-line implementation is removed, so it is not a test that passes regardless.
Full suite on Ruby 3.3.9, before and after:
554 runs, 4582 assertions, 0 failures, 0 errors.Unrelated heads-up while testing on Ruby 4.0.6: the full suite reports one error there,
NoMethodError: undefined method 'parse' for class CGI, fromCGI.parseattest/unit/authentication_endpoints_test.rb:480on master.CGI.parsewas removed in Ruby 4.0. It reproduces on a clean checkout of master without this branch, and nothing in this change touchesCGI— flagging it only becauserequired_ruby_versionis>= 3.3.0with no upper bound, so Ruby 4 users will hit it. Happy to open a separate issue or pull request for that if useful.One note on style:
lib/auth0/api/authentication_endpoints.rbuses single-quoted strings throughout and is listed underAllCops.Excludein.rubocop.yml("Files ported verbatim from the legacy ruby-auth0 SDK... Excluded to preserve exact compatibility with the original source"), so I followed the surrounding convention rather than thedouble_quotesstyle enforced elsewhere.bundle exec rubocopreports the same 134 offences with and without this branch — it introduces none.The two new tests pass on both Ruby 3.3.9 and Ruby 4.0.6 (
2 runs, 4 assertions, 0 failures).Checklist