From 74e80436874d38325ca2041d6a1caa0c9aa7ed28 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Tue, 18 Mar 2014 09:36:00 -0700 Subject: [PATCH 1/3] Split logoutOtherClients into two methods. One method call gives you a new token, and another method call removes all but the current token. Meteor.logoutOtherClients calls both these methods, and calls the user's callback when the second has returned. --- packages/accounts-base/accounts_client.js | 66 ++++++++--------- packages/accounts-base/accounts_server.js | 56 ++++++++++++++- packages/accounts-base/accounts_tests.js | 76 ++++++++++++++++++++ packages/accounts-password/password_tests.js | 75 ++++++++++--------- 4 files changed, 205 insertions(+), 68 deletions(-) diff --git a/packages/accounts-base/accounts_client.js b/packages/accounts-base/accounts_client.js index 18c8ed82db..140cdf1069 100644 --- a/packages/accounts-base/accounts_client.js +++ b/packages/accounts-base/accounts_client.js @@ -215,38 +215,40 @@ Meteor.logout = function (callback) { }; Meteor.logoutOtherClients = function (callback) { - // Call the `logoutOtherClients` method. Store the login token that we get - // back and use it to log in again. The server is not supposed to close - // connections on the old token for 10 seconds, so we should have time to - // store our new token and log in with it before being disconnected. If we get - // disconnected, then we'll immediately reconnect with the new token. If for - // some reason we get disconnected before storing the new token, then the - // worst that will happen is that we'll have a flicker from trying to log in - // with the old token before storing and logging in with the new one. - Accounts.connection.apply('logoutOtherClients', [], { wait: true }, - function (error, result) { - if (error) { - callback && callback(error); - } else { - var userId = Meteor.userId(); - storeLoginToken(userId, result.token, result.tokenExpires); - // If the server hasn't disconnected us yet by deleting our - // old token, then logging in now with the new valid token - // will prevent us from getting disconnected. If the server - // has already disconnected us due to our old invalid token, - // then we would have already tried and failed to login with - // the old token on reconnect, and we have to make sure a - // login method gets sent here with the new token. - Meteor.loginWithToken(result.token, function (err) { - if (err && - storedLoginToken() && - storedLoginToken().token === result.token) { - makeClientLoggedOut(); - } - callback && callback(err); - }); - } - }); + // We need to make two method calls: one to replace our current token, + // and another to remove all tokens except the current one. We want to + // call these two methods one after the other, without any other + // methods running between them. For example, we don't want `logout` + // to be called in between our two method calls (otherwise the second + // method call would return an error). Another example: we don't want + // logout to be called before the callback for `getNewToken`; + // otherwise we would momentarily log the user out and then write a + // new token to localStorage. + // + // To accomplish this, we make both calls as wait methods, and queue + // them one after the other, without spinning off the event loop in + // between. Even though we queue `removeOtherTokens` before + // `getNewToken`, we won't actually send the `removeOtherTokens` call + // until the `getNewToken` callback has finished running, because they + // are both wait methods. + Accounts.connection.apply( + 'getNewToken', + [], + { wait: true }, + function (err, result) { + if (! err) { + storeLoginToken(Meteor.userId(), result.token, result.tokenExpires); + } + } + ); + Accounts.connection.apply( + 'removeOtherTokens', + [], + { wait: true }, + function (err) { + callback && callback(err); + } + ); }; diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index a75c70134b..6b419604f6 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -424,6 +424,17 @@ Meteor.methods({ // use. Tests set Accounts._noConnectionCloseDelayForTest to delete tokens // immediately instead of using a delay. // + // XXX COMPAT WITH 0.7.2 + // This single `logoutOtherClients` method has been replaced with two + // methods, one that you call to get a new token, and another that you + // call to remove all tokens except your own. The new design allows + // clients to know when other clients have actually been logged + // out. (The `logoutOtherClients` method guarantees the caller that + // the other clients will be logged out at some point, but makes no + // guarantees about when.) This method is left in for backwards + // compatibility, especially since application code might be calling + // this method directly. + // // @returns {Object} Object with token and tokenExpires keys. logoutOtherClients: function () { var self = this; @@ -441,7 +452,7 @@ Meteor.methods({ var tokens = user.services.resume.loginTokens; var newToken = Accounts._generateStampedLoginToken(); var userId = self.userId; - Meteor.users.update(self.userId, { + Meteor.users.update(userId, { $set: { "services.resume.loginTokensToDelete": tokens, "services.resume.haveLoginTokensToDelete": true @@ -462,8 +473,49 @@ Meteor.methods({ tokenExpires: Accounts._tokenExpiration(newToken.when) }; } else { - throw new Error("You are not logged in."); + throw new Meteor.Error("You are not logged in."); } + }, + + getNewToken: function () { + var self = this; + var user = Meteor.users.findOne(self.userId, { + fields: { "services.resume.loginTokens": 1 } + }); + if (! self.userId || ! user) { + throw new Meteor.Error("You are not logged in."); + } + // Be careful not to generate a new token that has a later + // expiration than the curren token. Otherwise, a bad guy with a + // stolen token could use this method to stop his stolen token from + // ever expiring. + var currentHashedToken = Accounts._getLoginToken(self.connection.id); + var currentStampedToken = _.find( + user.services.resume.loginTokens, + function (stampedToken) { + return stampedToken.hashedToken === currentHashedToken; + } + ); + if (! currentStampedToken) { // safety belt: this should never happen + throw new Meteor.Error("Invalid login token"); + } + var newStampedToken = Accounts._generateStampedLoginToken(); + newStampedToken.when = currentStampedToken.when; + Accounts._insertLoginToken(self.userId, newStampedToken); + return loginUser(self, self.userId, newStampedToken); + }, + + removeOtherTokens: function () { + var self = this; + if (! self.userId) { + throw new Meteor.Error("You are not logged in."); + } + var currentToken = Accounts._getLoginToken(self.connection.id); + Meteor.users.update(self.userId, { + $pull: { + "services.resume.loginTokens": { hashedToken: { $ne: currentToken } } + } + }); } }); diff --git a/packages/accounts-base/accounts_tests.js b/packages/accounts-base/accounts_tests.js index e3eeaecd0c..1e49fd31f2 100644 --- a/packages/accounts-base/accounts_tests.js +++ b/packages/accounts-base/accounts_tests.js @@ -1,3 +1,9 @@ +Meteor.methods({ + getCurrentLoginToken: function () { + return Accounts._getLoginToken(this.connection.id); + } +}); + // XXX it'd be cool to also test that the right thing happens if options // *are* validated, but Accounts._options is global state which makes this hard // (impossible?) @@ -297,3 +303,73 @@ Tinytest.addAsync( ); } ); + +Tinytest.add( + 'accounts - get new token', + function (test) { + // Test that the `getNewToken` method returns us a valid token, with + // the same expiration as our original token. + var userId = Accounts.insertUserDoc({}, { username: Random.id() }); + var stampedToken = Accounts._generateStampedLoginToken(); + Accounts._insertLoginToken(userId, stampedToken); + var conn = DDP.connect(Meteor.absoluteUrl()); + conn.call('login', { resume: stampedToken.token }); + test.equal(conn.call('getCurrentLoginToken'), + Accounts._hashLoginToken(stampedToken.token)); + + var newTokenResult = conn.call('getNewToken'); + test.equal(newTokenResult.tokenExpires, + Accounts._tokenExpiration(stampedToken.when)); + test.equal(conn.call('getCurrentLoginToken'), + Accounts._hashLoginToken(newTokenResult.token)); + conn.disconnect(); + + // A second connection should be able to log in with the new token + // we got. + var secondConn = DDP.connect(Meteor.absoluteUrl()); + secondConn.call('login', { resume: newTokenResult.token }); + secondConn.disconnect(); + } +); + +Tinytest.addAsync( + 'accounts - remove other tokens', + function (test, onComplete) { + // Test that the `removeOtherTokens` method removes all tokens other + // than the caller's token, thereby logging out and closing other + // connections. + var userId = Accounts.insertUserDoc({}, { username: Random.id() }); + var stampedTokens = []; + var conns = []; + + _.times(2, function (i) { + stampedTokens.push(Accounts._generateStampedLoginToken()); + Accounts._insertLoginToken(userId, stampedTokens[i]); + var conn = DDP.connect(Meteor.absoluteUrl()); + conn.call('login', { resume: stampedTokens[i].token }); + test.equal(conn.call('getCurrentLoginToken'), + Accounts._hashLoginToken(stampedTokens[i].token)); + conns.push(conn); + }); + + conns[0].call('removeOtherTokens'); + simplePoll( + function () { + var tokens = _.map(conns, function (conn) { + return conn.call('getCurrentLoginToken'); + }); + return ! tokens[1] && + tokens[0] === Accounts._hashLoginToken(stampedTokens[0].token); + }, + function () { // success + _.each(conns, function (conn) { + conn.disconnect(); + }); + onComplete(); + }, + function () { // timed out + throw new Error("accounts - remove other tokens timed out"); + } + ); + } +); diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index 614862502c..45240231bc 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -468,13 +468,53 @@ if (Meteor.isClient) (function () { test.equal(Meteor.userId(), null); })); }, + logoutStep, + function (test, expect) { + var self = this; + // Test that Meteor.logoutOtherClients logs out a second + // authentcated connection while leaving Accounts.connection + // logged in. + var secondConn = DDP.connect(Meteor.absoluteUrl()); + var token; + + var expectSecondConnLoggedOut = expect(function (err, result) { + test.isTrue(err); + }); + + var expectAccountsConnLoggedIn = expect(function (err, result) { + test.isFalse(err); + }); + + var expectSecondConnLoggedIn = expect(function (err, result) { + test.equal(result.token, token); + test.isFalse(err); + Meteor.logoutOtherClients(function (err) { + test.isFalse(err); + secondConn.call('login', { resume: token }, expectSecondConnLoggedOut); + Accounts.connection.call('login', { + resume: Accounts._storedLoginToken() + }, expectAccountsConnLoggedIn); + }); + }); + + Meteor.loginWithPassword(this.username, this.password, expect(function (err) { + test.isFalse(err); + token = Accounts._storedLoginToken(); + test.isTrue(token); + secondConn.call('login', { resume: token }, expectSecondConnLoggedIn); + })); + }, + logoutStep, + + // The tests below this point are for the deprecated + // `logoutOtherClients` method. + function (test, expect) { var self = this; // Test that Meteor.logoutOtherClients logs out a second authenticated // connection while leaving Accounts.connection logged in. var token; - var userId; self.secondConn = DDP.connect(Meteor.absoluteUrl()); var expectLoginError = expect(function (err) { @@ -502,7 +542,6 @@ if (Meteor.isClient) (function () { token = Accounts._storedLoginToken(); self.beforeLogoutOthersToken = token; test.isTrue(token); - userId = Meteor.userId(); self.secondConn.call("login", { resume: token }, expectSecondConnLoggedIn); })); @@ -536,41 +575,9 @@ if (Meteor.isClient) (function () { ); }, logoutStep, - function (test, expect) { - var self = this; - // Test that, when we call logoutOtherClients, if the server disconnects - // us before the logoutOtherClients callback runs, then we still end up - // logged in. - var expectServerLoggedIn = expect(function (err, result) { - test.isFalse(err); - test.isTrue(Meteor.userId()); - test.equal(result, Meteor.userId()); - }); - Meteor.loginWithPassword( - self.username, - self.password, - expect(function (err) { - test.isFalse(err); - test.isTrue(Meteor.userId()); - // The test is only useful if things interleave in the following order: - // - logoutOtherClients runs on the server - // - onReconnect fires and sends a login method with the old token, - // which results in an error - // - logoutOtherClients callback runs and stores the new token and - // logs in with it - // In practice they seem to interleave this way, but I'm not sure how - // to make sure that they do. - Meteor.logoutOtherClients(function (err) { - test.isFalse(err); - Meteor.call("getUserId", expectServerLoggedIn); - }); - }) - ); - }, - logoutStep, function (test, expect) { var self = this; // Test that deleting a user logs out that user's connections. From 3054b09790c2af74a511e17d2dc338370e053abc Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Tue, 18 Mar 2014 21:12:40 -0700 Subject: [PATCH 2/3] whitespace and use 'self' instead of 'this' --- packages/accounts-password/password_tests.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index 45240231bc..adfde8594e 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -490,19 +490,25 @@ if (Meteor.isClient) (function () { test.isFalse(err); Meteor.logoutOtherClients(function (err) { test.isFalse(err); - secondConn.call('login', { resume: token }, expectSecondConnLoggedOut); + secondConn.call('login', { resume: token }, + expectSecondConnLoggedOut); Accounts.connection.call('login', { resume: Accounts._storedLoginToken() }, expectAccountsConnLoggedIn); }); }); - Meteor.loginWithPassword(this.username, this.password, expect(function (err) { - test.isFalse(err); - token = Accounts._storedLoginToken(); - test.isTrue(token); - secondConn.call('login', { resume: token }, expectSecondConnLoggedIn); - })); + Meteor.loginWithPassword( + self.username, + self.password, + expect(function (err) { + test.isFalse(err); + token = Accounts._storedLoginToken(); + test.isTrue(token); + secondConn.call('login', { resume: token }, + expectSecondConnLoggedIn); + }) + ); }, logoutStep, From d5af69eca601308c6a26dc277d0ab70b6bc5e8a6 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Mon, 24 Mar 2014 13:54:05 -0700 Subject: [PATCH 3/3] Add comments to logoutOtherClients methods --- packages/accounts-base/accounts_server.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 6b419604f6..d31ab63a23 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -477,6 +477,14 @@ Meteor.methods({ } }, + // Generates a new login token with the same expiration as the + // connection's current token and saves it to the database. Associates + // the connection with this new token and returns it. Throws an error + // if called on a connection that isn't logged in. + // + // @returns Object + // If successful, returns { token: , id: , + // tokenExpires: }. getNewToken: function () { var self = this; var user = Meteor.users.findOne(self.userId, { @@ -505,6 +513,9 @@ Meteor.methods({ return loginUser(self, self.userId, newStampedToken); }, + // Removes all tokens except the token associated with the current + // connection. Throws an error if the connection is not logged + // in. Returns nothing on success. removeOtherTokens: function () { var self = this; if (! self.userId) {