zitadel / oidc

Easy to use OpenID Connect client and server library written for Go and certified by the OpenID Foundation
https://zitadel.com
Apache License 2.0
1.38k stars 145 forks source link

fix: Omit non-standard, empty fields in `RefreshTokenRequest` when performing a token refresh #599

Closed ymarcus93 closed 5 months ago

ymarcus93 commented 5 months ago

The OIDC spec's definition of a refresh request does not include client_assertion or client_assertion_type as valid parameters for the refresh request. See request format here: https://openid.net/specs/openid-connect-core-1_0.html#RefreshingAccessToken. The document only displays client_id, client_secret, grant_type, refresh_token, and scope as acceptable parameters.

Therefore, I propose we add the omitempty tags to the ClientAssertion and ClientAssertionType fields in RefreshTokenRequest, so that the token refresh functionality provided by rp.RefreshTokens can work with identity providers that may have additional logic or different expectations when these fields are included in the refresh token request.

For example, when attempting to construct an OIDC client via rp.RelyingParty against an Okta identity provider, I ran into issues when performing refresh with rp.RefreshTokens. The Okta identity provider returned http status not ok: 400 Bad Request {"error":"invalid_request","error_description":"The client_assertion_type is invalid."} as an error. I assume I'm receiving this error because I'm calling the RefreshTokens func with clientAssertion="" and clientAssertionType=""; the addition of the omitempty tags resolves this issue and hopefully future proofs this library against other identity providers that have the same behavior.

Definition of Ready

livio-a commented 5 months ago

hey @ymarcus93 thanks for the fix and totally agree on that. Since scope and depending on the authentication also client_id and client_secret are optional, I'd add the omitempty tag there as well.

ymarcus93 commented 5 months ago

@livio-a Rebased and pushed b03e835 with omitempty on scope, client_id, and client_secret

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 62.04%. Comparing base (0992c5f) to head (b03e835). Report is 56 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #599 +/- ## ========================================== + Coverage 60.06% 62.04% +1.98% ========================================== Files 80 81 +1 Lines 6998 6192 -806 ========================================== - Hits 4203 3842 -361 + Misses 2498 2041 -457 - Partials 297 309 +12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 5 months ago

:tada: This PR is included in version 3.23.2 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: