Add clockTimestamp option to .verify() you can set the current time in seconds with it

[mborst]

This PR enables passing a value now to the verify function that is then used instead of Date.now() (or rather Math.floor(Date.now()/1000).
Tackles #240.
I didn't change the logic of options.maxAge because it uses milli seconds which seemed weird to use as a unit for options.now. (See #273)

[jfromaniello]

I am okay with this but is there a better name than now?

[mborst]

Sure, anything you like. Just timestamp maybe? referenceTime?

[mborst]

Can I speed this up somehow? Should I just change it to timestamp?

[ziluvatar]

Hi @mborst, sorry for the delay.

• We already have a property to workaround the tolerance on the time checks called clockTolerance. Let's put clockTimestamp so they are kind of grouped. Is that ok?

• Can you update the README with this new property and its time units?

• What do you think about allowing milliseconds as input for the field? You could modify the maxAge comparison to use the new property as well, and it seems more natural for a "current timestamp", that let's the library the decision to use seconds there or milliseconds here.

• Can you add a validation over the property and its test? ie: number check

[mborst]

No problem!

• Changed now to clockTimestamp.
• Added documentation.

But I don't think it's a good idea to allow milliseconds anywhere, to be honest. Basically every other thing is in seconds, specifically the specified fields of the JWT itself. Using ms somewhere could give users a wrong impression and one would have to check the code to see how rounding is handled behind the scenes.

[ziluvatar]

Ok, I buy your thought, let's leave the seconds.
More:

• Can you add a validation over the property and its test? ie: number check
• Can you add clockTolerance to maxAge, otherwise it would be weird to have that new property everywhere else. Maybe, until we think what to do with maxAge you can check it like (((options.clockTimestamp || 0) * 1000) || Date.now()) - (payload.iat...

[mborst]

Oh, sorry for forgetting the checks. Added them now.
To make maxAge work with clockTimestamp, I had to do quite some changes in the tests, because they use a hard-coded token that only has 1s difference between iat and exp.
I had to replace this token, I would suggest that maybe one would even use a dynamically generated token in general and hard-code it for only a couple of tests to check if signing works as expected.

In general, are you open towards a bit of clean-up work in the test suite? Also, are you open to introducing linting and some other things?

Side note: I couldn't reproduce the token I changed on my system with what I assumed to be the same parameters, even though I was able to verify it. I'm not well versed in cryptography, but that struck me as odd.

[mborst]

Oh, one more thing, I am not sure if the maxAge change could be considered backwards-compatibility breaking. There are definitely changes in behaviour since the number against which maxAge is compared used to have the same "milliseconds" as the current date, while now it's always a multiple of 1000.

-    if (Date.now() - (payload.iat * 1000) > maxAge + (options.clockTolerance || 0) * 1000) {
+    if ((clockTimestamp - payload.iat) * 1000 > maxAge + (options.clockTolerance || 0) * 1000) {

[ziluvatar]

Yep, it could break some usage. That's why I suggested this ugly line: (((options.clockTimestamp || 0) * 1000) || Date.now()) - (payload.iat... to only affect the ones who uses clockTimestamp, because it is new so it won't break anything old.

Basically instead of reusing the variable you used you have to use it directly (you can create a variable right there and add a comment about why you can't reuse the clockTimestamp variable)

Still... people with current usage if they want to use clockTimestamp they will have that behavior (token will last 0-999 ms more).
I see a couple of options:

• Compare maxAge in seconds when clockTimestamp is set, we would need to modify README and it seems kind of juggling.
• Assume that margin 0-999ms and recommend to pass maxAge with full seconds, ie: 1486669096000 when it is used with this option.
• Use milliseconds for clockTimestamp (we would expand the ms problem)
• Leave the PR marked for next-major, and prepare another PR to use maxAge with seconds waiting for next-major as well (I need to think about the maxAge change...)

Can you think in any other option?

[mborst]

I think you covered everything.
I agreed with the "juggling" part in the first way initially, but after implementing it just now, I think it's acceptable. I think that rather the current setup (comparing as if iat had milliseconds precision) is weird (and should maybe be warned against?).

[ziluvatar]

I think you would need to explicitly say something like also against maxAge, be sure you don't use milliseconds in maxAge when you use both together, since clockTimestamp works with seconds.

I just realized we don't have docs for maxAge, we could add it (now or later) suggesting the use of second as minimum unit just for common sense, warning that in the future it could be the minimum unit.

[mborst]

Fixed and added.

[ziluvatar]

Thanks @mborst !! I need to give a look other PRs and I may create a minor version with this and other changes soon.

[ziluvatar]

@mborst I just released a minor version 7.3.0 with your this PR included, in case you want to use it.