Merge pull request #139 from cboard-org/revert-138-revert-137-136-fix…

…-user-update-vulnerability

Fix vulnerability with updateUser route

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) {

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;
 
 /**

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: [email protected]
+      birthdate:
+        type: string
+        format: date
+        example: 2001-10-17
+      locale:
+        type: string
+        enum:  *APP_LANGS
   Translate:
       type: object
       required:

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) {

test/controllers/user.js

@@ -3,24 +3,25 @@ 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 () {
   let authToken;
   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: '[email protected]',
+        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);
   });
 });

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<PrepareUserResponse>}
+ */
+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,
 };