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

Continuation of JWKS support from PR #71 #85

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

davidallendj
Copy link

This PR continues the work from PR #71 to add JWKS support, but removes the dynamic verifier as requested.

@davidallendj
Copy link
Author

@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")

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?

Copy link
Author

@davidallendj davidallendj Aug 19, 2024

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.

Copy link

@alexlovelltroy alexlovelltroy Aug 19, 2024

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
}

Copy link
Author

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants