fix: Centralise user deserialization

api-server/server/boot_tests/fixtures.js

@@ -1,17 +1,20 @@
 export const firstChallengeUrl = '/learn/the/first/challenge';
 export const requestedChallengeUrl = '/learn/my/actual/challenge';
+
 export const mockChallenge = {
   id: '123abc',
   block: 'actual',
   superBlock: 'my',
   dashedName: 'challenge'
 };
+
 export const mockFirstChallenge = {
   id: '456def',
   block: 'first',
   superBlock: 'the',
   dashedName: 'challenge'
 };
+
 export const mockCompletedChallenge = {
   id: '890xyz',
   challengeType: 0,
@@ -26,6 +29,7 @@ export const mockCompletedChallenge = {
   ],
   completedDate: Date.now()
 };
+
 export const mockCompletedChallenges = [
   {
     id: 'bd7123c8c441eddfaeb5bdef',
@@ -57,12 +61,16 @@ export const mockCompletedChallenges = [
     files: []
   }
 ];
+export const mockUserID = '5c7d892aff9777c8b1c1a95e';
 export const mockUser = {
+  id: mockUserID,
   username: 'camperbot',
   currentChallengeId: '123abc',
   timezone: 'UTC',
-  completedChallenges: mockCompletedChallenges
+  completedChallenges: mockCompletedChallenges,
+  progressTimestamps: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
 };
+
 export const mockApp = {
   models: {
     Challenge: {
@@ -74,10 +82,20 @@ export const mockApp = {
           ? cb(null, mockChallenge)
           : cb(new Error('challenge not found'));
       }
+    },
+    User: {
+      findById(id, cb) {
+        if (id === mockUser.id) {
+          return cb(null, mockUser);
+        }
+        return cb(Error('No user'));
+      }
     }
   }
 };
+
 export const mockGetFirstChallenge = () => firstChallengeUrl;
+
 export const firstChallengeQuery = {
   // first challenge of the first block of the first superBlock
   where: { challengeOrder: 0, superOrder: 1, order: 0 }

api-server/server/component-passport.js

@@ -7,6 +7,7 @@ import {
 import url from 'url';
 import dedent from 'dedent';
 
+import { getUserById } from './utils/user-stats';
 import { homeLocation } from '../../config/env';
 import passportProviders from './passport-providers';
 import { setAccessTokenToResponse } from './utils/getSetAccessToken';
@@ -16,28 +17,6 @@ const passportOptions = {
   profileToUser: null
 };
 
-const fields = {
-  progressTimestamps: false
-};
-
-function getCompletedCertCount(user) {
-  return [
-    'isApisMicroservicesCert',
-    'is2018DataVisCert',
-    'isFrontEndLibsCert',
-    'isInfosecQaCert',
-    'isJsAlgoDataStructCert',
-    'isRespWebDesignCert'
-  ].reduce((sum, key) => (user[key] ? sum + 1 : sum), 0);
-}
-
-function getLegacyCertCount(user) {
-  return ['isFrontEndCert', 'isBackEndCert', 'isDataVisCert'].reduce(
-    (sum, key) => (user[key] ? sum + 1 : sum),
-    0
-  );
-}
-
 PassportConfigurator.prototype.init = function passportInit(noSession) {
   this.app.middleware('session:after', passport.initialize());
 
@@ -50,62 +29,15 @@ PassportConfigurator.prototype.init = function passportInit(noSession) {
   // Serialization and deserialization is only required if passport session is
   // enabled
 
-  passport.serializeUser((user, done) => {
+  passport.serializeUser((user, done) => done(null, user.id));
-    done(null, user.id);
-  });
 
-  passport.deserializeUser((id, done) => {
+  passport.deserializeUser(async (id, done) => {
-    this.userModel.findById(id, { fields }, (err, user) => {
+    const user = await getUserById(id).catch(done);
-      if (err || !user) {
+    return done(null, user);
-        return done(err, user);
-      }
-
-      return this.app.dataSources.db.connector
-        .collection('user')
-        .aggregate([
-          { $match: { _id: user.id } },
-          { $project: { points: { $size: '$progressTimestamps' } } }
-        ])
-        .get(function(err, [{ points = 1 } = {}]) {
-          if (err) {
-            console.error(err);
-            return done(err);
-          }
-          user.points = points;
-          let completedChallengeCount = 0;
-          let completedProjectCount = 0;
-          if ('completedChallenges' in user) {
-            completedChallengeCount = user.completedChallenges.length;
-            user.completedChallenges.forEach(item => {
-              if (
-                'challengeType' in item &&
-                (item.challengeType === 3 || item.challengeType === 4)
-              ) {
-                completedProjectCount++;
-              }
-            });
-          }
-          user.completedChallengeCount = completedChallengeCount;
-          user.completedProjectCount = completedProjectCount;
-          user.completedCertCount = getCompletedCertCount(user);
-          user.completedLegacyCertCount = getLegacyCertCount(user);
-          user.completedChallenges = [];
-          return done(null, user);
-        });
-    });
   });
 };
 
 export function setupPassport(app) {
-  // NOTE(Bouncey): Not sure this is doing much now
-  // Loopback complains about userCredentialModle when this
-  // setup is remoed from server/server.js
-  //
-  // I have split the custom callback in to it's own export that we can use both
-  // here and in boot:auth
-  //
-  // Needs more investigation...
-
   const configurator = new PassportConfigurator(app);
 
   configurator.setupModels({

api-server/server/middlewares/request-authorizaion.test.js

@@ -111,7 +111,7 @@ describe('request-authorization', () => {
       });
 
       it('adds the user to the request object', async done => {
-        expect.assertions(5);
+        expect.assertions(3);
         const validJWT = jwt.sign({ accessToken }, validJWTSecret);
         const req = mockReq({
           path: '/internal/some-path/that-needs/auth',
@@ -124,8 +124,6 @@ describe('request-authorization', () => {
         expect(next.called).toBe(true);
         expect(req).toHaveProperty('user');
         expect(req.user).toEqual(users['456def']);
-        expect(req.user).toHaveProperty('points');
-        expect(req.user.points).toEqual(4);
         return done();
       });
 
@@ -200,7 +198,7 @@ describe('request-authorization', () => {
       });
 
       it('adds the user to the request object', async done => {
-        expect.assertions(5);
+        expect.assertions(3);
         const validJWT = jwt.sign({ accessToken }, validJWTSecret);
         const req = mockReq({
           path: '/internal/some-path/that-needs/auth',
@@ -212,8 +210,6 @@ describe('request-authorization', () => {
         expect(next.called).toBe(true);
         expect(req).toHaveProperty('user');
         expect(req.user).toEqual(users['456def']);
-        expect(req.user).toHaveProperty('points');
-        expect(req.user.points).toEqual(4);
         return done();
       });
 

api-server/server/middlewares/request-authorization.js

@@ -1,5 +1,6 @@
-import loopback from 'loopback';
 import { isEmpty } from 'lodash';
+
+import { getUserById as _getUserById } from '../utils/user-stats';
 import {
   getAccessTokenFromRequest,
   errorTypes,
@@ -63,7 +64,6 @@ export default ({ jwtSecret = _jwtSecret, getUserById = _getUserById } = {}) =>
         return getUserById(userId)
           .then(user => {
             if (user) {
-              user.points = user.progressTimestamps.length;
               req.user = user;
             }
             return;
@@ -76,15 +76,3 @@ export default ({ jwtSecret = _jwtSecret, getUserById = _getUserById } = {}) =>
     }
     return Promise.resolve(next());
   };
-
-export function _getUserById(id) {
-  const User = loopback.getModelByType('User');
-  return new Promise((resolve, reject) =>
-    User.findById(id, (err, instance) => {
-      if (err) {
-        return reject(err);
-      }
-      return resolve(instance);
-    })
-  );
-}

api-server/server/utils/user-stats.js

@@ -1,10 +1,13 @@
+import loopback from 'loopback';
 import compose from 'lodash/fp/compose';
 import map from 'lodash/fp/map';
 import sortBy from 'lodash/fp/sortBy';
 import trans from 'lodash/fp/transform';
 import last from 'lodash/fp/last';
 import forEachRight from 'lodash/fp/forEachRight';
+import { isEmpty } from 'lodash';
 import moment from 'moment-timezone';
+
 import { dayCount } from '../utils/date-utils';
 
 const transform = trans.convert({ cap: false });
@@ -100,3 +103,54 @@ export function calcLongestStreak(cals, tz = 'UTC') {
 
   return dayCount(longest, tz);
 }
+
+export function getUserById(id, User = loopback.getModelByType('User')) {
+  return new Promise((resolve, reject) =>
+    User.findById(id, (err, instance) => {
+      if (err || isEmpty(instance)) {
+        return reject(err || 'No user instance found');
+      }
+      instance.points =
+        (instance.progressTimestamps && instance.progressTimestamps.length) ||
+        1;
+      let completedChallengeCount = 0;
+      let completedProjectCount = 0;
+      if ('completedChallenges' in instance) {
+        completedChallengeCount = instance.completedChallenges.length;
+        instance.completedChallenges.forEach(item => {
+          if (
+            'challengeType' in item &&
+            (item.challengeType === 3 || item.challengeType === 4)
+          ) {
+            completedProjectCount++;
+          }
+        });
+      }
+      instance.completedChallengeCount = completedChallengeCount;
+      instance.completedProjectCount = completedProjectCount;
+      instance.completedCertCount = getCompletedCertCount(instance);
+      instance.completedLegacyCertCount = getLegacyCertCount(instance);
+      instance.completedChallenges = [];
+      delete instance.progressTimestamps;
+      return resolve(instance);
+    })
+  );
+}
+
+function getCompletedCertCount(user) {
+  return [
+    'isApisMicroservicesCert',
+    'is2018DataVisCert',
+    'isFrontEndLibsCert',
+    'isInfosecQaCert',
+    'isJsAlgoDataStructCert',
+    'isRespWebDesignCert'
+  ].reduce((sum, key) => (user[key] ? sum + 1 : sum), 0);
+}
+
+function getLegacyCertCount(user) {
+  return ['isFrontEndCert', 'isBackEndCert', 'isDataVisCert'].reduce(
+    (sum, key) => (user[key] ? sum + 1 : sum),
+    0
+  );
+}

api-server/server/utils/user-stats.test.js

@@ -5,8 +5,10 @@ import sinon from 'sinon';
 import {
   prepUniqueDaysByHours,
   calcCurrentStreak,
-  calcLongestStreak
+  calcLongestStreak,
+  getUserById
 } from './user-stats';
+import { mockUserID, mockApp, mockUser } from '../boot_tests/fixtures';
 
 // setting now to 2016-02-03T11:00:00 (PST)
 const clock = sinon.useFakeTimers(1454526000000);
@@ -577,4 +579,51 @@ describe('user stats', () => {
       }
     );
   });
+
+  describe('getUserById', () => {
+    const stubUser = {
+      findById(id, cb) {
+        cb(null, { id: 123 });
+      }
+    };
+    it('returns a promise', () => {
+      expect.assertions(3);
+      expect(typeof getUserById('123', stubUser).then).toEqual('function');
+      expect(typeof getUserById('123', stubUser).catch).toEqual('function');
+      expect(typeof getUserById('123', stubUser).finally).toEqual('function');
+    });
+
+    it('resolves a user for a given id', done => {
+      expect.assertions(7);
+      return getUserById(mockUserID, mockApp.models.User)
+        .then(user => {
+          expect(user).toEqual(mockUser);
+
+          // fields added or removed by getUserById
+          expect(user).not.toHaveProperty('progressTimestamps');
+          expect(user).toHaveProperty('completedChallengeCount');
+          expect(user).toHaveProperty('completedProjectCount');
+          expect(user).toHaveProperty('completedCertCount');
+          expect(user).toHaveProperty('completedLegacyCertCount');
+          expect(user.completedChallenges).toEqual([]);
+        })
+        .then(done)
+        .catch(done);
+    });
+
+    it('throws when no user is found', done => {
+      const noUserError = new Error('No user found');
+      const throwyUserModel = {
+        findById(_, cb) {
+          cb(noUserError);
+        }
+      };
+      expect(
+        getUserById('not-a-real-id', throwyUserModel).catch(error => {
+          expect(error).toEqual(noUserError);
+          done();
+        })
+      );
+    });
+  });
 });