fix: fixed incorrect users lookup (#2229)

package.json

@@ -1,6 +1,6 @@
 {
   "name": "talk",
-  "version": "4.8.2",
+  "version": "4.8.3",
   "description": "A better commenting experience from Mozilla, The New York Times, and the Washington Post. https://coralproject.net",
   "main": "app.js",
   "private": true,

services/users.js

@@ -60,13 +60,17 @@ async function upsertUser(
   shouldSetDisplayName = false
 ) {
   let user = await User.findOne({
-    id,
-    profiles: {
-      $elemMatch: {
-        id,
-        provider,
+    $or: [
+      { id },
+      {
+        profiles: {
+          $elemMatch: {
+            id,
+            provider,
+          },
+        },
       },
-    },
+    ],
   });
   if (user) {
     user.wasUpserted = false;

test/helpers/mongoose.js

@@ -1,15 +1,31 @@
 const mongoose = require('../../services/mongoose');
 
-before(function(done) {
+const models = [
+  require('../../models/action'),
+  require('../../models/asset'),
+  require('../../models/comment'),
+  require('../../models/migration'),
+  require('../../models/setting'),
+  require('../../models/user'),
+  require('../../models/migration'),
+];
+
+before(async function() {
   this.timeout(30000);
 
-  mongoose.connection.on('open', function(err) {
-    if (err) {
-      return done(err);
-    }
+  // Ensure we can connect to the database.
+  await new Promise((resolve, reject) => {
+    mongoose.connection.on('open', err => {
+      if (err) {
+        return reject(err);
+      }
 
-    return done();
+      return resolve();
+    });
   });
+
+  // Ensure all the models have indexes created.
+  await Promise.all(models.map(model => model.ensureIndexes()));
 });
 
 beforeEach(async () => {

test/server/services/users.js

@@ -4,6 +4,7 @@ const mailer = require('../../../services/mailer');
 const Context = require('../../../graph/context');
 const timekeeper = require('timekeeper');
 const moment = require('moment');
+const uuid = require('uuid/v1');
 
 const chai = require('chai');
 chai.use(require('chai-as-promised'));
@@ -387,6 +388,64 @@ describe('services.UsersService', () => {
       expect(user.wasUpserted).to.be.false;
     });
 
+    it('should handle legacy users as well', async () => {
+      const ctx = Context.forSystem();
+      let user = await UsersService.upsertExternalUser(
+        ctx,
+        'an-id',
+        'a-provider',
+        'a-display-name'
+      );
+
+      expect(user).to.be.defined;
+      expect(user).to.have.property('id', 'an-id');
+      expect(user.wasUpserted).to.be.true;
+
+      // Change the ID to something else, mirroring the legacy behavior.
+      const id = uuid();
+      user.id = id;
+      await user.save();
+      expect(user).to.have.property('id', id);
+
+      // Ensure that the ID has been changed.
+      user = await UsersService.findById(id);
+
+      expect(user).to.be.defined;
+      expect(user).to.have.property('id', id);
+
+      // Test to see that future lookups work.
+      user = await UsersService.upsertExternalUser(
+        ctx,
+        'an-id',
+        'a-provider',
+        'a-display-name'
+      );
+
+      expect(user).to.be.defined;
+      expect(user).to.have.property('id', id);
+      expect(user.wasUpserted).to.be.false;
+    });
+
+    it('should handle token user lookups created via this method', async () => {
+      const id = uuid();
+      const ctx = Context.forSystem();
+      let user = await UsersService.upsertExternalUser(
+        ctx,
+        id,
+        'a-provider',
+        'a-display-name'
+      );
+
+      expect(user).to.be.defined;
+      expect(user).to.have.property('id', id);
+      expect(user.wasUpserted).to.be.true;
+
+      user = await UsersService.findOrCreateByIDToken(id, {});
+
+      expect(user).to.be.defined;
+      expect(user).to.have.property('id', id);
+    });
+
     it('should return a user when the desired user is not found', async () => {
       const ctx = Context.forSystem();
       let user = await UsersService.upsertExternalUser(