wesbos / advanced-react-rerecord

Trying things out. Feel free to follow along
247 stars 36 forks source link

Calculation for resetPassword might be off? #46

Closed vinay30 closed 3 years ago

vinay30 commented 4 years ago

Hey Wes,

My math could be off, but shouldn't these lines read:

const now = Date.now();
  const expiry = new Date(user.resetTokenExpiry).getTime();
  if (now - expiry >= 0) { // changed to >= 0
    throw new Error('This token is expired');
  }

Because in the requestReset flow, you set a Date object with an expiry date 1 hour in the future, so you want to check if now has "caught up" and/or passed the expiry time - i.e. if now - expiry is greater than or equal to 0? Otherwise, you allow for now to pass the expiry date by another hour.

https://github.com/wesbos/advanced-react-rerecord/blob/ae6a83f70b303e397f174f87717b5f460c2cf232/backend/mutations/resetPassword.js#L26

Cheers!

simonpweller commented 4 years ago

@vinay30 I believe you're right - the offset is accounted for here: https://github.com/wesbos/advanced-react-rerecord/blob/0aa4eaf875e2468eaef37b7ccae2d544abbaeb8b/backend/mutations/requestReset.js

You could then make the code a bit more expressive too:

const now = Date.now();
const expiry = new Date(user.resetTokenExpiry).getTime();
if (now >= expiry) {
  throw new Error('This token is expired');
}
vinay30 commented 4 years ago

Indeed! I like that :)

wesbos commented 3 years ago

thank you! Super good eye here. All fixed :)