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

send-password api #154

Closed
3 tasks
ddfridley opened this issue May 26, 2022 · 16 comments · Fixed by #228
Closed
3 tasks

send-password api #154

ddfridley opened this issue May 26, 2022 · 16 comments · Fixed by #228
Assignees

Comments

@ddfridley
Copy link
Contributor

The send password socket-api call is missing. This should be added to the civil-server repo.
This is the code, taken from the original synaccord repo:

'use strict';

import sequencer          from 'promise-sequencer';
import User               from '../models/user';
import sendEmail          from '../server/util/send-email';
import secret             from '../../secret.json';

function sendPassword (email, return_to, cb) {
  const { host } = this.handshake.headers;

  function emailBody (key, token) {
    const tail=token+(return_to||'');
    return secret['forgot password email']
      .replace(/\{key\}/g, key)
      .replace(/\{url\}/g, `http://${host}/page/reset-password/${tail}`);
  }

  new Promise((pass, fail) => {
    User
      .findOne({ email })

      .then(user => new Promise((pass, fail) => {
        if ( ! user ) {
          fail(new Error('User not found'));
        }

        else {
          sequencer(
            ()          =>    user.reactivate(),
            user        =>    sendEmail({
              from      :   secret.email.user,
              to        :   email,
              subject   :   'Reset password',
              text      :   emailBody(user.activation_key, user.activation_token)
            })
          ).then(pass, fail);
        }

      }).then(pass, fail))

      .catch(fail);
  })
    .then(cb)
    .catch(error => cb({ error : error.message }));
}

export default sendPassword;

In the above secret['forgot password email'] is:
"Hey,\r\n\r\nYou are receiving this email because you requested a password reset.\r\n\r\nYour reset key is:\r\n\r\n\t{key}\r\n\r\nTo reset your password, copy the reset key above and go to {url}\r\n\n\nThank you,\nThe Synaccord Team",

I didn't really need to be a secret. I would just include this text in the new send-password.js. In the future, or as a stretch goal, we can use SendInBlue to send the email, and create an HTML template for it.

It sends a link, that the users clicks on, that will take them to a page that will allow them to change their password. The link has a code in it, that is looked up in the database to find the user.
Here is the code for that route:

  resetPassword(){
    this.app.get(['/page/reset-password/:token','/page/reset-password/:token/*'], (req, res, next) => {
      try{
        if(req.params.token){
          User.findOne({activation_token: req.params.token}).then(user=>{
              if(user && user._id){
                req.user=user.toJSON();
                req.cookies.synuser={id: req.user._id, email: req.user.email} // passing the activation key also
                req.activation_key=user.activation_key;
                this.setUserCookie(req,res,next);
              }else
                next();
            },
            (error)=>{console.info("resetPassord found error",error); next(error)}
          )
        }
      }
      catch(error){
        next(error);
      }
    },serverReactRender.bind(this))
  }
  • The reset password token should only be valid for 10 minutes.
  • Build a test for this. Do as much as you can with jest. But you may also have to build a app/web-component and test it with the dev server.
  • Stretch goal: Build a cypress test for this.
@ice1080 ice1080 self-assigned this Sep 28, 2022
@ice1080
Copy link
Collaborator

ice1080 commented Oct 3, 2022

@ddfridley is the original synaccord repo you mentioned a public repo? I did some searching and couldn't find it. The send-email file is missing from the above example.
I'll eventually work on switching the above to SendInBlue, just figured I'd ask if you have it handy.

@ice1080
Copy link
Collaborator

ice1080 commented Oct 3, 2022

Also, I noticed that join.jsx in civil-client is currently unused. From what I can tell, I have to update loginForm.jsx in that project to add the resetPassword flow, would you like me to delete the unused Join file?
Same question for join.js in civil-server.

@ice1080
Copy link
Collaborator

ice1080 commented Oct 4, 2022

@ddfridley you mentioned using send-in-blue on civil-server. I noticed that there is currently neither send-in-blue nor nodemailer are setup in civil-server; just confirming that we do for sure want to add send-in-blue to this?

@ice1080
Copy link
Collaborator

ice1080 commented Oct 11, 2022

For historical tracking, I am copying the candidate-invitation template for the reset password template.
I am removing the unsubscribe section of the email since that's obviously not applicable here.

@ice1080
Copy link
Collaborator

ice1080 commented Oct 11, 2022

