Keats / jsonwebtoken

JWT lib in rust
MIT License
1.62k stars 252 forks source link

Add optional support for 3rd party time libraries like `time` and `chrono` #311

Closed nickguletskii closed 1 year ago

nickguletskii commented 1 year ago

Should resolve timezone-related issues mentioned in #300 .

This PR introduces 4 new cargo features:

Therefore, the changes should, at least in theory, be backwards compatible, and fixing the time zone issue should be as simple as adding default_time or default_chrono to the list of cargo features.

However, this PR does make the custom_time example obsolete and it will have to be rewritten at some point. It still works fine from what I can tell, it's just not necessary because using a different type for timestamps is now as easy as copying and adapting the implementations in jsonwebtoken::time.

Keats commented 1 year ago

Thanks but I'm not sure it helps?

I'm pretty sure time and chrono will just call out SystemTime anyway for UTC (eg https://docs.rs/chrono/latest/src/chrono/offset/utc.rs.html#67-73 and https://github.com/time-rs/time/blob/main/time/src/date_time.rs#L323) so that doesn't change the issue?

I'm in UTC+2 and I do get the same timestamp with time and SystemTime as expected. AFAIK the only way to solve that get the timezone offset of the server as part of the validation.

You can have only the time optional dep (no need for chrono) and in get_current_timestamp in a cfg if time dep enabled grab the current local offset (https://time-rs.github.io/api/time/struct.UtcOffset.html#method.current_local_offset) and apply that to the timestamp. No need for other changes I think.

nickguletskii commented 1 year ago

Sorry, you are right. I think the confusion comes from how things are named in std::time;

The issuer of the token must always use UTC for expiry times. That much is clear and the author of the original issue seems to agree with this.

What I think the original issue is actually claiming is that the UTC timestamps in the tokens are being compared to local time. However, that is not the case, because std::time::SystemTime is not the system's local time (i.e. time with local timezone), it's the system's UTC clock. Therefore, the timestamp validation in jsonwebtoken is correct and there's no need for this PR.