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

User JWT standard claims #102

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Conversation

jakubknejzlik
Copy link
Contributor

No description provided.

@pmrt
Copy link

pmrt commented Apr 7, 2019

I totally second this, I think this is a good idea since it could encourage people who are just getting into JWT to use the standard conventions.

Also we must consider that dgrijalva/jwt-go —the library used— includes built-in validation for some StandardClaims like exp so the Valid() method in JWTAccessClaims may be redundant because it's already being validated when the JWT is parsed if you use the jwt.StandardClaims.

It really depends on how the Valid() method is used by the authorization server so I can't say much more without digging deeper into the code but if the server first calls the Valid() method and then parses it with the library (or the other way around), it would be validating the token twice (again, just if you're using the jwt.StandardClaims), it's not a super expensive op but still redundant.

@LyricTian LyricTian marked this pull request as ready for review April 8, 2019 23:58
@LyricTian LyricTian merged commit 42a3322 into go-oauth2:master Apr 8, 2019
@jakubknejzlik
Copy link
Contributor Author

Oh, I didn't expect to get it merged so soon as it was only a draft PR. The tests should be updated too (I'm currently trying to setup the test pipeline locally to validate it)

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.

3 participants