From 2e66a7f1a05d59e043a269bd20fd07aa5bfeab12 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Fri, 14 Feb 2014 16:15:35 -0800 Subject: [PATCH] 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