-
Notifications
You must be signed in to change notification settings - Fork 321
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only some registered claims can be optionally required #378
Comments
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. |
馃憢 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 coreValidator
.Other fields like
iat
are hard-coded to not be required.jwt/validator.go
Lines 117 to 119 in 6bcdd9d
We also have a 'expected' claims like
expectedIss
which are hard-coded to be required.jwt/validator.go
Lines 130 to 134 in 6bcdd9d
Semantically, I find the line between "expected" and "required" to be extremely thin. I propose moving to a system like
expiratedAt
where norequired
field is hard-coded and all can be configured withParserOptions
jwt/validator.go
Lines 102 to 107 in 6bcdd9d
This change would standardize on a two boolean system for each registered claims:
WithFoo
which determines which claims should be verified if provided. For example, seeWithIssuedAt
andWithIssuedAt
WithFooRequired
which determines which claims should be required. For example, seeWithExpirationRequired
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?
The text was updated successfully, but these errors were encountered: