Merge pull request #988 from coralproject/password-reset-fix

Password reset fixes

app.js

@@ -6,13 +6,9 @@ const merge = require('lodash/merge');
 const helmet = require('helmet');
 const compression = require('compression');
 const cookieParser = require('cookie-parser');
-const {
-  BASE_URL,
-  BASE_PATH,
-  MOUNT_PATH,
-  STATIC_URL,
-  HELMET_CONFIGURATION,
-} = require('./url');
+const {HELMET_CONFIGURATION} = require('./config');
+const {MOUNT_PATH} = require('./url');
+const {applyLocals} = require('./services/locals');
 const routes = require('./routes');
 const debug = require('debug')('talk:app');
 
@@ -57,12 +53,8 @@ app.set('view engine', 'ejs');
 // ROUTES
 //==============================================================================
 
-// Apply the BASE_PATH, BASE_URL, and MOUNT_PATH on the app.locals, which will
-// make them available on the templates and the routers.
-app.locals.BASE_URL = BASE_URL;
-app.locals.BASE_PATH = BASE_PATH;
-app.locals.MOUNT_PATH = MOUNT_PATH;
-app.locals.STATIC_URL = STATIC_URL;
+// Add the locals to the app renderer.
+applyLocals(app.locals);
 
 debug(`mounting routes on the ${MOUNT_PATH} path`);
 

client/coral-embed-stream/src/components/Embed.js

