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

Update to golang-jwt V5 #332

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gblandinkingland
Copy link

Updates the golang-jwt/jwt dependency to V5
Updates an error check in the middleware due to expanded validation in V5
Modifies a check in the refresh flow due to the reworking of errors in V5

Let me know if additional changes need to be made, or if I missed anything in the contribution process.

Thanks!

The error types that were being used were removed in v5
Added default for parser option to propagate the time func
Also adjusted the get claims error check around expiry
Updated a related test due to changes
Copy link

codecov bot commented Feb 3, 2024

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 88.76%. Comparing base (a4e49c3) to head (110df2f).
Report is 3 commits behind head on master.

❗ Current head 110df2f differs from pull request most recent head 39f16e3. Consider uploading reports for the commit 39f16e3 to get more accurate results

Files Patch % Lines
auth_jwt.go 80.95% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #332      +/-   ##
==========================================
+ Coverage   87.36%   88.76%   +1.40%     
==========================================
  Files           1        1              
  Lines         459      454       -5     
==========================================
+ Hits          401      403       +2     
+ Misses         45       38       -7     
  Partials       13       13              
Flag Coverage Δ
go- 88.76% <80.95%> (+1.40%) ⬆️
go-1.18 88.76% <80.95%> (+1.40%) ⬆️
go-1.19 88.76% <80.95%> (+1.40%) ⬆️
go-1.20 88.76% <80.95%> (+1.40%) ⬆️
go-1.21 88.76% <80.95%> (+1.40%) ⬆️
go-1.22 ?
macos-latest 88.76% <80.95%> (+1.40%) ⬆️
ubuntu-latest 88.76% <80.95%> (+1.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +444 to 446
if claims["exp"] == nil {
mw.unauthorized(c, http.StatusBadRequest, mw.HTTPStatusMessageFunc(ErrMissingExpField, c))
return
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure if this should be kept, or if the WithExpirationRequired should just always be added similar to how I added WithTimeFunc.

gblandinkingland and others added 3 commits February 5, 2024 14:24
Added test case for using the expiration required parser option
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.

2 participants