eddyystop / feathers-service-verify-reset Goto Github PK

patch should be used rather than update in changeEmail, too.

I have not been able to turn off the error message that the user hasn't verified their email address when using 'Forgotten password' functionality.

This is deemed a security issue as that response/error verifies that there is such user account compared to the lack of it when the email is invalid.

Steps:

1. User has forgotten password - makes a request through front-end or postman
2. If user's email is not verified, the response indicates so = confirms the user has an account on the system
   If user is not a user, the error is 'User not found', which confirms the user isn't there.

Is there to blank out the errors by passing a configuration parameter. I've been through the documentation but haven't seen such consideration.
Penetration tests deem this to be low to medium severity issue and I was hoping there is a parameterised fix for it - similar to an always valid generic message - "Check your inbox, in case of problem, please contact support".

The docs said:

The wrappers return a Promise unless a callback is provided. See example/ for a working example of wrapper usage.

but actually in client.js, for example:

	this.emailChange = (password, email, user, cb) => {
		verifyReset.create({
			action: 'emailChange',
			value: {
				password,
				email
			},
		}, {
			user
		}, cb);
	};

These wrappers return nothing. should be:

	this.emailChange = (password, email, user, cb) => {
		return verifyReset.create({ // add 'return' here
			action: 'emailChange',
			value: {
				password,
				email
			},
		}, {
			user
		}, cb);
	};

Hello,

First of all this is an awesome lib! Thanks for doing this 🍕
So we have a use case where we reset the password by using:

verifyReset.create({
      action: 'sendResetPwd',
      value: email,
      notifierOptions: {
        transport: 'email',
        route: url
      }
    }, {})
    .then(() => this.context.router.push('/session/forgotten-password/sent'))
    .catch(() => this.context.router.push('/session/forgotten-password/error'));

