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

skipIsVerifiedCheck doesn't call custom notifier. #172

Closed MartynasRaf closed 2 years ago

MartynasRaf commented 2 years ago

Steps to reproduce

When implementing auth management, I have set up email verification and reset password functionality. The problem I'm getting is when I'm trying to bypass the check for email verification when resetting the password.

Curently I'm setting it up like this:

app.configure(authentication).configure( authManagement({ skipIsVerifiedCheck: true, notifier: (type, user, notifierOptions) => notifier(type, user, notifierOptions), }) );

The whole code is very similar to this tutorial: https://hackernoon.com/setting-up-email-verification-in-feathersjs-ce764907e4f2 Except for minor details such as sendResetPwd and resetPwd filled out to include email subject and body and reset route which all work when user is verified.

And while the email verification works fine and sends the email to the client, when sending a post request to reset the password returns the response as if it has been reset, but no email has been sent as though the call didn't go through. When checked in the database the user has been modified with exactly the same entry (password, email, etc.) so it bypassed my custom logic and just reset the password to itself it seems.

Tested it with console logs and it definitely doesn't go through my notifier when using skipISVerifiedCheck option.

Expected behavior

Tell us what should happen

When POST request has been received the service should skip checking the verification field and call my notifier with "sendResetPwd" action.

Actual behavior

Tell us what happens instead

When POST request has been received the service skips checking the verification field but doesn't call my notifier with the action.

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/feathers": "^4.5.12",

NodeJS version:

Operating System:

Win11

Browser Version:

React Native Version:

Module Loader:

wethinkagile commented 2 years ago

Could you possibly try the pre-release of version 4? (see PRs)

MartynasRaf commented 2 years ago

"feathers-authentication-management": "^4.0.0-pre.1"

Still seems to bypass my notifier when using the option

claustres commented 2 years ago

As far as I can see this option does not change the workflow of the module, only the props that are checked. As a consequence the notifier is called similarly in both cases. How are you sure that your notifier is not called, did you set a breakpoint ? Indeed, according to the source code if the action is performed correctly without raising any error then the notifier has been called.

MartynasRaf commented 2 years ago

That's what I've noticed too when checking the source code. What I did was put console logs in places to track whether notifier is called or diverted to default or somewhere else. What I got from that is when sending the verification action, the notifier does get called, but not when sending the sendResetPwd action.

When the option is used even verified user doesn't call notifier.

Maybe it's me doing something very wrong, but it seems, that using this option somehow ignores custom notifier.

claustres commented 2 years ago

console.log can be error prone, I'd personally prefer to use a debugger to be sure of what is called or not. What I can tell is that we are using this option in v2 and it works fine.

Could you please share your code/setup ?

MartynasRaf commented 2 years ago

Sure, here it is https://github.com/MartynasRaf/AuthManagementSkipIsVerifiedCheck

mailer and notifier needs to be modified with appropriate credentials to work :)

claustres commented 2 years ago

Just reading the code I don't understand how it can actually work. Here you import your notifier but the export does not seem to be the notifier function, the export should be this to match the expected signature doesn't it ?

Or you should probably use something like notifier(app).notifier here.

MartynasRaf commented 2 years ago

That was it! Somehow I didn't notice that, thanks for taking your time to investigate.