-
Notifications
You must be signed in to change notification settings - Fork 92
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
Continuation of JWKS support from PR #71 #85
Conversation
@pkieltyka Can I get a review for this? |
@@ -120,6 +137,10 @@ func VerifyToken(ja *JWTAuth, tokenString string) (jwt.Token, error) { | |||
} | |||
|
|||
func (ja *JWTAuth) Encode(claims map[string]interface{}) (t jwt.Token, tokenString string, err error) { | |||
if ja.keySet != nil { | |||
return nil, "", fmt.Errorf("encode not supported") |
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'm working on rebasing and adding more documentation. I don't understand why encode isn't supported in this case and I think we need a better error message for it. Can you explain what this is for?
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 think that's the wrong error message probably from some copy-pasta. I think the message should just reflect that the JWKS isn't set so nothing can be encoded here.
Edit: This may have been something added by the original author, so I'm not 100% sure of the intent here, so I'm speculating.
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.
Based on my understanding, I'm considering updating Encode like this:
// Encode generates a JWT token string with the provided claims.
// It returns the encoded token as a string, along with the token object and any error encountered.
// If the JWTAuth instance has a key set, encoding is not supported and an error is returned.
func (ja *JWTAuth) Encode(claims map[string]interface{}) (t jwt.Token, tokenString string, err error) {
if ja.keySet != nil {
return nil, "", fmt.Errorf("encoding is not supported with key set")
}
t = jwt.New()
for k, v := range claims {
if err := t.Set(k, v); err != nil {
return nil, "", err
}
}
payload, err := ja.sign(t)
if err != nil {
return nil, "", err
}
tokenString = string(payload)
return
}
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.
Looks fine to me. I was reading the code wrong and was thinking the ja.keySet
was an error. I'll have to go back and look at the entire PR to recall the context why this was done.
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'm refactoring it a bit now that I understand it better.
This PR continues the work from PR #71 to add JWKS support, but removes the dynamic verifier as requested.