From 1ea9be8aa294451391e1776ce6e2642403ec2911 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Thu, 12 Apr 2018 21:17:47 +0200 Subject: [PATCH] Preparatory Work for Smaller user doc (WIP) (#10245) * protect all paths in user.pre(save using this.isDirectSelected to see if a field is available * fix linting * authWithHeaders: specify user fields to exclude instead of the ones to include, add comments, doc and improve test * add more options to unit helper generateReq and add tests for excluding fields in authWithHeaders --- test/api/v3/integration/user/GET-user.test.js | 2 + test/api/v3/unit/middlewares/auth.test.js | 40 ++++++++++++++++++ test/helpers/api-unit.helper.js | 9 +++- website/server/controllers/api-v3/user.js | 5 +++ website/server/middlewares/auth.js | 23 ++++++---- website/server/models/user/hooks.js | 42 +++++++++---------- 6 files changed, 90 insertions(+), 31 deletions(-) create mode 100644 test/api/v3/unit/middlewares/auth.test.js diff --git a/test/api/v3/integration/user/GET-user.test.js b/test/api/v3/integration/user/GET-user.test.js index 555672b079..fc86e779db 100644 --- a/test/api/v3/integration/user/GET-user.test.js +++ b/test/api/v3/integration/user/GET-user.test.js @@ -34,6 +34,8 @@ describe('GET /user', () => { expect(returnedUser._id).to.equal(user._id); expect(returnedUser.achievements).to.exist; expect(returnedUser.items.mounts).to.exist; + // Notifications are always returned + expect(returnedUser.notifications).to.exist; expect(returnedUser.stats).to.not.exist; }); }); diff --git a/test/api/v3/unit/middlewares/auth.test.js b/test/api/v3/unit/middlewares/auth.test.js new file mode 100644 index 0000000000..c2b3a35993 --- /dev/null +++ b/test/api/v3/unit/middlewares/auth.test.js @@ -0,0 +1,40 @@ +import { + generateRes, + generateReq, +} from '../../../../helpers/api-unit.helper'; +import { authWithHeaders as authWithHeadersFactory } from '../../../../../website/server/middlewares/auth'; + +describe('auth middleware', () => { + let res, req, user; + + beforeEach(async () => { + res = generateRes(); + req = generateReq(); + user = await res.locals.user.save(); + }); + + describe('auth with headers', () => { + it('allows to specify a list of user field that we do not want to load', (done) => { + const authWithHeaders = authWithHeadersFactory(false, { + userFieldsToExclude: ['items', 'flags', 'auth.timestamps'], + }); + + req.headers['x-api-user'] = user._id; + req.headers['x-api-key'] = user.apiToken; + + authWithHeaders(req, res, (err) => { + if (err) return done(err); + + const userToJSON = res.locals.user.toJSON(); + expect(userToJSON.items).to.not.exist; + expect(userToJSON.flags).to.not.exist; + expect(userToJSON.auth.timestamps).to.not.exist; + expect(userToJSON.auth).to.exist; + expect(userToJSON.notifications).to.exist; + expect(userToJSON.preferences).to.exist; + + done(); + }); + }); + }); +}); diff --git a/test/helpers/api-unit.helper.js b/test/helpers/api-unit.helper.js index d32423dfb9..6e5c88891d 100644 --- a/test/helpers/api-unit.helper.js +++ b/test/helpers/api-unit.helper.js @@ -54,10 +54,15 @@ export function generateReq (options = {}) { body: {}, query: {}, headers: {}, - header: sandbox.stub().returns(null), + header (header) { + return this.headers[header]; + }, + session: {}, }; - return defaultsDeep(options, defaultReq); + const req = defaultsDeep(options, defaultReq); + + return req; } export function generateNext (func) { diff --git a/website/server/controllers/api-v3/user.js b/website/server/controllers/api-v3/user.js index 1ae8a17dfb..bf5009de87 100644 --- a/website/server/controllers/api-v3/user.js +++ b/website/server/controllers/api-v3/user.js @@ -55,6 +55,11 @@ let api = {}; * Tags * TasksOrder (list of all ids for dailys, habits, rewards and todos) * + * @apiParam (Query) {UUID} userFields A list of comma separated user fields to be returned instead of the entire document. Notifications are always returned. + * + * @apiExample {curl} Example use: + * curl -i https://habitica.com/api/v3/user?userFields=achievements,items.mounts + * * @apiSuccess {Object} data The user object * * @apiSuccessExample {json} Result: diff --git a/website/server/middlewares/auth.js b/website/server/middlewares/auth.js index b86f525d35..91c3c67eae 100644 --- a/website/server/middlewares/auth.js +++ b/website/server/middlewares/auth.js @@ -9,13 +9,22 @@ import url from 'url'; const COMMUNITY_MANAGER_EMAIL = nconf.get('EMAILS:COMMUNITY_MANAGER_EMAIL'); -function getUserFields (userFieldProjection, req) { - if (userFieldProjection) return `notifications ${userFieldProjection}`; +function getUserFields (userFieldsToExclude, req) { + // A list of user fields that aren't needed for the route and are not loaded from the db. + // Must be an array + if (userFieldsToExclude) { + return userFieldsToExclude.map(field => { + return `-${field}`; // -${field} means exclude ${field} in mongodb + }).join(' '); + } + // Allows GET requests to /user to specify a list of user fields to return instead of the entire doc + // Notifications are always included const urlPath = url.parse(req.url).pathname; - if (!req.query.userFields || urlPath !== '/user') return ''; + const userFields = req.query.userFields; + if (!userFields || urlPath !== '/user') return ''; - const userFieldOptions = req.query.userFields.split(','); + const userFieldOptions = userFields.split(','); if (userFieldOptions.length === 0) return ''; return `notifications ${userFieldOptions.join(' ')}`; @@ -25,7 +34,7 @@ function getUserFields (userFieldProjection, req) { // Authenticate a request through the x-api-user and x-api key header // If optional is true, don't error on missing authentication -export function authWithHeaders (optional = false, userFieldProjection = '') { +export function authWithHeaders (optional = false, options = {}) { return function authWithHeadersHandler (req, res, next) { let userId = req.header('x-api-user'); let apiToken = req.header('x-api-key'); @@ -40,8 +49,8 @@ export function authWithHeaders (optional = false, userFieldProjection = '') { apiToken, }; - const fields = getUserFields(userFieldProjection, req); - const findPromise = fields ? User.findOne(userQuery, fields) : User.findOne(userQuery); + const fields = getUserFields(options.userFieldsToExclude, req); + const findPromise = fields ? User.findOne(userQuery).select(fields) : User.findOne(userQuery); return findPromise .exec() diff --git a/website/server/models/user/hooks.js b/website/server/models/user/hooks.js index 0ebcfe6733..81d4e8b06d 100644 --- a/website/server/models/user/hooks.js +++ b/website/server/models/user/hooks.js @@ -208,19 +208,11 @@ schema.pre('save', true, function preSaveUser (next, done) { // we do not want to run any hook that relies on user.items because it will // use the default values defined in the user schema and not the real ones. // - // To check if a field was selected Document.isSelected('field') can be used. - // more info on its usage can be found at http://mongoosejs.com/docs/api.html#document_Document-isSelected - // IMPORTANT NOTE2 : due to a bug in mongoose (https://github.com/Automattic/mongoose/issues/5063) - // document.isSelected('items') will return true even if only a sub field (like 'items.mounts') - // was selected. So this fix only works as long as the entire subdoc is selected - // For example in the code below it won't work if only `achievements.beastMasterCount` is selected - // which is why we should only ever select the full paths and not subdocs, - // or if we really have to do the document.isSelected() calls should check for - // every specific subpath (items.mounts, items.pets, ...) but it's better to avoid it - // since it'll break as soon as a new field is added to the schema but not here. + // To check if a field was selected Document.isDirectSelected('field') can be used. + // more info on its usage can be found at http://mongoosejs.com/docs/api.html#document_Document-isDirectSelected // do not calculate achievements if items or achievements are not selected - if (this.isSelected('items') && this.isSelected('achievements')) { + if (this.isDirectSelected('items') && this.isDirectSelected('achievements')) { // Determines if Beast Master should be awarded let beastMasterProgress = common.count.beastMasterProgress(this.items.pets); @@ -250,7 +242,7 @@ schema.pre('save', true, function preSaveUser (next, done) { } // Manage unallocated stats points notifications - if (this.isSelected('stats') && this.isSelected('notifications') && this.isSelected('flags') && this.isSelected('preferences')) { + if (this.isDirectSelected('stats') && this.isDirectSelected('notifications') && this.isDirectSelected('flags') && this.isDirectSelected('preferences')) { const pointsToAllocate = this.stats.points; const classNotEnabled = !this.flags.classSelected || this.preferences.disableClasses; @@ -287,21 +279,27 @@ schema.pre('save', true, function preSaveUser (next, done) { } } - // Enable weekly recap emails for old users who sign in - if (this.flags.lastWeeklyRecapDiscriminator) { - // Enable weekly recap emails in 24 hours - this.flags.lastWeeklyRecap = moment().subtract(6, 'days').toDate(); - // Unset the field so this is run only once - this.flags.lastWeeklyRecapDiscriminator = undefined; + if (this.isDirectSelected('flags')) { + // Enable weekly recap emails for old users who sign in + if (this.flags.lastWeeklyRecapDiscriminator) { + // Enable weekly recap emails in 24 hours + this.flags.lastWeeklyRecap = moment().subtract(6, 'days').toDate(); + // Unset the field so this is run only once + this.flags.lastWeeklyRecapDiscriminator = undefined; + } } - if (_.isNaN(this.preferences.dayStart) || this.preferences.dayStart < 0 || this.preferences.dayStart > 23) { - this.preferences.dayStart = 0; + if (this.isDirectSelected('preferences')) { + if (_.isNaN(this.preferences.dayStart) || this.preferences.dayStart < 0 || this.preferences.dayStart > 23) { + this.preferences.dayStart = 0; + } } // our own version incrementer - if (_.isNaN(this._v) || !_.isNumber(this._v)) this._v = 0; - this._v++; + if (this.isDirectSelected('_v')) { + if (_.isNaN(this._v) || !_.isNumber(this._v)) this._v = 0; + this._v++; + } // Populate new users with default content if (this.isNew) {