Cannot refresh expired token, even if within MaxRefresh time

[oadk]

Hi,

I think that commit 59e38b5 (from PR #165) has introduced a change in behaviour.

Previously, the RefreshHandler would refresh an expired token, as long as it was within the MaxRefresh time.

Now, trying to refresh an expired token will always fail. This is because CheckIfTokenExpire() calls mw.ParseToken(), which calls jwt.Parse(). That library function will return an error if the token has expired (regardless of the MaxRefresh time).

Before the commit, the code ignored all errors from mw.ParseToken(): token, _ := mw.ParseToken(c). Now, it returns the error no matter what it was. I think that CheckIfTokenExpire() needs to explicitly check if the error returned by mw.ParseToken() if that the token has expired, so we can then go on to check the MaxRefresh time too.

Thanks

[oadk]

Hi @appleboy, do you have any thoughts on this?

@kingcw was this change of behaviour intentional?

[appleboy]

@oadk I will write the changelog. maybe it is a bug about MaxRefreshTime.

[kingcw]

for my implementation, this commit is intentional, because my app is SPA, and i call refresh handler on application load only, so if the user doesn't have a token, without this error check, backend will panic.

However, the code should work if you have a token and within MaxRefreshTime, if you look at CheckIfTokenExpire function:

```
    claims := token.Claims.(jwt.MapClaims)
origIat := int64(claims["orig_iat"].(float64))
if origIat < mw.TimeFunc().Add(-mw.MaxRefresh).Unix() {
	return nil, ErrExpiredToken
}
```

the logic is correct, and i tested it before

mw.ParseToken function only extract token from cookie or whatever, it doesn't do any MaxRefreshTime check, so I think it might be something wrong with your implementation or new commits

[kingcw]

jwt.Parse() is the function from library github.com/dgrijalva/jwt-go, i just looked at their code, it's not expiring token, so I think it should be something wrong with your implementation.

Here is the thing, if you use go 1.11, go.mod, use this format instead #171 , otherwise, it will download very old release

[oadk]

Sorry, but I still think commit 59e38b5 has introduced a bug which means that expired tokens cannot be refreshed, even if they are within the MaxRefreshTime.

the code should work if you have a token and within MaxRefreshTime, if you look at CheckIfTokenExpire function:

    claims := token.Claims.(jwt.MapClaims)
origIat := int64(claims["orig_iat"].(float64))
if origIat < mw.TimeFunc().Add(-mw.MaxRefresh).Unix() {
	return nil, ErrExpiredToken
}

That code in CheckIfTokenExpire checking the MaxRefresh never gets executed if the token has expired. This is because the call to mw.ParseToken on the first line of CheckIfTokenExpire always returns an error if the token has expired. The code currently looks like this:

	token, err := mw.ParseToken(c)
	if err != nil {
		return nil, err
	}

but I think it needs to look like this:

	token, err := mw.ParseToken(c)
	if err != nil {
		validationErr, ok := err.(*jwt.ValidationError)
		if !ok || validationErr.Errors != jwt.ValidationErrorExpired {
			return nil, err
		}
	}

mw.ParseToken function only extract token from cookie or whatever, it doesn't do any MaxRefreshTime check

Correct, mw.ParseToken doesn't do any MaxRefreshTime check. But it does check if the token has expired. Therefore, if the token has expired, but is within MaxRefreshTime, mw.ParseToken will still return an error.

[kingcw]

I see now, jwt-go library does a hard expire token check if token expire field is named "exp" in claims.go, the proposed solution would be to add a bool variable for Parser.SkipClaimsValidation, and in mw.ParseToken function, instead of calling jwt.Parse, create a new Parser instance, pass in that bool variable, if false, you are doing a hard token expire check which depends on jwt-go library, if true, you are doing a mw MaxRefreshTime check

[kingcw]

but..., this is not really a high priority to me cause, best practice is that if a token expired, let it expire and issue a new token, my timeout and maxrefresh value are the same, so it will only refresh if token not expired. i mean, if you use maxrefresh as your token lifespan, what's the point to have a timeout

[oadk]

So, I think we are in agreement that commit 59e38b5 (from PR #165) causes the change of behaviour that I describe? (specifically: you can no longer refresh expired tokens which are within the MaxRefresh time).

Even if this change does not affect your personal use-case, it appears to be an accidental side effect of your other changes, and it affects all users of this library. It has caused us significant problems in production (our clients can no longer refresh expired tokens which are within the MaxRefreshTime).

the proposed solution would be to add a bool variable for Parser.SkipClaimsValidation, and in mw.ParseToken function, instead of calling jwt.Parse, create a new Parser instance, pass in that bool variable, if false, you are doing a hard token expire check which depends on jwt-go library, if true, you are doing a mw MaxRefreshTime check

This solution is undesirable, because it will cause all claims validation to be skipped.

The change that I propose in #176 (comment) should be sufficient to revert the behaviour change, as it is checking if ParseToken failed specifically due to jwt.ValidationErrorExpired.

if a token expired, let it expire and issue a new token

In order to issue a new token, the user will have to be authenticated. How do you propose they authenticate themselves?

[kingcw]

if you are using refreshhandler function of this library, then i assume you also use TokenGenerator function of this library, which, only uses "exp" field of jwt-go validation fields, and jwt-go will only do validation on that field

if a token expires, let user re-authenticate. The notion of refreshing token is actually to have two tokens, one access token, and one refresh token

And by that, i would take out MaxRefreshTime of this library, cause it makes TimeOut useless on single access token

I didn't know that jwt-go does claims validation based on specific name fields before this day, which, to me, is doing more than what it should do. So CheckIfTokenExpire actually does two checks, which is redundant to me.

Well, i could change it, but it's whether to do it correctly or make it work, and I'm more toward to do it correctly, and for that, i don't think i'm the guy who should be changing it, cause i'm also a user just like you, i'm not sure what original developer had in mind. If he proves, then i'll make the change.

The change you proposed makes my use case, MaxRefresh=Timeout, doing two checks on the same value

[kingcw]

How about this, if you could convince me that your use case MaxRefresh>Timeout, is not the same as MaxRefresh=Timeout, then i'll make the change, or you could make the change yourself and submit pr. One more check means nothing to my use case given modern server, and i wouldn't want to break other users production use

[oadk]

I think we are mixing up two different questions here:

1. What is the ideal way for the token refresh behaviour to work.

2. Should the token refresh behaviour be changed in a way that breaks production code without giving library users any prior notice.

My bug report is solely concerned with question 2. If you want to change some behaviour, at least deprecate the old behaviour and give people time to update their code (note that updating client code is not something that can happen overnight if you don't have direct control over the clients).

Making breaking API changes without warning (or being slow to revert them if they happened accidentally) is not ideal.

@appleboy what is your view?

[fatihkahveci]

I have exact same issue too. I can't refresh my token even if max_refresh time didn't expire. Here is my config:

	Realm:       "test zone",
	Key:         []byte("secret"),
	Timeout:     time.Minute,
	MaxRefresh:  time.Hour,
	IdentityKey: identityKey,

@appleboy @kingcw

[gnuletik]

Same issue for me.
@appleboy can you validate @fatihkahveci's PR ? Or choose something to do ?
Thanks :)

[oadk]

Hi @appleboy , please can you validate @fatihkahveci's PR?

Thanks

[appleboy]

@oadk I will take it asap.

[appleboy]

@oadk @gnuletik @fatihkahveci @kingcw Done. Please help to check the latest code.

[oadk]

Hi,

Thank you for looking at the issue and merging a change. After thinking about the code more, I think the change may have a problem.

The start of CheckIfTokenExpire() now looks like this:

// CheckIfTokenExpire check if token expire
func (mw *GinJWTMiddleware) CheckIfTokenExpire(c *gin.Context) (jwt.MapClaims, error) {
	token, err := mw.ParseToken(c)
	// issue: Cannot refresh expired token, even if within MaxRefresh time
	// see https://github.com/appleboy/gin-jwt/issues/176
	if token == nil {
		return nil, err
	}

	...

The problem is that ParseToken might fail for different reasons than an expired token (e.g. the token might be invalid for other reasons). I think that we want to check if ParseToken returned an error, and only continue if we got a single error of jwt.ValidationErrorExpired. I think the start of CheckIfTokenExpire() needs to look like this:

// CheckIfTokenExpire check if token expire
func (mw *GinJWTMiddleware) CheckIfTokenExpire(c *gin.Context) (jwt.MapClaims, error) {
	token, err := mw.ParseToken(c)
	if err != nil {
		// If we receive an error, and the error is anything other than a single
		// ValidationErrorExpired, we want to return the error
		validationErr, ok := err.(*jwt.ValidationError)
		if !ok || validationErr.Errors != jwt.ValidationErrorExpired {
			return nil, err
		}
	}

	...

This means that we will still catch all other token validation errors, but will only continue to check the max refresh time if there are zero errors, or if there is one error of jwt.ValidationErrorExpired.

Please let me know if you would like me to submit a PR for the above.

[appleboy]

@oadk Sent the PR. Thanks.

[oadk]

Thanks for merging the PR :)