Nick code review items (mostly comments)

packages/accounts-password/password_client.js

@@ -1,16 +1,3 @@
-// The server requested an upgrade from the old SRP password format,
-// so supply the needed SRP identity to login.
-var srpUpgradePath = function (selector, password, identity, callback) {
-  Accounts.callLoginMethod({
-    methodArguments: [{
-      user: selector,
-      srp: SHA256(identity + ":" + password),
-      password: SHA256(password)
-    }],
-    userCallback: callback
-  });
-};
-
 // Attempt to log in with a password.
 //
 // @param selector {String|Object} One of the following:
@@ -30,11 +17,23 @@ Meteor.loginWithPassword = function (selector, password, callback) {
   Accounts.callLoginMethod({
     methodArguments: [{
       user: selector,
-      password: SHA256(password),
+      password: SHA256(password)
     }],
     userCallback: function (error, result) {
       if (error && error.error === 400 &&
           error.reason === 'old password format') {
+        // The "reason" string should match the error thrown in the
+        // password login handler in password_server.js.
+
+        // XXX COMPAT WITH 0.8.1.3
+        // If this user's last login was with a previous version of
+        // Meteor that used SRP, then the server throws this error to
+        // indicate that we should try again. The error includes the
+        // user's SRP identity. We provide a value derived from the
+        // identity and the password to prove to the server that we know
+        // the password without requiring a full SRP flow, as well as
+        // SHA256(password), which the server bcrypts and stores in
+        // place of the old SRP information for this user.
         var details;
         try {
           details = EJSON.parse(error.details);
@@ -53,6 +52,20 @@ Meteor.loginWithPassword = function (selector, password, callback) {
   });
 };
 
+// The server requested an upgrade from the old SRP password format,
+// so supply the needed SRP identity to login.
+var srpUpgradePath = function (selector, plaintextPassword,
+                               identity, callback) {
+  Accounts.callLoginMethod({
+    methodArguments: [{
+      user: selector,
+      srp: SHA256(identity + ":" + plaintextPassword),
+      password: SHA256(plaintextPassword)
+    }],
+    userCallback: callback
+  });
+};
+
 
 // Attempt to log in as a new user.
 Accounts.createUser = function (options, callback) {

packages/accounts-password/password_server.js

@@ -4,8 +4,8 @@ var bcrypt = Npm.require('bcrypt');
 var bcryptHash = Meteor._wrapAsync(bcrypt.hash);
 var bcryptCompare = Meteor._wrapAsync(bcrypt.compare);
 
-// Use bcrypt to hash the password (which was already hashed on the
-// client) for storage in the database.
+// Use bcrypt to hash the password (which was already hashed with SHA256
+// on the client) for storage in the database.
 //
 var hashPassword = function (password) {
   return bcryptHash(password, 10 /* number of rounds */);
@@ -115,7 +115,21 @@ Accounts.registerLoginHandler("password", function (options) {
   );
 });
 
-// Handler to login using the SRP upgrade path.
+// Handler to login using the SRP upgrade path. To use this login
+// handler, the client must provide:
+//   - srp: H(identity + ":" + password)
+//   - plaintextPassword or password (which is the SHA256 of the password)
+//
+// We use `options.srp` to verify that the client knows the correct
+// password without doing a full SRP flow. Once we've checked that, we
+// upgrade the user to bcrypt and remove the SRP information from the
+// user document.
+//
+// The client ends up using this login handler after trying the normal
+// login handler (above), which throws an error telling the client to
+// try the SRP upgrade path.
+//
+// XXX COMPAT WITH 0.8.1.3
 Accounts.registerLoginHandler("password", function (options) {
   if (!options.srp || !(options.password || options.plaintextPassword))
     return undefined; // don't handle
@@ -217,7 +231,7 @@ Accounts.setPassword = function (userId, newPlaintextPassword) {
 
   Meteor.users.update(
     {_id: user._id},
-    { $unset: {'services.password.srp': 1},
+    { $unset: {'services.password.srp': 1}, // XXX COMPAT WITH 0.8.1.3
       $set: {'services.password.bcrypt': hashPassword(SHA256(newPlaintextPassword))} }
   );
 };

packages/accounts-password/password_tests.js

@@ -736,29 +736,36 @@ if (Meteor.isClient) (function () {
     logoutStep,
     // Create user with old SRP credentials in the database.
     function (test, expect) {
-      Meteor.call("testCreateSRPUser", expect(function (error) {
+      var self = this;
+      Meteor.call("testCreateSRPUser", expect(function (error, result) {
         test.isFalse(error);
+        self.username = result;
       }));
     },
     // We are able to login with the old style credentials in the database.
     function (test, expect) {
-      Meteor.loginWithPassword('srptestuser', 'abcdef', expect(function (error) {
+      Meteor.loginWithPassword(this.username, 'abcdef', expect(function (error) {
         test.isFalse(error);
       }));
     },
     function (test, expect) {
-      Meteor.call("testSRPUpgrade", expect(function (error) {
+      Meteor.call("testSRPUpgrade", this.username, expect(function (error) {
         test.isFalse(error);
       }));
     },
     logoutStep,
     // After the upgrade to bcrypt we're still able to login.
     function (test, expect) {
-      Meteor.loginWithPassword('srptestuser', 'abcdef', expect(function (error) {
+      Meteor.loginWithPassword(this.username, 'abcdef', expect(function (error) {
         test.isFalse(error);
       }));
     },
-    logoutStep
+    logoutStep,
+    function (test, expect) {
+      Meteor.call("removeUser", this.username, expect(function (error) {
+        test.isFalse(error);
+      }));
+    }
   ]);
 }) ();
 

packages/accounts-password/password_tests_setup.js

@@ -121,8 +121,9 @@ Meteor.methods({
 
 Meteor.methods({
   testCreateSRPUser: function () {
-    Meteor.users.remove({username: 'srptestuser'});
-    var userId = Accounts.createUser({username: 'srptestuser'});
+    var username = Random.id();
+    Meteor.users.remove({username: username});
+    var userId = Accounts.createUser({username: username});
     Meteor.users.update(
       userId,
       { '$set': { 'services.password.srp': {
@@ -131,10 +132,11 @@ Meteor.methods({
           "verifier" : "2e8bce266b1357edf6952cc56d979db19f699ced97edfb2854b95972f820b0c7006c1a18e98aad40edf3fe111b87c52ef7dd06b320ce452d01376df2d560fdc4d8e74f7a97bca1f67b3cfaef34dee34dd6c76571c247d762624dc166dab5499da06bc9358528efa75bf74e2e7f5a80d09e60acf8856069ae5cfb080f2239ee76"
       } } }
     );
+    return username;
   },
 
-  testSRPUpgrade: function () {
-    var user = Meteor.users.findOne({username: 'srptestuser'});
+  testSRPUpgrade: function (username) {
+    var user = Meteor.users.findOne({username: username});
     if (user.services && user.services.password && user.services.password.srp)
       throw new Error("srp wasn't removed");
     if (!(user.services && user.services.password && user.services.password.bcrypt))

packages/srp/package.js

@@ -1,3 +1,8 @@
+// XXX COMPAT WITH 0.8.1.3
+// This package is replaced by the use of bcrypt in accounts-password,
+// but we are leaving in some of the code to allow existing user
+// databases to be upgraded from SRP to bcrypt.
+
 Package.describe({
   summary: "Library for Secure Remote Password (SRP) exchanges",
   internal: true
@@ -10,3 +15,10 @@ Package.on_use(function (api) {
   api.add_files(['biginteger.js', 'srp.js'],
                 ['client', 'server']);
 });
+
+Package.on_test(function (api) {
+  api.use('tinytest');
+  api.use('srp', ['client', 'server']);
+  api.use('underscore');
+  api.add_files(['srp_tests.js'], ['client', 'server']);
+});

packages/srp/srp.js

@@ -1,5 +1,9 @@
 // This package contains just enough of the original SRP code to
 // support the backwards-compatibility upgrade path.
+//
+// An SRP (and possibly also accounts-srp) package should eventually be
+// available in Atmosphere so that users can continue to use SRP if they
+// want to.
 
 SRP = {};
 

packages/srp/srp_tests.js

@@ -0,0 +1,19 @@
+Tinytest.add("srp - fixed values", function(test) {
+  // Test exact values outputted by `generateVerifier`. We have to be very
+  // careful about changing the SRP code, because changes could render
+  // people's existing user database unusable. This test is
+  // intentionally brittle to catch change that could affect the
+  // validity of user passwords.
+
+  var identity = "b73d9af9-4e74-4ce0-879c-484828b08436";
+  var salt = "85f8b9d3-744a-487d-8982-a50e4c9f552a";
+  var password = "95109251-3d8a-4777-bdec-44ffe8d86dfb";
+  var a = "dc99c646fa4cb7c24314bb6f4ca2d391297acd0dacb0430a13bbf1e37dcf8071";
+  var b = "cf878e00c9f2b6aa48a10f66df9706e64fef2ca399f396d65f5b0a27cb8ae237";
+
+  var verifier = SRP.generateVerifier(
+    password, {identity: identity, salt: salt});
+  test.equal(verifier.identity, identity);
+  test.equal(verifier.salt, salt);
+  test.equal(verifier.verifier, "56778b720d20b2e306f04e47180fb94335b88a6052808483acb0e85612606f9f1d8d5a3c6b85e0c7bfec7f08c07bdfbd0d40b032f517871dd8afd045b0f24e2edc05ccdc47b19f35d2eb9f7670521a38c1b358fcee63f052a1aedbb1282d3b92c7a554f8523f3379c2fbc6885be8227fbd426ad6960c3839809f8c94d80a6c51");
+});