Fix issue #4862. Don't wrap login callbacks with bindEnvironment.

Changes to packages/callback-hook:
- Add bindEnvironment option to Hook constructor (defaults to true).
- Add internal helper function dontBindEnvironment() which does all the
  error handling stuff like Meteor.bindEnvironment() but doesn't change
  the environment.
- Use dontBindEnvironment() instead of Meteor.bindEnvironment() when
  bindEnvironment option is false.
- Add tests.

Changes to packages/accounts-base:
- Create hooks with { bindEnvironment: false }
- Add test for whether Meteor.userId() is available in login callbacks.

packages/accounts-base/accounts_common.js

@@ -22,10 +22,12 @@ AccountsCommon = function _AccountsCommon(options) {
 
   // Callback exceptions are printed with Meteor._debug and ignored.
   this._onLoginHook = new Hook({
+    bindEnvironment: false,
     debugPrintExceptions: "onLogin callback"
   });
 
   this._onLoginFailureHook = new Hook({
+    bindEnvironment: false,
     debugPrintExceptions: "onLoginFailure callback"
   });
 };

packages/accounts-base/accounts_server.js

@@ -45,7 +45,7 @@ AccountsServer = function AccountsServer(server) {
   setupDefaultLoginHandlers(this);
   setExpireTokensInterval(this);
 
-  this._validateLoginHook = new Hook;
+  this._validateLoginHook = new Hook({ bindEnvironment: false });
   this._validateNewUserHooks = [
     defaultValidateNewUserHook.bind(this)
   ];

packages/accounts-base/accounts_tests.js

@@ -373,3 +373,44 @@ Tinytest.addAsync(
     );
   }
 );
+
+Tinytest.add(
+  'accounts - hook callbacks can access Meteor.userId()',
+  function (test) {
+    var userId = Accounts.insertUserDoc({}, { username: Random.id() });
+    var stampedToken = Accounts._generateStampedLoginToken();
+    Accounts._insertLoginToken(userId, stampedToken);
+
+    var validateStopper = Accounts.validateLoginAttempt(function(attempt) {
+      test.equal(Meteor.userId(), validateAttemptExpectedUserId, "validateLoginAttempt");
+      return true;
+    });
+    var onLoginStopper = Accounts.onLogin(function(attempt) {
+      test.equal(Meteor.userId(), onLoginExpectedUserId, "onLogin");
+    });
+    var onLoginFailureStopper = Accounts.onLoginFailure(function(attempt) {
+      test.equal(Meteor.userId(), onLoginFailureExpectedUserId, "onLoginFailure");
+    });
+
+    var conn = DDP.connect(Meteor.absoluteUrl());
+
+    // On a new connection, Meteor.userId() should be null until logged in.
+    var validateAttemptExpectedUserId = null;
+    var onLoginExpectedUserId = userId;
+    conn.call('login', { resume: stampedToken.token });
+
+    // Now that the user is logged in on the connection, Meteor.userId() should
+    // return that user.
+    validateAttemptExpectedUserId = userId;
+    conn.call('login', { resume: stampedToken.token });
+
+    // Trigger onLoginFailure callbacks
+    var onLoginFailureExpectedUserId = userId;
+    test.throws(function() { conn.call('login', { resume: "bogus" }) }, '403');
+
+    conn.disconnect();
+    validateStopper.stop();
+    onLoginStopper.stop();
+    onLoginFailureStopper.stop();
+  }
+);

packages/callback-hook/hook.js

@@ -9,9 +9,10 @@
 // conditionally decide not to call the callback (if, for example, the
 // observed object has been closed or terminated).
 //
-// Callbacks are bound with `Meteor.bindEnvironment`, so they will be
+// By default, callbacks are bound with `Meteor.bindEnvironment`, so they will be
 // called with the Meteor environment of the calling code that
-// registered the callback.
+// registered the callback. Override by passing { bindEnvironment: false }
+// to the constructor.
 //
 // Registering a callback returns an object with a single `stop`
 // method which unregisters the callback.
