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

[Token/JWT] Update to golang-jwt v5.2.0 #19802

Merged
merged 16 commits into from
Feb 23, 2024

Conversation

an-toine
Copy link
Contributor

@an-toine an-toine commented Jan 5, 2024

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 the v5 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:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

cc @Vad1mo

@an-toine an-toine requested a review from a team as a code owner January 5, 2024 11:18
@Vad1mo Vad1mo added the release-note/update Update or Fix label Jan 5, 2024
@Vad1mo Vad1mo enabled auto-merge (squash) January 5, 2024 12:45
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a3e1b1e) 67.45% compared to head (e22a356) 67.45%.
Report is 10 commits behind head on main.

❗ Current head e22a356 differs from pull request most recent head a788b68. Consider uploading reports for the commit a788b68 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           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           
Flag Coverage Δ
unittests 67.45% <75.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
src/core/service/token/authutils.go 69.14% <ø> (ø)
src/pkg/token/options.go 47.61% <ø> (ø)
src/server/middleware/security/v2_token.go 64.51% <100.00%> (+11.88%) ⬆️
src/pkg/token/claims/robot/robot.go 83.33% <66.66%> (+0.98%) ⬆️
src/pkg/token/claims/v2/claims.go 75.00% <66.66%> (+5.00%) ⬆️
src/pkg/token/token.go 51.02% <75.00%> (+1.02%) ⬆️

... and 4 files with indirect coverage changes

auto-merge was automatically disabled January 5, 2024 13:33

Head branch was pushed to by a user without write access

Signed-off-by: Antoine Jouve <[email protected]>
@MinerYang MinerYang self-assigned this Jan 10, 2024
@MinerYang
Copy link
Contributor

MinerYang commented Jan 12, 2024

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!

@an-toine an-toine marked this pull request as draft January 22, 2024 15:57
@an-toine an-toine force-pushed the feat/update_golang-jwt_v5.2.0 branch from eff7bdb to 8535534 Compare January 22, 2024 16:06
@an-toine an-toine force-pushed the feat/update_golang-jwt_v5.2.0 branch from 9331285 to 2bd67f8 Compare January 23, 2024 10:18
@an-toine an-toine marked this pull request as ready for review January 23, 2024 10:20
@an-toine
Copy link
Contributor Author

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!

I've fixed the conflict, rebased, updated leeways and added two test cases to validate change effectiveness.

Antoine

@an-toine an-toine force-pushed the feat/update_golang-jwt_v5.2.0 branch from c3643ae to 4b33598 Compare January 25, 2024 15:43
src/server/middleware/security/v2_token.go Outdated Show resolved Hide resolved
Signed-off-by: Antoine Jouve <[email protected]>
@MinerYang MinerYang self-requested a review January 29, 2024 06:01
@an-toine
Copy link
Contributor Author

@MinerYang : can you retest please ?

@MinerYang
Copy link
Contributor

@MinerYang : can you retest please ?

Hi @an-toine ,

Thanks and sorry for the late response since I was just came back from holiday.

src/pkg/token/token.go Show resolved Hide resolved
src/pkg/token/token.go Outdated Show resolved Hide resolved
src/pkg/token/token_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@MinerYang
Copy link
Contributor

Thanks @an-toine for your dedicated contribution!

Best,
Miner

@MinerYang MinerYang merged commit 73c2884 into goharbor:main Feb 23, 2024
10 checks passed
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.

7 participants