From 587e02a692da86eaed03a8ff7a2c5010cda17ba3 Mon Sep 17 00:00:00 2001 From: Dean Brettle Date: Sun, 2 Aug 2015 23:15:47 -0700 Subject: [PATCH] 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 | 2 + packages/accounts-base/accounts_server.js | 2 +- packages/accounts-base/accounts_tests.js | 41 ++++++++++++++++ packages/callback-hook/hook.js | 57 ++++++++++++++++++----- packages/callback-hook/hook_tests.js | 55 ++++++++++++++++++++++ packages/callback-hook/package.js | 6 +++ 6 files changed, 151 insertions(+), 12 deletions(-) create mode 100644 packages/callback-hook/hook_tests.js diff --git a/packages/accounts-base/accounts_common.js b/packages/accounts-base/accounts_common.js index ead8a7fe71..80e1b2431c 100644 --- a/packages/accounts-base/accounts_common.js +++ b/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" }); }; diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 82ff2ea149..d01204aeb8 100644 --- a/packages/accounts-base/accounts_server.js +++ b/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) ]; diff --git a/packages/accounts-base/accounts_tests.js b/packages/accounts-base/accounts_tests.js index 1e49fd31f2..6830f6638c 100644 --- a/packages/accounts-base/accounts_tests.js +++ b/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(); + } +); diff --git a/packages/callback-hook/hook.js b/packages/callback-hook/hook.js index 43c493904e..5d1212f1a7 100644 --- a/packages/callback-hook/hook.js +++ b/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(); + }; +}; diff --git a/packages/callback-hook/hook_tests.js b/packages/callback-hook/hook_tests.js new file mode 100644 index 0000000000..28b05242e9 --- /dev/null +++ b/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); +}); diff --git a/packages/callback-hook/package.js b/packages/callback-hook/package.js index 83e3d7b1a5..d83e31b4be 100644 --- a/packages/callback-hook/package.js +++ b/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'); +});