-
-
Notifications
You must be signed in to change notification settings - Fork 579
Conversation
Following. |
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.
Overall this looks great! My only real comments are we should accept an options struct that allows for setting the secret and the signing method, since not everyone uses HMAC. the default for those should be an env, like you have, and HMAC.
Other than that, this looks great!
middleware/tokenauth/tokenauth.go
Outdated
ErrBadSigningMethod = errors.New("unexpected signing method") | ||
) | ||
// Middleware enables jwt token verification | ||
func Middleware(next buffalo.Handler) buffalo.Handler { |
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.
Take an options struct that allows for setting the secret key and the signing method.
middleware/tokenauth/tokenauth.go
Outdated
func Middleware(next buffalo.Handler) buffalo.Handler { | ||
return func(c buffalo.Context) error { | ||
authString := c.Request().Header.Get("Authorization") | ||
SecretKey := envy.Get("JWT_SECRET", "secret") |
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.
lower case secretKey
I really, really want this but I'm late to the party (obviously). I'm currently writing my own middleware to do JWT auth. Please let me know if I can help contribute here to get this across the line |
@Shivakishore14 do you think you can make the requested changes in the next few days? If not, perhaps @arschles could make them. I'd love to get this in the next release. |
I can take a crack over the weekend or Monday. Let me know :) |
@arschles thanks for the offer, @markbates i am almost done with the code. I will be finishing up by tomorrow. |
Excellent! Lots of people are excited about this one. |
Do |
@Shivakishore14 also update docs 😉 |
@Shivakishore14 for sure. I'll look at this over the weekend. If there's more to do I'll do it in a follow-up. I don't want to hold up this PR |
@markbates can you review the code. |
middleware/tokenauth/tokenauth.go
Outdated
// helper function to get key for validation | ||
// checks the algorithm used for signing and returns key according to the algorithm used. | ||
// eg HMAC used a key string and RSA uses public key to validate | ||
func (options Options) getKey() (interface{}, error) { |
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.
This all seems a bit much, and doesn't allow for users to add their own way of getting a key, such as from a database, or other store.
What do you think of doing something like this:
type Options struct {
SignMethod jwt.SigningMethod
GetKey func(jwt.SigningMethod) (interface{}, error)
}
func GetHMACKey(jwt.SigningMethod) (interface{}, error) {
key, err := envy.MustGet("JWT_SECRET")
return []byte(key), err
}
func Middleware(options Options) buffalo.MiddlewareFunc {
// set sign method to HMAC if not provided
if options.SignMethod == nil {
options.SignMethod = jwt.SigningMethodHS256
}
if options.GetKey == nil {
options.GetKey = GetHMACKey
}
// ....
}
Then we can add a few "default" implementation functions, like the GetHMACKey example. Then people can easily plugin one of the default methods, and if none of them work for their situation, it's easy enough for the to add their own implementations.
Thoughts?
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.
Wow that is neat, i will change the implementation.
instead of setting default GetKey
to GetHMACKey
, can we do something like.
if options.GetKey == nil {
options.GetKey = selectGetKeyFunc(options.SignMethod)
}
so we get the GetKey
method that is appropriate to the sign method.
and GetKey
never uses the signing method so, it can be GetKey func() (interface{}, error)
@@ -0,0 +1,5 @@ | |||
-----BEGIN EC PRIVATE KEY----- |
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 assuming all the files in the data
folder are used for testing, correct? If so, perhaps we should rename that folder to something like test_certs
or something so that people don't get confused and want to use them in production, which would be bad.
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.
sure will do.
@markbates I have added the requested changes. |
middleware/tokenauth/tokenauth.go
Outdated
// Options for the JWT middleware | ||
type Options struct { | ||
SignMethod jwt.SigningMethod | ||
GetKey func()(interface{}, error) |
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.
This needs to take jwt.SigningMethod
for those who want to build their own function, they might need that information.
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.
@markbates added this.
@Shivakishore14 do |
458570c
to
9403001
Compare
9403001
to
0f77060
Compare
Great work on this! Thank you so much! |
@Shivakishore14 You really need to update the docs ;) |
@robbyoconnor Where do you want the docs to be on gobuffalo.io or the source code ? |
Added Basic JWT authentication middleware, resolves #399