From 00fa492c8651ac63d8ed1b99b21c4b2ae4e87690 Mon Sep 17 00:00:00 2001 From: Chris Troutner Date: Thu, 16 May 2019 15:24:55 -0700 Subject: [PATCH] Ported admin-type user validatoin from p2p vps server --- src/middleware/validators.js | 106 +++++++++++++++++++++++++++++--- src/models/users.js | 16 ++--- src/modules/users/controller.js | 12 ++++ src/modules/users/router.js | 10 +-- test/a01-auth.spec.js | 21 ++++--- test/a02-users.spec.js | 8 +-- 6 files changed, 141 insertions(+), 32 deletions(-) diff --git a/src/middleware/validators.js b/src/middleware/validators.js index 2a5a2d8..310351f 100644 --- a/src/middleware/validators.js +++ b/src/middleware/validators.js @@ -3,30 +3,122 @@ const config = require('../../config') const getToken = require('../utils/auth') const jwt = require('jsonwebtoken') -module.exports = async function ensureUser (ctx, next) { - //console.log(`getToken: ${typeof (getToken)}`) +async function ensureUser (ctx, next) { + // console.log(`getToken: ${typeof (getToken)}`) const token = getToken(ctx) if (!token) { - //console.log(`Err: Token not provided.`) + // console.log(`Err: Token not provided.`) ctx.throw(401) } let decoded = null try { - //console.log(`token: ${JSON.stringify(token, null, 2)}`) - //console.log(`config: ${JSON.stringify(config, null, 2)}`) + // console.log(`token: ${JSON.stringify(token, null, 2)}`) + // console.log(`config: ${JSON.stringify(config, null, 2)}`) decoded = jwt.verify(token, config.token) } catch (err) { - //console.log(`Err: Token could not be decoded: ${err}`) + // console.log(`Err: Token could not be decoded: ${err}`) ctx.throw(401) } ctx.state.user = await User.findById(decoded.id, '-password') if (!ctx.state.user) { - //console.log(`Err: Could not find user.`) + // console.log(`Err: Could not find user.`) ctx.throw(401) } return next() } + +// This funciton is almost identical to ensureUser, except at the end, it verifies +// that the 'type' associated with the user equals 'admin'. +async function ensureAdmin (ctx, next) { + // console.log(`getToken: ${typeof (getToken)}`) + const token = getToken(ctx) + + if (!token) { + // console.log(`Err: Token not provided.`) + ctx.throw(401) + } + + let decoded = null + try { + // console.log(`token: ${JSON.stringify(token, null, 2)}`) + // console.log(`config: ${JSON.stringify(config, null, 2)}`) + decoded = jwt.verify(token, config.token) + } catch (err) { + // console.log(`Err: Token could not be decoded: ${err}`) + ctx.throw(401) + } + + ctx.state.user = await User.findById(decoded.id, '-password') + if (!ctx.state.user) { + // console.log(`Err: Could not find user.`) + ctx.throw(401) + } + + if (ctx.state.user.type !== 'admin') { + ctx.throw(401, 'not admin') + } + + return next() +} + +// This middleware ensures that the :id used in the API endpoint matches the +// the ID used in the JWT, or failing that, the ID used in the JWT matches +// an Admin user. This prevents situations like users updating other users +// profiles or non-admins deleting users. +// TODO Tests must be developed before developing this function. +async function ensureTargetUserOrAdmin (ctx, next) { + // console.log(`getToken: ${typeof (getToken)}`) + const token = getToken(ctx) + + if (!token) { + // console.log(`Err: Token not provided.`) + ctx.throw(401) + } + + // The user ID targeted in this API call. + const targetId = ctx.params.id + // console.log(`targetId: ${JSON.stringify(targetId, null, 2)}`) + + let decoded = null + try { + // console.log(`token: ${JSON.stringify(token, null, 2)}`) + // console.log(`config: ${JSON.stringify(config, null, 2)}`) + decoded = jwt.verify(token, config.token) + } catch (err) { + // console.log(`Err: Token could not be decoded: ${err}`) + ctx.throw(401) + } + + ctx.state.user = await User.findById(decoded.id, '-password') + if (!ctx.state.user) { + // console.log(`Err: Could not find user.`) + ctx.throw(401) + } + + // console.log(`ctx.state.user: ${JSON.stringify(ctx.state.user, null, 2)}`) + // Ensure the calling user and the target user are the same. + if (ctx.state.user._id.toString() !== targetId.toString()) { + console.log(`Calling user and target user do not match!`) + console.log(`Calling user: ${ctx.state.user._id}`) + console.log(`Target user: ${targetId}`) + + // If they don't match, then the calling user better be an admin. + if (ctx.state.user.type !== 'admin') { + ctx.throw(401, 'not admin') + } else { + console.log(`It's ok. The user is an admin.`) + } + } + + return next() +} + +module.exports = { + ensureUser, + ensureAdmin, + ensureTargetUserOrAdmin +} diff --git a/src/models/users.js b/src/models/users.js index fb20037..0d8205b 100644 --- a/src/models/users.js +++ b/src/models/users.js @@ -4,7 +4,7 @@ const config = require('../../config') const jwt = require('jsonwebtoken') const User = new mongoose.Schema({ - type: { type: String, default: 'User' }, + type: { type: String, default: 'user' }, name: { type: String }, username: { type: String, required: true, unique: true }, password: { type: String, required: true } @@ -23,16 +23,16 @@ User.pre('save', function preSave (next) { resolve(salt) }) }) - .then(salt => { - bcrypt.hash(user.password, salt, (err, hash) => { - if (err) { throw new Error(err) } + .then(salt => { + bcrypt.hash(user.password, salt, (err, hash) => { + if (err) { throw new Error(err) } - user.password = hash + user.password = hash - next(null) + next(null) + }) }) - }) - .catch(err => next(err)) + .catch(err => next(err)) }) User.methods.validatePassword = function validatePassword (password) { diff --git a/src/modules/users/controller.js b/src/modules/users/controller.js index 3ce1d02..2ce703c 100644 --- a/src/modules/users/controller.js +++ b/src/modules/users/controller.js @@ -16,6 +16,7 @@ const User = require('../../models/users') * * @apiSuccess {Object} users User object * @apiSuccess {ObjectId} users._id User id + * @apiSuccess {String} user.type User type (admin or user) * @apiSuccess {String} users.name User name * @apiSuccess {String} users.username User username * @@ -69,6 +70,7 @@ async function createUser (ctx) { * * @apiSuccess {Object[]} users Array of user objects * @apiSuccess {ObjectId} users._id User id + * @apiSuccess {String} user.type User type (admin or user) * @apiSuccess {String} users.name User name * @apiSuccess {String} users.username User username * @@ -101,6 +103,7 @@ async function getUsers (ctx) { * * @apiSuccess {Object} users User object * @apiSuccess {ObjectId} users._id User id + * @apiSuccess {String} user.type User type (admin or user) * @apiSuccess {String} users.name User name * @apiSuccess {String} users.username User username * @@ -153,6 +156,7 @@ async function getUser (ctx, next) { * * @apiSuccess {Object} users User object * @apiSuccess {ObjectId} users._id User id + * @apiSuccess {String} user.type User type (admin or user) * @apiSuccess {String} users.name Updated name * @apiSuccess {String} users.username Updated username * @@ -180,8 +184,16 @@ async function getUser (ctx, next) { async function updateUser (ctx) { const user = ctx.body.user + // Save a copy of the original user type. + const userType = user.type + Object.assign(user, ctx.request.body.user) + // Unless the calling user is an admin, they can not change the user type. + if (userType !== 'admin') { + user.type = userType + } + await user.save() ctx.body = { diff --git a/src/modules/users/router.js b/src/modules/users/router.js index c8c43b0..a2c814c 100644 --- a/src/modules/users/router.js +++ b/src/modules/users/router.js @@ -1,4 +1,4 @@ -const ensureUser = require('../../middleware/validators') +const validator = require('../../middleware/validators') const user = require('./controller') // export const baseUrl = '/users' @@ -16,7 +16,7 @@ module.exports.routes = [ method: 'GET', route: '/', handlers: [ - ensureUser, + validator.ensureUser, user.getUsers ] }, @@ -24,7 +24,7 @@ module.exports.routes = [ method: 'GET', route: '/:id', handlers: [ - ensureUser, + validator.ensureUser, user.getUser ] }, @@ -32,7 +32,7 @@ module.exports.routes = [ method: 'PUT', route: '/:id', handlers: [ - ensureUser, + validator.ensureTargetUserOrAdmin, user.getUser, user.updateUser ] @@ -41,7 +41,7 @@ module.exports.routes = [ method: 'DELETE', route: '/:id', handlers: [ - ensureUser, + validator.ensureTargetUserOrAdmin, user.getUser, user.deleteUser ] diff --git a/test/a01-auth.spec.js b/test/a01-auth.spec.js index 90d474d..a84afc2 100644 --- a/test/a01-auth.spec.js +++ b/test/a01-auth.spec.js @@ -8,18 +8,15 @@ const utils = require('./utils') const rp = require('request-promise') const assert = require('chai').assert - + should() // const request = supertest.agent(app.listen()) const context = {} const LOCALHOST = 'http://localhost:5000' - - describe('Auth', () => { before(async () => { - await app.startServer() utils.cleanDb() @@ -60,7 +57,7 @@ describe('Auth', () => { let result = await rp(options) - // console.log(`result: ${JSON.stringify(result, null, 2)}`) + // console.log(`result: ${JSON.stringify(result, null, 2)}`) console.log(`result stringified: ${JSON.stringify(result, null, 2)}`) assert(false, 'Unexpected result') @@ -95,10 +92,18 @@ describe('Auth', () => { // console.log(`result: ${JSON.stringify(result, null, 2)}`) assert(result.statusCode === 200, 'Status Code 200 expected.') - assert(result.body.user.username === 'test', 'Username of test expected') - assert(result.body.user.password === undefined, 'Password expected to be omited') + assert( + result.body.user.username === 'test', + 'Username of test expected' + ) + assert( + result.body.user.password === undefined, + 'Password expected to be omited' + ) } catch (err) { - console.log('Error authenticating test user: ' + JSON.stringify(err, null, 2)) + console.log( + 'Error authenticating test user: ' + JSON.stringify(err, null, 2) + ) throw err } }) diff --git a/test/a02-users.spec.js b/test/a02-users.spec.js index c8857ba..0ac658e 100644 --- a/test/a02-users.spec.js +++ b/test/a02-users.spec.js @@ -277,7 +277,7 @@ describe('Users', () => { } }) - it("should throw 404 if user doesn't exist", async () => { + it('should throw 401 if non-admin updating other user', async () => { const { token } = context try { @@ -295,7 +295,7 @@ describe('Users', () => { await rp(options) assert.equal(true, false, 'Unexpected behavior') } catch (err) { - assert.equal(err.statusCode, 404) + assert.equal(err.statusCode, 401) } }) @@ -355,7 +355,7 @@ describe('Users', () => { } }) - it('should throw 404 if user doesn\'t exist', async () => { + it('should throw 401 if deleting other user', async () => { const { token } = context try { @@ -373,7 +373,7 @@ describe('Users', () => { await rp(options) assert.equal(true, false, 'Unexpected behavior') } catch (err) { - assert.equal(err.statusCode, 404) + assert.equal(err.statusCode, 401) } })