jeremyevans
commented
3 years ago Thanks for the patch!
Technically, this is a breakage of backwards compatibility, since users currently expecting a 400 status for expired tokens may break. So we probably can't change the default behavior until a major version bump. I would rename jwt_access_expired_status to expired_jwt_access_token_status and change the default value to 400.
In terms of implementation:
-
jwt_payload probably shouldn't rescue JWT::DecodeError if _jwt_payload is also rescuing it in all cases. The rescue in _jwt_payload can set @jwt_payload = false.
-
We shouldn't duplicate the error handling in both jwt and jwt_refresh. Should be fine to move the error handling to a separate method, and maybe make it configurable to allow the user more control. The jwt_refresh feature can override the method, set json_response[json_response_error_key] and response.status in the expired token case ($!.instance_of?(JWT::ExpiredSignature)), then call super. The json_response[json_response_error_key] needs to use ||= instead of = in that case.
-
Please update docs/jwt_refresh.rdoc to document any new configuration methods. You can document that expired_jwt_access_token_status is only 400 for backwards compatibility, and it is recommended to set it to 401.
AlexeyMatskevich
…xpired to jwt_refresh feature, for inform the client about the need to refresh tokens
access_tokenwithrefresh_tokenin many respects ideologically repeats oauth. The Oauth documentation https://tools.ietf.org/html/rfc6750 for the Bearer case describes that a 401 error should be returned for an expired access token. I think that the use of 401 status in this case is convenient in order to distinguish an expired access token from other cases, and thus inform the user that it is time for him to refresh a couple of tokens.