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
247 stars 98 forks source link

Invalid Data Received After Implementation #169

Closed joegrenan closed 3 years ago

joegrenan commented 3 years ago

Steps to reproduce

Expected behavior

Tell us what should happen Shouldn't give Invalid Data error, as Postgres database fields match the fields the API expects.

Actual behavior

Tell us what happens instead On user create via the users service, an error is returned so long as feathers-authenticaion-management is enabled. If I remove the verifyHooks.addVerification, user creation works fine again.

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working): "feathers-authentication-management": "^3.1.0", "@feathersjs/authentication": "^4.5.11", "@feathersjs/authentication-local": "^4.5.11", "@feathersjs/feathers": "^4.5.11",

NodeJS version: 14, latest stable

Operating System: Mac OS Big Sur

joegrenan commented 3 years ago

My guess is this could very well have to do with the Postgres column types, and in this case the timestamp type which is timestamp without timezone

joegrenan commented 3 years ago

Aha, after browsing through source code, you don't convert the Date() .toISOString(), which renders it incompatible for certain databases unless a workaround is done at the model level. However, if I update my application to modify verifyExpires and convert it toISOString(), will the rest of the package still work correctly?

claustres commented 3 years ago

As feathers is database agnostic the usual way to proceed is to use plain JS data types (eg Date Object or Number) then convert them with a hook if required for a specific database adapter. Typically verifyExpires is a number of milliseconds (UNIX EPOCH), so a simple number, and the module expects this to perform some computation (eg here). Converting it using .toISOString() will make it a string, which is not really explicit regarding a timestamp and will require to update the code at others places. I'm not a user of the PG adapter but it seems to me a bit surprising that it cannot natively handle a number. Maybe you have to declare it upfront in the model ? Maybe this can help.

joegrenan commented 3 years ago

Hooks were a thought, but performing the conversion in Objection's model (not via the PG adapter) before creation and after updates works fine instead. The thing I like about doing it this way, is my dates in the database are all consistent types. It's a small amount of overhead for a better design long term I think.