Return Invalid login message when user doesn’t exist #25
Conversation
Includes failing test. Fixes #10 (comment) once the test is fixed
| @@ -78,7 +78,7 @@ class LocalVerifier { | |||
| const payload = { [`${this.options.entity}Id`]: id }; | |||
| done(null, entity, payload); | |||
| }) | |||
| .catch(error => error ? done(error) : done(null, error)); | |||
| .catch(error => error ? done(error) : done(null, error, { message: 'Invalid login' })); | |||
There was a problem hiding this comment.
Maybe instead of this we should actually just reject with an error instead of returning false on line https://github.com/feathersjs/feathers-authentication-local/blob/master/src/verifier.js#L58. What do you think?
I was initially following how passport expects results to come back but I think in reality we do want to reject with either a 404, 400, or 403 error if invalid credentials are passed in. Likely a 404 since it is during the query look up by username that we can't find a user.
There was a problem hiding this comment.
@ekryski Yeah. I wasn't sure exactly what would happen here: https://github.com/jaredhanson/passport-local/blob/master/lib/strategy.js#L81 where passport does a special check for !user, which is probably what you're talking about. I don't really understand the need to differentiate it. Why does Passport care?
There was a problem hiding this comment.
I think for some passport strategies they respond differently based on the fail method being called vs. the error method. https://github.com/jaredhanson/passport-local/blob/master/lib/strategy.js#L81
Just trying to track it down to see if there is a difference.
There was a problem hiding this comment.
Ya so I think we can go either way here. For local auth it really doesn't matter. The fail method is used more with OAuth and OpenID flows because there are more ways that the authentication can actually fail that may not be a result of bad user credentials. https://github.com/jaredhanson/passport-oauth2/blob/master/lib/strategy.js#L129
There was a problem hiding this comment.
I think we should stick with your solution @marshallswain. It conforms more to what Passport advocates and also allows the developer to customize things much more. For example, how we handle fail calls: https://github.com/feathersjs/feathers-authentication/blob/master/src/hooks/authenticate.js#L60-L77
| @@ -78,7 +78,7 @@ class LocalVerifier { | |||
| const payload = { [`${this.options.entity}Id`]: id }; | |||
| done(null, entity, payload); | |||
| }) | |||
| .catch(error => error ? done(error) : done(null, error)); | |||
| .catch(error => error ? done(error) : done(null, error, { message: 'Invalid login' })); | |||
There was a problem hiding this comment.
I think we should stick with your solution @marshallswain. It conforms more to what Passport advocates and also allows the developer to customize things much more. For example, how we handle fail calls: https://github.com/feathersjs/feathers-authentication/blob/master/src/hooks/authenticate.js#L60-L77
|
I'm glad something has been done on this one and I don't have to create my own verifier just to send some useful error. Btw, can I also suggest to add a debug line in https://github.com/feathersjs/feathers-authentication-local/blob/eb47d2b06184cb43c62bf0bc601ce9076cea260a/src/verifier.js#L58 like |
|
@sydcanem very good suggestions. I'll work that into the PR. |
|
Thanks @marshallswain. |
marshallswain
force-pushed
the
better-error-message
branch
from
9fbd81f
to
3997654
Compare
Will fix #10 once the test is fixed.