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

Adds JWTConfig.ParseTokenFunc to JWT middleware to allow different libraries implementing JWT parsing. #1887

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Jun 6, 2021

Adds JWTConfig.ParseTokenFunc to JWT middleware to allow different libraries implementing JWT parsing.

ParseTokenFunc func(auth string, c echo.Context) (interface{}, error)

This tries to address problems that github.com/dgrijalva/jwt-go is unmaintained and as we use types from that library we can not change out implementation without breaking change.

Example how different version of github.com/dgrijalva/jwt-go can be used with this Echo version

signingKey := []byte("secret")

config := middleware.JWTConfig{
	ParseTokenFunc: func(auth string, c echo.Context) (interface{}, error) {
		keyFunc := func(t *jwt.Token) (interface{}, error) {
			if t.Method.Alg() != "HS256" {
				return nil, fmt.Errorf("unexpected jwt signing method=%v", t.Header["alg"])
			}
			return signingKey, nil
		}

		// claims are of type `jwt.MapClaims` when token is created with `jwt.Parse`
		token, err := jwt.Parse(auth, keyFunc)
		if err != nil {
			return nil, err
		}
		if !token.Valid {
			return nil, errors.New("invalid token")
		}
		return token, nil
	},
}

e.Use(middleware.JWTWithConfig(config))

@aldas aldas requested a review from lammel June 6, 2021 18:43
@codecov
Copy link

codecov bot commented Jun 6, 2021

Codecov Report

Merging #1887 (4184a9a) into master (2acb24a) will increase coverage by 0.40%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1887      +/-   ##
==========================================
+ Coverage   90.21%   90.61%   +0.40%     
==========================================
  Files          31       31              
  Lines        2770     2782      +12     
==========================================
+ Hits         2499     2521      +22     
+ Misses        173      168       -5     
+ Partials       98       93       -5     
Impacted Files Coverage Δ
middleware/jwt.go 90.26% <87.50%> (+10.45%) ⬆️
middleware/rate_limiter.go 100.00% <0.00%> (ø)
bind.go 89.41% <0.00%> (+0.19%) ⬆️

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 2acb24a...4184a9a. Read the comment docs.

@aldas aldas added this to the v4.4.0 milestone Jun 6, 2021
@aldas
Copy link
Contributor Author

aldas commented Jun 6, 2021

@lammel please review this PR. This will allow people to use forks of github.com/dgrijalva/jwt-go or completely different libraries for JWT

Copy link
Contributor

@pafuent pafuent left a comment

Choose a reason for hiding this comment

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

LGTM!

@lammel
Copy link
Contributor

lammel commented Jun 8, 2021

Seems like a step in the right direction.

I'd really like to remove the dependency on jwt-go in the future as still people will think that we are vulnerable to security issues raised against jwt-go. We probably should add a hint on the security issue in the docs too.

@aldas
Copy link
Contributor Author

aldas commented Jun 10, 2021

@lammel docs will come this this PR labstack/echox#213

@aldas aldas merged commit 1ac4a8f into labstack:master Jun 13, 2021
@aldas aldas mentioned this pull request Jun 13, 2021
3 tasks
lammel pushed a commit to labstack/echox that referenced this pull request Jun 13, 2021
@aldas aldas deleted the jwt_parsetokenfunc branch January 8, 2022 15:44
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.

None yet

3 participants