From 2e66a7f1a05d59e043a269bd20fd07aa5bfeab12 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Fri, 14 Feb 2014 16:15:35 -0800 Subject: [PATCH 1/6] Replace observe on all users with one observe per connection. --- packages/accounts-base/accounts_server.js | 136 +++++-------------- packages/accounts-password/password_tests.js | 8 +- 2 files changed, 40 insertions(+), 104 deletions(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 6f69912646..d219e03e1f 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -557,26 +557,23 @@ Accounts._clearAllLoginTokens = function (userId) { ); }; - -// hashed token -> list of connection ids -var connectionsByLoginToken = {}; +// connection id -> observe handle for the login token that this +// connection is currently associated with +var userObservesForConnections = {}; // test hook -Accounts._getTokenConnections = function (token) { - return connectionsByLoginToken[token]; +Accounts._getUserObserve = function (connectionId) { + return userObservesForConnections[connectionId]; }; -// Remove the connection from the list of open connections for the connection's -// token. +// Clean up this connection's association with the token: that is, stop +// the observe that we started when we associated the connection with +// this token. var removeConnectionFromToken = function (connectionId) { - var token = Accounts._getLoginToken(connectionId); - if (token) { - connectionsByLoginToken[token] = _.without( - connectionsByLoginToken[token], - connectionId - ); - if (_.isEmpty(connectionsByLoginToken[token])) - delete connectionsByLoginToken[token]; + var observe = userObservesForConnections[connectionId]; + if (observe) { + observe.stop(); + delete userObservesForConnections[connectionId]; } }; @@ -590,33 +587,33 @@ Accounts._setLoginToken = function (userId, connection, newToken) { Accounts._setAccountData(connection.id, 'loginToken', newToken); if (newToken) { - if (! _.has(connectionsByLoginToken, newToken)) - connectionsByLoginToken[newToken] = []; - connectionsByLoginToken[newToken].push(connection.id); - - // Now that we've added the connection to the - // connectionsByLoginToken map for the token, the connection will - // be closed if the token is removed from the database. However - // at this point the token might have already been deleted, which - // wouldn't have closed the connection because it wasn't in the - // map yet. - // - // We also did need to first add the connection to the map above - // (and now remove it here if the token was deleted), because we - // could be getting a response from the database that the token - // still exists, but then it could be deleted in another fiber - // before our `findOne` call returns... and then that other fiber - // would need for the connection to be in the map for it to close - // the connection. - // - // We defer this check because there's no need for it to be on the critical - // path for login; we just need to ensure that the connection will get - // closed at some point if the token has been deleted. + // Set up an observe for this token. If the token goes away, we need + // to close the connection. We defer this observe because there's + // no need for it to be on the critical path for login; we just need + // to ensure that the connection will get closed at some point if + // the token gets deleted. Meteor.defer(function () { - if (! Meteor.users.findOne({ + var foundMatchingUser; + userObservesForConnections[connection.id] = Meteor.users.find({ _id: userId, - "services.resume.loginTokens.hashedToken": newToken - })) { + 'services.resume.loginTokens.hashedToken': newToken + }, { fields: { 'services.resume': 1 } }).observe({ + added: function () { + foundMatchingUser = true; + }, + removed: function () { + connection.close(); + // The onClose callback for the connection takes care of + // cleaning up the observe handle and any other state we have + // lying around. + } + }); + if (! foundMatchingUser) { + // We've set up an observe on the user associated with `newToken`, + // so if the new token is removed from the database, we'll close + // the connection. But the token might have already been deleted + // before we set up the observe, which wouldn't have closed the + // connection because the observe wasn't running yet. removeConnectionFromToken(connection.id); connection.close(); } @@ -624,24 +621,6 @@ Accounts._setLoginToken = function (userId, connection, newToken) { } }; -// Close all open connections associated with any of the tokens in -// `tokens`. -var closeConnectionsForTokens = function (tokens) { - _.each(tokens, function (token) { - if (_.has(connectionsByLoginToken, token)) { - // safety belt. close should defer potentially yielding callbacks. - Meteor._noYieldsAllowed(function () { - _.each(connectionsByLoginToken[token], function (connectionId) { - var connection = Accounts._getAccountData(connectionId, 'connection'); - if (connection) - connection.close(); - }); - }); - } - }); -}; - - // Login handler for resume tokens. Accounts.registerLoginHandler("resume", function(options) { if (!options.resume) @@ -1186,44 +1165,3 @@ Meteor.startup(function () { deleteSavedTokens(user._id, user.services.resume.loginTokensToDelete); }); }); - -/// -/// LOGGING OUT DELETED USERS -/// - -// When login tokens are removed from the database, close any sessions -// logged in with those tokens. -// -// Because we upgrade unhashed login tokens to hashed tokens at login -// time, sessions will only be logged in with a hashed token. Thus we -// only need to pull out hashed tokens here. -var closeTokensForUser = function (userTokens) { - closeConnectionsForTokens(_.compact(_.pluck(userTokens, "hashedToken"))); -}; - -// Like _.difference, but uses EJSON.equals to compute which values to return. -var differenceObj = function (array1, array2) { - return _.filter(array1, function (array1Value) { - return ! _.some(array2, function (array2Value) { - return EJSON.equals(array1Value, array2Value); - }); - }); -}; - -Meteor.users.find({}, { fields: { "services.resume": 1 }}).observe({ - changed: function (newUser, oldUser) { - var removedTokens = []; - if (newUser.services && newUser.services.resume && - oldUser.services && oldUser.services.resume) { - removedTokens = differenceObj(oldUser.services.resume.loginTokens || [], - newUser.services.resume.loginTokens || []); - } else if (oldUser.services && oldUser.services.resume) { - removedTokens = oldUser.services.resume.loginTokens || []; - } - closeTokensForUser(removedTokens); - }, - removed: function (oldUser) { - if (oldUser.services && oldUser.services.resume) - closeTokensForUser(oldUser.services.resume.loginTokens || []); - } -}); diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index 2daaac4867..2cd12672df 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -766,7 +766,7 @@ if (Meteor.isServer) (function () { // XXX would be nice to test Accounts.config({forbidClientAccountCreation: true}) Tinytest.addAsync( - 'passwords - login tokens cleaned up', + 'passwords - login token observes get cleaned up', function (test, onComplete) { var username = Random.id(); Accounts.createUser({ @@ -778,8 +778,7 @@ if (Meteor.isServer) (function () { test, function (clientConn, serverConn) { serverConn.onClose(function () { - test.isFalse(_.contains( - Accounts._getTokenConnections(token), serverConn.id)); + test.isFalse(Accounts._getUserObserve(serverConn.id)); onComplete(); }); var result = clientConn.call('login', { @@ -789,8 +788,7 @@ if (Meteor.isServer) (function () { test.isTrue(result); var token = Accounts._getAccountData(serverConn.id, 'loginToken'); test.isTrue(token); - test.isTrue(_.contains( - Accounts._getTokenConnections(token), serverConn.id)); + test.isTrue(Accounts._getUserObserve(serverConn.id)); clientConn.disconnect(); }, onComplete From b79294a690c91b1ce3f97695b8a0bff3845ae1a9 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Tue, 18 Mar 2014 10:07:09 -0700 Subject: [PATCH 2/6] glasser review comments --- packages/accounts-base/accounts_server.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index d219e03e1f..9d7a18e1aa 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -498,7 +498,7 @@ Accounts._setAccountData = function (connectionId, field, value) { Meteor.server.onConnection(function (connection) { accountData[connection.id] = {connection: connection}; connection.onClose(function () { - removeConnectionFromToken(connection.id); + removeTokenFromConnection(connection.id); delete accountData[connection.id]; }); }); @@ -569,11 +569,11 @@ Accounts._getUserObserve = function (connectionId) { // Clean up this connection's association with the token: that is, stop // the observe that we started when we associated the connection with // this token. -var removeConnectionFromToken = function (connectionId) { +var removeTokenFromConnection = function (connectionId) { var observe = userObservesForConnections[connectionId]; if (observe) { - observe.stop(); delete userObservesForConnections[connectionId]; + observe.stop(); } }; @@ -583,7 +583,7 @@ Accounts._getLoginToken = function (connectionId) { // newToken is a hashed token. Accounts._setLoginToken = function (userId, connection, newToken) { - removeConnectionFromToken(connection.id); + removeTokenFromConnection(connection.id); Accounts._setAccountData(connection.id, 'loginToken', newToken); if (newToken) { @@ -594,10 +594,13 @@ Accounts._setLoginToken = function (userId, connection, newToken) { // the token gets deleted. Meteor.defer(function () { var foundMatchingUser; + // Because we upgrade unhashed login tokens to hashed tokens at + // login time, sessions will only be logged in with a hashed + // token. Thus we only need to observe hashed tokens here. userObservesForConnections[connection.id] = Meteor.users.find({ _id: userId, 'services.resume.loginTokens.hashedToken': newToken - }, { fields: { 'services.resume': 1 } }).observe({ + }, { fields: { _id: 1 } }).observeChanges({ added: function () { foundMatchingUser = true; }, @@ -614,7 +617,7 @@ Accounts._setLoginToken = function (userId, connection, newToken) { // the connection. But the token might have already been deleted // before we set up the observe, which wouldn't have closed the // connection because the observe wasn't running yet. - removeConnectionFromToken(connection.id); + removeTokenFromConnection(connection.id); connection.close(); } }); From b3e342ec7ef6d248dd5a00fca086d7d94cb54cc0 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Wed, 19 Mar 2014 21:31:11 -0700 Subject: [PATCH 3/6] Remove redundant call to 'removeTokenFromConnection' --- packages/accounts-base/accounts_server.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 9d7a18e1aa..51e85fa423 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -617,7 +617,6 @@ Accounts._setLoginToken = function (userId, connection, newToken) { // the connection. But the token might have already been deleted // before we set up the observe, which wouldn't have closed the // connection because the observe wasn't running yet. - removeTokenFromConnection(connection.id); connection.close(); } }); From 051faf8895c15bbd6bb192ae3f3f200306e67e91 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Wed, 19 Mar 2014 22:01:48 -0700 Subject: [PATCH 4/6] Poll for the observe to appear in a test. --- packages/accounts-password/password_tests.js | 21 ++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index 2cd12672df..614862502c 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -788,8 +788,25 @@ if (Meteor.isServer) (function () { test.isTrue(result); var token = Accounts._getAccountData(serverConn.id, 'loginToken'); test.isTrue(token); - test.isTrue(Accounts._getUserObserve(serverConn.id)); - clientConn.disconnect(); + + // We poll here, instead of just checking `_getUserObserve` + // once, because the login method defers the creation of the + // observe, and setting up the observe yields, so we could end + // up here before the observe has been set up. + simplePoll( + function () { + return !! Accounts._getUserObserve(serverConn.id); + }, + function () { + test.isTrue(Accounts._getUserObserve(serverConn.id)); + clientConn.disconnect(); + }, + function () { + test.fail("timed out waiting for user observe for connection " + + serverConn.id); + onComplete(); + } + ); }, onComplete ); From efd044a00429bd7891d6428b617c28a732468e53 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Wed, 19 Mar 2014 22:02:13 -0700 Subject: [PATCH 5/6] Clean up observes created for connections that were closed before the observe was set up. --- packages/accounts-base/accounts_server.js | 36 +++++++++++++++++++---- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 51e85fa423..872300b0d4 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -558,9 +558,16 @@ Accounts._clearAllLoginTokens = function (userId) { }; // connection id -> observe handle for the login token that this -// connection is currently associated with +// connection is currently associated with, or null. Null indicates that +// we are in the process of setting up the observe. var userObservesForConnections = {}; +// connection id -> boolean. Keeps track of connections that were closed +// before we had a chance to set up the observe on the user associated +// with this connection. To avoid leaking observes, we'll look in here +// immediately after setting up an observe. +var connectionsClosedBeforeObserve = {}; + // test hook Accounts._getUserObserve = function (connectionId) { return userObservesForConnections[connectionId]; @@ -571,9 +578,17 @@ Accounts._getUserObserve = function (connectionId) { // this token. var removeTokenFromConnection = function (connectionId) { var observe = userObservesForConnections[connectionId]; - if (observe) { - delete userObservesForConnections[connectionId]; - observe.stop(); + if (observe !== undefined) { + if (observe === null) { + // We're in the process of setting up an observe for this + // connection. We can't clean up that observe yet, but we can make + // note of it in `connectionsClosedBeforeObserve`, so that the + // observe will get torn down immediately after being set up. + connectionsClosedBeforeObserve[connectionId] = true; + } else { + delete userObservesForConnections[connectionId]; + observe.stop(); + } } }; @@ -588,10 +603,15 @@ Accounts._setLoginToken = function (userId, connection, newToken) { if (newToken) { // Set up an observe for this token. If the token goes away, we need - // to close the connection. We defer this observe because there's + // to close the connection. We defer the observe because there's // no need for it to be on the critical path for login; we just need // to ensure that the connection will get closed at some point if // the token gets deleted. + // + // Initially, we set the observe for this connection to null; this + // signifies to other code (which might run while we yield) that we + // are in the process of setting up an observe for this connection. + userObservesForConnections[connection.id] = null; Meteor.defer(function () { var foundMatchingUser; // Because we upgrade unhashed login tokens to hashed tokens at @@ -611,6 +631,12 @@ Accounts._setLoginToken = function (userId, connection, newToken) { // lying around. } }); + if (connectionsClosedBeforeObserve[connection.id]) { + // Oops, this connection was closed while we were setting up the + // observe. Clean it up now. + delete connectionsClosedBeforeObserve[connection.id]; + removeTokenFromConnection(connection.id); + } if (! foundMatchingUser) { // We've set up an observe on the user associated with `newToken`, // so if the new token is removed from the database, we'll close From e999ac782e2ecdcbaddf1798468a9a38aecf2832 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Fri, 21 Mar 2014 09:14:41 -0700 Subject: [PATCH 6/6] Remove `connectionsClosedBeforeObserve`. Instead of inserting into `connectionsClosedBeforeObserve`, we can just delete the null placeholder from `userObservesForConnections`, and then check for a deleted placeholder before storing the observe once it's been set up. --- packages/accounts-base/accounts_server.js | 35 +++++++++++++---------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 872300b0d4..a75c70134b 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -562,12 +562,6 @@ Accounts._clearAllLoginTokens = function (userId) { // we are in the process of setting up the observe. var userObservesForConnections = {}; -// connection id -> boolean. Keeps track of connections that were closed -// before we had a chance to set up the observe on the user associated -// with this connection. To avoid leaking observes, we'll look in here -// immediately after setting up an observe. -var connectionsClosedBeforeObserve = {}; - // test hook Accounts._getUserObserve = function (connectionId) { return userObservesForConnections[connectionId]; @@ -581,10 +575,10 @@ var removeTokenFromConnection = function (connectionId) { if (observe !== undefined) { if (observe === null) { // We're in the process of setting up an observe for this - // connection. We can't clean up that observe yet, but we can make - // note of it in `connectionsClosedBeforeObserve`, so that the - // observe will get torn down immediately after being set up. - connectionsClosedBeforeObserve[connectionId] = true; + // connection. We can't clean up that observe yet, but if we + // delete the null placeholder for this connection, then the + // observe will get cleaned up as soon as it has been set up. + delete userObservesForConnections[connectionId]; } else { delete userObservesForConnections[connectionId]; observe.stop(); @@ -610,14 +604,18 @@ Accounts._setLoginToken = function (userId, connection, newToken) { // // Initially, we set the observe for this connection to null; this // signifies to other code (which might run while we yield) that we - // are in the process of setting up an observe for this connection. + // are in the process of setting up an observe for this + // connection. Once the observe is ready to go, we replace null with + // the real observe handle (unless the placeholder has been deleted, + // signifying that the connection was closed already -- in this case + // we just clean up the observe that we started). userObservesForConnections[connection.id] = null; Meteor.defer(function () { var foundMatchingUser; // Because we upgrade unhashed login tokens to hashed tokens at // login time, sessions will only be logged in with a hashed // token. Thus we only need to observe hashed tokens here. - userObservesForConnections[connection.id] = Meteor.users.find({ + var observe = Meteor.users.find({ _id: userId, 'services.resume.loginTokens.hashedToken': newToken }, { fields: { _id: 1 } }).observeChanges({ @@ -631,12 +629,19 @@ Accounts._setLoginToken = function (userId, connection, newToken) { // lying around. } }); - if (connectionsClosedBeforeObserve[connection.id]) { + + if (_.has(userObservesForConnections, connection.id)) { + if (userObservesForConnections[connection.id] !== null) { + throw new Error("Non-null user observe for connection " + + connection.id + " while observe was being set up?"); + } + userObservesForConnections[connection.id] = observe; + } else { // Oops, this connection was closed while we were setting up the // observe. Clean it up now. - delete connectionsClosedBeforeObserve[connection.id]; - removeTokenFromConnection(connection.id); + observe.stop(); } + if (! foundMatchingUser) { // We've set up an observe on the user associated with `newToken`, // so if the new token is removed from the database, we'll close