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.33k stars 138 forks source link

feat: Added the ability to verify ID tokens using the value of id_token_signing_alg_values_supported retrieved from DiscoveryEndpoint #579

Closed otakakot closed 4 months ago

otakakot commented 5 months ago

Definition of Ready

fixes #574

Fixed discoveryConfiguration.IDTokenSigningAlgValuesSupported to be set to WithSupportedSigningAlgorithms().

I thought about adding and setting discoveryConfiguration to the relyingParty struct field and doing for _, optFunc := range options{} after getting discoveryConfiguration, WithCustomDiscoveryUrl() had to be processed before client.Discover().

otakakot commented 5 months ago

I would like to add a test to check the behavior of the function I am adding, which is the best way to implement it?

muhlemmer commented 5 months ago

I would like to add a test to check the behavior of the function I am adding, which is the best way to implement it?

You could add an integration test that starts the example OP and then initialize the RP with your the new option.

https://github.com/zitadel/oidc/blob/8a21d3813610c0dc64b349de3d7d01b2480120f0/pkg/client/integration_test.go#L63-L71

codecov[bot] commented 5 months ago

Codecov Report

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

Project coverage is 62.17%. Comparing base (0992c5f) to head (9e700d2). Report is 49 commits behind head on main.

:exclamation: Current head 9e700d2 differs from pull request most recent head 0553090. Consider uploading reports for the commit 0553090 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #579 +/- ## ========================================== + Coverage 60.06% 62.17% +2.11% ========================================== Files 80 81 +1 Lines 6998 6192 -806 ========================================== - Hits 4203 3850 -353 + Misses 2498 2037 -461 - Partials 297 305 +8 ```

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

muhlemmer commented 4 months ago

@otakakot I understood you still want to add tests, is that correct? I am waiting with the review until that's done.

otakakot commented 4 months ago

@muhlemmer Excuse me, may I have a review at this stage?

muhlemmer commented 4 months ago

The implementation looks good to me :+1:

otakakot commented 4 months ago

I would like to add a test to check the behavior of the function I am adding, which is the best way to implement it?

You could add an integration test that starts the example OP and then initialize the RP with your the new option.

https://github.com/zitadel/oidc/blob/8a21d3813610c0dc64b349de3d7d01b2480120f0/pkg/client/integration_test.go#L63-L71

https://github.com/zitadel/oidc/pull/579/commits/75723536781fca72bb911366055cd835620f46e1 Tests have been added.

github-actions[bot] commented 4 months ago

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

The release is available on GitHub release

Your semantic-release bot :package::rocket: