jwt-go
jwt-go copied to clipboard
dgrijalva
Allow int types in MapClaims validations
This will allow the MapClaims validators to be used with Unix timestamps generated like time.Now().Unix(). Prior to this PR, only json.Number and float64 types were allowed.
This will allow the validation to pass when supplying int values as well.
The following types were added as allowed:
-
int(architecture dependent) -
int64(non-architecture dependent)
How will a MapClaims object be created that has int values? The reason we look at json.Number and float64 is that those are the two possible types returned from the json parser.
Ok, an example :)
We have an API that was constructed with jwt-go 2.7.0. To not break any APIs, we simply made the following to be able to utilize the validators in v3.0.0 (map_claims.go) in a smart manner:
example.go
// .... package + imports skipped
type ExtJWT struct {
// ...not complete claims set, there are more claims in real struct...
ExpiresAt time.Time
Extra map[string]interface{}
}
func (e *ExtJWT) ToTest() jwt.MapClaims {
test := make(map[string]interface{}, len(e.Extra))
for k, v := range e.Extra {
test[k] = v
}
test["exp"] = e.ExpiresAt.Unix() // <-- This will not work, wrong type
//test["exp"] = float64(e.ExpiresAt.Unix()) // <-- This will work, correct type
return test
}
example_test.go
// .... package + imports skipped
func TestToTest(t *testing.T) {
// This should be OK. It is.... Sort of...
err := (&ExtJWT{ExpiresAt: time.Now().Add(time.Hour)}).
ToTest().Valid()
fmt.Println(err)
if err != nil {
t.Errorf("expected nil")
}
// This should fail! But it does not! Err == nil!
err = (&ExtJWT{ExpiresAt: time.Now().Add(-2 * time.Hour)}).
ToTest().Valid()
if err == nil {
t.Errorf("expected not nil")
}
}
Since we have to do this to not break any internal (or external) structures, we will have the ability to test our implementations as well with wrong types.. Of course, after jwt.New -> jwt.Parse() the types will be correct, but for certain internal tests BEFORE we actually create a token (and regular testing), the validators will fail, since the values have not been marshalled yet. I know, this is a super rare condition and probably "the wrong way" of doing this, but we like being more safe than sorry ;)
In the spirit of being safe, would you add some unit tests to this?
It would help me understand your example test if there was a before and after of what the change unbreaks. What went from red to green?
Sorry for the delay. I've been on vacation.
@johnlockwood-wf I don't understand your question about the example test.
@dgrijalva In the example @leetal posted, I want to understand what part is broken before this code change and thus what is fixed after. Maybe if he shows the output of go test before and after.
I'm sort of on the fence about it. There's no real harm in adding extra tests, however, I expect I'll get support tickets from people who are confused by this. The number type thing is already a source of confusion for users of this library.
Oh. The issue is, the MapClaims types expects numbers to be either float64 or json.Number. If you make a token by hand (vs parsing one), you can put int numbers in there and it will not validate correctly.
I don't understand the need to have the number anything but float64 or json.Number.
Adding support of some arbitrary other number types to this lib doesn't make sense.
Actually @johnlockwood-wf. It would make sense if you used the MapClaims type internally BEFORE actually sending it to the marshaller. Since by using the MapClaims reduces the need for some internal struct i many cases. An example is OAuth2/OpenID Connect implementations where you have to do lots of work on the claims BEFORE actually creating the token. Thus, this change would benefit such cases. And in reality, a type assertion should not confuse a GO developer that much. This is such a mere change that wouldn't break a thing. It would benefit cases where you actually have to work with the claims internally (thus using timestamps as Unix-time int64´s for example). That's where i'm standing. But i think that if @dgrijalva thinks this will confuse people more, we'll just have to cast all of our internal timestamps to float64. It just feels more ugly to do that, than add an extra type assertion in jwt-go.
@leetal I have been looking at the Valid method as if it is exclusively used for validating what has been unmarshalled from JSON. But, you're using it to validate a MapClaims before it has been marshalled.
@johnlockwood-wf Yes, i must use it prior to marshalling the token since that will solve lots of issues later on, especially due to the nature of OAuth2/OpenID Connect. Actually there is no need to do extra cpu-cycles just to marshal when i actually can validate the claims prior to marshalling them into the token. Perhaps i'm misusing the library, but i thought it was far more giving (in my case at least) to validate the claims prior to marshal.
My opinion on this as a user of the library is that types and their associated functions should work the same no matter how they were created. It can be very disconcerting to manually create a MapClaims object, for a variety of purposes, only to realize that it actually only fully works if it was created by parsing a JWT.
This is a byproduct of using a map[string]interface{} as a data structure. If the structure can store anything-- it does allow storing interface{} after all-- it is pretty harsh to say to users, "Actually, I didn't mean it could be anything... I really mean it depends on what the types the Json parser generates...". 🤕 As a new user to the library, I can empathize with the intent of this PR, as I have seen the same in my experience with claims in general. Users are given a very long rope with which to hang themselves.
Barring a redesign of MapClaims, I think it would be wise to embrace the dynamic nature of the type and put up guard rails for users. Certainly testability should not be disregarded as that will only make things better for users in the long run.
A data structure should be designed to maintain it's integrity, so that all preconditions for any functions operating on this data are valid. Basically, users should not be able to create a MapClaims object that the associated functions fail to work on. At a minimum, placing some panics and explaining what went wrong would be helpful to guide users to using the library correctly.
For me, storing an int in the Claims is being retrieved as a float64
tokenString, _ := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{"id": 5}).SignedString(JWTSecret)
token, _ := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) {
return JWTSecret, nil
})
_, ok := token.Claims.(jwt.MapClaims)["id"].(int)
if !ok {
fmt.Println("not an int")
}
_, ok = token.Claims.(jwt.MapClaims)["id"].(float64)
if ok {
fmt.Println("is a float64")
}
Is that solved with this PR? Or is there a way within the master branch to store and retrieve ints in the JWT without type conversion?
@everdev this is a side effect of how the standard library JSON parser works. You can use the JSONNumber flag to use that library's number type, which is more flexible. Alternatively, you can define a struct type for your claims and follow that path.
I would also love if this could get merged. For all the reasons mentioned above. But even more. In OIDC setting, now we have to make claims be float to pass validation done by this library, but then if we use same claims to make a JSON, sometimes those floats can be encoded in their exponential form. Which does not work well with some consumers which assume those numbers will always be represented as integers. See here for similar issue. Yes, exponential form is valid JSON number but it is still better to not do it if it is not necessary. So that outputs are more compatible.
