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

refactor: use struct types for claim related types #283

Merged
merged 19 commits into from
Mar 10, 2023

Conversation

muhlemmer
Copy link
Collaborator

@muhlemmer muhlemmer commented Feb 17, 2023

BREAKING change.
The following types are changed from interface to struct type:

  • AccessTokenClaims
  • IDTokenClaims
  • IntrospectionResponse
  • UserInfo and related types.
  • JWTProfileAssertionClaims

The following methods of OPStorage now take a pointer to a struct type,
instead of an interface:

  • SetUserinfoFromScopes
  • SetUserinfoFromToken
  • SetIntrospectionFromToken

The following functions are now generic, so that type-safe extension
of Claims is now possible:

  • op.VerifyIDTokenHint
  • op.VerifyAccessToken
  • rp.VerifyTokens
  • rp.VerifyIDToken

This is still a work in process. Need to create some regression tests and I want to verify if the current approach is what we want.

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Merging #283 (5bf7208) into next (4dca29f) will increase coverage by 4.78%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             next     #283      +/-   ##
==========================================
+ Coverage   41.74%   46.52%   +4.78%     
==========================================
  Files          83       74       -9     
  Lines        7254     5567    -1687     
==========================================
- Hits         3028     2590     -438     
+ Misses       3927     2748    -1179     
+ Partials      299      229      -70     
Impacted Files Coverage Δ
pkg/client/client.go 20.10% <0.00%> (ø)
pkg/client/rp/cli/cli.go 0.00% <0.00%> (ø)
pkg/client/rs/resource_server.go 29.72% <0.00%> (ø)
pkg/oidc/token_request.go 12.85% <0.00%> (ø)
pkg/oidc/verifier.go 66.94% <ø> (+28.81%) ⬆️
pkg/op/auth_request.go 54.73% <0.00%> (ø)
pkg/op/mock/storage.mock.go 11.05% <0.00%> (ø)
pkg/op/storage.go 50.00% <ø> (ø)
pkg/op/token_exchange.go 47.39% <0.00%> (ø)
pkg/op/token_intospection.go 2.70% <0.00%> (ø)
... and 13 more

... and 10 files 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.

@muhlemmer muhlemmer force-pushed the concrete-claims-type branch 3 times, most recently from a33086e to 4585640 Compare February 17, 2023 17:30
@muhlemmer
Copy link
Collaborator Author

Created regression tests, based on next branch (4bd8030) and rebased this PR on that. This already caught some bugs :).

@muhlemmer
Copy link
Collaborator Author

muhlemmer commented Feb 19, 2023

TODO

Write unit tests for functions:

  • op.VerifyIDTokenHint
  • op.VerifyAccessToken
  • rp.VerifyTokens
  • rp.VerifyIDToken
  • Write examples on these functions, how to extend with custom types

Write unit tests for types (getters, setters):

  • AccessTokenClaims
  • IDTokenClaims
  • IntrospectionResponse
  • UserInfo and related types.

Write Go doc with references to OpenID / Oauth2 on types:

  • AccessTokenClaims
  • IDTokenClaims
  • IntrospectionResponse
  • UserInfo and related types.

this helps to verify that the same JSON is produced,
after these types are refactored.
BREAKING change.
The following types are changed from interface to struct type:

- AccessTokenClaims
- IDTokenClaims
- IntrospectionResponse
- UserInfo and related types.

The following methods of OPStorage now take a pointer to a struct type,
instead of an interface:

- SetUserinfoFromScopes
- SetUserinfoFromToken
- SetIntrospectionFromToken

The following functions are now generic, so that type-safe extension
of Claims is now possible:

- op.VerifyIDTokenHint
- op.VerifyAccessToken
- rp.VerifyTokens
- rp.VerifyIDToken
- Changed UserInfoAddress to pointer in UserInfo and
IntrospectionResponse.
This was needed to make omitempty work correctly.

- Copy or merge maps in IntrospectionResponse GetUserInfo
and SetUserInfo
- Add get methods for Address fields to handle nil pointers as we used to
WithIssuedAtMaxAge assigned its value to v.maxAge, which was wrong.
This change fixes that by assiging the duration to v.maxAgeIAT.
- IDTokenClaims
- IntrospectionResponse
- UserInfo
@muhlemmer muhlemmer marked this pull request as ready for review March 6, 2023 11:22
@muhlemmer muhlemmer requested a review from livio-a March 6, 2023 11:22
@muhlemmer muhlemmer added this to the v2 milestone Mar 6, 2023
pkg/oidc/token.go Outdated Show resolved Hide resolved
example/server/storage/storage.go Outdated Show resolved Hide resolved
pkg/client/rp/verifier.go Show resolved Hide resolved
pkg/oidc/token.go Outdated Show resolved Hide resolved
pkg/op/verifier_access_token_example_test.go Outdated Show resolved Hide resolved
pkg/client/rp/verifier_tokens_example_test.go Outdated Show resolved Hide resolved
pkg/op/verifier_access_token_example_test.go Outdated Show resolved Hide resolved
pkg/oidc/token_test.go Show resolved Hide resolved
pkg/oidc/token_test.go Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@muhlemmer muhlemmer merged commit dea8bc9 into next Mar 10, 2023
@muhlemmer muhlemmer deleted the concrete-claims-type branch March 10, 2023 14:31
@github-actions
Copy link

🎉 This PR is included in version 2.0.0-next.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

muhlemmer added a commit that referenced this pull request Mar 21, 2023
JWTTokenRequest.GetIssuedAt() was returning the ExpiresAt field.
This change corrects that by returning IssuedAt instead.

This bug was introduced in #283
muhlemmer added a commit that referenced this pull request Mar 21, 2023
JWTTokenRequest.GetIssuedAt() was returning the ExpiresAt field.
This change corrects that by returning IssuedAt instead.

This bug was introduced in #283
muhlemmer added a commit that referenced this pull request Mar 21, 2023
JWTTokenRequest.GetIssuedAt() was returning the ExpiresAt field.
This change corrects that by returning IssuedAt instead.

This bug was introduced in #283
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.

return concrete types of Claims, using generics
2 participants