feathersjs-ecosystem / feathers-authentication-management

Adds sign up verification, forgotten password reset, and other capabilities to local feathers-authentication
https://feathers-a-m.netlify.app/
MIT License
246 stars 98 forks source link

action "resetPwdLong" does result in error "dateOrNumber.getTime() is not a function" #219

Closed zanzara closed 10 months ago

zanzara commented 10 months ago

Steps to reproduce

Calling action resetPwdLong does result in error dateOrNumber.getTime() is not a function Last stack trace call ends here: src/helpers/get_user_data.js

function dateOrNumberToNumber(dateOrNumber) {
  if (!dateOrNumber) return 0;
  return typeof dateOrNumber === 'number'
    ? dateOrNumber
    : dateOrNumber.getTime();
}

Idk why param dateOrNumber isn't a date object.

Expected behavior

It should run w/o error. So param dateOrNumber seems to be no valid date object.

Actual behavior

It raises an error. See subject.

System configuration

Module versions (especially the part that's not working): f-a-m version ^5.0.0

NodeJS version: 16.16.0

Operating System: Windows 10

fratzinger commented 10 months ago

Can you tell the typeof dateOrNumber? Is it a string?

zanzara commented 10 months ago

How can I check this?

zanzara commented 10 months ago

Can you tell the typeof dateOrNumber? Is it a string?

@fratzinger Yes, I could checked it now. typeof says it is a string (although value seems to be sm as a long UTC number) So no date object.

zanzara commented 10 months ago

Seems to be a bug: I assume it should more look like that:

function dateOrNumberToNumber(dateOrNumber) {
  if (!dateOrNumber) return 0;
  return typeof dateOrNumber === 'number'
    ? dateOrNumber
    : new Date(dateOrNumber).getTime();
}

At least this works w/o errors in dev.mode

zanzara commented 10 months ago

It seems that my previous post is kinda nonsense. This should be better in case of Javascript (not Typescript)

    switch(typeof dateOrNumber) {
      case 'number' : return dateOrNumber
      case 'string' : return Number(dateOrNumber)
      case 'object' : return new Date(dateOrNumber) !== 'Invalid Date' ? dateOrNumber.getTime() : 0
      default: return 0
}

Could anybody confirm pls?

zanzara commented 10 months ago

@fratzinger Sry to ping u again here, but could this issue now be seen as a bug? Could u prolly confirm my workarround? (so that I could make at least a PR out of it?) TIA

fratzinger commented 10 months ago

What adapter do you use? mongo, knex, sequelize, objection, ...? And what is the type of the db properties verifyExpires and resetExpires for your service users?

Please also print what the actual dateOrNumber in your case is. I already know that it's a string, but I don't understand if it should be a number or if it is a date ISO string. Your last suggestion with switch imply that the string really should be a number.

This makes much more sense to me:

function dateOrNumberToNumber(dateOrNumber) {
  if (!dateOrNumber) return 0;
  return typeof dateOrNumber === 'number'
    ? dateOrNumber
    : new Date(dateOrNumber).getTime();
}

otherwise we should check if the string can be safely parsed to a number first and if not we should convert it to a Date object.

Please tell me what your actual dateOrNumber is and which adapter you use.

zanzara commented 10 months ago

hi TYVM. Really appreciating ur response.

I'm using objection with knex. As DB PostgreSQL verifyExpires and resetExpires are defined as bigint

I have to restart my tests to check what value dateOrNumber had. I'll post here....

zanzara commented 10 months ago

Now i got the value: dateOrNumber typeof: string value: 1699135623475

But strange smh:
If I do:

new Date('1699135623475').getTime();  

I got a NaN If I do:

new Date(1699135623475).getTime();  

It prints out 1699135623475 (tested in F12 console)

and this works

new Date(Number('1699135623475')).getTime();  

It prints out 1699135623475

fratzinger commented 10 months ago

Thanks for your appreciation and sorry for the delay. bigint and strings makes perfectly sense. I did not think about that.

In that case we should really check something like isNaN:

function isStringOfDigits(value: string) {
  return /^\d+$/.test(value);
}

function dateOrNumberToNumber(dateOrNumber: Date | number | string | undefined | null): number {
  if (!dateOrNumber) return 0;
  return typeof dateOrNumber === 'number'
    ? dateOrNumber
    : typeof dateOrNumber === 'string'
      ? isStringOfDigits(dateOrNumber)
        ? Number(dateOrNumber)
        : new Date(dateOrNumber).getTime()
    : new Date(dateOrNumber).getTime();
}

This should do it. I think it catches BigInt values but also ISOStrings like '2011-10-05T14:48:00.000Z'

zanzara commented 10 months ago

Okay. Fine. But maybe the switch makes it perhaps more readable , than cascading tertiary ?: expressions? Do u will make a PR? Will it come to a 5.0.1 release then?

(it should be also changed in the js file)

fratzinger commented 10 months ago

published as 5.0.1