Publish 0.3.5 fixing bug #1 - no resume can cause user deletion

.versions

@@ -7,7 +7,7 @@ binary-heap@1.0.4
 blaze@2.1.3
 blaze-tools@1.0.4
 boilerplate-generator@1.0.4
-brettle:accounts-add-service@0.3.4
+brettle:accounts-add-service@0.3.5
 brettle:accounts-multiple@0.3.1
 brettle:accounts-testing-support@0.4.3
 brettle:workaround-issue-4331@0.0.4
@@ -32,7 +32,7 @@ html-tools@1.0.5
 htmljs@1.0.5
 id-map@1.0.4
 jquery@1.11.4
-local-test:brettle:accounts-add-service@0.3.4
+local-test:brettle:accounts-add-service@0.3.5
 localstorage@1.0.5
 logging@1.0.8
 meteor@1.1.10

CHANGELOG.md

@@ -4,6 +4,13 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+## [0.3.5] 2015-11-13
+
+### Fixed
+
+Bug #1: Existing account without resume service deleted if logged in user logs
+into it. For details, see the [Notes section of README.md](README.md#notes)
+
 ## [0.3.4] 2015-11-09
 
 ### Fixed

README.md

@@ -11,12 +11,13 @@ for an overview of the suite and a live demo.
 ## Features
 
 - When a logged in user attempts to login using credentials that don't belong to
-  an existing user, this package adds those credentials to the logged in user, so they are available for future logins. Any other top-level or profile properties that would have
-  been associated with a new user with those credentials are also non-destructively
-  merged into the existing user.
+  an existing user, this package adds those credentials to the logged in user,
+  so they are available for future logins. Any other top-level or profile
+  properties that would have belonged to a new user with those credentials are
+  also non-destructively merged into the existing user.
 
 - When a logged in user tries to login using credentials that belong to an
-  existing user, the user is logged in as the existing user (unless that is
+  existing user, the user gets logged in as the existing user (unless that is
   otherwise prevented, for example by brettle:accounts-logout-to-switch).
 
 - Works with any login service (accounts-password, acccounts-google, etc.)
@@ -33,3 +34,21 @@ meteor add brettle:accounts-add-service
 ## Usage
 
 Nothing to do. It should just work once installed.
+
+## Notes
+
+This package adds a `hasLoggedIn` property to a user's account when he logs in,
+and does a one-time migration to add the same property to existing users that
+have the `resume` service created by the Meteor accounts system when a user logs
+in. This is to ensure that a user counts as having logged in even if his
+`resume` service is later removed (e.g. to force him to re-authenticate for
+security reasons).
+
+The migration runs from a `Meteor.startup` handler. It uses an internal
+`Mongo.Collection` to keep track of whether it already started. This ensures
+that it runs at most once and doesn't run on more than one server. If for some
+reason you need to disable the migration, use:
+
+```js
+AccountsAddService.databaseMigrationEnabled = false;
+```

accounts-add-service-server-tests.js

@@ -109,3 +109,130 @@ Tinytest.add(
     test.isUndefined(user.services.test1, 'no user.services.test1');
   }
 );
+
+Tinytest.add(
+  'brettle:accounts-add-service - add hasLoggedIn to existing resume users',
+  function (test) {
+    AccountsMultiple._unregisterAll();
+    AccountsAddService._init();
+    var connection = DDP.connect(Meteor.absoluteUrl());
+
+    // Add a user who logged in after 0.3.5 and didn't lose his resume service
+    // (i.e. has services.resume and hasLoggedIn). This user should be left
+    // alone.
+    connection.call('login', { test1: "resumeYesHasLoggedInYes" });
+
+    // Add a user who didn't log in or logged in before 0.3.5 but lost his
+    // resume service (i.e. has neither services.resume nor hasLoggedIn). This
+    // user should be left alone because we can't tell which is the case.
+    Meteor.users.update(connection.call('login', { 
+      test1: "resumeNoHasLoggedInNo" 
+    }).id, {
+      $unset: {
+        hasLoggedIn: '',
+        'services.resume': ''
+      }
+    });    
+
+    // Add a user who logged in after 0.3.5 and lost his resume service (i.e.
+    // doesn't have services.resume but has hasLoggedIn). This user should be
+    // left alone.
+    Meteor.users.update(connection.call('login', {
+      test1: "resumeNoHasLoggedInYes" 
+    }).id, {
+      $unset: {
+        'services.resume': ''
+      }
+    });
+
+    // Add a user who logged in before 0.3.5 and didn't lose his resume service
+    // (i.e. has services.resume but not hasLoggedIn). This user should get
+    // hasLoggedIn added.
+    var resumeYesHasLoggedInNoId = 
+      connection.call('login', { test1: "resumeYesHasLoggedInNo" }).id;
+    Meteor.users.update(resumeYesHasLoggedInNoId, {
+      $unset: { hasLoggedIn: '' }
+    });
+
+    connection.call('logout');
+
+    var usersAdded = [];
+    var usersRemoved = [];
+    var usersChanged = [];
+    var observer = Meteor.users.find().observeChanges({
+      added: function (newUserId) {
+        usersAdded.push(newUserId);
+      },
+      changed: function (userId, fields) {
+        usersChanged.push({ userId: userId, fields: fields });
+      },
+      removed: function (oldUserId) {
+        usersRemoved.push(oldUserId);
+      }
+    });
+    usersAdded = []; // Ignore the users that were there already
+
+    // Clear the control collection
+    AccountsAddService._migrationControl.remove({}); 
+
+    AccountsAddService.databaseMigrationEnabled = false;
+    test.isFalse(AccountsAddService._migrateDatabase(), 'migration disabled');
+    waitForCallbacksToFinish();
+    test.equal(usersAdded.length + usersRemoved.length + usersChanged.length, 0,
+      'no users affected when migration disabled');
+
+    AccountsAddService.databaseMigrationEnabled = true;
+    test.equal(AccountsAddService._migrateDatabase(), 1, '1 user updated');
+    waitForCallbacksToFinish();
+    test.equal(usersAdded.length + usersRemoved.length, 0, 
+      'no users added/removed when migration enabled');
+    test.equal(usersChanged, [{
+      userId: resumeYesHasLoggedInNoId,
+      fields: {
+        hasLoggedIn: true
+      }
+    }], 'only updated user who has resume service but not hasLoggedIn');
+
+    // Unmigrate the user for the remainder of the test
+    Meteor.users.update(resumeYesHasLoggedInNoId, {
+      $unset: { hasLoggedIn: '' }
+    });
+    waitForCallbacksToFinish();
+    usersAdded = [];
+    usersRemoved = [];
+    usersChanged = [];
+    test.isFalse(AccountsAddService._migrateDatabase(), 'migrate only once');
+    waitForCallbacksToFinish();
+    test.equal(usersAdded.length + usersRemoved.length + usersChanged.length, 0,
+      'no users affected when already migrated');
+
+    AccountsAddService._migrationControl.update('addHasLoggedIn', {
+      $set: {
+        startedOn: new Date()
+      }
+    });
+    test.isFalse(AccountsAddService._migrateDatabase(), 'migrate in progress');
+    waitForCallbacksToFinish();
+    test.equal(usersAdded.length + usersRemoved.length + usersChanged.length, 0,
+      'no users affected when migration in progress');
+
+    observer.stop();
+  }
+
+);
+
+// Wait until the observeChanges callbacks have been called before returning. To
+// do this, we insert into to a separate collection that we observe, and assume
+// that by the time that the added callback is called on our collection, all
+// other observe callbacks have already been called.
+var testCollection = new Mongo.Collection("AccountsAddServiceTest");
+var waitForCallbacksToFinish = Meteor.wrapAsync(function (cb) {
+  testCollection.remove({});
+  var observer = testCollection.find().observe({
+    added: function () {
+      observer.stop();
+      cb.call();
+    }
+  });
+  testCollection.insert({});
+});

accounts-add-service-server.js

@@ -3,6 +3,70 @@
 "use strict";
 var mergeUserErrorReason = AccountsAddService._mergeUserErrorReason;
 
+AccountsAddService.databaseMigrationEnabled = true;
+
+AccountsAddService._migrationControl = 
+  new Mongo.Collection("AccountsAddService._migrationControl");
+
+// returns false if the migration isn't run, or the number of users updated
+// if it is.
+AccountsAddService._migrateDatabase = function () {
+  if (! AccountsAddService.databaseMigrationEnabled) {
+    return false;
+  }
+  var addHasLoggedIn = 
+    AccountsAddService._migrationControl.findOne('addHasLoggedIn');
+  if (addHasLoggedIn && addHasLoggedIn.startedAt) {
+    return false;
+  }
+  if (! addHasLoggedIn) {
+    try {
+      AccountsAddService._migrationControl.insert({ _id: 'addHasLoggedIn' });
+    } catch (err) {
+      // Ignore duplicate key error thrown if already id already exists due to
+      // concurrent insertion attempts
+      if (! (err.name === 'MongoError' && err.message.indexOf('E11000'))) {
+        throw err;
+      }
+    }
+  }
+  var numAffected = AccountsAddService._migrationControl.update({
+    _id: 'addHasLoggedIn',
+    startedAt: { $exists: false }
+   }, {
+    $set: {
+      startedAt: new Date(),
+    }
+  });
+  // Only one server will return numAffected !== 0. When numAffect === 0,
+  // The migration was already started (and possibly finished)
+  if (numAffected === 0) {
+    return false;
+  }
+
+  var selector = { 
+    hasLoggedIn: { $exists: false }, 
+    'services.resume': { $exists: true }
+  };
+  var numUsersUpdated = Meteor.users.update(selector, {
+    $set: { hasLoggedIn: true }
+  });
+  if (numUsersUpdated > 0) {
+    console.log('brettle:accounts-add-service set hasLoggedIn = true for ' +
+      numUsersUpdated + ' existing user(s).');  
+  }
+  AccountsAddService._migrationControl.update({
+    _id: 'addHasLoggedIn',
+   }, {
+    $set: {
+      finishedAt: new Date(),
+    }
+  });
+  return numUsersUpdated;
+};
+
+Meteor.startup(AccountsAddService._migrateDatabase);
+
 // The first time a user logs in, we set his hasLoggedIn property so that
 // we don't accidentally merge his account if his "resume" service is removed.
 Accounts.onLogin(function (attemptInfo) {

package.js

@@ -2,7 +2,7 @@
 
 Package.describe({
   name: 'brettle:accounts-add-service',
-  version: '0.3.4',
+  version: '0.3.5',
   summary: 'Allow users to add login services to their accounts',
   git: 'https://github.com/brettle/meteor-accounts-add-service.git',
   documentation: 'README.md'
@@ -11,6 +11,7 @@ Package.describe({
 Package.onUse(function(api) {
   api.versionsFrom('1.0.4');
   api.use('underscore');
+  api.use('mongo');
   api.use('brettle:workaround-issue-4331@0.0.1');
   api.use('brettle:workaround-issue-4970@0.0.1');
   api.use('brettle:workaround-issue-5110@0.0.1');
@@ -27,6 +28,7 @@ Package.onTest(function(api) {
   api.use('ddp');
   api.use('accounts-base');
   api.use('accounts-password');
+  api.use('mongo');
   api.use('brettle:accounts-add-service@0.3.3');
   api.use('brettle:accounts-testing-support@0.4.3');
   api.use('brettle:accounts-multiple@0.3.1', 'server');