Replace abandoned dgrijalva/jwt-go with golang-jwt/jwt #2339
Conversation
| return nil, serviceerror.NewPermissionDenied("invalid token with no claims", "") | ||
| } | ||
| if err := claims.Valid(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
will this be caught and returned as a permission denied error elsewhere?
| } else { | ||
| parser = jwt.NewParser(jwt.WithAudience(audience)) | ||
| } | ||
| parser := jwt.NewParser(jwt.WithValidMethods(keyProvider.SupportedMethods())) | ||
| token, err := parser.Parse(tokenString, func(token *jwt.Token) (interface{}, error) { | ||
|
|
||
| kid, ok := token.Header["kid"].(string) |
There was a problem hiding this comment.
is this switch statement here for HmacKey redundant now that we are using the SupportedMethods feature?
There was a problem hiding this comment.
I don't this it's redundant because we need to call keyProvider.HmacKey() in case keyProvider (configured as a separate plugin) supports HMAC. We just don't support it in defaultTokenKeyProvider. They are indeed currently coupled here, but only for convenience.
| @@ -110,6 +110,10 @@ func (a *defaultTokenKeyProvider) EcdsaKey(alg string, kid string) (*ecdsa.Publi | |||
| return key, nil | |||
| } | |||
|
|
|||
| func (a *defaultTokenKeyProvider) SupportedMethods() []string { | |||
| return []string{"RS256", "ES256"} | |||
There was a problem hiding this comment.
Just curious is it necessary for this to be uppercase now? Also can we make these a const, it seems to be reused throughout this code.
There was a problem hiding this comment.
Good point. I replaced them with references to jwt.SigningMethod* now.
What changed?
Replaced abandoned dependency
dgrijalva/jwt-gowithgolang-jwt/jwt.Adjusted the code to address the differences in API.
Added
SupportedMethodsmethod toTokenKeyProvider. This is a small breaking change that can be avoided by introducing a separate interface, but I don't think that would be better.Why?
Fixes #2313
Abandoned dependencies are a concern.
How did you test it?
Unit tests
Potential risks
This is a different implementation of the JWT library, which could in theory break something.
I could have made a mistake in adjusting the code.
Is hotfix candidate?
No