@@ -40,6 +41,10 @@ Hook = function (options) {
   options = options || {};
   self.nextCallbackId = 0;
   self.callbacks = {};
+  // Whether to wrap callbacks with Meteor.bindEnvironment
+  self.bindEnvironment = true;
+  if (options.bindEnvironment === false)
+    self.bindEnvironment = false;
 
   if (options.exceptionHandler)
     self.exceptionHandler = options.exceptionHandler;
@@ -53,16 +58,18 @@ Hook = function (options) {
 _.extend(Hook.prototype, {
   register: function (callback) {
     var self = this;
+    var exceptionHandler =  self.exceptionHandler || function (exception) {
+      // Note: this relies on the undocumented fact that if bindEnvironment's
+      // onException throws, and you are invoking the callback either in the
+      // browser or from within a Fiber in Node, the exception is propagated.
+      throw exception;
+    };
 
-    callback = Meteor.bindEnvironment(
-      callback,
-      self.exceptionHandler || function (exception) {
-        // Note: this relies on the undocumented fact that if bindEnvironment's
-        // onException throws, and you are invoking the callback either in the
-        // browser or from within a Fiber in Node, the exception is propagated.
-        throw exception;
-      }
-    );
+    if (self.bindEnvironment) {
+      callback = Meteor.bindEnvironment(callback, exceptionHandler);
+    } else {
+      callback = dontBindEnvironment(callback, exceptionHandler);
+    }
 
     var id = self.nextCallbackId++;
     self.callbacks[id] = callback;
@@ -105,3 +112,31 @@ _.extend(Hook.prototype, {
     }
   }
 });
+
+// Copied from Meteor.bindEnvironment and removed all the env stuff.
+var dontBindEnvironment = function (func, onException, _this) {
+  if (!onException || typeof(onException) === 'string') {
+    var description = onException || "callback of async function";
+    onException = function (error) {
+      Meteor._debug(
+        "Exception in " + description + ":",
+        error && error.stack || error
+      );
+    };
+  }
+
+  return function (/* arguments */) {
+    var args = _.toArray(arguments);
+
+    var runAndHandleExceptions = function () {
+      try {
+        var ret = func.apply(_this, args);
+      } catch (e) {
+        onException(e);
+      }
+      return ret;
+    };
+
+    return runAndHandleExceptions();
+  };
+};

packages/callback-hook/hook_tests.js

@@ -0,0 +1,55 @@
+Tinytest.add("callback-hook - binds to registrar's env by default", function (test) {
+  var hook = new Hook();
+  var envVar = new Meteor.EnvironmentVariable;
+  envVar.withValue("registrar's value", function() {
+    hook.register(function() {
+      test.equal(envVar.get(), "registrar's value");
+    });
+  });
+  envVar.withValue("invoker's value", function() {
+    hook.each(function(callback) {
+      callback();
+    });
+  });
+});
+
+Tinytest.add("callback-hook - uses invoker's env with {bindEnvironment: false}", function (test) {
+  var hook = new Hook({ bindEnvironment: false });
+  var envVar = new Meteor.EnvironmentVariable;
+  envVar.withValue("registrar's value", function() {
+    hook.register(function() {
+      test.equal(envVar.get(), "invoker's value");
+    });
+  });
+  envVar.withValue("invoker's value", function() {
+    hook.each(function(callback) {
+      callback();
+    });
+  });
+});
+
+Tinytest.add("callback-hook - exceptions unhandled with {bindEnvironment: false}", function (test) {
+  var hook = new Hook({ bindEnvironment: false });
+  hook.register(function() {
+    throw new Error("Test error");
+  });
+  hook.each(function(callback) {
+    test.throws(callback, "Test error");
+  });
+});
+
+Tinytest.add("callback-hook - exceptionHandler used with {bindEnvironment: false}", function (test) {
+  var exToThrow = new Error("Test error");
+  var thrownEx = null;
+  var hook = new Hook({
+    bindEnvironment: false,
+    exceptionHandler: function (ex) { thrownEx = ex; }
+  });
+  hook.register(function() {
+    throw exToThrow;
+  });
+  hook.each(function(callback) {
+    callback();
+  });
+  test.equal(exToThrow, thrownEx);
+});

packages/callback-hook/package.js

@@ -10,3 +10,9 @@ Package.onUse(function (api) {
 
   api.addFiles('hook.js', ['client', 'server']);
 });
+
+Package.onTest(function (api) {
+  api.use('callback-hook');
+  api.use('tinytest');
+  api.addFiles('hook_tests.js', 'server');
+});