From a48ea5e529d4491e1d0dbf9c3abcaf61b5717905 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Mon, 12 Jun 2017 19:49:43 +0200 Subject: [PATCH 01/24] adding tests for as-is code. --- src/crud/create.js | 4 +++ test/crud/create.unit.js | 72 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 test/crud/create.unit.js diff --git a/src/crud/create.js b/src/crud/create.js index 9c37dc1..87f65de 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -22,8 +22,12 @@ function addCreateRoute(router, crudMiddleware, maps) { .describe(router.metadata.creationDescription || description(router.metadata)); return router; } +addCreateRoute.getSteps = getSteps; +addCreateRoute.sendCreateResult = sendCreateResult; +addCreateRoute.description = description; addCreateRoute.setStatusIfApplicable = setStatusIfApplicable; addCreateRoute.setOwnerIfApplicable = setOwnerIfApplicable; + module.exports = addCreateRoute; function getSteps(router, crudMiddleware, maps) { diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js new file mode 100644 index 0000000..99d9a0f --- /dev/null +++ b/test/crud/create.unit.js @@ -0,0 +1,72 @@ +'use strict'; +require('../@util/init.js'); +const addCreateRoute = require('../../src/crud/create'); +const httpMocks = require('node-mocks-http'); +const events = require('events'); + +describe('Crud - create', function() { + describe('setStatusIfApplicable', function() { + it('Should not set req.body.status if the provided schema has no statuses', function(done) { + const metadata = buildMetadata(); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + expect(reqOptions.body.status).to.not.be.ok(); + done(); + } + }); + it('Should set req.body.status to the first status in the schema', function(done) { + const metadata = buildMetadata([{ name: 'a' }, { name: 'b' }]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + expect(reqOptions.body.status).to.equal('a'); + done(); + } + }); + }); +}); + +function buildMetadata(statuses) { + const metadata = { + schemas: { + core: {} + } + }; + if (statuses) { + metadata.schemas.core.statuses = statuses; + } + return metadata; +} + +function mockRequest(middlewareOrRouter, reqOptions, responseCallback, nextCallback) { + const req = httpMocks.createRequest(reqOptions); + const res = httpMocks.createResponse({ + eventEmitter: events.EventEmitter + }); + res.on('end', function() { + let resToReturn; + try { + resToReturn = { + statusCode: res._getStatusCode(), + body: JSON.parse(res._getData()), + headers: res._getHeaders(), + raw: res + }; + } catch (err) { + return responseCallback(err); + } + responseCallback(null, resToReturn); + }); + middlewareOrRouter(req, res, nextCallback); +} From ab5f71d600343e286837974ad2cf7e1c3141914e Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Mon, 12 Jun 2017 20:02:07 +0200 Subject: [PATCH 02/24] more assertions --- test/crud/create.unit.js | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 99d9a0f..f45f2c6 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -3,6 +3,7 @@ require('../@util/init.js'); const addCreateRoute = require('../../src/crud/create'); const httpMocks = require('node-mocks-http'); const events = require('events'); +const moment = require('moment'); describe('Crud - create', function() { describe('setStatusIfApplicable', function() { @@ -17,9 +18,12 @@ describe('Crud - create', function() { function next(error) { expect(error).to.not.be.ok(); expect(reqOptions.body.status).to.not.be.ok(); + expect(reqOptions.body.statusDate).to.not.be.ok(); + expect(reqOptions.body.statusLog).to.not.be.ok(); done(); } }); + it('Should set req.body.status to the first status in the schema', function(done) { const metadata = buildMetadata([{ name: 'a' }, { name: 'b' }]); const middleware = addCreateRoute.setStatusIfApplicable(metadata); @@ -34,6 +38,35 @@ describe('Crud - create', function() { done(); } }); + + it('Should set req.body.statusDate to now', function(done) { + const metadata = buildMetadata([{ name: 'a' }]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + expect(moment(reqOptions.body.statusDate).diff(new Date())).to.be.lessThan(1); + done(); + } + }); + it('Should create a status log with one entry', function(done) { + const metadata = buildMetadata([{ name: 'a' }]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + expect(reqOptions.body.statusLog).to.be.an('array').that.has.length(1); + done(); + } + }); }); }); From fb3f4b0e96e5f2c90e66d7be7257f4e01919a1fc Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Mon, 12 Jun 2017 20:06:40 +0200 Subject: [PATCH 03/24] More as-is analysis --- test/crud/create.unit.js | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index f45f2c6..3a0b6ec 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -53,6 +53,7 @@ describe('Crud - create', function() { done(); } }); + it('Should create a status log with one entry', function(done) { const metadata = buildMetadata([{ name: 'a' }]); const middleware = addCreateRoute.setStatusIfApplicable(metadata); @@ -67,6 +68,36 @@ describe('Crud - create', function() { done(); } }); + + it('Should create a status log entry with the status set to the first one in the schema', function(done) { + const metadata = buildMetadata([{ name: 'a' }]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + expect(reqOptions.body.statusLog[0].status).to.equal('a'); + done(); + } + }); + + it('Should create a status log entry with the statusDate set to now', function(done) { + const metadata = buildMetadata([{ name: 'a' }]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + expect(moment(reqOptions.body.statusLog[0].statusDate).diff(new Date())).to.be.lessThan(1); + done(); + } + }); }); }); From 67581aa7effe377cf38288c632b583407a942aa2 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Mon, 12 Jun 2017 21:11:49 +0200 Subject: [PATCH 04/24] updating how status data is set on creation. --- src/crud/create.js | 48 ++++++++++------- test/crud/create.unit.js | 111 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 19 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index 87f65de..b9a10b2 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -88,9 +88,7 @@ function description(metadata) { responses: { '201': { description: - 'Informs the caller that the ' + - metadata.title.toLowerCase() + - ' was successfully created.', + 'Informs the caller that the ' + metadata.title.toLowerCase() + ' was successfully created.', commonHeaders: [correlationIdOptions.resHeader], model: metadata.schemas.output.name } @@ -104,21 +102,38 @@ function setStatusIfApplicable(metadata) { if (!statuses || statuses.length <= 0) { return next(); } - req.body.status = statuses[0].name; + const statusToSet = statuses[0]; + req.body.status = statusToSet.name; req.body.statusDate = moment.utc().toDate(); - req.body.statusLog = [ - { - status: req.body.status, - data: { - reason: 'Initial Status' //todo need to set this logically somehow - }, - statusDate: req.body.statusDate - } - ]; + const logEntry = { + status: req.body.status, + statusDate: req.body.statusDate + }; + if (statusToSet.initialData) { + const fromReq = getFromReqObject(statusToSet.initialData.fromReq, req); + logEntry.data = _.merge({}, statusToSet.initialData.static, fromReq); + } + req.body.statusLog = [logEntry]; return next(); }; } +function getFromReqObject(map, req) { + if (!map) { + return; + } + const data = {}; + Object.keys(map).forEach(function(key) { + const value = map[key]; + if (_.isObject(value)) { + data[key] = getFromReqObject(value, req); + return; + } + data[key] = _.get(req, value); + }); + return data; +} + function setOwnerIfApplicable(metadata) { return function _setOwnerIfApplicable(req, res, next) { let ownership = metadata.schemas.core.ownership; @@ -129,12 +144,7 @@ function setOwnerIfApplicable(metadata) { req.body.owner = _.get(req, ownership.setOwnerExpression); if (!req.body.owner) { return next( - boom.badRequest( - util.format( - 'Owner from expression "%s" was blank', - ownership.setOwnerExpression - ) - ) + boom.badRequest(util.format('Owner from expression "%s" was blank', ownership.setOwnerExpression)) ); } } else { diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 3a0b6ec..3169413 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -98,6 +98,117 @@ describe('Crud - create', function() { done(); } }); + + describe('Status log data', function() { + it('Should not exist if initialData was missing', function(done) { + const statusToSet = { + name: 'a' + }; + const metadata = buildMetadata([statusToSet]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + const data = reqOptions.body.statusLog[0].data; + expect(data).to.not.be.ok(); + done(); + } + }); + + it('Should be an empty object if initialData was an empty object', function(done) { + const statusToSet = { + name: 'a', + initialData: {} + }; + const metadata = buildMetadata([statusToSet]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + const data = reqOptions.body.statusLog[0].data; + expect(Object.keys(data)).to.have.lengthOf(0); + done(); + } + }); + + it('Should be an object that deep equals initialData.static if only initialData.static was set', function( + done + ) { + const statusToSet = { + name: 'a', + initialData: { + static: { + number: 1, + string: 'test', + bool: true, + array: [2, 'test', true, null, {}, []], + object: {} + } + } + }; + const metadata = buildMetadata([statusToSet]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + const reqOptions = { + body: {} + }; + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + const data = reqOptions.body.statusLog[0].data; + expect(data).to.deep.equal(statusToSet.initialData.static); + done(); + } + }); + it('Should be an object with properties taken from the request object if only initialData.fromReq was set', function( + done + ) { + const reqOptions = { + body: {}, + user: { + username: 'Bob' + } + }; + const statusToSet = { + name: 'a', + initialData: { + fromReq: { + username: 'user.username', + doesNotExist: 'a', + nested: { + initialUsername: 'user.username' + } + //TODO what if not a string or object value? i.e. [boolean, null, undefined, array, number] + //TODO what if value isn't found? should we use "asdasd: ['a','defaultValue']" to denote using defaults? or do we throw an error? + //TODO security around retrieving things from request? Maybe only from certain parts of req? req.params? req.query? req.body? req.process? + //TODO if fromReq and static are both set, it will merge in an order, what about conflicts? Error? + //TODO refactor this fromReq code with detailed logic into it's own describe block for getFromReqObject + } + } + }; + const metadata = buildMetadata([statusToSet]); + const middleware = addCreateRoute.setStatusIfApplicable(metadata); + + mockRequest(middleware, reqOptions, null, next); + + function next(error) { + expect(error).to.not.be.ok(); + const data = reqOptions.body.statusLog[0].data; + expect(data.username).to.equal('Bob'); + expect(data.doesNotExist).to.not.be.ok(); + expect(data.nested.initialUsername).to.equal('Bob'); + done(); + } + }); + }); }); }); From 82819d13e651df4c8f9e3da5b54d79d51971a5ab Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 16:55:38 +0200 Subject: [PATCH 05/24] Adding shared code. --- test/crud/create.unit.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 3169413..8b0850d 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -245,3 +245,27 @@ function mockRequest(middlewareOrRouter, reqOptions, responseCallback, nextCallb }); middlewareOrRouter(req, res, nextCallback); } + +function shouldNotCallNext(done) { + return function next(err) { + if (err) { + return done(err); + } + return done(new Error('Next should not have been called')); + }; +} + +function shouldCallNext(done) { + return function next(err) { + if (err) { + return done(err); + } + return done(); + }; +} + +function shouldNotReturnResponse(done) { + return function resComplete() { + done(new Error('res.end should not have been called')); + }; +} From 0a79c42b19f1841b15262c19029cb128b9b3cf8d Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 17:34:28 +0200 Subject: [PATCH 06/24] refactoring code to be more reusable. --- src/crud/create.js | 25 ++++++++++++-------- test/crud/create.unit.js | 49 +++++++++++++++++++++------------------- 2 files changed, 42 insertions(+), 32 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index b9a10b2..22dbb8c 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -27,6 +27,7 @@ addCreateRoute.sendCreateResult = sendCreateResult; addCreateRoute.description = description; addCreateRoute.setStatusIfApplicable = setStatusIfApplicable; addCreateRoute.setOwnerIfApplicable = setOwnerIfApplicable; +addCreateRoute.getFromReqObject = getFromReqObject; module.exports = addCreateRoute; @@ -105,19 +106,25 @@ function setStatusIfApplicable(metadata) { const statusToSet = statuses[0]; req.body.status = statusToSet.name; req.body.statusDate = moment.utc().toDate(); - const logEntry = { - status: req.body.status, - statusDate: req.body.statusDate - }; - if (statusToSet.initialData) { - const fromReq = getFromReqObject(statusToSet.initialData.fromReq, req); - logEntry.data = _.merge({}, statusToSet.initialData.static, fromReq); - } - req.body.statusLog = [logEntry]; + req.body.statusLog = [ + { + status: req.body.status, + data: getData(statusToSet.initialData, req), + statusDate: req.body.statusDate + } + ]; return next(); }; } +function getData(rules, req) { + if (!rules) { + return; + } + const fromReq = getFromReqObject(rules.fromReq, req); + return _.merge({}, rules.static, fromReq); +} + function getFromReqObject(map, req) { if (!map) { return; diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 8b0850d..9b1ce60 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -210,6 +210,9 @@ describe('Crud - create', function() { }); }); }); + describe('getFromReqObject', function() { + + }); }); function buildMetadata(statuses) { @@ -246,26 +249,26 @@ function mockRequest(middlewareOrRouter, reqOptions, responseCallback, nextCallb middlewareOrRouter(req, res, nextCallback); } -function shouldNotCallNext(done) { - return function next(err) { - if (err) { - return done(err); - } - return done(new Error('Next should not have been called')); - }; -} - -function shouldCallNext(done) { - return function next(err) { - if (err) { - return done(err); - } - return done(); - }; -} - -function shouldNotReturnResponse(done) { - return function resComplete() { - done(new Error('res.end should not have been called')); - }; -} +// function shouldNotCallNext(done) { +// return function next(err) { +// if (err) { +// return done(err); +// } +// return done(new Error('Next should not have been called')); +// }; +// } +// +// function shouldCallNext(done) { +// return function next(err) { +// if (err) { +// return done(err); +// } +// return done(); +// }; +// } +// +// function shouldNotReturnResponse(done) { +// return function resComplete() { +// done(new Error('res.end should not have been called')); +// }; +// } From 5ac3e3c869dad4f1bb6f53169563b4e052aa9956 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 17:49:10 +0200 Subject: [PATCH 07/24] more tests --- src/crud/create.js | 1 + test/crud/create.unit.js | 61 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/crud/create.js b/src/crud/create.js index 22dbb8c..0d700fd 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -28,6 +28,7 @@ addCreateRoute.description = description; addCreateRoute.setStatusIfApplicable = setStatusIfApplicable; addCreateRoute.setOwnerIfApplicable = setOwnerIfApplicable; addCreateRoute.getFromReqObject = getFromReqObject; +addCreateRoute.getData = getData; module.exports = addCreateRoute; diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 9b1ce60..b7b5a7e 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -210,8 +210,69 @@ describe('Crud - create', function() { }); }); }); + + describe('getData', function() { + it('Should not exist if rules was falsy', function() { + const data = addCreateRoute.getData(null, {}); + expect(data).to.not.be.ok(); + }); + it('Should be an empty object if rules was an empty object', function() { + const data = addCreateRoute.getData({}, {}); + expect(data).to.be.ok(); + expect(Object.keys(data)).to.have.lengthOf(0); + }); + }); + describe('getFromReqObject', function() { + it('Should map shallow properties from the req using the map', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: 'a' + }; + const data = addCreateRoute.getFromReqObject(map, req); + expect(data.answer).to.equal('b'); + }); + it('Should map deep properties from the req using the map', function() { + const req = httpMocks.createRequest({ + a: { + b: 'c' + } + }); + const map = { + answer: 'a.b' + }; + const data = addCreateRoute.getFromReqObject(map, req); + expect(data.answer).to.equal('c'); + }); + it('Should support a nested map structure', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + nested: { + answer: 'a' + } + }; + const data = addCreateRoute.getFromReqObject(map, req); + expect(data.nested.answer).to.equal('b'); + }); + it('Should support a nested map structure with a nested request object', function() { + const req = httpMocks.createRequest({ + a: { + b: 'c' + } + }); + const map = { + nested: { + answer: 'a.b' + } + }; + const data = addCreateRoute.getFromReqObject(map, req); + expect(data.nested.answer).to.equal('c'); + }); }); }); From 9abef242393fef818482c4a5a84966ceacbc39a7 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 17:56:47 +0200 Subject: [PATCH 08/24] checking depth. --- package.json | 5 ++++- src/crud/create.js | 24 ++++++++++++++++++++---- test/crud/create.unit.js | 22 ++++++++++++++++++++-- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index c51e873..e54d0a0 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,10 @@ { "name": "node-api-seed", "title": "Node Api Seed", - "version": "0.0.0", + "version": "0.0.1", + "engines": { + "node": ">=6.10.0" + }, "deploymentDate": "2016-11-09T19:15:04.309Z", "description": "The seed for pretty much any api I write in node.js", "main": "index.js", diff --git a/src/crud/create.js b/src/crud/create.js index 0d700fd..6b15738 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -90,7 +90,9 @@ function description(metadata) { responses: { '201': { description: - 'Informs the caller that the ' + metadata.title.toLowerCase() + ' was successfully created.', + 'Informs the caller that the ' + + metadata.title.toLowerCase() + + ' was successfully created.', commonHeaders: [correlationIdOptions.resHeader], model: metadata.schemas.output.name } @@ -126,15 +128,24 @@ function getData(rules, req) { return _.merge({}, rules.static, fromReq); } -function getFromReqObject(map, req) { +const maxDepth = 10; +function getFromReqObject(map, req, depth = 0) { if (!map) { return; } + if (depth > maxDepth) { + throw new Error( + util.format( + 'Circular reference detected in map object after maximum depth (%s) reached', + maxDepth + ) + ); + } const data = {}; Object.keys(map).forEach(function(key) { const value = map[key]; if (_.isObject(value)) { - data[key] = getFromReqObject(value, req); + data[key] = getFromReqObject(value, req, depth + 1); return; } data[key] = _.get(req, value); @@ -152,7 +163,12 @@ function setOwnerIfApplicable(metadata) { req.body.owner = _.get(req, ownership.setOwnerExpression); if (!req.body.owner) { return next( - boom.badRequest(util.format('Owner from expression "%s" was blank', ownership.setOwnerExpression)) + boom.badRequest( + util.format( + 'Owner from expression "%s" was blank', + ownership.setOwnerExpression + ) + ) ); } } else { diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index b7b5a7e..6c56aab 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -69,7 +69,9 @@ describe('Crud - create', function() { } }); - it('Should create a status log entry with the status set to the first one in the schema', function(done) { + it('Should create a status log entry with the status set to the first one in the schema', function( + done + ) { const metadata = buildMetadata([{ name: 'a' }]); const middleware = addCreateRoute.setStatusIfApplicable(metadata); const reqOptions = { @@ -94,7 +96,9 @@ describe('Crud - create', function() { function next(error) { expect(error).to.not.be.ok(); - expect(moment(reqOptions.body.statusLog[0].statusDate).diff(new Date())).to.be.lessThan(1); + expect( + moment(reqOptions.body.statusLog[0].statusDate).diff(new Date()) + ).to.be.lessThan(1); done(); } }); @@ -273,6 +277,20 @@ describe('Crud - create', function() { const data = addCreateRoute.getFromReqObject(map, req); expect(data.nested.answer).to.equal('c'); }); + it('Should throw an error for circular reference maps', function() { + const req = httpMocks.createRequest({ + a: { + b: 'c' + } + }); + const map = { + nested: {} + }; + map.nested.answer = map; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(/circular reference/i); + }); }); }); From aaf76d80d5b354b6652b387f6c2fd458a48c32b4 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 17:58:25 +0200 Subject: [PATCH 09/24] using util to inspect. --- src/crud/create.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index 6b15738..f2c0328 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -90,9 +90,7 @@ function description(metadata) { responses: { '201': { description: - 'Informs the caller that the ' + - metadata.title.toLowerCase() + - ' was successfully created.', + 'Informs the caller that the ' + metadata.title.toLowerCase() + ' was successfully created.', commonHeaders: [correlationIdOptions.resHeader], model: metadata.schemas.output.name } @@ -136,8 +134,9 @@ function getFromReqObject(map, req, depth = 0) { if (depth > maxDepth) { throw new Error( util.format( - 'Circular reference detected in map object after maximum depth (%s) reached', - maxDepth + 'Circular reference detected in map object after maximum depth (%s) reached. Partial map\n%j\n', + maxDepth, + util.inspect(map, true, maxDepth) ) ); } @@ -163,12 +162,7 @@ function setOwnerIfApplicable(metadata) { req.body.owner = _.get(req, ownership.setOwnerExpression); if (!req.body.owner) { return next( - boom.badRequest( - util.format( - 'Owner from expression "%s" was blank', - ownership.setOwnerExpression - ) - ) + boom.badRequest(util.format('Owner from expression "%s" was blank', ownership.setOwnerExpression)) ); } } else { From e5615021d7a396664c75dbe9305483bc5600f64e Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 18:16:24 +0200 Subject: [PATCH 10/24] Updating code to allow stubbing. --- src/crud/create.js | 2 +- test/crud/create.unit.js | 39 +++++++++++++++++++++++++++++++++------ 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index f2c0328..da05f2f 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -122,7 +122,7 @@ function getData(rules, req) { if (!rules) { return; } - const fromReq = getFromReqObject(rules.fromReq, req); + const fromReq = addCreateRoute.getFromReqObject(rules.fromReq, req); return _.merge({}, rules.static, fromReq); } diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 6c56aab..9ce0beb 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -4,6 +4,7 @@ const addCreateRoute = require('../../src/crud/create'); const httpMocks = require('node-mocks-http'); const events = require('events'); const moment = require('moment'); +const sinon = require('sinon'); describe('Crud - create', function() { describe('setStatusIfApplicable', function() { @@ -69,9 +70,7 @@ describe('Crud - create', function() { } }); - it('Should create a status log entry with the status set to the first one in the schema', function( - done - ) { + it('Should create a status log entry with the status set to the first one in the schema', function(done) { const metadata = buildMetadata([{ name: 'a' }]); const middleware = addCreateRoute.setStatusIfApplicable(metadata); const reqOptions = { @@ -96,9 +95,7 @@ describe('Crud - create', function() { function next(error) { expect(error).to.not.be.ok(); - expect( - moment(reqOptions.body.statusLog[0].statusDate).diff(new Date()) - ).to.be.lessThan(1); + expect(moment(reqOptions.body.statusLog[0].statusDate).diff(new Date())).to.be.lessThan(1); done(); } }); @@ -225,6 +222,34 @@ describe('Crud - create', function() { expect(data).to.be.ok(); expect(Object.keys(data)).to.have.lengthOf(0); }); + it('Should return an object that deep equals rules.static if only initialData.static was set', function() { + const req = {}; + const rules = { + static: { + number: 1, + string: 'test', + bool: true, + array: [2, 'test', true, null, {}, []], + object: {} + } + }; + const data = addCreateRoute.getData(rules, req); + expect(data).to.deep.equal(rules.static); + }); + it('Should merge the result from getFromReqObject if rules.fromReq existed', function() { + const req = {}; + const rules = { + fromReq: {} + }; + const stubbedData = { + bob: true + }; + const stub = sinon.stub(addCreateRoute, 'getFromReqObject'); + stub.returns(stubbedData); + const data = addCreateRoute.getData(rules, req); + stub.restore(); + expect(data.bob).to.equal(true); + }); }); describe('getFromReqObject', function() { @@ -263,6 +288,7 @@ describe('Crud - create', function() { const data = addCreateRoute.getFromReqObject(map, req); expect(data.nested.answer).to.equal('b'); }); + it('Should support a nested map structure with a nested request object', function() { const req = httpMocks.createRequest({ a: { @@ -277,6 +303,7 @@ describe('Crud - create', function() { const data = addCreateRoute.getFromReqObject(map, req); expect(data.nested.answer).to.equal('c'); }); + it('Should throw an error for circular reference maps', function() { const req = httpMocks.createRequest({ a: { From dc8ee407ad4d7cf0e3af5dfb28fec96d87c366dc Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 18:19:06 +0200 Subject: [PATCH 11/24] test for merging. --- test/crud/create.unit.js | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 9ce0beb..cd41982 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -217,11 +217,13 @@ describe('Crud - create', function() { const data = addCreateRoute.getData(null, {}); expect(data).to.not.be.ok(); }); + it('Should be an empty object if rules was an empty object', function() { const data = addCreateRoute.getData({}, {}); expect(data).to.be.ok(); expect(Object.keys(data)).to.have.lengthOf(0); }); + it('Should return an object that deep equals rules.static if only initialData.static was set', function() { const req = {}; const rules = { @@ -236,6 +238,7 @@ describe('Crud - create', function() { const data = addCreateRoute.getData(rules, req); expect(data).to.deep.equal(rules.static); }); + it('Should merge the result from getFromReqObject if rules.fromReq existed', function() { const req = {}; const rules = { @@ -250,6 +253,34 @@ describe('Crud - create', function() { stub.restore(); expect(data.bob).to.equal(true); }); + it('Should merge the result from getFromReqObject and static if both were set', function() { + const req = {}; + const rules = { + fromReq: {}, + static: { + number: 1, + string: 'test', + bool: true, + array: [2, 'test', true, null, {}, []], + object: { + subObject: {} + } + } + }; + const stubbedData = { + bob: true + }; + const stub = sinon.stub(addCreateRoute, 'getFromReqObject'); + stub.returns(stubbedData); + const data = addCreateRoute.getData(rules, req); + stub.restore(); + expect(data.bob).to.equal(true); + expect(data.number).to.equal(rules.static.number); + expect(data.string).to.equal(rules.static.string); + expect(data.bool).to.equal(rules.static.bool); + expect(data.array).to.deep.equal(rules.static.array); + expect(data.object).to.deep.equal(rules.static.object); + }); }); describe('getFromReqObject', function() { From 48fc9dfa0e0c5c9c332568279484bbb25180cb57 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 18:23:50 +0200 Subject: [PATCH 12/24] simplifying top level test. --- src/crud/create.js | 2 +- test/crud/create.unit.js | 70 +++++++++++----------------------------- 2 files changed, 20 insertions(+), 52 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index da05f2f..3cf7b51 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -110,7 +110,7 @@ function setStatusIfApplicable(metadata) { req.body.statusLog = [ { status: req.body.status, - data: getData(statusToSet.initialData, req), + data: addCreateRoute.getData(statusToSet.initialData, req), statusDate: req.body.statusDate } ]; diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index cd41982..1c45815 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -140,72 +140,32 @@ describe('Crud - create', function() { } }); - it('Should be an object that deep equals initialData.static if only initialData.static was set', function( - done - ) { + it('Should be merge the result from getData', function(done) { const statusToSet = { name: 'a', - initialData: { - static: { - number: 1, - string: 'test', - bool: true, - array: [2, 'test', true, null, {}, []], - object: {} - } - } + initialData: {} }; const metadata = buildMetadata([statusToSet]); const middleware = addCreateRoute.setStatusIfApplicable(metadata); const reqOptions = { body: {} }; - mockRequest(middleware, reqOptions, null, next); - - function next(error) { - expect(error).to.not.be.ok(); - const data = reqOptions.body.statusLog[0].data; - expect(data).to.deep.equal(statusToSet.initialData.static); - done(); - } - }); - it('Should be an object with properties taken from the request object if only initialData.fromReq was set', function( - done - ) { - const reqOptions = { - body: {}, - user: { - username: 'Bob' - } - }; - const statusToSet = { - name: 'a', - initialData: { - fromReq: { - username: 'user.username', - doesNotExist: 'a', - nested: { - initialUsername: 'user.username' - } - //TODO what if not a string or object value? i.e. [boolean, null, undefined, array, number] - //TODO what if value isn't found? should we use "asdasd: ['a','defaultValue']" to denote using defaults? or do we throw an error? - //TODO security around retrieving things from request? Maybe only from certain parts of req? req.params? req.query? req.body? req.process? - //TODO if fromReq and static are both set, it will merge in an order, what about conflicts? Error? - //TODO refactor this fromReq code with detailed logic into it's own describe block for getFromReqObject - } + const stubbedData = { + bob: true, + asd: { + value: 1, + name: 'bob' } }; - const metadata = buildMetadata([statusToSet]); - const middleware = addCreateRoute.setStatusIfApplicable(metadata); - + const stub = sinon.stub(addCreateRoute, 'getData'); + stub.returns(stubbedData); mockRequest(middleware, reqOptions, null, next); function next(error) { + stub.restore(); expect(error).to.not.be.ok(); const data = reqOptions.body.statusLog[0].data; - expect(data.username).to.equal('Bob'); - expect(data.doesNotExist).to.not.be.ok(); - expect(data.nested.initialUsername).to.equal('Bob'); + expect(data).to.deep.equal(stubbedData); done(); } }); @@ -213,6 +173,8 @@ describe('Crud - create', function() { }); describe('getData', function() { + //TODO if fromReq and static are both set, it will merge in an order, what about conflicts? Error? + it('Should not exist if rules was falsy', function() { const data = addCreateRoute.getData(null, {}); expect(data).to.not.be.ok(); @@ -253,6 +215,7 @@ describe('Crud - create', function() { stub.restore(); expect(data.bob).to.equal(true); }); + it('Should merge the result from getFromReqObject and static if both were set', function() { const req = {}; const rules = { @@ -284,6 +247,10 @@ describe('Crud - create', function() { }); describe('getFromReqObject', function() { + //TODO what if not a string or object value? i.e. [boolean, null, undefined, array, number] + //TODO what if value isn't found? should we use "asdasd: ['a','defaultValue']" to denote using defaults? or do we throw an error? + //TODO security around retrieving things from request? Maybe only from certain parts of req? req.params? req.query? req.body? req.process? + it('Should map shallow properties from the req using the map', function() { const req = httpMocks.createRequest({ a: 'b' @@ -294,6 +261,7 @@ describe('Crud - create', function() { const data = addCreateRoute.getFromReqObject(map, req); expect(data.answer).to.equal('b'); }); + it('Should map deep properties from the req using the map', function() { const req = httpMocks.createRequest({ a: { From 1c47491b39bbeb5b2591b5ea5440c2f70fa8112f Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 18:42:06 +0200 Subject: [PATCH 13/24] test for merging --- test/crud/create.unit.js | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 1c45815..fcaa32f 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -173,8 +173,6 @@ describe('Crud - create', function() { }); describe('getData', function() { - //TODO if fromReq and static are both set, it will merge in an order, what about conflicts? Error? - it('Should not exist if rules was falsy', function() { const data = addCreateRoute.getData(null, {}); expect(data).to.not.be.ok(); @@ -244,6 +242,43 @@ describe('Crud - create', function() { expect(data.array).to.deep.equal(rules.static.array); expect(data.object).to.deep.equal(rules.static.object); }); + + it('Should prioritise fields from getFromReqObject over static if both were set', function() { + const req = {}; + const rules = { + fromReq: {}, + static: { + number: 1, + string: 'test', + bool: true, + array: [2, 'test', true, null, {}, []], + object: { + subObject: {} + } + } + }; + const stubbedData = { + bob: true, + number: 2, + string: 'test2', + bool: false, + array: [], + object: { + betterSubObject: {} + } + }; + const stub = sinon.stub(addCreateRoute, 'getFromReqObject'); + stub.returns(stubbedData); + const data = addCreateRoute.getData(rules, req); + stub.restore(); + expect(data.bob).to.equal(true); + expect(data.number).to.equal(stubbedData.number); + expect(data.string).to.equal(stubbedData.string); + expect(data.bool).to.equal(stubbedData.bool); + expect(data.array).to.deep.equal(rules.static.array); + expect(data.object.subObject).to.be.ok; + expect(data.object.betterSubObject).to.be.ok; + }); }); describe('getFromReqObject', function() { From cda4e7a9a825218ace5084e7add919154e40c6c6 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 20:07:11 +0200 Subject: [PATCH 14/24] More tests for formatting --- src/crud/create.js | 5 +++ test/crud/create.unit.js | 75 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/src/crud/create.js b/src/crud/create.js index 3cf7b51..aa0ce52 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -110,6 +110,7 @@ function setStatusIfApplicable(metadata) { req.body.statusLog = [ { status: req.body.status, + //we use 'addCreateRoute.' here to allow stubbing in the unit tests data: addCreateRoute.getData(statusToSet.initialData, req), statusDate: req.body.statusDate } @@ -122,6 +123,7 @@ function getData(rules, req) { if (!rules) { return; } + //we use 'addCreateRoute.' here to allow stubbing in the unit tests const fromReq = addCreateRoute.getFromReqObject(rules.fromReq, req); return _.merge({}, rules.static, fromReq); } @@ -147,6 +149,9 @@ function getFromReqObject(map, req, depth = 0) { data[key] = getFromReqObject(value, req, depth + 1); return; } + if (!_.isString(value)) { + throw new Error(util.format('Invalid map value, must be a string : \n%j\n', value)); + } data[key] = _.get(req, value); }); return data; diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index fcaa32f..28d5952 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -352,6 +352,81 @@ describe('Crud - create', function() { addCreateRoute.getFromReqObject(map, req); }).to.throw(/circular reference/i); }); + + const notAString = /must be a string/i; + + it('Should throw an error if the map was a number', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: 1 + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(notAString); + }); + + it('Should throw an error if the map was a boolean', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: true + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(notAString); + }); + + it('Should not throw an error if the map was an empty object', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: {} + }; + const data = addCreateRoute.getFromReqObject(map, req); + expect(data.answer).to.deep.equal({}); + }); + + it('Should throw an error if the map was null', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: null + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(notAString); + }); + + it('Should throw an error if the map was undefined', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: undefined + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(notAString); + }); + + it('Should if the map was a array', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + option1: ['asdasd', 12], //defaultValue? + option2: [], //throw error? + option3: ['a.doesNotExist', 'a.b'] // try get first one, if fails go onto next? + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(notAString); + }); }); }); From 0de1824e1a139dd95ea3b413749a06523f4d125d Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 20:15:58 +0200 Subject: [PATCH 15/24] implementing default value logic. --- src/crud/create.js | 4 ++++ test/crud/create.unit.js | 14 +++++--------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index aa0ce52..84fb0e5 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -145,6 +145,10 @@ function getFromReqObject(map, req, depth = 0) { const data = {}; Object.keys(map).forEach(function(key) { const value = map[key]; + if (_.isArray(value)) { + data[key] = _.get(req, value[0], value[1]); + return; + } if (_.isObject(value)) { data[key] = getFromReqObject(value, req, depth + 1); return; diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 28d5952..5ce38f0 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -282,8 +282,6 @@ describe('Crud - create', function() { }); describe('getFromReqObject', function() { - //TODO what if not a string or object value? i.e. [boolean, null, undefined, array, number] - //TODO what if value isn't found? should we use "asdasd: ['a','defaultValue']" to denote using defaults? or do we throw an error? //TODO security around retrieving things from request? Maybe only from certain parts of req? req.params? req.query? req.body? req.process? it('Should map shallow properties from the req using the map', function() { @@ -414,18 +412,16 @@ describe('Crud - create', function() { }).to.throw(notAString); }); - it('Should if the map was a array', function() { + //TODO what if value isn't found? should we use "asdasd: ['a','defaultValue']" to denote using defaults? or do we throw an error? + it('Should use the default value if one was supplied', function() { const req = httpMocks.createRequest({ a: 'b' }); const map = { - option1: ['asdasd', 12], //defaultValue? - option2: [], //throw error? - option3: ['a.doesNotExist', 'a.b'] // try get first one, if fails go onto next? + answer: ['c', 'd'] }; - expect(function() { - addCreateRoute.getFromReqObject(map, req); - }).to.throw(notAString); + const result = addCreateRoute.getFromReqObject(map, req); + expect(result.answer).to.equal('d'); }); }); }); From a4ba2c831e03b7354309bcbdce9c078db80f2ce1 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 20:21:56 +0200 Subject: [PATCH 16/24] Test for array value not being a string. --- src/crud/create.js | 10 +++++++--- test/crud/create.unit.js | 24 +++++++++++++++++++++++- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index 84fb0e5..231ab70 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -146,6 +146,7 @@ function getFromReqObject(map, req, depth = 0) { Object.keys(map).forEach(function(key) { const value = map[key]; if (_.isArray(value)) { + ensureMapIsString(value[0]); data[key] = _.get(req, value[0], value[1]); return; } @@ -153,13 +154,16 @@ function getFromReqObject(map, req, depth = 0) { data[key] = getFromReqObject(value, req, depth + 1); return; } - if (!_.isString(value)) { - throw new Error(util.format('Invalid map value, must be a string : \n%j\n', value)); - } + ensureMapIsString(value); data[key] = _.get(req, value); }); return data; } +function ensureMapIsString(map){ + if (!_.isString(map)) { + throw new Error(util.format('Invalid map value, must be a string : \n%j\n', map)); + } +} function setOwnerIfApplicable(metadata) { return function _setOwnerIfApplicable(req, res, next) { diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 5ce38f0..c39c768 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -412,7 +412,6 @@ describe('Crud - create', function() { }).to.throw(notAString); }); - //TODO what if value isn't found? should we use "asdasd: ['a','defaultValue']" to denote using defaults? or do we throw an error? it('Should use the default value if one was supplied', function() { const req = httpMocks.createRequest({ a: 'b' @@ -423,6 +422,29 @@ describe('Crud - create', function() { const result = addCreateRoute.getFromReqObject(map, req); expect(result.answer).to.equal('d'); }); + + it('Should use the first value in the array for the map if only that is specified', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: ['a'] + }; + const result = addCreateRoute.getFromReqObject(map, req); + expect(result.answer).to.equal('b'); + }); + + it('Should throw an error if the first value in the array was not a string', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: [1] + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(notAString); + }); }); }); From 47f9a84cf2c0b0eb546340d8de5cbdb1bb2f85e2 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 20:25:31 +0200 Subject: [PATCH 17/24] Check for too many items in the array. --- src/crud/create.js | 5 ++++- test/crud/create.unit.js | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/crud/create.js b/src/crud/create.js index 231ab70..fe3a566 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -147,6 +147,9 @@ function getFromReqObject(map, req, depth = 0) { const value = map[key]; if (_.isArray(value)) { ensureMapIsString(value[0]); + if (value.length > 2) { + throw new Error(util.format('Too many items in array, should be at most 2. %j', value)); + } data[key] = _.get(req, value[0], value[1]); return; } @@ -159,7 +162,7 @@ function getFromReqObject(map, req, depth = 0) { }); return data; } -function ensureMapIsString(map){ +function ensureMapIsString(map) { if (!_.isString(map)) { throw new Error(util.format('Invalid map value, must be a string : \n%j\n', map)); } diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index c39c768..7cd9845 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -445,6 +445,30 @@ describe('Crud - create', function() { addCreateRoute.getFromReqObject(map, req); }).to.throw(notAString); }); + + it('Should throw an error if the array is empty', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: [] + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(notAString); + }); + + it('Should throw an error if the array has more than 2 entries', function() { + const req = httpMocks.createRequest({ + a: 'b' + }); + const map = { + answer: ['a', 'a', 'a'] + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(/too many items in array/i); + }); }); }); From fa3e5fb0987694f714b4c5840b99b07c22e897db Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Wed, 14 Jun 2017 21:01:44 +0200 Subject: [PATCH 18/24] Implementing disallowedSuffixList --- src/crud/create.js | 16 +++++++++++++--- test/crud/create.unit.js | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index fe3a566..f94be6a 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -128,8 +128,9 @@ function getData(rules, req) { return _.merge({}, rules.static, fromReq); } +const defaultDisallowedSuffixList = ['password', 'passwordHash', 'passwordSalt']; const maxDepth = 10; -function getFromReqObject(map, req, depth = 0) { +function getFromReqObject(map, req, depth = 0, disallowedSuffixList = defaultDisallowedSuffixList) { if (!map) { return; } @@ -150,7 +151,7 @@ function getFromReqObject(map, req, depth = 0) { if (value.length > 2) { throw new Error(util.format('Too many items in array, should be at most 2. %j', value)); } - data[key] = _.get(req, value[0], value[1]); + data[key] = getValue(req, value[0], value[1], disallowedSuffixList); return; } if (_.isObject(value)) { @@ -158,10 +159,19 @@ function getFromReqObject(map, req, depth = 0) { return; } ensureMapIsString(value); - data[key] = _.get(req, value); + data[key] = getValue(req, value, undefined, disallowedSuffixList); }); return data; } + +function getValue(req, map, defaultValue, disallowedSuffixList) { + const disallowed = disallowedSuffixList.find(suffix => map.endsWith(suffix)); + if (disallowed) { + throw new Error('Map is not allowed to end with ' + disallowed); + } + return _.get(req, map, defaultValue); +} + function ensureMapIsString(map) { if (!_.isString(map)) { throw new Error(util.format('Invalid map value, must be a string : \n%j\n', map)); diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 7cd9845..02a3b56 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -282,8 +282,6 @@ describe('Crud - create', function() { }); describe('getFromReqObject', function() { - //TODO security around retrieving things from request? Maybe only from certain parts of req? req.params? req.query? req.body? req.process? - it('Should map shallow properties from the req using the map', function() { const req = httpMocks.createRequest({ a: 'b' @@ -469,6 +467,37 @@ describe('Crud - create', function() { addCreateRoute.getFromReqObject(map, req); }).to.throw(/too many items in array/i); }); + + //TODO security around retrieving things from request? Maybe only from certain parts of req? req.params? req.query? req.body? req.process? + //todo const allowedPrefixList = ['user', 'process', 'body', 'params', 'query']; + it('Should not allow map values that end with something on the exception list', function() { + const req = httpMocks.createRequest({ + a: { + b: 'c' + } + }); + const map = { + answer: 'a.b' + }; + const disallowedSuffixList = ['.b']; + expect(function() { + addCreateRoute.getFromReqObject(map, req, 0, disallowedSuffixList); + }).to.throw(/Map is not allowed to end with/i); + }); + + it('Should not allow map values that end with something on the default exception list', function() { + const req = httpMocks.createRequest({ + a: { + b: 'c' + } + }); + const map = { + answer: 'a.password' + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(/Map is not allowed to end with/i); + }); }); }); From ed44bc47f7896c2cadb68e5bcf66104e3cf399a5 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Thu, 15 Jun 2017 07:55:23 +0200 Subject: [PATCH 19/24] Protecting the req object from accessing non specified properties. --- src/crud/create.js | 21 +++++++++++++---- test/crud/create.unit.js | 49 ++++++++++++++++++++++++++++++++-------- 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index f94be6a..cf80b4a 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -129,8 +129,15 @@ function getData(rules, req) { } const defaultDisallowedSuffixList = ['password', 'passwordHash', 'passwordSalt']; +const defaultAllowedPrefixList = ['user', 'process', 'body', 'params', 'query']; const maxDepth = 10; -function getFromReqObject(map, req, depth = 0, disallowedSuffixList = defaultDisallowedSuffixList) { +function getFromReqObject( + map, + req, + depth = 0, + disallowedSuffixList = defaultDisallowedSuffixList, + allowedPrefixList = defaultAllowedPrefixList +) { if (!map) { return; } @@ -151,24 +158,28 @@ function getFromReqObject(map, req, depth = 0, disallowedSuffixList = defaultDis if (value.length > 2) { throw new Error(util.format('Too many items in array, should be at most 2. %j', value)); } - data[key] = getValue(req, value[0], value[1], disallowedSuffixList); + data[key] = getValue(req, value[0], value[1], disallowedSuffixList, allowedPrefixList); return; } if (_.isObject(value)) { - data[key] = getFromReqObject(value, req, depth + 1); + data[key] = getFromReqObject(value, req, depth + 1, disallowedSuffixList, allowedPrefixList); return; } ensureMapIsString(value); - data[key] = getValue(req, value, undefined, disallowedSuffixList); + data[key] = getValue(req, value, undefined, disallowedSuffixList, allowedPrefixList); }); return data; } -function getValue(req, map, defaultValue, disallowedSuffixList) { +function getValue(req, map, defaultValue, disallowedSuffixList, allowedPrefixList) { const disallowed = disallowedSuffixList.find(suffix => map.endsWith(suffix)); if (disallowed) { throw new Error('Map is not allowed to end with ' + disallowed); } + const allowed = allowedPrefixList.find(prefix => map.startsWith(prefix)); + if (!allowed) { + throw new Error(util.format('Map must start with one of %j', allowedPrefixList)); + } return _.get(req, map, defaultValue); } diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 02a3b56..5771066 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -289,7 +289,7 @@ describe('Crud - create', function() { const map = { answer: 'a' }; - const data = addCreateRoute.getFromReqObject(map, req); + const data = addCreateRoute.getFromReqObject(map, req, 0, undefined, ['a']); expect(data.answer).to.equal('b'); }); @@ -302,7 +302,7 @@ describe('Crud - create', function() { const map = { answer: 'a.b' }; - const data = addCreateRoute.getFromReqObject(map, req); + const data = addCreateRoute.getFromReqObject(map, req, 0, undefined, ['a']); expect(data.answer).to.equal('c'); }); @@ -315,7 +315,7 @@ describe('Crud - create', function() { answer: 'a' } }; - const data = addCreateRoute.getFromReqObject(map, req); + const data = addCreateRoute.getFromReqObject(map, req, 0, undefined, ['a']); expect(data.nested.answer).to.equal('b'); }); @@ -330,7 +330,7 @@ describe('Crud - create', function() { answer: 'a.b' } }; - const data = addCreateRoute.getFromReqObject(map, req); + const data = addCreateRoute.getFromReqObject(map, req, 0, undefined, ['a']); expect(data.nested.answer).to.equal('c'); }); @@ -417,7 +417,7 @@ describe('Crud - create', function() { const map = { answer: ['c', 'd'] }; - const result = addCreateRoute.getFromReqObject(map, req); + const result = addCreateRoute.getFromReqObject(map, req, 0, undefined, ['c']); expect(result.answer).to.equal('d'); }); @@ -428,7 +428,7 @@ describe('Crud - create', function() { const map = { answer: ['a'] }; - const result = addCreateRoute.getFromReqObject(map, req); + const result = addCreateRoute.getFromReqObject(map, req, 0, undefined, ['a']); expect(result.answer).to.equal('b'); }); @@ -468,8 +468,7 @@ describe('Crud - create', function() { }).to.throw(/too many items in array/i); }); - //TODO security around retrieving things from request? Maybe only from certain parts of req? req.params? req.query? req.body? req.process? - //todo const allowedPrefixList = ['user', 'process', 'body', 'params', 'query']; + const invalidSuffix = /Map is not allowed to end with/i; it('Should not allow map values that end with something on the exception list', function() { const req = httpMocks.createRequest({ a: { @@ -482,7 +481,7 @@ describe('Crud - create', function() { const disallowedSuffixList = ['.b']; expect(function() { addCreateRoute.getFromReqObject(map, req, 0, disallowedSuffixList); - }).to.throw(/Map is not allowed to end with/i); + }).to.throw(invalidSuffix); }); it('Should not allow map values that end with something on the default exception list', function() { @@ -496,7 +495,37 @@ describe('Crud - create', function() { }; expect(function() { addCreateRoute.getFromReqObject(map, req); - }).to.throw(/Map is not allowed to end with/i); + }).to.throw(invalidSuffix); + }); + + const invalidPrefix = /Map must start with one of /i; + it('Should not allow access to req properties are not on the exception list', function() { + const req = httpMocks.createRequest({ + a: { + b: 'c' + } + }); + const map = { + answer: 'a.b' + }; + const allowedPrefixList = []; + expect(function() { + addCreateRoute.getFromReqObject(map, req, 0, undefined, allowedPrefixList); + }).to.throw(invalidPrefix); + }); + + it('Should not allow map values that end with something on the default exception list', function() { + const req = httpMocks.createRequest({ + body: { + b: 'c' + } + }); + const map = { + answer: 'body.password' + }; + expect(function() { + addCreateRoute.getFromReqObject(map, req); + }).to.throw(invalidSuffix); }); }); }); From 7af130baf989310b0bc6df1abceab820d8f7a2b1 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Thu, 15 Jun 2017 16:42:57 +0200 Subject: [PATCH 20/24] Moving request-mocking to its own file. --- test/@util/request-mocking.js | 56 +++++++++++++++++++++++++++++++++++ test/crud/create.unit.js | 50 ++----------------------------- 2 files changed, 58 insertions(+), 48 deletions(-) create mode 100644 test/@util/request-mocking.js diff --git a/test/@util/request-mocking.js b/test/@util/request-mocking.js new file mode 100644 index 0000000..d66bb17 --- /dev/null +++ b/test/@util/request-mocking.js @@ -0,0 +1,56 @@ +'use strict'; +const httpMocks = require('node-mocks-http'); +const events = require('events'); + +module.exports = { + mockRequest, + shouldNotCallNext, + shouldCallNext, + shouldNotReturnResponse +}; + +function mockRequest(middlewareOrRouter, reqOptions, responseCallback, nextCallback) { + const req = httpMocks.createRequest(reqOptions); + const res = httpMocks.createResponse({ + eventEmitter: events.EventEmitter + }); + res.on('end', function() { + let resToReturn; + try { + resToReturn = { + statusCode: res._getStatusCode(), + body: JSON.parse(res._getData()), + headers: res._getHeaders(), + raw: res + }; + } catch (err) { + return responseCallback(err); + } + responseCallback(null, resToReturn); + }); + middlewareOrRouter(req, res, nextCallback); +} + +function shouldNotCallNext(done) { + return function next(err) { + if (err) { + return done(err); + } + return done(new Error('Next should not have been called')); + }; +} + +function shouldCallNext(done) { + return function next(err) { + if (err) { + return done(err); + } + return done(); + }; +} + +function shouldNotReturnResponse(done) { + return function resComplete() { + done(new Error('res.end should not have been called')); + }; +} diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 5771066..253e934 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -1,10 +1,10 @@ 'use strict'; require('../@util/init.js'); const addCreateRoute = require('../../src/crud/create'); -const httpMocks = require('node-mocks-http'); -const events = require('events'); +const mockRequest = require('../@util/request-mocking').mockRequest; const moment = require('moment'); const sinon = require('sinon'); +const httpMocks = require('node-mocks-http'); describe('Crud - create', function() { describe('setStatusIfApplicable', function() { @@ -541,49 +541,3 @@ function buildMetadata(statuses) { } return metadata; } - -function mockRequest(middlewareOrRouter, reqOptions, responseCallback, nextCallback) { - const req = httpMocks.createRequest(reqOptions); - const res = httpMocks.createResponse({ - eventEmitter: events.EventEmitter - }); - res.on('end', function() { - let resToReturn; - try { - resToReturn = { - statusCode: res._getStatusCode(), - body: JSON.parse(res._getData()), - headers: res._getHeaders(), - raw: res - }; - } catch (err) { - return responseCallback(err); - } - responseCallback(null, resToReturn); - }); - middlewareOrRouter(req, res, nextCallback); -} - -// function shouldNotCallNext(done) { -// return function next(err) { -// if (err) { -// return done(err); -// } -// return done(new Error('Next should not have been called')); -// }; -// } -// -// function shouldCallNext(done) { -// return function next(err) { -// if (err) { -// return done(err); -// } -// return done(); -// }; -// } -// -// function shouldNotReturnResponse(done) { -// return function resComplete() { -// done(new Error('res.end should not have been called')); -// }; -// } From dbd468a91b80338d631269ee47135e55c11fa260 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Thu, 15 Jun 2017 17:10:06 +0200 Subject: [PATCH 21/24] Updating todo's --- src/metadata/hydrate-schema.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/metadata/hydrate-schema.js b/src/metadata/hydrate-schema.js index 5e8432c..baa5249 100644 --- a/src/metadata/hydrate-schema.js +++ b/src/metadata/hydrate-schema.js @@ -45,9 +45,9 @@ function addStatusInfo(schema) { properties: { status: schema.properties.status, statusDate: schema.properties.statusDate, - data: schema.updateStatusSchema + data: schema.updateStatusSchema //todo anyof? }, - required: ['status', 'statusDate', 'data'], + required: ['status', 'statusDate', 'data'], //todo - data check schema anyof? additionalProperties: false }, additionalItems: false @@ -84,7 +84,7 @@ function addOwnerInfo(schema) { type: ['object', 'string'] //todo? } }, - required: ['owner', 'ownerDate', 'data'], + required: ['owner', 'ownerDate', 'data'], //todo - data check schema anyof? additionalProperties: false }, additionalItems: false From 7812249e1e7e8971895b36e048787f0ed0183e81 Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Thu, 15 Jun 2017 17:18:00 +0200 Subject: [PATCH 22/24] adding static inital data for user statuses. --- src/routes/users/user.json | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/routes/users/user.json b/src/routes/users/user.json index dbe8764..a65c7fc 100644 --- a/src/routes/users/user.json +++ b/src/routes/users/user.json @@ -7,7 +7,12 @@ "statuses": [ { "name": "active", - "description": "Default status, shows that the user is active and can login" + "description": "Default status, shows that the user is active and can login", + "initialData":{ + "static":{ + "reason":"testing" + } + } }, { "name": "inactive", From bfd721028e6b08ed5a661fc9e06c2384ea78157f Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Thu, 15 Jun 2017 17:35:32 +0200 Subject: [PATCH 23/24] running lint step --- src/crud/create.js | 23 +++++++++++++++++++---- test/crud/create.unit.js | 8 ++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/crud/create.js b/src/crud/create.js index cf80b4a..8a7c18f 100644 --- a/src/crud/create.js +++ b/src/crud/create.js @@ -90,7 +90,9 @@ function description(metadata) { responses: { '201': { description: - 'Informs the caller that the ' + metadata.title.toLowerCase() + ' was successfully created.', + 'Informs the caller that the ' + + metadata.title.toLowerCase() + + ' was successfully created.', commonHeaders: [correlationIdOptions.resHeader], model: metadata.schemas.output.name } @@ -156,13 +158,21 @@ function getFromReqObject( if (_.isArray(value)) { ensureMapIsString(value[0]); if (value.length > 2) { - throw new Error(util.format('Too many items in array, should be at most 2. %j', value)); + throw new Error( + util.format('Too many items in array, should be at most 2. %j', value) + ); } data[key] = getValue(req, value[0], value[1], disallowedSuffixList, allowedPrefixList); return; } if (_.isObject(value)) { - data[key] = getFromReqObject(value, req, depth + 1, disallowedSuffixList, allowedPrefixList); + data[key] = getFromReqObject( + value, + req, + depth + 1, + disallowedSuffixList, + allowedPrefixList + ); return; } ensureMapIsString(value); @@ -199,7 +209,12 @@ function setOwnerIfApplicable(metadata) { req.body.owner = _.get(req, ownership.setOwnerExpression); if (!req.body.owner) { return next( - boom.badRequest(util.format('Owner from expression "%s" was blank', ownership.setOwnerExpression)) + boom.badRequest( + util.format( + 'Owner from expression "%s" was blank', + ownership.setOwnerExpression + ) + ) ); } } else { diff --git a/test/crud/create.unit.js b/test/crud/create.unit.js index 253e934..0af0c90 100644 --- a/test/crud/create.unit.js +++ b/test/crud/create.unit.js @@ -70,7 +70,9 @@ describe('Crud - create', function() { } }); - it('Should create a status log entry with the status set to the first one in the schema', function(done) { + it('Should create a status log entry with the status set to the first one in the schema', function( + done + ) { const metadata = buildMetadata([{ name: 'a' }]); const middleware = addCreateRoute.setStatusIfApplicable(metadata); const reqOptions = { @@ -95,7 +97,9 @@ describe('Crud - create', function() { function next(error) { expect(error).to.not.be.ok(); - expect(moment(reqOptions.body.statusLog[0].statusDate).diff(new Date())).to.be.lessThan(1); + expect( + moment(reqOptions.body.statusLog[0].statusDate).diff(new Date()) + ).to.be.lessThan(1); done(); } }); From f922b9afebd9f1775d5e4e212a1ff1e7b6e5ce4e Mon Sep 17 00:00:00 2001 From: Ryan Kotzen Date: Thu, 15 Jun 2017 17:39:55 +0200 Subject: [PATCH 24/24] fixing wildcard issue --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index e54d0a0..f35dea0 100644 --- a/package.json +++ b/package.json @@ -77,8 +77,7 @@ "winston": "2.3.1", "winston-daily-rotate-file": "1.4.6", "winston-graylog2": "0.6.0", - "winston-loggly": "1.3.1", - "snyk": "^1.33.0" + "winston-loggly": "1.3.1" }, "devDependencies": { "chai": "4.0.1", @@ -99,6 +98,7 @@ "proxyquire": "1.8.0", "sinon": "2.3.2", "sinon-chai": "2.10.0", + "snyk": "1.33.0", "supertest": "3.0.0", "swagger-ui": "3.0.13" },