search

golang-jwt / jwt

Go implementation of JSON Web Tokens (JWT).
https://golang-jwt.github.io/jwt/
MIT License
6.98k stars 335 forks source link

Only some registered claims can be optionally required #378

Open WalkerGriggs opened 7 months ago

WalkerGriggs commented 7 months ago

👋 I noticed that only a subset of the registered claims can be configured to be required.

PR #351, for example, adds the requireExp field to the core Validator.

Other fields like iat are hard-coded to not be required.

https://github.com/golang-jwt/jwt/blob/6bcdd9d5b6ecb03a80ac123d1a9dc363441cbffe/validator.go#L117-L119

We also have a 'expected' claims like expectedIss which are hard-coded to be required.

https://github.com/golang-jwt/jwt/blob/6bcdd9d5b6ecb03a80ac123d1a9dc363441cbffe/validator.go#L130-L134

Semantically, I find the line between "expected" and "required" to be extremely thin. I propose moving to a system like expiratedAt where no required field is hard-coded and all can be configured with ParserOptions

https://github.com/golang-jwt/jwt/blob/6bcdd9d5b6ecb03a80ac123d1a9dc363441cbffe/validator.go#L102-L107

This change would standardize on a two boolean system for each registered claims:

The existing API (from what I can see) will remain the same. This change would only add ParserOption funcs to fill in the missing gaps.

I'd be happy to work on a PR. Is this a welcome change? Does anyone have any feedback?

oxisto commented 7 months ago

That's a good point. To be honest, we didn't pay too much intention on the internal naming of "required" vs "expected". As long as the existing API will stay the same, I am open to fill the gaps with some parser options, provided this will not "explode" in the sense that there are now dozens of these functions, we somehow need to maintain and make sure that we "never" change them (at least in this major version).

For me, what would be important is that the STANDARD behaviour, i.e., without any options is exactly as specified in the RFC. Any additional expectations and requirements are up to the user, but we should not diverge from the standard.

TL;DR: Yes. Please go ahead, but be mindful in the changes introduced and expect that it will take some time until we finally decide to merge such a PR.