First attempt at making parsing more flexible in types

[crxpandion]

Makes the claims an interface.

#64

[dgrijalva]

Is this the right behavior? These fields are not required. Missing != invalid

[crxpandion]

Good point, but the way that the validation is done currently error here means "do not check the expiration". Maybe if it returned (int64, bool) it would be more clear.

[dgrijalva]

If exp exists and is not a number, it shouldn't be skipped. Validation should fail with "invalid value for exp". Is return 0, nil a better option? 0 isn't a valid value for either exp or nbf.

[dgrijalva]

I think my other comment got swallowed up. This doesn't allow us to abort on invalid data. If exp == "pants", we probably want to shut it down, vs ignoring it. Tricky.

[crxpandion]

Ah I understand your previous concern better now. hmm tricky indeed. This is related to your comment below re validation. I'll respond there.

[dgrijalva]

I think this is the best solution I've seen for this issue. I have a couple concerns:

• While this change appears to be backwards compatible, changes to the Claims interface would not be. What's the likelihood it will change frequently? When adding more claims for example.
• This puts more of the parsing and validation burden on the developer. It opens up opportunities for mistakes that cause vulnerabilities/unexpected behavior. See the difficulty coming up with an ideal method signature as an example.

Neither of these are insurmountable, but they deserve consideration.

[dgrijalva]

Actually, this is not backward compatible as the type of Claims has changed. One other side effect of this is existing users will have to type check Claims everywhere they use it.

[crxpandion]

One way to address the Claims interface would be to make it a generic Valid() bool. While this would push more of the validation burden onto the developer, this is already the case, as the developer is already forced to make validation considerations in the keyfunc.

I think there are two levels of validation going on here: Token level validation (e.g. hash alg checks, kid) and claims level validation (e.g. exp and nbf). Maybe there is a solution that deals with that difference better.

I don't really have a good suggestion for maintaining backwards compatibility. This seems to be a active question within the go community right now. I think that the validation questions raised here will fundamentally break the api no matter the solution. :(

[itsjamie]

I agree, you're definitely going to be breaking backwards compatibility with this change. However, that's what major version bumps are for!

I ran with @crxpandion suggestion of using a more generic interface, if you look at the PR #73 it isn't as complete, in that the tests haven't been updated, but it shows the other method of accomplishing this. Perhaps in a way that will be easier to be backwards compatible in the future.

[dgrijalva]

Getting close to landing #73, which is based on this.