Add custom jwt extractor to jwt config

[RashadAnsari]

This PR help users to extract their JWT tokens in which way they want.

[codecov]

Codecov Report

Merging #1967 (b6bb420) into master (7d41537) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1967      +/-   ##
==========================================
+ Coverage   91.03%   91.33%   +0.30%     
==========================================
  Files          32       33       +1     
  Lines        2800     2875      +75     
==========================================
+ Hits         2549     2626      +77     
+ Misses        160      159       -1     
+ Partials       91       90       -1     

Impacted Files	Coverage Δ
middleware/jwt.go	90.26% <100.00%> (ø)
middleware/compress.go	84.00% <0.00%> (-0.32%)	⬇️
echo.go	94.15% <0.00%> (ø)
middleware/timeout.go	100.00% <0.00%> (ø)
middleware/rate_limiter.go	100.00% <0.00%> (ø)
middleware/request_logger.go	97.59% <0.00%> (ø)
router.go	95.76% <0.00%> (+0.07%)	⬆️
middleware/request_id.go	85.71% <0.00%> (+1.50%)	⬆️
middleware/decompress.go	96.29% <0.00%> (+7.92%)	⬆️

---

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 d793521...b6bb420. Read the comment docs.

[RashadAnsari]

@aldas Could you please review these changes?

[RashadAnsari]

Does no one want to review this PR?

[aldas]

I have seen this but I am not very keen to accept it. I do not know the use-case. Currently JWT middleware allows to extract query, param, cookie, form, header values. I do not understand is there case these 5 variants does no cover?

Note: absolute minimal custom JWT middleware would be and extractor part can be whatever needed

e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
	return func(c echo.Context) error {
		authHeader := c.Request().Header.Get(echo.HeaderAuthorization)
		rawToken := strings.TrimPrefix(authHeader, "Bearer ")
		if len(authHeader) == len(rawToken) {
			return middleware.ErrJWTMissing
		}
		jwtToken, err := jwt.Parse(rawToken, func(t *jwt.Token) (interface{}, error) {
			if t.Method.Alg() != "HS256" {
				return nil, fmt.Errorf("unexpected jwt signing method=%v", t.Header["alg"])
			}
			return "MySuperSecretSigningKey", nil
		})
		if err != nil {
			return err
		}
		if !jwtToken.Valid {
			return errors.New("invalid token")
		}
		c.Set("user", jwtToken)
		return next(c)
	}
})

[RashadAnsari]

I have seen this but I am not very keen to accept it. I do not know the use-case. Currently JWT middleware allows to extract query, param, cookie, form, header values. I do not understand is there case these 5 variants does no cover?

Note: absolute minimal custom JWT middleware would be and extractor part can be whatever needed

e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
	return func(c echo.Context) error {
		authHeader := c.Request().Header.Get(echo.HeaderAuthorization)
		rawToken := strings.TrimPrefix(authHeader, "Bearer ")
		if len(authHeader) == len(rawToken) {
			return middleware.ErrJWTMissing
		}
		jwtToken, err := jwt.Parse(rawToken, func(t *jwt.Token) (interface{}, error) {
			if t.Method.Alg() != "HS256" {
				return nil, fmt.Errorf("unexpected jwt signing method=%v", t.Header["alg"])
			}
			return "MySuperSecretSigningKey", nil
		})
		if err != nil {
			return err
		}
		if !jwtToken.Valid {
			return errors.New("invalid token")
		}
		c.Set("user", jwtToken)
		return next(c)
	}
})

If you see the test case I wrote, you will understand why I made this change. The standard header extractor does not handle this case because the auth scheme always is Bearer but, if you look at the tests with a custom extractor, you can implement whatever you want.
Actually, the default values that you assumed for some variables like auth scheme will make a problem for me.

[aldas]

Still. This seems to be shortcoming of JWTConfig.TokenLookup and JWTConfig.AuthScheme as usecase where token needs to be extracted from header does not work properly.

Ala it should be possible to:

	e.Use(JWTWithConfig(JWTConfig{
		TokenLookup: "header:X-API-Key",
		AuthScheme:  "", // maybe force empty somehow
		SigningKey:  []byte("secret"),
	}))

[RashadAnsari]

Still. This seems to be shortcoming of JWTConfig.TokenLookup and JWTConfig.AuthScheme as usecase where token needs to be extracted from header does not work properly.

Ala it should be possible to:

	e.Use(JWTWithConfig(JWTConfig{
		TokenLookup: "header:X-API-Key",
		AuthScheme:  "", // maybe force empty somehow
		SigningKey:  []byte("secret"),
	}))

This doesn't work because for initializing the middleware, we have this condition:

if config.AuthScheme == "" {
    config.AuthScheme = DefaultJWTConfig.AuthScheme
}

[aldas]

I think we can modify "header" to support custom authscheme for each header with these changes

echo/middleware/jwt.go

Line 195 in 6b5e62b

case "header":

		case "header":
			prefix := config.AuthScheme
			if len(parts) > 2 {
				prefix = parts[2]
			}
			extractors = append(extractors, jwtFromHeader(parts[1], prefix))

echo/middleware/jwt.go

Line 297 in 6b5e62b

l := len(authScheme)

		l := len(authScheme)
		if l == 0 {
			return auth, nil
		}
		if len(auth) > l+1 && strings.EqualFold(auth[:l], authScheme) {

With this we can have

	e.Use(JWTWithConfig(JWTConfig{
		TokenLookup: "header:X-API-Key:", // auth scheme will be empty
		SigningKey:  []byte("secret"),
	}))

	e.Use(JWTWithConfig(JWTConfig{
		TokenLookup: "header:X-API-Key:MyAuth", // auth cheme will be "MyAuth"
		SigningKey:  []byte("secret"),
	}))

	e.Use(JWTWithConfig(JWTConfig{
		TokenLookup: "header:X-API-Key", // auth scheme will be "Bearer"
		SigningKey:  []byte("secret"),
	}))

Note: this would align with v5 changes - as in v5 JWTConfig.AuthScheme is dropped

[RashadAnsari]

Actually, for me is better to make sense to add the ability to extract the token by a custom extractor. In this case, anyone can quickly write his own extractor if he needs and supports almost all varieties of custom use cases.
This is just an idea but, I think your proposed solution makes it a little complex and unreadable.

[aldas]

Fair enough, but change it so that:

1. nameJWTExtractors to TokenLookupFuncs - lets call things with same names. We already have TokenLookup and other stuff has "*Func" prefixes.
2. TokenLookup default value and TokenLookupFuncs are mutually exclusive - when you provide your own extractor funcs TokenLookup will not default to "header:Authorization". Atm extractors take the priority but that "Authorization: bearer xxxx" is also there at the end, added by the defaults.
3. And add note to fields that you can mix TokenLookup (when you explicitly provide something) with funcs and funcs are executed first

[RashadAnsari]

Great, I will make the changes.

[RashadAnsari]

@aldas, Are you ok with the changes that I made?

[aldas]

LGTM

[RashadAnsari]

@aldas, Thank you for your approval.
Do you know when this PR will get merged?

[aldas]

done, there will be patch release probably in couple of days.

[RashadAnsari]

Great, thank you so much.

[aldas]

Hi, if you only want to handle missing token case you do not need custom extractor. You can intercept missing token in error handler ``` e.Use(middleware.JWTWithConfig(middleware.JWTConfig{ TokenLookup: "header:Authorization", AuthScheme: "bearer", ErrorHandler: func(err error) error { if err == middleware.ErrJWTMissing { return &echo.HTTPError{ Code: http.StatusUnauthorized, Message: "missing token", } } return err }, SigningKey: []byte("secret"), })) ```

…

On Wed, Feb 2, 2022 at 4:51 PM xanderflood ***@***.***> wrote: Looks like this wasn't included in that patch release (which makes sense, since it's a new feature). Any idea when the next minor version might be cut? My team could use this change because we need to override the error handling of the JWT extractor, because we have automated clients outside of our control who expect a 401 rather than a 400. Right now we're achieving this by modifying the global variable: middleware.ErrJWTMissing.Code = http.StatusUnauthorized but this is not ideal. I don't think the ErrorHandler configuration covers the case of missing JWTs. — Reply to this email directly, view it on GitHub <#1967 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARWPLLS4HMF2B43KNQ6E23UZFAHNANCNFSM5COFSLCA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[xanderflood]

Yes! Right after I posted I looked at the source code and realized that was an option, so I deleted my question. Thank you!