From bbd4e9a64b64cdb15445442ac8e03198f1852de1 Mon Sep 17 00:00:00 2001 From: Simon Richard <46613478+sylvansson@users.noreply.github.com> Date: Sat, 27 Mar 2021 13:05:43 -0400 Subject: [PATCH] Revert "Revert "Fix vulnerability with updateUser route"" --- api/controllers/user.js | 18 +++++- api/models/User.js | 4 ++ api/swagger/swagger.yaml | 19 ++++++- test/controllers/board.js | 10 +--- test/controllers/user.js | 114 +++++++++++++++++++++++++++---------- test/helper.js | 117 ++++++++++++++++++-------------------- 6 files changed, 180 insertions(+), 102 deletions(-) diff --git a/api/controllers/user.js b/api/controllers/user.js index bea1a79a..b8647b62 100644 --- a/api/controllers/user.js +++ b/api/controllers/user.js @@ -242,8 +242,22 @@ async function getUser(req, res) { } } +const UPDATEABLE_FIELDS = [ + 'email', + 'name', + 'birthdate', + 'locale', +] + function updateUser(req, res) { const id = req.swagger.params.id.value; + + if (!req.user.isAdmin && req.auth.id !== id) { + return res.status(403).json({ + message: 'Only admins can update another user.' + }) + } + User.findById(id) .populate('communicators') .populate('boards') @@ -260,7 +274,9 @@ function updateUser(req, res) { }); } for (let key in req.body) { - user[key] = req.body[key]; + if (UPDATEABLE_FIELDS.includes(key)) { + user[key] = req.body[key]; + } } user.save(function(err, user) { if (err) { diff --git a/api/models/User.js b/api/models/User.js index b4fb470d..14ccb175 100644 --- a/api/models/User.js +++ b/api/models/User.js @@ -107,6 +107,10 @@ userSchema.virtual('boards', { foreignField: 'email' }); +userSchema.virtual('isAdmin').get(function() { + return this.role === 'admin'; +}); + const validatePresenceOf = value => value && value.length; /** diff --git a/api/swagger/swagger.yaml b/api/swagger/swagger.yaml index 86cb7a5e..a8016b45 100644 --- a/api/swagger/swagger.yaml +++ b/api/swagger/swagger.yaml @@ -798,7 +798,7 @@ paths: in: body required: true schema: - $ref: "#/definitions/User" + $ref: "#/definitions/UserUpdate" responses: "200": description: Success @@ -943,6 +943,23 @@ definitions: locale: type: string enum: &APP_LANGS ['ar-SA','bn-BD','cs-CZ','da-DK','de-DE','el-GR','en-US','en-GB','es-ES','fi-FI','fr-FR','he-IL','hi-IN','hu-HU','id-ID','it-IT','ja-JP','km-KH','ko-KR','ne-NP','nl-NL','no-NO','pl-PL','pt-BR','pt-PT','ro-RO','ru-RU','si-LK','sk-SK','sv-SE','th-TH','tr-TR','uk-UA','vi-VN','zh-CN','zu-ZA'] + UserUpdate: + type: object + properties: + name: + type: string + example: Alice + email: + type: string + format: email + example: alice@example.com + birthdate: + type: string + format: date + example: 2001-10-17 + locale: + type: string + enum: *APP_LANGS Translate: type: object required: diff --git a/test/controllers/board.js b/test/controllers/board.js index 577d8020..92caa9b9 100644 --- a/test/controllers/board.js +++ b/test/controllers/board.js @@ -18,13 +18,9 @@ describe('Board API calls', function () { var authToken; var boardId; - before(async function (done) { - this.timeout(5000); //to await the email server process - helper.prepareUser(server) - .then(token => { - authToken = token; - done(); - }); + before(async function() { + const res = await helper.prepareUser(server); + authToken = res.token; }); it('it should POST a board', function (done) { diff --git a/test/controllers/user.js b/test/controllers/user.js index 24d00c7e..4fdcd607 100644 --- a/test/controllers/user.js +++ b/test/controllers/user.js @@ -3,12 +3,16 @@ process.env.NODE_ENV = 'test'; const request = require('supertest'); const chai = require('chai'); var assert = chai.assert; +const expect = chai.expect; + +const uuid = require('uuid'); const server = require('../../app'); const helper = require('../helper'); const { copy } = require('../../app'); const nev = require('../../api/mail/index'); +const User = require('../../api/models/User'); //Parent block describe('User API calls', function () { @@ -16,11 +20,8 @@ describe('User API calls', function () { let url; let userid; - before(async function (done) { - helper.deleteMochaUser(server).then((token) => { - authToken = token; - done(); - }); + before(async function() { + await helper.deleteMochaUser(); }); it('it should to create a new temporary user', function (done) { @@ -126,20 +127,68 @@ describe('User API calls', function () { }); }); - it('it should update a specific user', function (done) { - request(server) - .put('/user/' + userid) - .send({ role: 'admin' }) - .set('Authorization', 'Bearer ' + authToken) - .set('Accept', 'application/json') - .expect('Content-Type', /json/) - .expect(200) - .end(function (err, res) { - if (err) done(err); - const updateUser = res.body; - updateUser.should.to.have.property('role').to.equal('admin'); - done(); + describe('PUT /user/:userId', function() { + it('only allows an admin user to update another user', async function() { + const admin = await helper.prepareUser(server, { + role: 'admin', + email: helper.generateEmail(), }); + + const user = await helper.prepareUser(server, { + role: 'user', + email: helper.generateEmail(), + }); + + // Try to update another user as a regular user. + // This should fail. + await request(server) + .put(`/user/${admin.userId}`) + .set('Authorization', `Bearer ${user.token}`) + .expect({ + message: 'Only admins can update another user.', + }) + .expect(403); + + // Try to update another user as an admin user. + // This should succeed. + await request(server) + .put(`/user/${user.userId}`) + .set('Authorization', `Bearer ${admin.token}`) + .expect(200); + }); + + it('only allows updating a subset of fields', async function() { + const user = await helper.prepareUser(server, { + role: 'user', + email: helper.generateEmail(), + }); + + const update = { + // Updateable. + email: 'alice@example.com', + name: 'Alice', + birthdate: '2001-10-17', + locale: 'klingon', + + // Not updateable. + role: 'foobar', + password: uuid.v4(), + }; + + const res = await request(server) + .put(`/user/${user.userId}`) + .send(update) + .set('Authorization', `Bearer ${user.token}`) + .expect(200); + + expect(res.body.email).to.equal(update.email); + expect(res.body.name).to.equal(update.name); + expect(res.body.birthdate).to.contain(update.birthdate); + expect(res.body.locale).to.equal(update.locale); + + expect(res.body.role).to.equal('user'); + expect(res.body.password).not.to.equal(update.password); + }); }); it('it should Destroys user session and authentication token', function (done) { @@ -221,18 +270,21 @@ describe('User API calls', function () { }); }); - it('it should delete a user', function (done) { - request(server) - .del('/user/' + helper.userForgotPassword.userid) - .set('Authorization', 'Bearer ' + authToken) - .set('Accept', 'application/json') - .expect('Content-Type', /json/) - .expect(200) - .end(function (err, res) { - if (err) done(err); - const userDeleted = res.body; - userDeleted.should.to.have.any.keys('name', 'email', 'locale'); - done(); - }); + it('it should delete a user', async function() { + const admin = await helper.prepareUser(server, { + role: 'admin', + email: helper.generateEmail(), + }); + + const targetUserId = helper.userForgotPassword.userid; + + expect(await User.exists({ _id: targetUserId })).to.equal(true); + + const res = await request(server) + .del(`/user/${targetUserId}`) + .set('Authorization', `Bearer ${admin.token}`) + .expect(200); + + expect(await User.exists({ _id: targetUserId })).to.equal(false); }); }); diff --git a/test/helper.js b/test/helper.js index e5ba8c16..fba922a6 100644 --- a/test/helper.js +++ b/test/helper.js @@ -1,5 +1,6 @@ //Require the dev-dependencies const chai = require('chai'); +const { Express } = require('express'); const mongoose = require('mongoose'); const { token } = require('morgan'); var request = require('supertest'); @@ -94,70 +95,61 @@ function prepareDb() { }); } -function prepareUser(server) { - var token; - var url; - return new Promise((resolve, reject) => { - request(server) - .post('/user') - .send(userData) - .expect(200) - .expect(function (res) { - url = res.body.url; - }) - .end(function () { - request(server) - .post('/user/activate/' + url) - .send('') - .expect(200) - .end(function (err, res) { - userid = res.body.userid; - request(server) - .post('/user/login') - .send(userData) - .expect(200) - .expect(function (res) { - token = res.body.authToken; - }) - .end(function () { - resolve(token, userid); - }); - }); - }); - }); +function generateEmail() { + return `test${Date.now()}@example.com`; } -function deleteMochaUser(server) { - let authToken; - return new Promise((resolve, reject) => { - User.findOne({ email: userData.email }, function (err, user) { - userid = user._id; - request(server) - .post('/user/login') - .send(userData) - .expect(200) - .expect(function (res) { - authToken = res.body.authToken; - }) - .end(function () { - request(server) - .put('/user/' + userid) - .set('Authorization', 'Bearer ' + authToken) - .send({ role: 'admin' }) - .end(function () { - request(server) - .del('/user/' + userid) - .set('Authorization', 'Bearer ' + authToken) - .set('Accept', 'application/json') - .expect('Content-Type', /json/) - .expect(200) - .end(function () { - resolve(); - }); - }); - }); - }); - }); +/** + * A newly created test user. + * @typedef {Object} PrepareUserResponse + * + * @property {string} token + * @property {string} userId + */ + +/** + * Create a test user and generate a token for them. + * + * @param {Express} server + * + * @param {Object} options + * @param {Object} options.overrides - Properties to overwrite the default + * user data with. + * + * @returns {Promise} + */ +async function prepareUser(server, overrides = {}) { + const data = { + ...userData, + ...overrides, + }; + + const createUser = await request(server) + .post('/user') + .send(data) + .expect(200); + + const activationUrl = createUser.body.url; + + const activateUser = await request(server) + .post(`/user/activate/${activationUrl}`) + .send('') + .expect(200); + + const userId = activateUser.body.userid; + + const login = await request(server) + .post('/user/login') + .send(data) + .expect(200); + + const token = login.body.authToken; + + return { token, userId }; +} + +async function deleteMochaUser() { + return User.deleteOne({ email: userData.email }); } module.exports = { @@ -170,4 +162,5 @@ module.exports = { boardData, userData, userForgotPassword, + generateEmail: generateEmail, };