go-chi / jwtauth

JWT authentication middleware for Go HTTP services
MIT License
550 stars 91 forks source link

Implement custom search functions #20

Closed lukasmalkmus closed 7 years ago

lukasmalkmus commented 7 years ago

I think there are many ways to implement the configuration proposal I made in #19. I made up this draft for the following reason:

The jwtauth package has two main components used by package users: The Verifier and the Authenticator functions. Implementing a custom Authenticator is easy because the provided one is ~10 lines of code. But implementing a custom Verifier is a little bit trickier because this is where the token gets extracted from the request, validated, etc.

So instead of adding more function parameters or a custom options struct, I came up with this PR which adds so called Extractors. What sounds complicated is pretty easy: Instead of hard coding where the Verifier searches for the token string in the request, we provide it with ExtractorFuncs that do this task for the Verifier. Furthermore, the signature of the original Verifier function has not been changed, so there won't be any breaking changes for users relaying on the default behaviour of this package.

What got removed are the params. But I don't think that's critical. They can be replaced by implementing a custom ExtractorFunc.

Furthermore there was no need to adjust the tests.

So what got added in this PR?

I think it's best if @pkieltyka or another maintainer takes a look at the code and provides me with some feedback.

lukasmalkmus commented 7 years ago

I think it is also worth mentioning that this PR preserves the token search order. If a users implements the default Verifier middleware, the request is searched for the token in the URL first, then in the "Authorization" header and finally in the "jwt" cookie. So no breaking changes for package users.

But this PR also fixes #5: Users can implement the Verify function and pass extractors in the desired order:

jwtauth.Verify(auth, jwtauth.ExtractCookie, jwtauth.ExtractHeader, jwtauth.ExtractQuery)
pkieltyka commented 7 years ago

well done. I like this a lot - only feedback is can we think of a better verb than "Extractor" / "Extract" ? cc: @VojtechVitek

I'll think as well

lukasmalkmus commented 7 years ago

Right, Extractor and ExtractorFunc don't fit very good. Because the function signature of the ExtractorFunc type is so short, we could just omit it. func(*http.Request) string isn't that complicated. So we wouldn't introduce a new type.

Maybe a good name would be From... for the "search" functions? jwtauth.Verify(auth, jwtauth.FromCookie, jwtauth.FromHeader, jwtauth.FromQuery)

pkieltyka commented 7 years ago

thanks @VojtechVitek for the feedback. I think FromCookie isn't clear, it should be clear we're looking for a token from a http.Request, within the cookie. FindCookieToken etc. is more clear

lukasmalkmus commented 7 years ago

I implemented the discussed changes. Is the naming ok? I decided to go with TokenFrom.... Feedback on this pattern? It only clashes with the TokenCtxKey but that should be ok.

pkieltyka commented 7 years ago

hey @lukasmalkmus my apologies for the delay - I was away on vacation for a few weeks. I've reviewed it again and looks good, going to merge! thank you again for this awesome contribution.

lukasmalkmus commented 7 years ago

Nothing to apologise for :) Thanks for merging. I just pulled in the latest release and as expected, nothing to change in my code, since the package API did not change 🎉

Also closed #19