Skip to content

Conversation

@grounded042
Copy link
Contributor

@grounded042 grounded042 commented May 6, 2021

This started off as bringing the middleware tests up to date with the validate token pieces, but I ended up re-working some of the pieces. I've left comments throughout the code to explain the changes. The two major changes are:

  • the error handler now sits in the http handler functions and not in CheckJWT
  • we've dropped debug logging as the consumer can do that

We've left out a lot of documentation changes as that will be added in the v2 branch.

type JWTMiddleware struct {
Options Options
// Option is how options for the middleware are setup.
type Option func(*JWTMiddleware)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to go with the options pattern as that is what we are doing in the josev2 validation piece.

// New constructs a new Secure instance with supplied options.
func New(opts ...Option) *JWTMiddleware {
m := &JWTMiddleware{
validateToken: func(string) (interface{}, error) { panic("not implemented") },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before v2 release we will either default to the josev2 for this, or require a specific validate token func to be passed into New.

)

// TODO: replace this with default validate token func once it is merged in
func REPLACE_ValidateToken(token string) (interface{}, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be replaced after the merge into v2

Comment on lines +147 to +150
// DefaultErrorHandler is the default error handler implementation for the
// middleware. If an error handler is not provided via the WithErrorHandler
// option this will be used.
func DefaultErrorHandler(w http.ResponseWriter, r *http.Request, err error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We made this public in case consumers want to wrap this default handler.

// If debugging is turned on, log the outcome
token, err := m.tokenExtractor(r)
if err != nil {
m.logf("Error extracting JWT: %v", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that we have completely dropped debug functionality for several reasons:

  • in some places we were logging the full string of the JWT 😱 and that is unacceptable.
  • as a library it's more idiomatic to pass errors up and let those be logged by the package consumer instead of logging them in the package

@grounded042 grounded042 marked this pull request as ready for review May 7, 2021 20:21
@grounded042 grounded042 requested a review from a team as a code owner May 7, 2021 20:21
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2021

Codecov Report

❗ No coverage uploaded for pull request base (v2@2329d11). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##             v2      #90   +/-   ##
=====================================
  Coverage      ?   95.08%           
=====================================
  Files         ?        1           
  Lines         ?       61           
  Branches      ?        0           
=====================================
  Hits          ?       58           
  Misses        ?        2           
  Partials      ?        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2329d11...27418c2. Read the comment docs.

@auth0 auth0 deleted a comment from jfatta May 14, 2021
Copy link

@cyx cyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Examples for chi, and other famous frameworks would be a bonus, but otherwise :shipit:

@grounded042 grounded042 merged commit 0d1f50b into v2 May 14, 2021
@grounded042 grounded042 deleted the jon/main-tests branch May 14, 2021 20:52
sergiught pushed a commit that referenced this pull request Nov 1, 2021
d10i pushed a commit to Hikely/go-jwt-middleware that referenced this pull request Nov 2, 2021
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.

4 participants