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

Allow JWT middleware to gracefully fail #2048

Closed
wants to merge 1 commit into from

Conversation

sborsje
Copy link

@sborsje sborsje commented Dec 17, 2021

This PR adds a CredentialsOptional configuration option to the JWT middleware, allowing it to gracefully fail. It allows the next handler to be called, even when there is no valid JWT token present.

This was brought up before in #1039, but I think it might have gotten lost in the PR that was ultimately merged. This PR reintroduces the flag and adds some tests to validate the behavior.

@aldas
Copy link
Contributor

aldas commented Dec 17, 2021

This is good change.

In my own applications I have wrapped JWT into middlewarefunct to call next if errors is returned, but direct support for that in official middleware is better.

Example:

// NewJWTExtraction extracts JWToken for request and set it into context (if request had it)
// will return error when token existed and we had problems with it (ala token was expired)
// will not error when token did not exist - so our "not secured" routes will work
// real resource authorization is done in PrivilegeCheck middleware
func NewJWTExtraction(config MiddlewareConfig) echo.MiddlewareFunc {
	jwtConf := middleware.JWTWithConfig(middleware.JWTConfig{
		Claims:      &JwtClaims{},
		Skipper:     APISkipper,
		SigningKey:  []byte(config.JwtSigningKey),
		TokenLookup: "header:" + echo.HeaderAuthorization,
		ContextKey:  tokenContextKey,
	})
	return func(next echo.HandlerFunc) echo.HandlerFunc {
		// dummy function for jwt middleware success path to execute
		jwtFunc := jwtConf(func(context echo.Context) error {
			return nil
		})
		return func(c echo.Context) error {
			err := jwtFunc(c)
			if err != nil && err != middleware.ErrJWTMissing {
				return err
			}
			return next(c)
		}
	}
}

in v5 we have this "feature" supported and implemented as such

// Allow error handler to swallow the error and continue handler chain execution
// Useful in cases when portion of your site/api is publicly accessible and has extra features for authorized users
// In that case you can use ErrorHandler to set default public token to request and continue with handler chain
if handledErr := config.ErrorHandler(c, err); handledErr != nil {
	return handledErr
}
return next(c)

so maybe we could align v4 with how this would work in v5

if config.ErrorHandlerWithContext != nil {
	if err := config.ErrorHandlerWithContext(err, c); err != nil {
		return err
	} else {
		return next(c)
	}
}

note: in v5 the JWTConfig.JWTErrorHandler func(error) error field is removed and only ErrorHandlerWithContext remains.

I propose instead of adding CredentialsOptional field to modify ErrorHandlerWithContext block to resume middleware chain when error handler swallows the error and returns nil

@codecov
Copy link

codecov bot commented Dec 18, 2021

Codecov Report

Merging #2048 (b7dd125) into master (6b5e62b) will decrease coverage by 0.05%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2048      +/-   ##
==========================================
- Coverage   91.33%   91.28%   -0.06%     
==========================================
  Files          33       33              
  Lines        2875     2879       +4     
==========================================
+ Hits         2626     2628       +2     
- Misses        159      160       +1     
- Partials       90       91       +1     
Impacted Files Coverage Δ
middleware/jwt.go 88.88% <50.00%> (-1.38%) ⬇️

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 6b5e62b...b7dd125. Read the comment docs.

@aldas
Copy link
Contributor

aldas commented Mar 12, 2022

Closing this feature is supported by using ContinueOnIgnoredError configuration field

echo/middleware/jwt.go

Lines 35 to 40 in 5ebed44

// ContinueOnIgnoredError allows the next middleware/handler to be called when ErrorHandlerWithContext decides to
// ignore the error (by returning `nil`).
// This is useful when parts of your site/api allow public access and some authorized routes provide extra functionality.
// In that case you can use ErrorHandlerWithContext to set a default public JWT token value in the request context
// and continue. Some logic down the remaining execution chain needs to check that (public) token value then.
ContinueOnIgnoredError bool

How it works:

echo/middleware/jwt.go

Lines 243 to 249 in 5ebed44

if config.ErrorHandlerWithContext != nil {
tmpErr := config.ErrorHandlerWithContext(err, c)
if config.ContinueOnIgnoredError && tmpErr == nil {
return next(c)
}
return tmpErr
}

@aldas aldas closed this Mar 12, 2022
@sborsje
Copy link
Author

sborsje commented Mar 13, 2022

This is great, and works like a charm. Thank you, @aldas 🙌

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.

2 participants