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

v2: validator for golang-jwt/jwt #91

Merged
merged 5 commits into from
Jun 12, 2021
Merged

Conversation

oxisto
Copy link
Contributor

@oxisto oxisto commented May 29, 2021

https://github.com/golang-jwt/jwt will serve as a long-term replacement to https://github.com/dgrijalva/jwt-go. Following the ideas discussed in #73, I have added a validator that uses the new repository.

It follows largely the same style as the jose one, with a few exceptions:

  • Clock skew is not supported
  • Expected claims can not be set

I want to hold of on marking this "ready" until there is a first v3.2.1 release of https://github.com/golang-jwt/jwt, which will happened soon.

Additionally, it would probably make sense to put the validators in their own go module, so that only the validators have a dependency to the actual jwt libraries, not the middleware itself.

@oxisto oxisto changed the base branch from master to v2 May 29, 2021 16:39
@oxisto oxisto changed the title jwt go validator v2: validator for golang-jwt/jwt May 29, 2021
@oxisto oxisto marked this pull request as ready for review June 8, 2021 13:41
@oxisto oxisto requested a review from a team as a code owner June 8, 2021 13:41
@grounded042
Copy link
Contributor

Hey @oxisto thanks for this! I'm a little swamped at the moment, but I'm carving out some time on Friday to review this.

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2021

Codecov Report

Merging #91 (fe44742) into v2 (665e7da) will increase coverage by 1.85%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v2      #91      +/-   ##
==========================================
+ Coverage   90.56%   92.42%   +1.85%     
==========================================
  Files           2        3       +1     
  Lines         106      132      +26     
==========================================
+ Hits           96      122      +26     
  Misses          9        9              
  Partials        1        1              
Impacted Files Coverage Δ
validate/jwt-go/jwtgo.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 665e7da...fe44742. Read the comment docs.

Copy link
Contributor

@grounded042 grounded042 left a comment

Choose a reason for hiding this comment

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

Overall this looks great and thank you for the contribution! I left a couple of comments and nits on cleanups.

It follows largely the same style as the jose one, with a few exceptions:

  • Clock skew is not supported
  • Expected claims can not be set

Not supporting clock skew isn't a blocker, but it is a downside here. One of the features we want to support is clock skew. Is it possible that golang-jwt/jwt will support this in the future?

Not supporting expected claims is no problem as that's a setup from the other package.

@oxisto
Copy link
Contributor Author

oxisto commented Jun 11, 2021

Overall this looks great and thank you for the contribution! I left a couple of comments and nits on cleanups.

It follows largely the same style as the jose one, with a few exceptions:

  • Clock skew is not supported
  • Expected claims can not be set

Not supporting clock skew isn't a blocker, but it is a downside here. One of the features we want to support is clock skew. Is it possible that golang-jwt/jwt will support this in the future?

Not supporting expected claims is no problem as that's a setup from the other package.

Thanks for the review, I will look at the smaller changes. I am planning to add clock skew among other validation features from the old v4 to golang-jwt/jwt in a backwards compatible way (see golang-jwt/jwt#16). Unfortunately, I do not have a dedicate time-frame for this, yet. June is quite busy here.

@grounded042
Copy link
Contributor

Awesome! Knowing it's on the backlock is a big help!

Copy link
Contributor

@grounded042 grounded042 left a comment

Choose a reason for hiding this comment

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

Thanks @oxisto, this looks great!

@grounded042 grounded042 merged commit 66a443b into auth0:v2 Jun 12, 2021
sergiught pushed a commit that referenced this pull request Nov 1, 2021
d10i pushed a commit to Hikely/go-jwt-middleware that referenced this pull request Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants