Merge pull request #1601 from mozilla/issue1592

close #1592

Issue1592

lib/db/json.js

@@ -236,7 +236,7 @@ exports.emailForVerificationSecret = function(secret, cb) {
   process.nextTick(function() {
     sync();
     if (!db.staged[secret]) return cb("no such secret");
-    cb(null, db.staged[secret].email, db.staged[secret].existing_user);
+    cb(null, db.staged[secret].email, db.staged[secret].existing_user, db.staged[secret].passwd);
   });
 };
 

lib/db/mysql.js

@@ -265,14 +265,14 @@ exports.haveVerificationSecret = function(secret, cb) {
 
 exports.emailForVerificationSecret = function(secret, cb) {
   client.query(
-    "SELECT email, existing_user FROM staged WHERE secret = ?", [ secret ],
+    "SELECT email, existing_user, passwd FROM staged WHERE secret = ?", [ secret ],
     function(err, rows) {
       if (err) return cb("database unavailable");
 
       // if the record was not found, fail out
       if (!rows || rows.length != 1) return cb("no such secret");
 
-      cb(null, rows[0].email, rows[0].existing_user);
+      cb(null, rows[0].email, rows[0].existing_user, rows[0].passwd);
     });
 };
 
@@ -291,7 +291,7 @@ exports.authForVerificationSecret = function(secret, cb) {
       if (o.passwd) return cb(null, o.passwd, o.existing_user);
 
       // otherwise, let's get the passwd from the user record
-      if (!o.existing_user) cb("no password for user");
+      if (!o.existing_user) return cb("no password for user");
 
       exports.checkAuth(o.existing_user, function(err, hash) {
         cb(err, hash, o.existing_user);
@@ -337,8 +337,9 @@ exports.gotVerificationSecret = function(secret, cb) {
       if (err) {
         logUnexpectedError(err);
         cb(err);
-      } else if (rows.length === 0) cb("unknown secret");
-      else {
+      } else if (rows.length === 0) {
+        cb("unknown secret");
+      } else {
         var o = rows[0];
 
         // delete the record

lib/wsapi/complete_email_addition.js

@@ -21,13 +21,40 @@ exports.process = function(req, res) {
   //
   // 1. you must already be authenticated as the user who initiated the verification
   // 2. you must provide the password of the initiator.
-  //
+
+  // TRANSITIONAL CODE COMMENT
+  // for issue 1000 we moved initial password selection to the browserid dialog (from
+  // the verification page).  Rolling out this change causes some temporal pain.
+  // Outstannding verification links sent before the change was deployed will have
+  // email addition requests that require passwords without passwords in the stage table.
+  // When the verification page is loaded for
+  // these links, we prompt the user for a password.  That password is sent up with
+  // the request.  this code and comment should all be purged after the new code
+  // has been in production for 2 weeks.
+
+  var transitionalPassword = null;
+
+  // END TRANSITIONAL CODE COMMENT
+
+
   db.authForVerificationSecret(req.body.token, function(err, initiator_hash, initiator_uid) {
     if (err) {
       logger.info("unknown verification secret: " + err);
       return wsapi.databaseDown(res, err);
     }
 
+    // TRANSITIONAL CODE
+    if (!initiator_hash) {
+      if (!req.body.pass) return httputils.authRequired(res, "password required");
+      var err = wsapi.checkPassword(req.body.pass);
+      if (err) {
+        logger.warn("invalid password received: " + err);
+        return httputils.badRequest(res, err);
+      }
+      transitionalPassword = req.body.pass;
+      postAuthentication();
+    } else
+    // END TRANSITIONAL CODE
     if (req.session.userid === initiator_uid) {
       postAuthentication();
     } else if (typeof req.body.pass === 'string') {
@@ -53,6 +80,23 @@ exports.process = function(req, res) {
         } else {
           wsapi.authenticateSession(req.session, uid, 'password');
           res.json({ success: true });
+
+          // TRANSITIONAL CODE
+          if (transitionalPassword) {
+            wsapi.bcryptPassword(transitionalPassword, function(err, hash) {
+              if (err) {
+                logger.warn("couldn't bcrypt pass for old verification link: " + err);
+                return;
+              }
+
+              db.updatePassword(uid, hash, function(err) {
+                if (err) {
+                  logger.warn("couldn't bcrypt pass for old verification link: " + err);
+                }
+              });
+            });
+          }
+          // END TRANSITIONAL CODE
         }
       });
     };

lib/wsapi/complete_user_creation.js

@@ -28,18 +28,36 @@ exports.process = function(req, res) {
   // and then control a browserid account that they can use to prove they own
   // the email address of the attacked.
 
+  // TRANSITIONAL CODE COMMENT
+  // for issue 1000 we moved initial password selection to the browserid dialog (from
+  // the verification page).  Rolling out this change causes some temporal pain.
+  // Outstannding verification links sent before the change was deployed will have
+  // new user requests without passwords.  When the verification page is loaded for
+  // these links, we prompt the user for a password.  That password is sent up with
+  // the request.  this code and comment should all be purged after the new code
+  // has been in production for 2 weeks.
+  // END TRANSITIONAL CODE COMMENT
+
   // is this the same browser?
   if (typeof req.session.pendingCreation === 'string' &&
       req.body.token === req.session.pendingCreation) {
-    postAuthentication();
+    return postAuthentication();
   }
   // is a password provided?
   else if (typeof req.body.pass === 'string') {
     return db.authForVerificationSecret(req.body.token, function(err, hash) {
+      // TRANSITIONAL CODE
+      // if hash is null, no password was provided during verification and
+      // this is an old-style verification.  We accept the password and will
+      // update it after the verification is complete.
+      if (err == 'no password for user' || !hash) return postAuthentication();
+      // END TRANSITIONAL CODE
+
       if (err) {
         logger.warn("couldn't get password for verification secret: " + err);
         return wsapi.databaseDown(res, err);
       }
+
       bcrypt.compare(req.body.pass, hash, function (err, success) {
         if (err) {
           logger.warn("max load hit, failing on auth request with 503: " + err);
@@ -47,7 +65,7 @@ exports.process = function(req, res) {
         } else if (!success) {
           return httputils.authRequired(res, "password mismatch");
         } else {
-          postAuthentication();
+          return postAuthentication();
         }
       });
     });
@@ -65,19 +83,58 @@ exports.process = function(req, res) {
 
       if (!known) return res.json({ success: false} );
 
-      db.gotVerificationSecret(req.body.token, function(err, email, uid) {
-        if (err) {
-          logger.warn("couldn't complete email verification: " + err);
-          wsapi.databaseDown(res, err);
-        } else {
-          // FIXME: not sure if we want to do this (ba)
-          // at this point the user has set a password associated with an email address
-          // that they've verified.  We create an authenticated session.
-          wsapi.authenticateSession(req.session, uid, 'password',
-                                    config.get('ephemeral_session_duration_ms'));
-          res.json({ success: true });
+      // TRANSITIONAL CODE
+      // user is authorized (1 or 2 above) OR user has no password set, in which
+      // case for a short time we'll accept the password provided with the verification
+      // link, and set it as theirs.
+      var transitionalPassword = null;
+
+      db.authForVerificationSecret(req.body.token, function(err, hash) {
+        if (err == 'no password for user' || !hash) {
+          if (!req.body.pass) return httputils.authRequired(res, "password required");
+          err = wsapi.checkPassword(req.body.pass);
+          if (err) {
+            logger.warn("invalid password received: " + err);
+            return httputils.badRequest(res, err);
+          }
+          transitionalPassword = req.body.pass;
         }
+        completeCreation();
       });
+      // END TRANSITIONAL CODE
+
+      function completeCreation() {
+        db.gotVerificationSecret(req.body.token, function(err, email, uid) {
+          if (err) {
+            logger.warn("couldn't complete email verification: " + err);
+            wsapi.databaseDown(res, err);
+          } else {
+            // FIXME: not sure if we want to do this (ba)
+            // at this point the user has set a password associated with an email address
+            // that they've verified.  We create an authenticated session.
+            wsapi.authenticateSession(req.session, uid, 'password',
+                                      config.get('ephemeral_session_duration_ms'));
+            res.json({ success: true });
+
+            // TRANSITIONAL CODE
+            if (transitionalPassword) {
+              wsapi.bcryptPassword(transitionalPassword, function(err, hash) {
+                if (err) {
+                  logger.warn("couldn't bcrypt pass for old verification link: " + err);
+                  return;
+                }
+
+                db.updatePassword(uid, hash, function(err) {
+                  if (err) {
+                    logger.warn("couldn't bcrypt pass for old verification link: " + err);
+                  }
+                });
+              });
+            }
+            // END TRANSITIONAL CODE
+          }
+        });
+      }
     });
   }
 };

lib/wsapi/email_for_token.js

@@ -19,7 +19,7 @@ exports.args = ['token'];
 exports.i18n = false;
 
 exports.process = function(req, res) {
-  db.emailForVerificationSecret(req.query.token, function(err, email, uid) {
+  db.emailForVerificationSecret(req.query.token, function(err, email, uid, hash) {
     if (err) {
       if (err === 'database unavailable') {
         httputils.serviceUnavailable(res, err);
@@ -30,24 +30,64 @@ exports.process = function(req, res) {
         });
       }
     } else {
-      // must the user authenticate?  This is true if they are not authenticated
-      // as the uid who initiated the verification, and they are not on the same
-      // browser as the initiator
-      var must_auth = true;
+      function checkMustAuth() {
+        // must the user authenticate?  This is true if they are not authenticated
+        // as the uid who initiated the verification, and they are not on the same
+        // browser as the initiator
+        var must_auth = true;
 
-      if (uid && req.session.userid === uid) {
-        must_auth = false;
+        if (uid && req.session.userid === uid) {
+          must_auth = false;
+        }
+        else if (!uid && typeof req.session.pendingCreation === 'string' &&
+                 req.query.token === req.session.pendingCreation) {
+          must_auth = false;
+        }
+
+        res.json({
+          success: true,
+          email: email,
+          must_auth: must_auth
+        });
+      }
+
+      // backwards compatibility - issue #1592
+      // if there is no password in the user record, and no password in the staged
+      // table, then we require a password be fetched from the user upon verification.
+      // these checks are temporary and should disappear in 1 trains time.
+      function needsPassword() {
+        // no password is set neither in the user table nor in the staged record.
+        // the user must pick a password
+        res.json({
+          success: true,
+          email: email,
+          needs_password: true
+        });
       }
-      else if (!uid && typeof req.session.pendingCreation === 'string' &&
-               req.query.token === req.session.pendingCreation) {
-        must_auth = false;
+
+      if (!hash) {
+        if (!uid) {
+          needsPassword();
+        } else {
+          db.checkAuth(uid, function(err, hash) {
+            if (err) {
+              return res.json({
+                success: false,
+                reason: err
+              });
+            }
+
+            if (!hash) {
+              needsPassword();
+            } else {
+              checkMustAuth();
+            }
+          });
+        }
+      } else {
+        checkMustAuth();
       }
 
-      res.json({
-        success: true,
-        email: email,
-        must_auth: must_auth
-      });
     }
   });
 };

resources/static/css/style.css

@@ -669,7 +669,7 @@ h1 {
   margin-bottom: 10px;
 }
 
-.siteinfo, #congrats, .password_entry, .enter_password .hint, #unknown_secondary, #primary_verify, .verify_primary .submit {
+.siteinfo, #congrats, .password_entry, #verify_password, .enter_password .hint, #unknown_secondary, #primary_verify, .verify_primary .submit {
   display: none;
 }
 
@@ -677,7 +677,7 @@ h1 {
   float: left;
 }
 
-.enter_password .password_entry, .known_secondary .password_entry,
+.enter_password .password_entry, .enter_verify_password #verify_password, .known_secondary .password_entry,
 .unknown_secondary #unknown_secondary, .verify_primary #verify_primary {
   display: block;
 }

resources/static/pages/verify_secondary_address.js

@@ -17,6 +17,7 @@ BrowserID.verifySecondaryAddress = (function() {
       validation = bid.Validation,
       token,
       sc,
+      needsPassword,
       mustAuth,
       verifyFunction;
 
@@ -36,7 +37,11 @@ BrowserID.verifySecondaryAddress = (function() {
 
   function submit(oncomplete) {
     var pass = dom.getInner("#password") || undefined,
-        valid = !mustAuth || validation.password(pass);
+        vpass = dom.getInner("#vpassword") || undefined,
+        valid = (!needsPassword ||
+                    validation.passwordAndValidationPassword(pass, vpass))
+             && (!mustAuth ||
+                    validation.password(pass));
 
     if (valid) {
       user[verifyFunction](token, pass, function(info) {
@@ -56,13 +61,25 @@ BrowserID.verifySecondaryAddress = (function() {
       if(info) {
         showRegistrationInfo(info);
 
+        needsPassword = info.needs_password;
         mustAuth = info.must_auth;
 
-        if (mustAuth) {
+        if (needsPassword) {
+          // This is a fix for legacy users who started the user creation
+          // process without setting their password in the dialog.  If the user
+          // needs a password, they must set it now.  Once all legacy users are
+          // verified or their links invalidated, this flow can be removed.
+          dom.addClass("body", "enter_password");
+          dom.addClass("body", "enter_verify_password");
+          complete(oncomplete, true);
+        }
+        else if (mustAuth) {
+          // These are users who have set their passwords inside of the dialog.
           dom.addClass("body", "enter_password");
           complete(oncomplete, true);
         }
         else {
+          // These are users who do not have to set their passwords at all.
           submit(oncomplete);
         }
       }

resources/static/test/cases/pages/verify_secondary_address.js

@@ -116,6 +116,7 @@
     xhr.useResult("mustAuth");
     createController(config, function() {
       xhr.useResult("valid");
+      testHasClass("body", "enter_password");
       controller.submit(function(status) {
         equal(status, true, "correct status");
         testHasClass("body", "complete");
@@ -134,4 +135,88 @@
     });
   });
 
+  asyncTest("must set password, successful login", function() {
+    xhr.useResult("needsPassword");
+    createController(config, function() {
+      xhr.useResult("valid");
+
+      $("#password").val("password");
+      $("#vpassword").val("password");
+
+      testHasClass("body", "enter_password");
+      testHasClass("body", "enter_verify_password");
+
+      controller.submit(function(status) {
+        equal(status, true, "correct status");
+        testHasClass("body", "complete");
+        start();
+      });
+    });
+  });
+
+  asyncTest("must set password, too short a password", function() {
+    xhr.useResult("needsPassword");
+    createController(config, function() {
+      xhr.useResult("valid");
+
+      $("#password").val("pass");
+      $("#vpassword").val("pass");
+
+      controller.submit(function(status) {
+        equal(status, false, "correct status");
+        testHelpers.testTooltipVisible();
+        start();
+      });
+    });
+  });
+
+  asyncTest("must set password, too long a password", function() {
+    xhr.useResult("needsPassword");
+    createController(config, function() {
+      xhr.useResult("valid");
+
+      var pass = testHelpers.generateString(81);
+      $("#password").val(pass);
+      $("#vpassword").val(pass);
+
+      controller.submit(function(status) {
+        equal(status, false, "correct status");
+        testHelpers.testTooltipVisible();
+        start();
+      });
+    });
+  });
+
+  asyncTest("must set password, missing verification password", function() {
+    xhr.useResult("needsPassword");
+    createController(config, function() {
+      xhr.useResult("valid");
+
+      $("#password").val("password");
+      $("#vpassword").val("");
+
+      controller.submit(function(status) {
+        equal(status, false, "correct status");
+        testHelpers.testTooltipVisible();
+        start();
+      });
+    });
+  });
+
+  asyncTest("must set password, mismatched passwords", function() {
+    xhr.useResult("needsPassword");
+    createController(config, function() {
+      xhr.useResult("valid");
+
+      $("#password").val("password");
+      $("#vpassword").val("password1");
+
+      controller.submit(function(status) {
+        equal(status, false, "correct status");
+        testHelpers.testTooltipVisible();
+        start();
+      });
+    });
+  });
+
 }());

resources/static/test/mocks/xhr.js

@@ -34,6 +34,7 @@ BrowserID.Mocks.xhr = (function() {
       "get /wsapi/session_context contextAjaxError": undefined,
       "get /wsapi/email_for_token?token=token valid": { email: "testuser@testuser.com" },
       "get /wsapi/email_for_token?token=token mustAuth": { email: "testuser@testuser.com", must_auth: true },
+      "get /wsapi/email_for_token?token=token needsPassword": { email: "testuser@testuser.com", needs_password: true },
       "get /wsapi/email_for_token?token=token invalid": { success: false },
       "post /wsapi/authenticate_user valid": { success: true, userid: 1 },
       "post /wsapi/authenticate_user invalid": { success: false },

resources/views/add_email_address.ejs

@@ -31,6 +31,20 @@
                       <%= gettext('Password must be between 8 and 80 characters long.') %>
                     </div>
                 </li>
+
+                <li class="password_entry" id="verify_password">
+                    <label class="serif" for="vpassword"><%= gettext('Verify Password') %></label>
+                    <input class="sans" id="vpassword" placeholder="<%= gettext('Repeat Password') %>" type="password" maxlength="80">
+
+                    <div id="vpassword_required" class="tooltip" for="vpassword">
+                      <%= gettext('Verification password is required.') %>
+                    </div>
+
+                    <div class="tooltip" id="passwords_no_match" for="vpassword">
+                      <%= gettext ('Passwords do not match.') %>
+                    </div>
+
+                </li>
             </ul>
 
             <div class="submit cf password_entry">

resources/views/verify_email_address.ejs

@@ -18,6 +18,7 @@
                     <label class="serif" for="email"><%= gettext('Email Address') %></label>
                     <input class="youraddress sans" id="email" placeholder="<%= gettext('Your Email') %>" type="email" value="" disabled="disabled" maxlength="254" />
                 </li>
+
                 <li class="password_entry">
                     <label class="serif" for="password"><%= gettext('Password') %></label>
                     <input class="sans" id="password" placeholder="<%= gettext('Your Password') %>" type="password" autofocus maxlength=80 />
@@ -30,6 +31,20 @@
                       <%= gettext('Password must be between 8 and 80 characters long.') %>
                     </div>
                 </li>
+
+                <li class="password_entry" id="verify_password">
+                    <label class="serif" for="vpassword"><%= gettext('Verify Password') %></label>
+                    <input class="sans" id="vpassword" placeholder="<%= gettext('Repeat Password') %>" type="password" maxlength="80">
+
+                    <div id="vpassword_required" class="tooltip" for="vpassword">
+                      <%= gettext('Verification password is required.') %>
+                    </div>
+
+                    <div class="tooltip" id="passwords_no_match" for="vpassword">
+                      <%= gettext ('Passwords do not match.') %>
+                    </div>
+
+                </li>
             </ul>
 
             <div class="submit cf password_entry">