Once user has received the email and navigated to the link within (example: http://localhost:3000/session/forgotten-password/reset/8dce33f3a30ad1df0e3d4676fe7d91) we want to check whether user.resetToken is still valid.

Is there a method or plan to do this through verifyReset?

Edit: I can always make myself a simple endpoint for checking this, however I think it would be an awesome add-on to the lib.

Thank you,

I am using feathers-mongoose to handle mongo database on the server side, and make the users schema like this:

new Schema({
  username: { type: String, required: false, unique: true },
  email: { type: String, required: true, unique: true },
  password: { type: String, required: true },

  isVerified: { type: Boolean, required: false },
  verifyExpires: { type: Date, required: false },
  verifyToken: { type: String, required: false },
  resetExpires: { type: Date, required: false },
  resetToken: { type: String, required: false },

  slugType: {type: String, require: false},

  createdAt: { type: Date, 'default': Date.now },
  updatedAt: { type: Date, 'default': Date.now }
});

Here the username and email are both unique.

So, when I call changeEmail from client and try to change an email to an existing one, it will cause duplicate key error on the server side. But currently this error is not caught by users.update(..., function (err, user12) {...} source code here, and so the error is consumed on the server-side without feeding back to front-end, leading to a time-out error on the front-end.

I don't know why this error is not caught be users.update, might be a bug of feathers. But at least we can wrap a try/catch around the user.update block to avoid this, I guess.

I've been playing with the example, and I must be missing something really obvious!

It looks as though the resetToken and verifyToken need to be present in the user object (ie not removed by hooks) in order for the emailer function (passed to the verifyReset service) to use them in the emails that are sent to the user. However, this means they are included in the user object that's returned to the VerifyReset client.js calls.

In the console for the example app, I can call verifyReset.sendResetPassword("[email protected]", (e,u) => {console.log(u)}) to generate a resetToken, that's then passed straight back to me!

There must be some setting or something I missed, otherwise this would be a pretty massive security oversight!

The verify functionality enforces looking for users in pagination and doesn't work if users are not paginated.

Hi, first of all thank you for this awesome utils!
I ran into situation where I need to have almost all users functionality but name my service e.g. customers instead of users for project reasons.
As far as I get from this line https://github.com/eddyystop/feathers-service-verify-reset/blob/master/src/index.js#L390
users is a must.
Do you consider adding smth like options.servicePath to make it play with some other services?

Hello,

I am not totally sure on how to implement the e-mail sending.

I was able to implement every hook and services, however, after seeing this function (copied from your example) I was not totally sure if I needed to implement to e-mail sending based on each parameters:

function emailer(action, user1, params, cb) {
    console.log(`-- Sending email for ${action}`);
    const provider = params.provider;
    const route = provider === 'rest' ? 'rest' : 'socket';

    switch (action) {
        case 'resend': // send another email with link for verifying user's email addr
            console.log(`Dear ${user1.username}, please click this link to verify your email addr.`);
            console.log(`  http://localhost:3030/${route}/verify/${user1.verifyToken}`);
            break;
        case 'verify': // inform that user's email is now confirmed
            break;
        case 'forgot': // send email with link for resetting forgotten password
            console.log(`Dear ${user1.username}, please click this link to reset your password.`);
            console.log(`  http://localhost:3030/${route}/reset/${user1.resetToken}`);
            break;
        case 'reset': // inform that forgotten password has now been reset
            break;
        default:
            break;
    }

    cb(null);
}

Am I missing something?

Thanks

Currently we only verify email address when users sign up. But when they call emailChange, the new email address isn't verified. Would this be a problem?

Sending a verification url to user's email is the old and traditional way to do verify/reset. But nowadays people use mobile phones or social network systems much more frequently than emails. Some young people even don't have their own email address -- you have to contact them via facebook, twitter, line, or wechat/qq in China.

Therefore today we get a different way to do verify/reset: not sending a url to people's email and wait for them clicking it, but just sent a verification code (long or short, it depends) to any type of address the user is trying to verify, no matter it is an email, or a cellphone number SMS, or a facebook message, or a line message, etc.

After the user received this code, they have to manually input into our app both the account name they are trying to verify/reset, and the code they received. Both the two info must be correct, or the verify/reset is invalidated and the code is expired. The use has to ask for another code again.

This way, when verifying/resetting, we can match not only the code, but also the user account name, and the user has only one chance to try. This helps avoiding hacking, and therefore the code doesn't have to be very long. [0-9a-zA-Z]{6} is always enough, and it is easy to input on a cellphone.

I think this new verification code way is used more and more widely compared with the old verification url way, and hence suggest that this package should also support this code mode.

We don't need to break the current API to support this code mode. Just an expansion of API should be enough, say, like this:

When the user is signing up in our app:

app.service('users').create({
  email: '[email protected]',
  username: 'beeplin',
  cellphone: '5101112222',
  password: 'xxxxxx',
  primaryContactMethod: 'cellphone', // 'email' by default, the target to send to url/code to. any better name?
  verificationType: 'code' // 'url' by default. generates long slug for url, and short slug for code. any better name?
})

Then the verification URL/code is sent via email or SMS. If the mode is URL, after the user clicks the link and is routed to our page, verifySignUp is called with the slug embedded in the URL. If the mode is code, our app directs the user to a new page and let him/her input the code he/she received, together with his/her email or cellphone (depends on the primaryContactMethod he/she chose just before. Then the verifySignUp is called with the two info:

verifyReset.verifySignUp = function (slugOrOptions, callback) { ... }

when slugOrOptions is a string, it is the slug, and this means the traditional verification URL mode. Do the verify as we did before, nothing new;

when slugOrOptions is an object, it is a map for options, and this means the new verification code mode. The option could be like:

{
  email: '[email protected]',
  slug: 'xxxxxx'
}

This means the user is trying to verify the email [email protected] with a received code xxxxxx. Do the verification and return the result/error to the user.

The key point here is that, instead of passing a slug string in, we could also pass an option object in, having a finer control of the user's verify/reset behavior, without breaking the existing API syntax.

The sendResetPassword, saveResetPassword can also be extended in this way.

I'd like to know how you think about this?

i am trying the example and the verification runs smoothly, but when resetting password, the token is like: http://localhost:3030/rest/reset/undefined, and the procedure failed.

Do I did something wrong? I just created a new account, verified it, signed in, logged out, and then clicked the reset password button.

Test that passwords are not hashed twice regardless of what the users hooks have. This will require Feathers instances with users hooks, rather than using fakes.

Hi folks,
I've upgraded from 0.8 to the latest version and ran into the following pitfall with userNotifier.
Initially, my goal is send emails using hostname, thus I have the following middleware:

.configure(socketio(io => {
    io.use((socket, next) => {
      socket.feathers.hostname = socket.request.headers.host;
      next();
    });
}))

Cause signature userNotifier was (type, user1, params, cb), I just grabbed host from hook.params.
ATM, userNotifier is employed like

userNotifier('sendResetPwd', user, notifierOptions)
// or
userNotifier('resetPwd', user1)

I'm looking for good solution how to connect past and current )
Thanks in advance

during verifySignUp, user's password is among the fields to be updated:

users.update(user.id || user._id, user, {}, function (err, user1) {
...
}

password is in the user object and so could be updated:

user:  { _id: xxx,                                       
  username: 'xxxx',                                                             
  email: 'xxxx',                                                                
  password: 'XXXXXXXXXXXXXXX',   
  isVerified: true,                                                           
  verifyExpires: null,                                                        
  verifyToken: null, 
  ...
}                                                         

So here is the problem: if people use auth.hashPassword hook before updating, then the password would be hashed AGAIN.

I don't think password needs to be updated in this case. So it would be good to exclude password from user object before updating.

Currently 1.0.0 supports short verification token for SMS/social media, but email is still the essential way to identify a user. For example, code here

  this.authenticate = (email, password, cb) => {
    let cbCalled = false;

    app.authenticate({ type: 'local', email, password })
      .then((result) => {
         ...
      });
  };

Here we still suppose email and password is the only way to login. And code here:

  this.emailChange = (password, email, user, cb) => {
    verifyReset.create({
      action: 'emailChange',
      value: { password, email },
    }, { user }, cb);
  };

The API only supports changing email and then sending a notification message to the old email.

But consider a project in which most users don't even have emails ... like my on-going project for teenagers and elders in China. Our survey shows most of our potential teenage customers were born at the era of cellphone and QQ and wechat (just whatsapp and facebook and snapchat for teenagers in the US), and most of our elder customers just missed the email era -- they don't know how to register an email account and how to check email, but all of them have smart phones and wechat accounts.

So perhaps we could just treat email as just one of the multiple possible communication channels to make contact with our users, just like others like cellphone, snapchat etc. I noticed now there is no more emailer but a new userNotifier, so likewise, I think email should be no more treated as a specialty in our API, either.

My current way is to add a primaryCommunication field in the users table in database. It determines which way is the primary communication way to contact with the user, like email or sms or other channels. It is chosen by users when they sign up. Of course we could make email the default, but people have other options. They could choose to sign up with their cellphone number instead of email, and then every communication with them (verify, reset, notification of cellphone number/pasword change) should be sent through sms.

And for login or authenticate, it is unnecessary to force uses to login with email/password. Many apps let people log in with a unique username. Or, if a user's primaryCommuniation is not email but cellphone or sms, he/she could log in with his/her cellphone number.

That's to say, when the user is logging in, instead of the two params named email and password, an options map should be passed to app.authenticate. The developer can decide whether a username is required, or an email, or a cellphone number, or any of them works. So the API at here can be changed into this:

  this.authenticate = (options, cb) => {
    let cbCalled = false;
    options.type = 'local'; // add type field to options
    app.authenticate(options) // pass options directly to app.authenticate()
      .then((result) => {
         ...
      });

And it's up to developer which fields should be included in options.

Similarly, this.emailChange might be rename to this.PrimaryCommunicationChange or something alike. If the user's primaryCommunication is not 'email' but 'snapchat', this method would patch the new value to the 'snapchat' field, not the 'email' field, and then notify the user via snapchat instead of email.

Some other methods like sendResetPwd and passwordChange also need to be changed a bit.

So to wrap up, my whole idea is to treat email equally with other possible communiation channels. People who aren't familiar with emails would appreciate that. This is not only about the long/short token when verify/reset, but also about the way for users to sign up, log in, etc.

Sorry I didn't bring this up before 1.0.0 is published. This idea just came up later than that. :)

At the example the REST route for verifyToken is created by proxying the request via client code. I've incorporated the server code to my app and the module fails to setup a valid GET for token verification via REST. This code defines the route /verifyReset/:action/:value, but it is only acessible via POST requests because it uses create method.

sometimes we prefer SMS or other mobile ways to send verify and reset tokens instead of email, and then let the user input the recieved token in the app. Here the token cannot be too long or too complicated, or it is difficult to input. Typicllay it would be 4 or 6 digital numbers.

so i suggest providing an option to generate simple 6-digit token instead of tje long hashed one.