-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Token/JWT] Update to golang-jwt v5.2.0 #19802
[Token/JWT] Update to golang-jwt v5.2.0 #19802
Conversation
Signed-off-by: Antoine Jouve <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #19802 +/- ##
=======================================
Coverage 67.45% 67.45%
=======================================
Files 996 996
Lines 109773 109770 -3
Branches 2720 2720
=======================================
+ Hits 74044 74049 +5
+ Misses 31747 31739 -8
Partials 3982 3982
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Antoine Jouve <[email protected]>
Head branch was pushed to by a user without write access
Signed-off-by: Antoine Jouve <[email protected]>
Hi @an-toine , Could you help to resolve conflict and rebase the branch as well as update the leeway to 60s as we discussed at your convenience? Appreciate! |
eff7bdb
to
8535534
Compare
Signed-off-by: Antoine Jouve <[email protected]>
Signed-off-by: Antoine Jouve <[email protected]>
Signed-off-by: Antoine Jouve <[email protected]>
Signed-off-by: Antoine Jouve <[email protected]>
Signed-off-by: Antoine Jouve <[email protected]>
9331285
to
2bd67f8
Compare
I've fixed the conflict, rebased, updated leeways and added two test cases to validate change effectiveness. Antoine |
Signed-off-by: Antoine Jouve <[email protected]>
c3643ae
to
4b33598
Compare
Signed-off-by: Antoine Jouve <[email protected]>
@MinerYang : can you retest please ? |
Hi @an-toine , Thanks and sorry for the late response since I was just came back from holiday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Signed-off-by: Antoine Jouve <[email protected]>
Thanks @an-toine for your dedicated contribution! Best, |
Comprehensive Summary of your change
This PR is a follow-up of #19796 and, if validated, could supplant it.
Both PR share a similar goal, providing some leeway around token validity timestamps to account for clock skew on distributed systems.
This time though, this PR is leveraging the built-in
leeway
attribute brought by thev5
branch of golang-jwt instead of setting a fix offset on claims.This code should be carefully reviewed, I'm new to the Harbor codebase and I may have missed some key issues during the migration (I used https://github.com/golang-jwt/jwt/blob/main/MIGRATION_GUIDE.md for this task).
Finally, the leeway value was set arbitrarily, we can maybe use an higher value (up to 1 minute) to improve reliability.
Issue being fixed
Fixes #18880
Please indicate you've done the following:
cc @Vad1mo