From 44629cf80055a84f2ee55377dec795b363790659 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Mon, 18 Nov 2013 15:26:08 -0800 Subject: [PATCH] Avoid overwriting fresh tokens from other tabs. * Before wiping a bad token from storage on reconnect, make sure that we're wiping the token that we tried and failed to log in with. Avoids logging out another tab that might have gotten a fresh valid token while we were logging in with the old, invalid one (though it is still theoretically possible). * In the logoutOtherClients callback, try to log in with the token that we get in the response. Accounts for the situation where the server disconnects us before the callback runs. * If we fail to log in with a token found during a localStorage poll, make the client logged out. * Add a test that attempts to simulate one tab getting a fresh new token while another tab logs in with an old invalid token on reconnect. --- packages/accounts-base/accounts_client.js | 60 ++++++++++++++++---- packages/accounts-base/localstorage_token.js | 10 +++- packages/accounts-password/password_tests.js | 49 +++++++++++++++- 3 files changed, 102 insertions(+), 17 deletions(-) diff --git a/packages/accounts-base/accounts_client.js b/packages/accounts-base/accounts_client.js index 0e7df5582b..d21bdad805 100644 --- a/packages/accounts-base/accounts_client.js +++ b/packages/accounts-base/accounts_client.js @@ -115,8 +115,28 @@ Accounts.callLoginMethod = function (options) { // need to show a logging-in animation. _suppressLoggingIn: true, userCallback: function (error) { + var storedTokenNow = storedLoginToken(); if (error) { - makeClientLoggedOut(); + // If we had a login error AND the current stored token is the + // one that we tried to log in with, then declare ourselves + // logged out. If there's a token in storage but it's not the + // token that we tried to log in with, we don't know anything + // about whether that token is valid or not, so do nothing. The + // periodic localStorage poll will decide if we are logged in or + // out with this token, if it hasn't already. Of course, even + // with this check, another tab could insert a new valid token + // immediately before we clear localStorage here, which would + // lead to both tabs being logged out, but by checking the token + // in storage right now we hope to make that unlikely to happen. + // + // If there is no token in storage right now, we don't have to + // do anything; whatever code removed the token from storage was + // responsible for calling `makeClientLoggedOut()`, or the + // periodic localStorage poll will call `makeClientLoggedOut` + // eventually if another tab wiped the token from storage. + if (storedTokenNow && storedTokenNow === result.token) { + makeClientLoggedOut(); + } } // Possibly a weird callback to call, but better than nothing if // there is a reconnect between "login result received" and "data @@ -194,23 +214,41 @@ Meteor.logout = function (callback) { }; Meteor.logoutOtherClients = function (callback) { - // Call the `logoutOtherClients` method and store the login token that we get - // back. 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 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. - Meteor.apply('logoutOtherClients', [], + // 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. + Meteor.apply('logoutOtherClients', [], { wait: true }, function (error, result) { - if (! error) { + 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); + }); } }); }; + /// /// LOGIN SERVICES /// diff --git a/packages/accounts-base/localstorage_token.js b/packages/accounts-base/localstorage_token.js index 24c6e25318..e30d4514c7 100644 --- a/packages/accounts-base/localstorage_token.js +++ b/packages/accounts-base/localstorage_token.js @@ -115,10 +115,14 @@ var pollStoredLoginToken = function() { // != instead of !== just to make sure undefined and null are treated the same if (lastLoginTokenWhenPolled != currentLoginToken) { - if (currentLoginToken) - Meteor.loginWithToken(currentLoginToken); // XXX should we pass a callback here? - else + if (currentLoginToken) { + Meteor.loginWithToken(currentLoginToken, function (err) { + if (err) + makeClientLoggedOut(); + }); + } else { Meteor.logout(); + } } lastLoginTokenWhenPolled = currentLoginToken; }; diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index 59ae584d98..73f7ea1db0 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -1,5 +1,13 @@ Accounts._noConnectionCloseDelayForTest = true; +if (Meteor.isServer) { + Meteor.methods({ + getUserId: function () { + return this.userId; + } + }); +} + if (Meteor.isClient) (function () { // XXX note, only one test can do login/logout things at once! for @@ -404,10 +412,10 @@ if (Meteor.isClient) (function () { var self = this; // copied from livedata/client_convenience.js - var ddpUrl = '/'; + self.ddpUrl = '/'; if (typeof __meteor_runtime_config__ !== "undefined") { if (__meteor_runtime_config__.DDP_DEFAULT_CONNECTION_URL) - ddpUrl = __meteor_runtime_config__.DDP_DEFAULT_CONNECTION_URL; + self.ddpUrl = __meteor_runtime_config__.DDP_DEFAULT_CONNECTION_URL; } // XXX can we get the url from the existing connection somehow // instead? @@ -416,7 +424,7 @@ if (Meteor.isClient) (function () { // connection while leaving Meteor.connection logged in. var token; var userId; - self.secondConn = DDP.connect(ddpUrl); + self.secondConn = DDP.connect(self.ddpUrl); var expectLoginError = expect(function (err) { test.isTrue(err); @@ -477,6 +485,41 @@ 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.