From 325162cb3ef40c07ca8ac8d083151b52aaf9d80d Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 22 Nov 2016 17:15:14 -0700 Subject: [PATCH] Added authorization pieces + test functions --- middleware/authorization.js | 44 +++++++++++++-------- models/action.js | 16 +++++++- routes/api/actions/index.js | 3 +- routes/api/asset/index.js | 11 ------ routes/api/auth/index.js | 2 +- routes/api/comments/index.js | 9 +++-- routes/api/index.js | 7 ++-- routes/api/queue/index.js | 4 +- routes/api/settings/index.js | 5 ++- routes/api/stream/index.js | 2 +- routes/api/user/index.js | 5 ++- tests/models/action.js | 61 +++++++++++++++++++++-------- tests/routes/api/assets/index.js | 63 +----------------------------- tests/routes/api/comments/index.js | 30 +++++++++++--- tests/routes/api/queue/index.js | 2 + tests/routes/api/settings/index.js | 54 +++++++++++++------------ tests/utils/passport.js | 25 ++++++++++++ 17 files changed, 190 insertions(+), 153 deletions(-) create mode 100644 tests/utils/passport.js diff --git a/middleware/authorization.js b/middleware/authorization.js index 238219a06..65f77b753 100644 --- a/middleware/authorization.js +++ b/middleware/authorization.js @@ -2,7 +2,9 @@ * authorization contains the references to the authorization middleware. * @type {Object} */ -const authorization = module.exports = {}; +const authorization = module.exports = { + middleware: [] +}; const debug = require('debug')('talk:middleware:authorization'); @@ -33,21 +35,29 @@ authorization.has = (user, ...roles) => roles.every((role) => user.roles.indexOf * @param {Array} roles all the roles that a user must have * @return {Callback} connect middleware */ -authorization.needed = (...roles) => (req, res, next) => { - // All routes that are wrapepd with this middleware actually require a role. - if (!req.user) { - debug(`No user on request, returning with ${ErrNotAuthorized}`); - return next(ErrNotAuthorized); - } +authorization.needed = (...roles) => [ - // Check to see if the current user has all the roles requested for the given - // array of roles requested, if one is not on the user, then this will - // evaluate to true. - if (!authorization.has(req.user, ...roles)) { - debug('User does not have all the required roles to access this page'); - return next(ErrNotAuthorized); - } + // Insert the pre-needed middlware. + ...authorization.middleware, - // Looks like they're allowed! - return next(); -}; + // Insert the actual middleware to check for the required role. + (req, res, next) => { + + // All routes that are wrapepd with this middleware actually require a role. + if (!req.user) { + debug(`No user on request, returning with ${ErrNotAuthorized}`); + return next(ErrNotAuthorized); + } + + // Check to see if the current user has all the roles requested for the given + // array of roles requested, if one is not on the user, then this will + // evaluate to true. + if (!authorization.has(req.user, ...roles)) { + debug('User does not have all the required roles to access this page'); + return next(ErrNotAuthorized); + } + + // Looks like they're allowed! + return next(); + } +]; diff --git a/models/action.js b/models/action.js index 91378b0b0..48c0a9a18 100644 --- a/models/action.js +++ b/models/action.js @@ -41,7 +41,7 @@ ActionSchema.statics.findByItemIdArray = function(item_ids) { * Returns summaries of actions for an array of ids * @param {String} ids array of user identifiers (uuid) */ -ActionSchema.statics.getActionSummaries = function(item_ids) { +ActionSchema.statics.getActionSummaries = function(item_ids, current_user_id = '') { return Action.aggregate([ { @@ -71,6 +71,18 @@ ActionSchema.statics.getActionSummaries = function(item_ids) { // just grabbing the last instance of the item type here. item_type: { $last: '$item_type' + }, + + current_user: { + $max: { + $cond: { + if: { + $eq: ['$user_id', current_user_id], + }, + then: '$$CURRENT', + else: null + } + } } } }, @@ -89,7 +101,7 @@ ActionSchema.statics.getActionSummaries = function(item_ids) { item_type: '$item_type', // set the current user to false here - current_user: {$literal: false} + current_user: '$current_user' } } ]) diff --git a/routes/api/actions/index.js b/routes/api/actions/index.js index f2f9be390..9724f5a73 100644 --- a/routes/api/actions/index.js +++ b/routes/api/actions/index.js @@ -6,7 +6,8 @@ const router = express.Router(); router.delete('/:action_id', (req, res, next) => { Action .findOneAndRemove({ - id: req.params.action_id + id: req.params.action_id, + user_id: req.user.id }) .then(() => { res.status(204).end(); diff --git a/routes/api/asset/index.js b/routes/api/asset/index.js index b261da883..217c49547 100644 --- a/routes/api/asset/index.js +++ b/routes/api/asset/index.js @@ -30,15 +30,4 @@ router.get('/:id', (req, res, next) => { }); -// Upsert an asset and return the affected document. -router.put('/', (req, res, next) => { - - Asset.upsert(req.body) - .then((asset) => { - res.json(asset); - }) - .catch(next); - -}); - module.exports = router; diff --git a/routes/api/auth/index.js b/routes/api/auth/index.js index ae6fba01c..b88b6fe12 100644 --- a/routes/api/auth/index.js +++ b/routes/api/auth/index.js @@ -14,7 +14,7 @@ router.get('/', authorization.needed(), (req, res) => { /** * This destroys the session of a user, if they have one. */ -router.delete('/', (req, res) => { +router.delete('/', authorization.needed(), (req, res) => { req.session.destroy(() => { res.status(204).end(); }); diff --git a/routes/api/comments/index.js b/routes/api/comments/index.js index d318857d6..1507e82aa 100644 --- a/routes/api/comments/index.js +++ b/routes/api/comments/index.js @@ -1,10 +1,11 @@ const express = require('express'); const Comment = require('../../../models/comment'); const wordlist = require('../../../services/wordlist'); +const authorization = require('../../../middleware/authorization'); const router = express.Router(); -router.get('/', (req, res, next) => { +router.get('/', authorization.needed('admin'), (req, res, next) => { let query; if (req.query.status) { @@ -49,7 +50,7 @@ router.post('/', wordlist.filter('body'), (req, res, next) => { }); }); -router.get('/:comment_id', (req, res, next) => { +router.get('/:comment_id', authorization.needed('admin'), (req, res, next) => { Comment .findById(req.params.comment_id) .then(comment => { @@ -65,7 +66,7 @@ router.get('/:comment_id', (req, res, next) => { }); }); -router.delete('/:comment_id', (req, res, next) => { +router.delete('/:comment_id', authorization.needed('admin'), (req, res, next) => { Comment .removeById(req.params.comment_id) .then(() => { @@ -76,7 +77,7 @@ router.delete('/:comment_id', (req, res, next) => { }); }); -router.put('/:comment_id/status', (req, res, next) => { +router.put('/:comment_id/status', authorization.needed('admin'), (req, res, next) => { const { status diff --git a/routes/api/index.js b/routes/api/index.js index 7192a1324..8c6014ed8 100644 --- a/routes/api/index.js +++ b/routes/api/index.js @@ -1,14 +1,15 @@ const express = require('express'); +const authorization = require('../../middleware/authorization'); const router = express.Router(); router.use('/asset', require('./asset')); router.use('/auth', require('./auth')); -router.use('/comments', require('./comments')); +router.use('/comments', authorization.needed(), require('./comments')); router.use('/queue', require('./queue')); -router.use('/settings', require('./settings')); +router.use('/settings', authorization.needed('admin'), require('./settings')); router.use('/stream', require('./stream')); router.use('/user', require('./user')); -router.use('/actions', require('./actions')); +router.use('/actions', authorization.needed(), require('./actions')); module.exports = router; diff --git a/routes/api/queue/index.js b/routes/api/queue/index.js index f8ab95d6b..edc5602ef 100644 --- a/routes/api/queue/index.js +++ b/routes/api/queue/index.js @@ -1,7 +1,7 @@ const express = require('express'); const Comment = require('../../../models/comment'); - const Setting = require('../../../models/setting'); +const authorization = require('../../../middleware/authorization'); const router = express.Router(); @@ -13,7 +13,7 @@ const router = express.Router(); // depending on the settings. The :moderation overwrites this settings. // Pre-moderation: New comments are shown in the moderator queues immediately. // Post-moderation: New comments do not appear in moderation queues unless they are flagged by other users. -router.get('/comments/pending', (req, res, next) => { +router.get('/comments/pending', authorization.needed('admin'), (req, res, next) => { Setting.getModerationSetting().then(function({moderation}){ Comment.moderationQueue(moderation).then((comments) => { res.status(200).json(comments); diff --git a/routes/api/settings/index.js b/routes/api/settings/index.js index 2665cacc8..6e8c64d4f 100644 --- a/routes/api/settings/index.js +++ b/routes/api/settings/index.js @@ -1,7 +1,8 @@ -const _ = require('lodash'); const express = require('express'); -const router = express.Router(); const Setting = require('../../../models/setting'); +const _ = require('lodash'); + +const router = express.Router(); router.get('/', (req, res, next) => { Setting diff --git a/routes/api/stream/index.js b/routes/api/stream/index.js index acbfe3d77..a48548608 100644 --- a/routes/api/stream/index.js +++ b/routes/api/stream/index.js @@ -41,7 +41,7 @@ router.get('/', (req, res, next) => { asset.id, ...comments.map((comment) => comment.id), ...comments.map((comment) => comment.author_id) - ])) + ]), req.user ? req.user.id : '') ]); }) .then(([assets, comments, users, actions]) => { diff --git a/routes/api/user/index.js b/routes/api/user/index.js index 3d5b49d93..3251e0608 100644 --- a/routes/api/user/index.js +++ b/routes/api/user/index.js @@ -7,8 +7,9 @@ const fs = require('fs'); const path = require('path'); const resetEmailFile = fs.readFileSync(path.resolve(__dirname, '../../../views/password-reset-email.ejs')); const resetEmailTemplate = ejs.compile(resetEmailFile.toString()); +const authorization = require('../../../middleware/authorization'); -router.get('/', (req, res, next) => { +router.get('/', authorization.needed('admin'), (req, res, next) => { const { value = '', field = 'created_at', @@ -49,7 +50,7 @@ router.get('/', (req, res, next) => { .catch(next); }); -router.post('/:user_id/role', (req, res, next) => { +router.post('/:user_id/role', authorization.needed('admin'), (req, res, next) => { User .addRoleToUser(req.params.user_id, req.body.role) .then(role => { diff --git a/tests/models/action.js b/tests/models/action.js index 78f5fd1d0..87d53b7da 100644 --- a/tests/models/action.js +++ b/tests/models/action.js @@ -5,23 +5,25 @@ const expect = require('chai').expect; describe('Action: models', () => { let mockActions; + beforeEach(() => { return Action.create([{ action_type: 'flag', item_id: '123', - item_type: 'comments' + item_type: 'comment', + user_id: 'flagginguserid' }, { action_type: 'flag', item_id: '456', - item_type: 'comments' + item_type: 'comment' }, { action_type: 'flag', item_id: '123', - item_type: 'comments' + item_type: 'comment' }, { action_type: 'like', item_id: '123', - item_type: 'comments' + item_type: 'comment' }]).then((actions) => { mockActions = actions; }); @@ -30,8 +32,7 @@ describe('Action: models', () => { describe('#findById()', () => { it('should find an action by id', () => { return Action.findById(mockActions[0].id).then((result) => { - expect(result).to.have.property('action_type') - .and.to.equal('flag'); + expect(result).to.have.property('action_type', 'flag'); }); }); }); @@ -46,27 +47,55 @@ describe('Action: models', () => { describe('#getActionSummaries()', () => { it('should return properly formatted summaries from an array of item_ids', () => { - return Action.getActionSummaries(['123', '789']).then((result) => { - expect(result).to.have.length(2); + return Action.getActionSummaries(['123', '789']).then((summaries) => { + expect(summaries).to.have.length(2); - const sorted = result.sort((a, b) => a.count - b.count); - - expect(sorted[0]).to.deep.equal({ + expect(summaries).to.deep.include({ action_type: 'like', count: 1, item_id: '123', - item_type: 'comments', - current_user: false + item_type: 'comment', + current_user: null }); - expect(sorted[1]).to.deep.equal({ + expect(summaries).to.deep.include({ action_type: 'flag', count: 2, item_id: '123', - item_type: 'comments', - current_user: false + item_type: 'comment', + current_user: null }); }); }); + + it('should include a current user when one is passed', () => { + return Action + .getActionSummaries(['123'], 'flagginguserid') + .then((summaries) => { + expect(summaries).to.have.length(2); + + let summary = summaries.find((s) => s.item_id === '123' && s.action_type === 'flag'); + + expect(summary).to.not.be.undefined; + expect(summary.current_user).to.not.be.null; + expect(summary.current_user).to.have.property('item_id', '123'); + expect(summary.current_user).to.have.property('item_type', 'comment'); + expect(summary.current_user).to.have.property('user_id', 'flagginguserid'); + expect(summary.current_user).to.have.property('action_type', 'flag'); + }); + }); + + it('should not include a current user when one is passed for a user that doesn\'t have an action', () => { + return Action + .getActionSummaries(['123'], 'flagginguserid2') + .then((summaries) => { + expect(summaries).to.have.length(2); + + summaries.forEach((summary) => { + expect(summary).to.not.be.undefined; + expect(summary).to.have.property('current_user', null); + }); + }); + }); }); }); diff --git a/tests/routes/api/assets/index.js b/tests/routes/api/assets/index.js index aa764e214..542e88c42 100644 --- a/tests/routes/api/assets/index.js +++ b/tests/routes/api/assets/index.js @@ -1,22 +1,13 @@ require('../../../utils/mongoose'); +const passport = require('../../../utils/passport'); const chai = require('chai'); -const expect = chai.expect; const server = require('../../../../app'); // Setup chai. chai.should(); chai.use(require('chai-http')); -let fixture = { - 'url': 'http://hhgg.com/total-perspective-vortex', - 'type': 'article', - 'headline': 'The Total Perspective Vortex', - 'summary': 'You are an insignificant dot on an insignificant dot.', - 'section': 'Everything', - 'authors': ['Ford Prefect'] -}; - describe('Asset: routes', () => { describe('/GET Asset', () => { @@ -25,6 +16,7 @@ describe('Asset: routes', () => { chai.request(server) .get('/api/v1/asset') + .set(passport.inject({roles: ['admin']})) .end((err, res) => { if (err) { @@ -41,55 +33,4 @@ describe('Asset: routes', () => { }); }); - // This test checks PUT and read - describe('/PUT Asset', () => { - describe('#put', () => { - it('It should save an asset and load it again.', (done) => { - - chai.request(server) - .put('/api/v1/asset') - .send(fixture) - .end((err, res) => { - - if (err) { - throw new Error(err); - } - - res.should.have.status(200); - res.body.should.be.a('object'); - - // Id should be generated by the model if absent. - res.body.should.have.property('id'); - - // Save the asset id to compare with GET result. - let assetId = res.body.id; - - // Load the asset to make sure it's really there. - chai.request(server) - .get(`/api/v1/asset?url=${encodeURIComponent(fixture.url)}`) - .end((err, res) => { - - if (err) { - throw new Error(err); - } - - res.should.have.status(200); - res.body.should.be.an('array'); - - let asset = res.body[0]; - - expect(asset).to.have.property('id'); - - // Ensure the asset has the same id as above. - // This tests the single url per Id concept. - expect(assetId).to.equal(asset.id); - - done(); - - }); - }); - }); - }); - }); // End describe /PUT Asset - }); diff --git a/tests/routes/api/comments/index.js b/tests/routes/api/comments/index.js index ec9e05d03..be3d14f69 100644 --- a/tests/routes/api/comments/index.js +++ b/tests/routes/api/comments/index.js @@ -1,6 +1,7 @@ process.env.NODE_ENV = 'test'; require('../../../utils/mongoose'); +const passport = require('../../../utils/passport'); const app = require('../../../../app'); const chai = require('chai'); @@ -68,6 +69,7 @@ describe('Get /comments', () => { it('should return all the comments', () => { return chai.request(app) .get('/api/v1/comments') + .set(passport.inject({roles: ['admin']})) .then((res) => { expect(res).to.have.status(200); @@ -126,6 +128,7 @@ describe('Get comments by status and action', () => { it('should return all the rejected comments', () => { return chai.request(app) .get('/api/v1/comments?status=rejected') + .set(passport.inject({roles: ['admin']})) .then((res) => { expect(res).to.have.status(200); expect(res.body[0]).to.have.property('id', 'abc'); @@ -135,6 +138,7 @@ describe('Get comments by status and action', () => { it('should return all the approved comments', () => { return chai.request(app) .get('/api/v1/comments?status=accepted') + .set(passport.inject({roles: ['admin']})) .then((res) => { expect(res).to.have.status(200); expect(res.body[0]).to.have.property('id', 'hij'); @@ -144,6 +148,7 @@ describe('Get comments by status and action', () => { it('should return all the new comments', () => { return chai.request(app) .get('/api/v1/comments?status=new') + .set(passport.inject({roles: ['admin']})) .then((res) => { expect(res).to.have.status(200); expect(res.body[0]).to.have.property('id', 'def'); @@ -153,6 +158,7 @@ describe('Get comments by status and action', () => { it('should return all the flagged comments', () => { return chai.request(app) .get('/api/v1/comments?action_type=flag') + .set(passport.inject({roles: ['admin']})) .then((res) => { expect(res).to.have.status(200); @@ -195,6 +201,7 @@ describe('Post /comments', () => { it('should create a comment', () => { return chai.request(app) .post('/api/v1/comments') + .set(passport.inject({roles: []})) .send({'body': 'Something body.', 'author_id': '123', 'asset_id': '1', 'parent_id': ''}) .then((res) => { expect(res).to.have.status(201); @@ -205,6 +212,7 @@ describe('Post /comments', () => { it('should create a comment with a rejected status if it contains a bad word', () => { return chai.request(app) .post('/api/v1/comments') + .set(passport.inject({roles: []})) .send({'body': 'bad words are the baddest', 'author_id': '123', 'asset_id': '1', 'parent_id': ''}) .then((res) => { expect(res).to.have.status(201); @@ -262,6 +270,7 @@ describe('Get /:comment_id', () => { it('should return the right comment for the comment_id', () => { return chai.request(app) .get('/api/v1/comments/abc') + .set(passport.inject({roles: ['admin']})) .then((res) => { expect(res).to.have.status(200); expect(res).to.have.property('body'); @@ -318,6 +327,7 @@ describe('Remove /:comment_id', () => { it('it should remove comment', () => { return chai.request(app) .delete('/api/v1/comments/abc') + .set(passport.inject({roles: ['admin']})) .then((res) => { expect(res).to.have.status(204); @@ -329,11 +339,6 @@ describe('Remove /:comment_id', () => { }); }); -process.on('unhandledRejection', (reason) => { - console.error('Reason: '); - console.error(reason); -}); - describe('Put /:comment_id/status', () => { const comments = [{ @@ -384,12 +389,26 @@ describe('Put /:comment_id/status', () => { it('it should update status', function() { return chai.request(app) .put('/api/v1/comments/abc/status') + .set(passport.inject({roles: ['admin']})) .send({status: 'accepted'}) .then((res) => { expect(res).to.have.status(204); expect(res.body).to.be.empty; }); }); + + it('it should not allow a non-admin to update status', () => { + return chai.request(app) + .put('/api/v1/comments/abc/status') + .set(passport.inject({roles: []})) + .send({status: 'accepted'}) + .then((res) => { + expect(res).to.be.empty; + }) + .catch((err) => { + expect(err).to.have.property('status', 401); + }); + }); }); describe('Post /:comment_id/actions', () => { @@ -442,6 +461,7 @@ describe('Post /:comment_id/actions', () => { it('it should update actions', () => { return chai.request(app) .post('/api/v1/comments/abc/actions') + .set(passport.inject({roles: ['admin']})) .send({'user_id': '456', 'action_type': 'flag'}) .then((res) => { expect(res).to.have.status(201); diff --git a/tests/routes/api/queue/index.js b/tests/routes/api/queue/index.js index f21fe1331..4eff3d384 100644 --- a/tests/routes/api/queue/index.js +++ b/tests/routes/api/queue/index.js @@ -1,6 +1,7 @@ process.env.NODE_ENV = 'test'; require('../../../utils/mongoose'); +const passport = require('../../../utils/passport'); const app = require('../../../../app'); const chai = require('chai'); @@ -71,6 +72,7 @@ describe('Get moderation queues rejected, pending, flags', () => { it('should return all the pending comments', function(done){ chai.request(app) .get('/api/v1/queue/comments/pending') + .set(passport.inject({roles: ['admin']})) .end(function(err, res){ expect(err).to.be.null; expect(res).to.have.status(200); diff --git a/tests/routes/api/settings/index.js b/tests/routes/api/settings/index.js index 41c37828e..86feca1eb 100644 --- a/tests/routes/api/settings/index.js +++ b/tests/routes/api/settings/index.js @@ -1,13 +1,15 @@ process.env.NODE_ENV = 'test'; require('../../../utils/mongoose'); +const passport = require('../../../utils/passport'); const app = require('../../../../app'); const chai = require('chai'); -const chaiHttp = require('chai-http'); -chai.use(chaiHttp); const expect = chai.expect; +chai.should(); +chai.use(require('chai-http')); + const Setting = require('../../../../models/setting'); const defaults = {id: '1', moderation: 'pre'}; @@ -17,15 +19,16 @@ describe('GET /settings', () => { return Setting.update({id: '1'}, {$setOnInsert: defaults}, {upsert: true}); }); - it('should return a settings object', done => { - chai.request(app) + it('should return a settings object', () => { + return chai.request(app) .get('/api/v1/settings') - .end((err, res) => { - expect(err).to.be.null; + .set(passport.inject({ + roles: ['admin'] + })) + .then((res) => { expect(res).to.have.status(200); expect(res).to.be.json; expect(res.body).to.have.property('moderation', 'pre'); - done(err); }); }); }); @@ -33,25 +36,26 @@ describe('GET /settings', () => { // update the settings. describe('update settings', () => { it('should respond ok to a PUT', () => { - return Setting.update({id: '1'}, {$setOnInsert: defaults}, {upsert: true}) - .then(() => { - return chai.request(app) - .put('/api/v1/settings') - .send({moderation: 'post'}) - .then(res => { - expect(res).to.have.status(204); + return Setting + .update({id: '1'}, {$setOnInsert: defaults}, {upsert: true}) + .then(() => { + return chai.request(app) + .put('/api/v1/settings') + .set(passport.inject({ + roles: ['admin'] + })) + .send({moderation: 'post'}); + }) + .then(res => { + expect(res).to.have.status(204); - return Setting.getSettings(); + return Setting.getSettings(); + }) + .then(settings => { - }) - .then(settings => { - // confirm updated settings in db - expect(settings).to.have.property('moderation'); - expect(settings.moderation).to.equal('post'); - }) - .catch(err => { - throw err; - }); - }); + // confirm updated settings in db + expect(settings).to.have.property('moderation'); + expect(settings.moderation).to.equal('post'); + }); }); }); diff --git a/tests/utils/passport.js b/tests/utils/passport.js new file mode 100644 index 000000000..401e50b77 --- /dev/null +++ b/tests/utils/passport.js @@ -0,0 +1,25 @@ +const authorization = require('../../middleware/authorization'); + +// Add the passport middleware here before it's setup. +authorization.middleware.push((req, res, next) => { + req.user = JSON.parse(new Buffer(req.get('X-Mock-Authorization'), 'base64').toString('ascii')); + + next(); +}); + +const MockStrategy = { + + /** + * Injects the new user into the request header for the mock middleware to + * interpret. + * @param {Object} user the user to inject + * @return {Object} the headers to add to the request + */ + inject(user) { + return { + 'X-Mock-Authorization': new Buffer(JSON.stringify(user)).toString('base64') + }; + } +}; + +module.exports = MockStrategy;