Hey @ddfridley, you mentioned a cypress test for this one in the description. I added a cypress test that creates a user, logs in as the user, and then sends the reset password email. I'm not sure it's possible to actually intercept the email request from the backend to send-in-blue (cypress only lets you intercept frontend to backend calls as far as I know).
If there is some way to seed the db with a user before running, then we should be able to insert a fake activationToken etc, and then test that the reset password link works and we can login with the new password after.

Alternatively, I can add some scripts to the package.json that perform what I mentioned using mongodb cli commands. I'll pursue that for now unless I hear back from you on a simpler way to seed and reset the db.

@ddfridley
Copy link
Contributor Author

ddfridley commented Oct 11, 2022 via email

@ddfridley
Copy link
Contributor Author

ddfridley commented Oct 11, 2022 via email

@ice1080
Copy link
Collaborator

ice1080 commented Oct 13, 2022

Just about have this issue complete. I see it working when running civil-server on its own, and just saw it working inside undebate-ssp too. Remaining work:

  • update undebate-ssp to correctly handle the response from the reset-password socket
  • create custom reset password page for undebate-ssp to match undebate-ssp signup/login popup
  • stretch goal - rewrite authForm.jsx in civil-client to use the useAuth custom use-method

@ddfridley
Copy link
Contributor Author

ddfridley commented Oct 13, 2022 via email

@ice1080
Copy link
Collaborator

ice1080 commented Oct 13, 2022

Cypress is currently in the optional dependencies of civil-server and there is already an existing test in there. I was able to write some pretty good cypress tests for this flow in that repo so I was planning on leaving it at that.
When you mention testDependencies, isn't that pretty much what the devDependencies are for? They don't get distributed with the prod build.

@ddfridley
Copy link
Contributor Author

ddfridley commented Oct 13, 2022 via email

@ice1080
Copy link
Collaborator

ice1080 commented Oct 13, 2022

Ah ya that makes sense. Fyi though, most contributors of projects don't look at closed issues, so your comment will most likely stay ignored. I'd recommend opening a new issue and explaining why it's different than the other issue, or why it's not solved by the closing of the other issue.

@ice1080
Copy link
Collaborator

ice1080 commented Oct 19, 2022

Just got back from vacation so trying to get this one finished up.
Our jest tests don't currently test any frontend interactions and it would take quite a bit to get to that point.
As mentioned before, cypress is currently already installed in civil-server so I was using that to test many of these interactions. Is that alright to leave in there?
If we added cypress to undebate-ssp in the dev dependencies, would that still get packaged inside heroku? I know you said it would for dependencies' dev dependencies, but wasn't sure for direct dev dependencies.
Let me know how you'd like to proceed with this.

@ice1080
Copy link
Collaborator

ice1080 commented Oct 20, 2022

Another thing I just noticed was this error message code in civil-server:

function route() {
  const apiLimiter = expressRateLimit({
    windowMs: 60 * 1000,
    max: 2,
    message: 'Too many attempts logging in, please try again after 24 hours.',
  })  
  this.app.post('/sign/in', apiLimiter, signIn, this.setUserCookie, sendUserId)
}

The error message says to try again after 24 hours, but it really only checks that you don't try 3 times in a row within 60 seconds. Should I update the message or was this intentional? I waited 60 seconds instead of 24 hours and was able to sign in again.

@ddfridley
Copy link
Contributor Author

What if we do something based on NODE_ENV being development or production. For production it should be 24 hours. for development it should be 60 seconds.

@ice1080
Copy link
Collaborator

ice1080 commented Oct 20, 2022

@ddfridley Phew this one was a doozy. Here are all three pull requests for this issue:

On civil-server you can run npm run test to see the jest tests passing, and you can run the dev server and run npm run cypress:headless or npm run cypress to see the cypress tests passing. I also confirmed that the jest tests on undebate-ssp were still passing.

In order to truly test all of this hooked together prior to merging the PRs, you would have to use temp branches. Here are the dependencies I was using in undebate-ssp's package.json, which should all be up to date. If you go this route, manually change them in package.json, then remove your node_modules and reinstall. Sometimes you have to use npm uninstall <dependency> to remove it from the npm cache too, and then reinstall it and move it back into the peerDependencies section.

  • "civil-client": "github:ice1080/civil-client#undebate-ssp-send-password#154",
  • "civil-server": "github:ice1080/civil-server#isaac-temp"
  • "undebate": "github:ice1080/undebate#isaac-temp"

Let me know if you have any questions or changes!

@ice1080 ice1080 linked a pull request Oct 20, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants