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
JWT: Upgrade from v3 to v5 #193
base: master
Are you sure you want to change the base?
Conversation
I resolved a go mod conflict, but then forgot to run |
Sorry about that. I got it to pass the build and test portion in my forked repo. Now I'm stuck at a linting error that says I can't use |
can this be set in NewService (auth.go)? Or this be too late? |
The unit tests in the |
Well, the token's Service has token.NewService constructor, which to me seems to be the good place to put this jwt init thing. From what i can see all the tests for token start with NewService |
…it function to NewService constructor
Moved the variable. The linter didn't seem to run right on my fork, but at least the tests did. |
there we go! all passed 😅 |
Pull Request Test Coverage Report for Build 7563891591
💛 - Coveralls |
@@ -22,7 +22,7 @@ import ( | |||
log "github.com/go-pkgz/lgr" | |||
"github.com/go-pkgz/rest" | |||
"github.com/go-pkgz/rest/logger" | |||
"github.com/golang-jwt/jwt" | |||
oldjwt "github.com/golang-jwt/jwt" |
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.
Is there any reason to keep old version of JWT for an example instead of updating it here as well?
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.
The external library wants to consume that specific function. Once the external library also upgrades, it will make sense to bump.
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.
Can you please clarify this part? What exactly "the external library" do you mean?
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.
oldjwt is used in only one place -- as input to github.com/go-oauth2/oauth2/v4/manage
. Sure, you could rewrite it, but that's not what the manage
package expects.
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.
I've tried, and I think it doesn't make sense. The dependency will move from direct to indirect, and example code will become more complicated. See the link for the results.
Just so you know these are the changes which have to be implemented in Remark42 after updating to the I propose to mark the current master as the last release of v1 and then bump the major version of this library after the merge. I thought of updating to |
The order of "aud" and "iss" are different because the new version of the library changed the types of each of the members. "aud" is now a union type of |
Will the new version properly verify the old message? It should be because the signature matches the content. But what will happen next? I mean, will the old message be correctly decoded/unmarshaled, or will the order of fields prevent it from happening? Having a test consuming the old token should help to clarify this scenario |
Field ordering doesn't matter for golang's |
@freddierice Thank you very much, we will deal with this. @paskal assuming old tokens will continue to work, what is the part breaking back compatibility? I don't see anything on the exposed level that will require any change on the consumer side. I have not checked it in detail, but on the surface, it seems to be back-compatible. |
Here are the changes due to migration to v5. I prefer to cut the current master as v1 of this library and release version v2 with JWTv5 + a minor breaking change of setting the type explicitly here in RefreshCache as it has As a user of this library JWTv5 changes seem inconvenient enough to cut a new release for me. |
I've prepared #200, and once it's merged, I'll ask you to change the PR to target the v2 directory. I want to clarify one interface used for cache to specify types explicitly, which is a breaking change strictly speaking. I consider changes in this PR breaking as well, as they require some users to change their code after the update. |
FYI, I think you can copy all changed files to v2, fix imports, re-run go get to get v5 of JWT, and it will be enough to get PR ready for v2. |
The changes in branch https://github.com/go-pkgz/auth/tree/paskal/v2_jwt5 have been moved to the v2 directory. Feel free to copy them to this PR. |
The current jwt code has been deprecated in the upstream This PR aims to upgrade the library to v5.
This will make upgrading for security vulnerabilities / bugs easier in the future.
Big Changes
StandardClaims
has been deprecated -- RegisteredClaims is the new struct to use.MarshalSingleStringAsArray
)