Skip to content
This repository has been archived by the owner on Oct 26, 2019. It is now read-only.

password reset #89

Merged
merged 7 commits into from
Aug 25, 2017
Merged

password reset #89

merged 7 commits into from
Aug 25, 2017

Conversation

fawaf
Copy link
Member

@fawaf fawaf commented Apr 21, 2016

No description provided.

@fawaf
Copy link
Member Author

fawaf commented Apr 27, 2016

@fawaf fawaf self-assigned this Aug 17, 2016
@fawaf fawaf changed the title Waf reset pw password reset Aug 17, 2016
@goto-bus-stop
Copy link
Member

goto-bus-stop commented Dec 23, 2016

Sorry for leaving this hanging for so long :(

I think to finish this up on the server side, there are two more things to do. First, the reset controller should return a standard API response, using toItemResponse({}) (the item can be the empty object, since the token should not be sent back to the browser). nvm that

Second, the sendEmail method should take a transport option so the way email is sent can be configured. I think for welovekpop.club we'll use a free plan on a service like SendGrid or Mailchimp rather than a local server, for example. There's not a great way in master to pass options to controllers yet, so currently a transport option would have to be passed down from the ApiV1 class, into the authenticate router, into the reset controller method, into the sendEmail method. for reference, the authenticate router already receives a secret option for password encryption that's passed down in a similar way.

Then sending the email would be a case of calling the sendEmail method on the transport option if available, and otherwise creating a default transport that uses localhost like what the current code does. this was wrong, sorry :x

If that's unclear please poke 🙈

@goto-bus-stop
Copy link
Member

Is it clear what I'm looking for re: the mail transport stuff? The end goal is that server hosts can do something like

new ApiV1(uwaveInstance, {
  mailTransport: {
    // options for `nodemailer.createTransport(options)`
  }
})

and

import sendgridTransport from 'nodemailer-sendgrid-transport'
new ApiV1(uwaveInstance, {
  mailTransport: sendgridTransport({
    auth: { api_key: 'sendgrid api key' }
  })
})

and have the reset password mail be sent over a Nodemailer transport with the given configuration.

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

Thanks for continuing to work on this 🙇‍♀️ I'll try to be a bit quicker at reviewing as well :') I have two comments for now.

src/email.js Outdated
}

// create reusable transporter object using the default SMTP transport
const transporter = mailTransport.createTransport(smtpOptions);
Copy link
Member

Choose a reason for hiding this comment

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

Custom nodemailer transports work by passing the transport to nodemailer.createTransport. So this line should be nodemailer.createTransport(options.mailTransport || smtpOptions), in order to use the custom transport if the option is specified, or else the default smtp transport.

src/email.js Outdated
};

// send mail with defined transport object
if (transporter.sendMail(mailOptions)) {
Copy link
Member

Choose a reason for hiding this comment

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

sendMail returns a promise, you need to wait for the promise to resolve before you can use the result. Here it'll always return true because the if will cast a Promise object to true, even if it represents a failed operation.

Here we'd want to return the Promise object if something failed, so that the web API will return an error response. If it all succeeded we want to return the Promise object but with the contents redacted (since nodemailer returns a bunch of information about the server it sends mail to or whatever). The simplest way to do that would be to:

return transporter.sendMail(mailOptions)
  .then(() => ({})) // ← confusing but () => ({}) is a function that returns an empty object

Which would return a successful Promise object with the success value {} (empty object) if the email was sent, or a failed Promise object if something went wrong.

@fawaf
Copy link
Member Author

fawaf commented Jul 9, 2017

btw, @goto-bus-stop still need an html page for the reset password form.

@goto-bus-stop goto-bus-stop merged commit f710364 into master Aug 25, 2017
goto-bus-stop added a commit that referenced this pull request Aug 25, 2017
@goto-bus-stop
Copy link
Member

Rebased&merged in f710364, thanks for working on this! 🎉

@fawaf fawaf deleted the waf-reset-pw branch August 27, 2017 04:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants