in User.resetPassword callback called without error if user not founded

[estin]

In line https://github.com/strongloop/loopback/blob/master/common/models/user.js#L445 called callback without error about not founded user by email.

// on existed email - PASSED
app.models.User.resetPassword({email: 'existed@mail.com'}, function (err) {
 assert(err === null);
});

// not existed email - FAILED
app.models.User.resetPassword({email: 'not-existed@mail.com'}, function (err) {
 assert(err);
});

Sorry for my poor English.

[superkhau]

Please create a test project on github to reproduce the issue per https://github.com/strongloop/loopback/wiki/Issues#bug-report

[estin]

repo for this issue https://github.com/estin/loopback-sandbox

$ npm test

  #947
    ✓ reset password by existed email, err is undefined (240ms)
    ✓ reset password by not existed email, should be err defined 
    1) reset password by not existed email, should be err defined

  2 passing (258ms)
  1 failing

  1) #947 reset password by not existed email, should be err defined:
     Uncaught AssertionError: undefined == true
      at /home/user/job/whiteteam/tmp/loopback-sandbox/test/test.js:19:8
      at /home/user/job/whiteteam/tmp/loopback-sandbox/node_modules/loopback/common/models/user.js:450:11
      at /home/user/job/whiteteam/tmp/loopback-sandbox/node_modules/loopback-datasource-juggler/lib/dao.js:1162:62
      at allCb (/home/user/job/whiteteam/tmp/loopback-sandbox/node_modules/loopback-datasource-juggler/lib/dao.js:1112:7)
      at /home/user/job/whiteteam/tmp/loopback-sandbox/node_modules/loopback-datasource-juggler/lib/connectors/memory.js:345:7
      at process._tickDomainCallback (node.js:381:11)
      at process.<anonymous> (/home/user/job/whiteteam/tmp/loopback-sandbox/node_modules/loopback/node_modules/continuation-local-storage/node_modules/async-listener/index.js:18:15)

npm ERR! Test failed.  See above for more details.

P.S. I`m have not enough experience with nodejs and loopback...

[fabien]

This is a possible security and certainly a privacy issue: anyone is able to figure out if someone is a registered user just by trying out the person's email address and getting feedback.

[simoami]

@Fabien Following up from my other issue #1374.

I surely understand your point on privary, but I don't think it should be sensored at an internal API. It should be possible for developers to get the real picture and decide what they want to return to the end user. Otherwise it's like a db query refusing to return a password hash for you to compare.

And to counter example your privacy issue, I can attempt to re-register an existing email to find out if it exists. And you can see, it's totally ineffective to protect the "email exists" if you're not going to address it at multiple levels.

And as a bonus. Here are some popular apps that tell you if your email exists:

Google Atlassian Jira Atlassian Confluence Facebook Twitter

Perhaps it's a thing of the past.

[simoami]

@here

Proposed fix: https://github.com/strongloop/loopback/issues/1374

[shairez]

BTW in your docs there is a suggestion to reimplement the requestPasswordReset which do return an error if the user doesn't exists.

https://github.com/strongloop/loopback-faq-user-management/blob/master/server/boot/routes.js#L52-L66

so if it is a privacy threat, your example should be changed, right? (maybe I'm missing something)

[julien-sarazin]

What is the status of this issue? Do we have to handle this on our own?

[superkhau]

I agree with the fix proposed by @simoami . It should be up to the implementer to decide what to do if a user is not found. Would you like to submit a PR for #1374?

@bajtos @raymondfeng @ritch Thoughts?

@shairez You are right about the example, we should probably not send the err with the 401 response. Would you like to submit a PR for that?

@julien-sarazin I would like to get input from @bajtos @raymondfeng or @ritch before moving ahead with this.

[julien-sarazin]

@superkhau Alright. Thanks.
Just as a proposal for those who want to send a 401 if the email does not exist.

    Coder.requestPasswordReset = function(email, cb){
        Coder.findOne({where:{email: email}})
            .then(ensureOne)
            .then(applyRequest);

        function ensureOne(data) {
            return (data)? data : cb({statusCode: 401, message: 'email.does.not.exist'})
        }

        function applyRequest() {
            Coder.resetPassword({email: email}, cb);
        }
    };

    Coder.remoteMethod('requestPasswordReset', {
        description: 'Allows Coders to request a password reset, returns an error if the' +
        ' specified email does not exist.',
        http: {path: '/password-reset', verb: 'post'},
        accepts: [{arg: 'email',    type: 'string', http: {source: 'form'}}],
        returns: {arg: 'status',    type: 'boolean'}
    });

[estin]

@julien-sarazin in you proposal for one HTTP request for reset password would be two SQL requests to fetch user by email.

1. Coder.findOne({where:{email: email}})
2. if user was founded in resetPassword would be new findOne again by email

sorry for my poor English

[julien-sarazin]

@estin no worries. You're totally right. It's just a "temporary solution" for the pending situation.

[bajtos]

@superkhau the proposal in #1374 LGTM in general, I don't have any objections against this change.

[superkhau]

@estin @julien-sarazin Can one of you guys take the implementation in #1374 and submit it as a PR? I will merge it after a small review.

[bajtos]

Fixed by #1789