Skip to content
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

feat: merge the verifier types #336

Merged
merged 8 commits into from
Mar 22, 2023
Merged

feat: merge the verifier types #336

merged 8 commits into from
Mar 22, 2023

Conversation

muhlemmer
Copy link
Collaborator

@muhlemmer muhlemmer commented Mar 17, 2023

BREAKING CHANGE:

  • The various verifier types are merged into a oidc.Verifier.
  • oidc.Verfier became a struct with exported fields
  • use type aliases for oidc.Verifier, this binds the correct contstructor to each verifier usecase.

Closes #314

TODO:

  • Unit tests for oidc/verifier.go
  • unit test for op/auth_request.go: ValidateAuthReqIDTokenHint()
  • unit tests for op/verifier_jwt_profile.go

BREAKING CHANGE:

- The various verifier types are merged into a oidc.Verifir.
- oidc.Verfier became a struct with exported fields
this binds the correct contstructor to each verifier usecase.
@muhlemmer muhlemmer changed the base branch from next to main March 20, 2023 19:33
@muhlemmer muhlemmer changed the base branch from main to next March 20, 2023 19:33
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #336 (137fcdf) into next (c8cf15e) will increase coverage by 1.43%.
The diff coverage is 95.69%.

@@            Coverage Diff             @@
##             next     #336      +/-   ##
==========================================
+ Coverage   50.84%   52.28%   +1.43%     
==========================================
  Files          74       74              
  Lines        5631     5587      -44     
==========================================
+ Hits         2863     2921      +58     
+ Misses       2508     2414      -94     
+ Partials      260      252       -8     
Impacted Files Coverage Δ
pkg/op/client.go 92.98% <ø> (ø)
pkg/op/mock/authorizer.mock.go 0.00% <0.00%> (ø)
pkg/op/mock/authorizer.mock.impl.go 0.00% <0.00%> (ø)
pkg/op/session.go 32.91% <ø> (ø)
pkg/op/token_intospection.go 43.24% <ø> (ø)
pkg/op/token_jwt_profile.go 16.66% <ø> (ø)
pkg/op/token_request.go 59.59% <ø> (ø)
pkg/op/token_revocation.go 32.75% <ø> (ø)
pkg/op/userinfo.go 40.00% <ø> (ø)
pkg/client/rp/verifier.go 94.11% <96.77%> (-0.89%) ⬇️
... and 9 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

JWTTokenRequest.GetIssuedAt() was returning the ExpiresAt field.
This change corrects that by returning IssuedAt instead.

This bug was introduced in #283
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is used only for tests. Changes here are to accommodate newly added tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes cherry-picked from #339

@@ -192,7 +192,7 @@ func (j *JWTTokenRequest) GetExpiration() time.Time {

// GetIssuedAt implements the Claims interface
func (j *JWTTokenRequest) GetIssuedAt() time.Time {
return j.ExpiresAt.AsTime()
return j.IssuedAt.AsTime()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cherry -pick from #341

@muhlemmer muhlemmer marked this pull request as ready for review March 21, 2023 13:43
@muhlemmer muhlemmer linked an issue Mar 21, 2023 that may be closed by this pull request
4 tasks
@muhlemmer muhlemmer requested a review from stebenz March 22, 2023 15:20
pkg/oidc/verifier.go Show resolved Hide resolved
pkg/oidc/verifier.go Show resolved Hide resolved
pkg/oidc/verifier.go Outdated Show resolved Hide resolved
pkg/oidc/verifier.go Show resolved Hide resolved
pkg/op/verifier_jwt_profile_test.go Show resolved Hide resolved
internal/testutil/token.go Show resolved Hide resolved
@muhlemmer muhlemmer merged commit 33c716d into next Mar 22, 2023
@muhlemmer muhlemmer deleted the merge-verifiers branch March 22, 2023 17:18
@github-actions
Copy link

🎉 This PR is included in version 3.0.0-next.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@muhlemmer muhlemmer added this to the v3 milestone Apr 22, 2023
@github-actions
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cleanup and merge all Verifier variations
2 participants