@@ -29,7 +29,7 @@ export default class Embed extends React.Component {
   };
 
   render() {
-    const {activeTab, commentId, root, data, auth: {showSignInDialog, signInDialogFocus}, blurSignInDialog, focusSignInDialog, hideSignInDialog} = this.props;
+    const {activeTab, commentId, root, data, auth: {showSignInDialog, signInDialogFocus}, blurSignInDialog, focusSignInDialog, hideSignInDialog, router: {location: {query: {parentUrl}}}} = this.props;
     const {user} = this.props.auth;
     const hasHighlightedComment = !!commentId;
 
@@ -37,7 +37,7 @@ export default class Embed extends React.Component {
       <div className={cn('talk-embed-stream', {'talk-embed-stream-highlight-comment': hasHighlightedComment})}>
         <IfSlotIsNotEmpty slot="login">
           <Popup
-            href='embed/stream/login'
+            href={`embed/stream/login?parentUrl=${encodeURIComponent(parentUrl)}`}
             title='Login'
             features='menubar=0,resizable=0,width=500,height=550,top=200,left=500'
             open={showSignInDialog}

routes/api/account/index.js

@@ -4,9 +4,6 @@ const UsersService = require('../../../services/users');
 const mailer = require('../../../services/mailer');
 const authorization = require('../../../middleware/authorization');
 const errors = require('../../../errors');
-const {
-  ROOT_URL
-} = require('../../../config');
 
 //==============================================================================
 // ROUTES
@@ -50,22 +47,17 @@ router.post('/password/reset', async (req, res, next) => {
 
   try {
     let token = await UsersService.createPasswordResetToken(email, loc);
-    if (!token) {
-      res.status(204).end();
-      return;
+    if (token) {
+      await mailer.sendSimple({
+        template: 'password-reset',
+        locals: {
+          token,
+        },
+        subject: 'Password Reset',
+        to: email
+      });
     }
 
-    // Send the password reset email.
-    await mailer.sendSimple({
-      template: 'password-reset',             // needed to know which template to render!
-      locals: {                                     // specifies the template locals.
-        token,
-        rootURL: ROOT_URL
-      },
-      subject: 'Password Reset',
-      to: email
-    });
-
     res.status(204).end();
   } catch (e) {
     return next(e);
@@ -78,22 +70,23 @@ router.post('/password/reset', async (req, res, next) => {
  * 2) the new password {String}
  */
 router.put('/password/reset', async (req, res, next) => {
-
-  const {
-    token,
-    password
-  } = req.body;
+  const {check} = req.query;
+  const {token, password} = req.body;
 
   if (!token) {
     return next(errors.ErrMissingToken);
   }
 
-  if (!password || password.length < 8) {
+  if (check !== 'true' && (!password || password.length < 8)) {
     return next(errors.ErrPasswordTooShort);
   }
 
   try {
     let [user, loc] = await UsersService.verifyPasswordResetToken(token);
+    if (check === 'true') {
+      res.status(204).end();
+      return;
+    }
 
     // Change the users' password.
     await UsersService.changePassword(user.id, password);

services/domainlist.js

@@ -2,6 +2,8 @@ const debug = require('debug')('talk:services:domainlist');
 const _ = require('lodash');
 const SettingsService = require('./settings');
 
+const {ROOT_URL} = require('../config');
+
 /**
  * The root domainlist object.
  * @type {Object}
@@ -17,31 +19,24 @@ class Domainlist {
   /**
    * Loads domains white list in from the database
    */
-  load() {
-    return SettingsService
-      .retrieve()
-      .then((settings) => {
-
-        // Insert the settings domains whitelist.
-        this.upsert(settings.domains);
-      });
+  async load() {
+    const {domains} = await SettingsService.retrieve();
+    this.upsert(domains);
   }
 
   /**
    * Inserts the domains whitelist data
    * @param  {Array} list list of domains to be set to the whitelist
    */
-  upsert(lists) {
+  async upsert(lists) {
 
     // Add the domains to this array and also be sure are all unique domains
     if (!('whitelist' in lists)) {
       return;
     }
 
-    this.lists['whitelist'] = Domainlist.parseList(lists['whitelist']);
-    debug(`Added ${lists['whitelist'].length} domains to the whitelist.`);
-
-    return Promise.resolve(this);
+    this.lists.whitelist = Domainlist.parseList(lists.whitelist);
+    debug(`Added ${lists.whitelist.length} domains to the whitelist.`);
   }
 
   /**
@@ -51,19 +46,22 @@ class Domainlist {
    */
   match(list, url) {
 
+    // Parse the url that we're matching with.
     const domainToMatch = Domainlist.parseURL(url);
 
     // This will return true in the event that at least one blockword is found
     // in the phrase.
-    for (let i = 0; i < list.length; i++) {
-      if (list[i] === domainToMatch) {
-        return true;
-      }
-    }
+    return list.indexOf(domainToMatch) >= 0;
+  }
 
-    // We've walked over all the whitelisted domains, and haven't had a
-    // mismatch... It is not an allowed domain!
-    return false;
+  /**
+   * Checks to see if the passed url matches the domain of the root path.
+   *
+   * @param {String} url
+   * @returns {Boolean} true if the domains match
+   */
+  static matchMount(url) {
+    return Domainlist.parseURL(url) === Domainlist.parseURL(ROOT_URL);
   }
 
   /**
@@ -84,7 +82,7 @@ class Domainlist {
     let domain;
 
     // removes protocol and get domain
-    if (url.indexOf('://') > -1) {
+    if (url.indexOf('//') > -1) {
       domain = url.split('/')[2];
     } else {
       domain = url.split('/')[0];
@@ -96,13 +94,14 @@ class Domainlist {
     return domain.toLowerCase();
   }
 
-  static urlCheck(url) {
+  static async urlCheck(url) {
     const dl = new Domainlist();
 
-    return dl.load()
-      .then(() => {
-        return dl.match(dl.lists['whitelist'], url);
-      });
+    // Load the domain list.
+    await dl.load();
+
+    // Perform a match.
+    return dl.match(dl.lists.whitelist, url);
   }
 
 }

services/email/email-confirm.html.ejs

@@ -1,3 +1,3 @@
 <p><%= t('email.confirm.has_been_requested') %> <b><%= email %></b>.</p>
-<p><%= t('email.confirm.to_confirm') %> <a href="<%= rootURL %>/admin/confirm-email#<%= token %>">Confirm Email</a></p>
+<p><%= t('email.confirm.to_confirm') %> <a href="<%= BASE_URL %>admin/confirm-email#<%= token %>">Confirm Email</a></p>
 <p><%= t('email.confirm.if_you_did_not') %></p>

services/email/email-confirm.txt.ejs

@@ -4,6 +4,6 @@
 
 <%= t('email.confirm.to_confirm') %>
 
-    <%= rootURL %>/confirm/endpoint#<%= token %>
+    <%= BASE_URL %>confirm/endpoint#<%= token %>
 
 <%= t('email.confirm.if_you_did_not') %>

services/email/password-reset.html.ejs

@@ -1,2 +1,2 @@
 <p><%= t('email.password_reset.we_received_a_request') %><br />
-<%= t('email.password_reset.if_you_did') %> <a href="<%= rootURL %>/admin/password-reset#<%= token %>"><%= t('email.password_reset.please_click') %></a>.</p>
+<%= t('email.password_reset.if_you_did') %> <a href="<%= BASE_URL %>admin/password-reset#<%= token %>"><%= t('email.password_reset.please_click') %></a>.</p>

services/email/password-reset.txt.ejs

@@ -1,3 +1,3 @@
 <%= t('email.password_reset.we_received_a_request') %>. <%= t('email.password_reset.if_you_did') %> <%= t('email.password_reset.please_click') %>:
 
-<%= rootURL %>/admin/password-reset#<%= token %>
+<%= BASE_URL %>admin/password-reset#<%= token %>

services/locals.js

@@ -0,0 +1,20 @@
+const {
+  BASE_URL,
+  BASE_PATH,
+  MOUNT_PATH,
+  STATIC_URL,
+} = require('../url');
+
+const applyLocals = (locals) => {
+
+  // Apply the BASE_PATH, BASE_URL, and MOUNT_PATH on the app.locals, which will
+  // make them available on the templates and the routers.
+  locals.BASE_URL = BASE_URL;
+  locals.BASE_PATH = BASE_PATH;
+  locals.MOUNT_PATH = MOUNT_PATH;
+  locals.STATIC_URL = STATIC_URL;
+};
+
+module.exports = {
+  applyLocals,
+};

services/mailer.js

@@ -4,6 +4,7 @@ const kue = require('./kue');
 const path = require('path');
 const fs = require('fs');
 const _ = require('lodash');
+const {applyLocals} = require('./locals');
 
 const i18n = require('./i18n');
 
@@ -92,6 +93,9 @@ const mailer = module.exports = {
     // Prefix the subject with `[Talk]`.
     subject = `[Talk] ${subject}`;
 
+    applyLocals(locals);
+
+    // Attach the templating function.
     locals['t'] = i18n.t;
 
     return Promise.all([

services/users.js

@@ -1,8 +1,6 @@
 const assert = require('assert');
 const uuid = require('uuid');
 const bcrypt = require('bcryptjs');
-const url = require('url');
-const Wordlist = require('./wordlist');
 const errors = require('../errors');
 
 const {
@@ -22,9 +20,10 @@ const USER_ROLES = require('../models/enum/user_roles');
 const RECAPTCHA_WINDOW_SECONDS = 60 * 10; // 10 minutes.
 const RECAPTCHA_INCORRECT_TRIGGER = 5; // after 3 incorrect attempts, recaptcha will be required.
 
-const SettingsService = require('./settings');
 const ActionsService = require('./actions');
 const MailerService = require('./mailer');
+const Wordlist = require('./wordlist');
+const Domainlist = require('./domainlist');
 
 const EMAIL_CONFIRM_JWT_SUBJECT = 'email_confirm';
 const PASSWORD_RESET_JWT_SUBJECT = 'password_reset';
@@ -557,11 +556,10 @@ module.exports = class UsersService {
 
     email = email.toLowerCase();
 
-    const [user, settings] = await Promise.all([
+    const [user, domainValidated] = await Promise.all([
       UserModel.findOne({profiles: {$elemMatch: {id: email}}}),
-      SettingsService.retrieve(),
+      Domainlist.urlCheck(loc),
     ]);
-
     if (!user) {
 
       // Since we don't want to reveal that the email does/doesn't exist
@@ -569,19 +567,11 @@ module.exports = class UsersService {
       // endpoint.
       return;
     }
-    let redirectDomain;
-    try {
-      const {hostname, port} = url.parse(loc);
-      redirectDomain = hostname;
-      if (port) {
-        redirectDomain += `:${port}`;
-      }
-    } catch (e) {
-      throw new Error('redirect location is invalid');
-    }
 
-    if (settings.domains.whitelist.indexOf(redirectDomain) === -1) {
-      throw new Error('redirect location is not on the list of acceptable domains');
+    // If the domain didn't match any of the whitelisted domains and if it
+    // didn't match the mount domain, then throw an error.
+    if (!domainValidated && !Domainlist.matchMount(loc)) {
+      throw new Error('user supplied location exists on non-permitted domain');
     }
 
     const payload = {
@@ -619,16 +609,18 @@ module.exports = class UsersService {
    * Verifies a jwt and returns the associated user.
    * @param {String} token the JSON Web Token to verify
    */
-  static verifyPasswordResetToken(token) {
-    return UsersService
-      .verifyToken(token, {
-        subject: PASSWORD_RESET_JWT_SUBJECT
-      })
+  static async verifyPasswordResetToken(token) {
+    const {userId, loc, version} = await UsersService.verifyToken(token, {
+      subject: PASSWORD_RESET_JWT_SUBJECT
+    });
 
-      // TODO: add search by __v as well
-      .then((decoded) => {
-        return Promise.all([UsersService.findById(decoded.userId), decoded.loc]);
-      });
+    const user = await UsersService.findById(userId);
+
+    if (version !== user.__v) {
+      throw new Error('password reset token has expired');
+    }
+
+    return [user, loc];
   }
 
   /**

test/server/services/domainlist.js

@@ -26,13 +26,84 @@ describe('services.Domainlist', () => {
 
   });
 
+  describe('#parseURL', () => {
+    it('parses the domain correctly', () => {
+      [
+        ['http://google.ca/test', 'google.ca'],
+        ['http://google.ca:80/test', 'google.ca'],
+        ['https://google.ca/test', 'google.ca'],
+        ['https://google.ca:443/test', 'google.ca'],
+        ['//google.ca/test', 'google.ca'],
+        ['//google.ca:80/test', 'google.ca'],
+        ['//google.ca:443/test', 'google.ca'],
+        ['google.ca/test', 'google.ca'],
+        ['google.ca:80/test', 'google.ca'],
+        ['google.ca:443/test', 'google.ca'],
+        ['http://google.ca/', 'google.ca'],
+        ['http://google.ca:80/', 'google.ca'],
+        ['https://google.ca/', 'google.ca'],
+        ['https://google.ca:443/', 'google.ca'],
+        ['//google.ca/', 'google.ca'],
+        ['//google.ca:80/', 'google.ca'],
+        ['//google.ca:443/', 'google.ca'],
+        ['google.ca/', 'google.ca'],
+        ['google.ca:80/', 'google.ca'],
+        ['google.ca:443/', 'google.ca'],
+        ['google.ca', 'google.ca'],
+        ['http://google.ca', 'google.ca'],
+        ['http://google.ca:80', 'google.ca'],
+        ['https://google.ca', 'google.ca'],
+        ['https://google.ca:443', 'google.ca'],
+        ['//google.ca', 'google.ca'],
+        ['//google.ca:80', 'google.ca'],
+        ['//google.ca:443', 'google.ca'],
+        ['google.ca', 'google.ca'],
+        ['google.ca:80', 'google.ca'],
+        ['google.ca:443', 'google.ca'],
+        ['http://google.Ca/test', 'google.ca'],
+        ['http://google.ca:80/test', 'google.ca'],
+        ['https://google.Ca/test', 'google.ca'],
+        ['https://google.ca:443/test', 'google.ca'],
+        ['//google.Ca/test', 'google.ca'],
+        ['//google.Ca:80/test', 'google.ca'],
+        ['//google.Ca:443/test', 'google.ca'],
+        ['google.Ca/test', 'google.ca'],
+        ['google.ca:80/test', 'google.ca'],
+        ['google.ca:443/test', 'google.ca'],
+        ['http://Google.ca/', 'google.ca'],
+        ['http://google.Ca:80/', 'google.ca'],
+        ['https://Google.ca/', 'google.ca'],
+        ['https://google.Ca:443/', 'google.ca'],
+        ['//Google.ca/', 'google.ca'],
+        ['//google.Ca:80/', 'google.ca'],
+        ['//google.Ca:443/', 'google.ca'],
+        ['Google.ca/', 'google.ca'],
+        ['google.Ca:80/', 'google.ca'],
+        ['google.Ca:443/', 'google.ca'],
+        ['Google.ca', 'google.ca'],
+        ['http://Google.ca', 'google.ca'],
+        ['http://google.Ca:80', 'google.ca'],
+        ['https://Google.ca', 'google.ca'],
+        ['https://google.Ca:443', 'google.ca'],
+        ['//Google.ca', 'google.ca'],
+        ['//google.Ca:80', 'google.ca'],
+        ['//google.Ca:443', 'google.ca'],
+        ['Google.ca', 'google.ca'],
+        ['google.Ca:80', 'google.ca'],
+        ['google.Ca:443', 'google.ca'],
+      ].forEach(([domain, hostname]) => {
+        expect(Domainlist.parseURL(domain), `domain ${domain} should be parsed as ${hostname}`).to.equal(hostname);
+      });
+    });
+  });
+
   describe('#match', () => {
 
     const whiteList = Domainlist.parseList(domainlists['whitelist']);
 
     it('does match on an included domain', () => {
       [
-        'wapo.com',
+        'http://wapo.com',
         'nytimes.com'
       ].forEach((domain) => {
         expect(domainlist.match(whiteList, domain)).to.be.true;

views/admin/password-reset.ejs

@@ -15,11 +15,13 @@
         background: #fff;
       }
 
-      #root form {
+      .container {
         max-width: 300px;
-        border: 1px solid lightgrey;
-        box-shadow: 0px 10px 24px 2px rgba(0,0,0,0.2);
         margin: 50px auto;
+      }
+
+      #root form {
+        display: none;
         padding: 15px;
       }
 
@@ -81,7 +83,8 @@
   </head>
   <body>
     <div id="root">
-      <form id="reset-password-form">
+      <div class="error-console container"></div>
+      <form id="reset-password-form" class="container">
         <legend class="legend">Set new password</legend>
         <label for="password">
           New password
@@ -94,14 +97,18 @@
           <input type="password" name="confirm-password" placeholder="confirm password" />
         </label>
         <button class="submit-password-reset" type="submit">Apply</button>
-        <div class="error-console">foo</div>
       </form>
     </div>
     <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
     <script>
       $(function () {
-        function showError(message) {
-          $('.error-console').text(message).addClass('active');
+        function showError(error) {
+          try {
+            var err = JSON.parse(error);
+            $('.error-console').text(err.message).addClass('active');
+          } catch (err) {
+            $('.error-console').text(error).addClass('active');
+          }
         }
 
         function handleSubmit (e) {
@@ -111,8 +118,13 @@
           var password = $('[name="password"]').val();
           var confirm = $('[name="confirm-password"]').val();
 
-          if (password !== confirm || password === '' || password.length < 8) {
-            showError('passwords must match and be 8 characters.');
+          if (password === '' || password.length < 8) {
+            showError('Passwords must be at least 8 characters.');
+            return false;
+          }
+
+          if (password !== confirm) {
+            showError('New password and confirm password must match');
             return false;
           }
 
@@ -128,7 +140,17 @@
           });
         }
 
-        $('#reset-password-form').on('submit', handleSubmit);
+
+        $.ajax({
+          url: '<%= BASE_PATH %>api/v1/account/password/reset?check=true',
+          contentType: 'application/json',
+          method: 'PUT',
+          data: JSON.stringify({token: location.hash.replace('#', '')})
+        }).then(function () {
+          $('#reset-password-form').fadeIn().on('submit', handleSubmit);
+        }).catch(function (error) {
+          showError(error.responseText);
+        });
       });
     </script>
   </body>