From fce03104ddc1bce984c6338dd1fdcb973bfb31df Mon Sep 17 00:00:00 2001 From: David Luecke Date: Thu, 20 Dec 2018 14:25:35 -0800 Subject: [PATCH 1/2] Upgrade to @feathersjs/adapter-commons --- lib/index.js | 210 +++++++++++++++++++++++---------------------- lib/utils.js | 21 +---- package-lock.json | 23 +++-- package.json | 7 +- test/connection.js | 11 ++- test/index.test.js | 84 +++++++++++++++--- test/utils.test.js | 20 ----- 7 files changed, 205 insertions(+), 171 deletions(-) diff --git a/lib/index.js b/lib/index.js index 304ab11..059bf70 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1,30 +1,47 @@ -const omit = require('lodash.omit'); -const Proto = require('uberproto'); const errors = require('@feathersjs/errors'); -const { select, filterQuery } = require('@feathersjs/commons'); +const { _ } = require('@feathersjs/commons'); +const { select, AdapterService } = require('@feathersjs/adapter-commons'); const utils = require('./utils'); - -class Service { +const defaultOperators = Op => { + return { + $eq: Op.eq, + $ne: Op.ne, + $gte: Op.gte, + $gt: Op.gt, + $lte: Op.lte, + $lt: Op.lt, + $in: Op.in, + $nin: Op.notIn, + $like: Op.like, + $notLike: Op.notLike, + $iLike: Op.ilike, + $notILike: Op.notILike, + $or: Op.or, + $and: Op.and + }; +}; + +class Service extends AdapterService { constructor (options) { - if (!options) { - throw new Error('Sequelize options have to be provided'); - } - if (!options.Model) { throw new Error('You must provide a Sequelize Model'); } - this.paginate = options.paginate || false; - this.options = options; - this.Model = options.Model; - this.id = options.id || 'id'; - this.events = options.events; + const defaultOps = defaultOperators(options.Model.sequelize.Op); + const operators = Object.assign(defaultOps, options.operators); + const whitelist = Object.keys(operators).concat(options.whitelist || []); + + super(Object.assign({ + id: 'id', + operators, + whitelist + }, options)); this.raw = options.raw !== false; } getModel (params) { - return this.Model; + return this.options.Model; } applyScope (params) { @@ -34,13 +51,47 @@ class Service { return this.getModel(params); } - extend (obj) { - return Proto.extend(obj, this); + filterQuery (params) { + const filtered = super.filterQuery(params); + const operators = this.options.operators; + const convertOperators = query => { + if (Array.isArray(query)) { + return query.map(convertOperators); + } + + if (!_.isObject(query)) { + return query; + } + + return Object.keys(query).reduce((result, prop) => { + const value = query[prop]; + const key = operators[prop] ? operators[prop] : prop; + + result[key] = convertOperators(value); + + return result; + }, {}); + }; + + filtered.query = convertOperators(filtered.query); + + return filtered; } - _find (params, getFilter = filterQuery, paginate) { - const { filters, query } = getFilter(params.query || {}); - const where = utils.getWhere(query); + // returns either the model intance for an id or all unpaginated + // items for `params` if id is null + _getOrFind (id, params) { + if (id === null) { + return this._find(Object.assign(params, { + paginate: false + })); + } + + return this._get(id, params); + } + + _find (params) { + const { filters, query: where, paginate } = this.filterQuery(params); const order = utils.getOrder(filters.$sort); const q = Object.assign({ @@ -70,7 +121,7 @@ class Service { delete q.raw; } - if (paginate) { + if (paginate && paginate.default) { return Model.findAndCountAll(q).then(result => { return { total: result.count, @@ -79,28 +130,13 @@ class Service { data: result.rows }; }).catch(utils.errorHandler); - } else { - return Model.findAll(q).then(result => { - return { - data: result - }; - }).catch(utils.errorHandler); - } - } - - find (params) { - const paginate = (params && typeof params.paginate !== 'undefined') ? params.paginate : this.paginate; - const result = this._find(params, where => filterQuery(where, paginate), paginate); - - if (!paginate.default) { - return result.then(page => page.data); } - return result; + return Model.findAll(q).catch(utils.errorHandler); } _get (id, params) { - const where = utils.getWhere(params.query); + const { query: where } = this.filterQuery(params); // Attach 'where' constraints, if any were used. const q = Object.assign({ @@ -118,28 +154,12 @@ class Service { } return result[0]; - }) - .then(select(params, this.id)) - .catch(error => { - throw new errors.NotFound(`No record found for id '${id}'`, error); - }); - } - - // returns either the model intance for an id or all unpaginated - // items for `params` if id is null - _getOrFind (id, params) { - if (id === null) { - return this._find(params).then(page => page.data); - } - - return this._get(id, params); + }).then(select(params, this.id)).catch(error => { + throw new errors.NotFound(`No record found for id '${id}'`, error); + }); } - get (id, params) { - return this._get(id, params).then(select(params, this.id)); - } - - create (data, params) { + _create (data, params) { const options = Object.assign({ raw: this.raw }, params.sequelize); // Model.create's `raw` option is different from other methods. // In order to use `raw` consistently to serialize the result, @@ -170,9 +190,9 @@ class Service { }).catch(utils.errorHandler); } - patch (id, data, params) { - const where = Object.assign({}, filterQuery(params.query || {}).query); - const mapIds = page => page.data.map(current => current[this.id]); + _patch (id, data, params) { + const where = Object.assign({}, this.filterQuery(params).query); + const mapIds = data => data.map(current => current[this.id]); if (id !== null) { where[this.id] = id; @@ -187,7 +207,7 @@ class Service { options.returning = true; return this._getOrFind(id, params) - .then(results => this.getModel(params).update(omit(data, this.id), options)) + .then(results => this.getModel(params).update(_.omit(data, this.id), options)) .then(results => { if (id === null) { return results[1]; @@ -209,53 +229,39 @@ class Service { const ids = id === null ? this._find(params) .then(mapIds) : Promise.resolve([ id ]); - return ids - .then(idList => { - // Create a new query that re-queries all ids that - // were originally changed - const findParams = Object.assign({}, params, { - query: { [this.id]: { $in: idList } } + return ids.then(idList => { + // Create a new query that re-queries all ids that + // were originally changed + const findParams = Object.assign({}, params, Object.assign({}, { + query: Object.assign({ [this.id]: { $in: idList } }, params.query) + })); + + return Model.update(_.omit(data, this.id), options) + .then(() => { + if (params.$returning !== false) { + return this._getOrFind(id, findParams); + } else { + return Promise.resolve([]); + } }); - - return Model.update(omit(data, this.id), options) - .then(() => { - if (params.$returning !== false) { - return this._getOrFind(id, findParams); - } else { - return Promise.resolve([]); - } - }); - }) - .then(select(params, this.id)) - .catch(utils.errorHandler); + }).then(select(params, this.id)).catch(utils.errorHandler); } - update (id, data, params) { - const where = Object.assign({}, filterQuery(params.query || {}).query); + _update (id, data, params) { + const where = Object.assign({}, this.filterQuery(params).query); const options = Object.assign({ raw: this.raw }, params.sequelize); - if (Array.isArray(data)) { - return Promise.reject(new errors.BadRequest('Not replacing multiple records. Did you mean `patch`?')); - } - // Force the {raw: false} option as the instance is needed to properly // update const updateOptions = Object.assign({}, params.sequelize, { raw: false }); const getOptions = Object.assign({}, params, { query: where, sequelize: { raw: false } }); return this._get(id, getOptions).then(instance => { - if (!instance) { - throw new errors.NotFound(`No record found for id '${id}'`); - } + const copy = Object.keys(instance.toJSON()).reduce((result, key) => { + result[key] = typeof data[key] === 'undefined' ? null : data[key]; - let copy = {}; - Object.keys(instance.toJSON()).forEach(key => { - if (typeof data[key] === 'undefined') { - copy[key] = null; - } else { - copy[key] = data[key]; - } - }); + return result; + }, {}); return instance.update(copy, updateOptions).then(() => this._get(id, { sequelize: options })); }) @@ -263,15 +269,15 @@ class Service { .catch(utils.errorHandler); } - remove (id, params) { + _remove (id, params) { const opts = Object.assign({ raw: this.raw }, params); - const where = Object.assign({}, filterQuery(params.query || {}).query); + const where = Object.assign({}, this.filterQuery(params).query); if (id !== null) { where[this.id] = id; } - const options = Object.assign({}, params.sequelize, { where }); + const options = Object.assign({}, { where }, params.sequelize); let Model = this.applyScope(params); @@ -289,9 +295,7 @@ class Service { } } -function init (options) { - return new Service(options); -} +const init = options => new Service(options); module.exports = init; diff --git a/lib/utils.js b/lib/utils.js index 3f57c9a..da49ce3 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -1,6 +1,6 @@ const errors = require('@feathersjs/errors'); -exports.errorHandler = function errorHandler (error) { +exports.errorHandler = error => { let feathersError = error; if (error.name) { @@ -32,22 +32,3 @@ exports.getOrder = (sort = {}) => Object.keys(sort).reduce((order, name) => { return order; }, []); - -exports.getWhere = (query = {}) => Object.keys(query).reduce((result, prop) => { - if (prop === '$select') { - return result; - } - - let value = query[prop]; - - if (value && value.$nin) { - value = Object.assign({}, value); - - value.$notIn = value.$nin; - delete value.$nin; - } - - result[prop] = value; - - return result; -}, {}); diff --git a/package-lock.json b/package-lock.json index 1a94d1f..7d78974 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4,10 +4,19 @@ "lockfileVersion": 1, "requires": true, "dependencies": { + "@feathersjs/adapter-commons": { + "version": "1.0.4", + "resolved": "https://registry.npmjs.org/@feathersjs/adapter-commons/-/adapter-commons-1.0.4.tgz", + "integrity": "sha512-6qC/mCxxKT9qgx8/PK7goKgBrD5HoKejPoSe4saHk5Z8kspkm8RTehWhysFO/yppLJofIvBejcs9ABrnDjzOLg==", + "requires": { + "@feathersjs/commons": "^4.0.0", + "@feathersjs/errors": "^3.3.5" + } + }, "@feathersjs/commons": { - "version": "1.4.4", - "resolved": "https://registry.npmjs.org/@feathersjs/commons/-/commons-1.4.4.tgz", - "integrity": "sha512-ZPpzyZA3CPfoa9AuFv3BJUI/ubzaaXixp8T/pqeMFPT6DOaU/6oF7lz1RxwimzfJNna4gy/HByt0EoLSI3BKWg==" + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/@feathersjs/commons/-/commons-4.0.0.tgz", + "integrity": "sha512-hyCLse4RiNt7WLPQ0KSudjoVnwiRykhYzIZdtDon9i2usPaoYkFw/fEYZrkE5HVB+sUTwphZoZ1oOntRk1MShg==" }, "@feathersjs/errors": { "version": "3.3.5", @@ -2243,11 +2252,6 @@ "integrity": "sha512-cQKh8igo5QUhZ7lg38DYWAxMvjSAKG0A8wGSVimP07SIUEK2UO+arSRKbRZWtelMtN5V0Hkwh5ryOto/SshYIg==", "dev": true }, - "lodash.omit": { - "version": "4.5.0", - "resolved": "https://registry.npmjs.org/lodash.omit/-/lodash.omit-4.5.0.tgz", - "integrity": "sha1-brGa5aHuHdnfC5aeZs4Lf6MLXmA=" - }, "long": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/long/-/long-4.0.0.tgz", @@ -3923,7 +3927,8 @@ "uberproto": { "version": "2.0.4", "resolved": "https://registry.npmjs.org/uberproto/-/uberproto-2.0.4.tgz", - "integrity": "sha512-c/5xjTcztW9XVhrkCycHQRBIAxww5JpDKk/q0zc2tVdQn6ZQvnChWgLvQaWAT1Al5JvRyvloUI15ad41m6dYwg==" + "integrity": "sha512-c/5xjTcztW9XVhrkCycHQRBIAxww5JpDKk/q0zc2tVdQn6ZQvnChWgLvQaWAT1Al5JvRyvloUI15ad41m6dYwg==", + "dev": true }, "uglify-js": { "version": "3.4.9", diff --git a/package.json b/package.json index b1c5cba..ad9487c 100644 --- a/package.json +++ b/package.json @@ -57,10 +57,9 @@ "lib": "lib" }, "dependencies": { - "@feathersjs/commons": "^1.3.0", - "@feathersjs/errors": "^3.3.5", - "lodash.omit": "^4.5.0", - "uberproto": "^2.0.4" + "@feathersjs/adapter-commons": "^1.0.5", + "@feathersjs/commons": "^4.0.0", + "@feathersjs/errors": "^3.3.5" }, "devDependencies": { "@feathersjs/express": "^1.3.0", diff --git a/test/connection.js b/test/connection.js index ae53f35..a2146f8 100644 --- a/test/connection.js +++ b/test/connection.js @@ -1,21 +1,26 @@ const Sequelize = require('sequelize'); module.exports = (DB) => { + const operatorsAliases = false; + if (DB === 'postgres') { return new Sequelize('sequelize', 'postgres', '', { host: 'localhost', - dialect: 'postgres' + dialect: 'postgres', + operatorsAliases }); } else if (DB === 'mysql') { return new Sequelize('sequelize', 'root', '', { host: '127.0.0.1', - dialect: 'mysql' + dialect: 'mysql', + operatorsAliases }); } else { return new Sequelize('sequelize', '', '', { dialect: 'sqlite', storage: './db.sqlite', - logging: false + logging: false, + operatorsAliases }); } }; diff --git a/test/index.test.js b/test/index.test.js index f932f94..b906635 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -2,13 +2,72 @@ const pg = require('pg'); const assert = require('assert'); const { expect } = require('chai'); -const { base } = require('feathers-service-tests'); - const Sequelize = require('sequelize'); const errors = require('@feathersjs/errors'); const feathers = require('@feathersjs/feathers'); +const adaptertests = require('@feathersjs/adapter-commons/tests'); const service = require('../lib'); +const testSuite = adaptertests([ + '.options', + '.events', + '._get', + '._find', + '._create', + '._update', + '._patch', + '._remove', + '.get', + '.get + $select', + '.get + id + query', + '.get + NotFound', + '.find', + '.remove', + '.remove + $select', + '.remove + id + query', + '.remove + multi', + '.update', + '.update + $select', + '.update + id + query', + '.update + NotFound', + '.patch', + '.patch + $select', + '.patch + id + query', + '.patch multiple', + '.patch multi query', + '.patch + NotFound', + '.create', + '.create + $select', + '.create multi', + 'internal .find', + 'internal .get', + 'internal .create', + 'internal .update', + 'internal .patch', + 'internal .remove', + '.find + equal', + '.find + equal multiple', + '.find + $sort', + '.find + $sort + string', + '.find + $limit', + '.find + $limit 0', + '.find + $skip', + '.find + $select', + '.find + $or', + '.find + $in', + '.find + $nin', + '.find + $lt', + '.find + $lte', + '.find + $gt', + '.find + $gte', + '.find + $ne', + '.find + $gt + $lt + $sort', + '.find + $or nested + $sort', + '.find + paginate', + '.find + paginate + $limit + $skip', + '.find + paginate + $limit 0', + '.find + paginate + params' +]); // The base tests require the use of Sequelize.BIGINT to avoid 'out of range errors' // Unfortunetly BIGINT's are serialized as Strings: @@ -109,10 +168,6 @@ describe('Feathers Sequelize Service', () => { }); describe('Initialization', () => { - it('throws an error when missing options', () => { - expect(service.bind(null)).to.throw('Sequelize options have to be provided'); - }); - it('throws an error when missing a Model', () => { expect(service.bind(null, { name: 'Test' })).to.throw(/You must provide a Sequelize Model/); }); @@ -130,8 +185,8 @@ describe('Feathers Sequelize Service', () => { id: 'customid' })); - base(app, errors); - base(app, errors, 'people-customid', 'customid'); + testSuite(app, errors, 'people', 'id'); + testSuite(app, errors, 'people-customid', 'customid'); }); describe('Feathers-Sequelize Specific Tests', () => { @@ -141,14 +196,17 @@ describe('Feathers Sequelize Service', () => { paginate: { default: 10 }, - events: [ 'testing' ] + events: [ 'testing' ], + multi: true })) .use('/orders', service({ - Model: Order + Model: Order, + multi: true })) .use('/custom-getter-setter', service({ Model: CustomGetterSetter, - events: [ 'testing' ] + events: [ 'testing' ], + multi: true })); before(() => app.service('people') @@ -309,7 +367,8 @@ describe('Feathers Sequelize Service', () => { const app = feathers(); app.use('/raw-people', service({ Model, - events: [ 'testing' ] + events: [ 'testing' ], + multi: true })); const rawPeople = app.service('raw-people'); @@ -317,6 +376,7 @@ describe('Feathers Sequelize Service', () => { app.use('/people', service({ Model, events: [ 'testing' ], + multi: true, raw: false // -> this is what we are testing })); const people = app.service('people'); diff --git a/test/utils.test.js b/test/utils.test.js index d7f4fb3..f74ebad 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -113,24 +113,4 @@ describe('Feathers Sequelize Utils', () => { expect(order).to.deep.equal([ ['name', 'ASC'], ['age', 'DESC'] ]); }); }); - - describe('getWhere', () => { - it('returns empty object when nothing is passed in', () => { - let where = utils.getWhere(); - - expect(where).to.deep.equal({}); - }); - - it('returns where conditions properly converted', () => { - let where = utils.getWhere({ name: 'Joe', age: { $lte: 25 } }); - - expect(where).to.deep.equal({ name: 'Joe', age: { $lte: 25 } }); - }); - - it('converts $nin to $notIn', () => { - let where = utils.getWhere({ name: { $nin: ['Joe', 'Alice'] } }); - - expect(where).to.deep.equal({ name: { $notIn: ['Joe', 'Alice'] } }); - }); - }); }); From 4ed67199dea2e45f9f9c4ae723d780ae6c162b14 Mon Sep 17 00:00:00 2001 From: David Luecke Date: Thu, 20 Dec 2018 14:49:35 -0800 Subject: [PATCH 2/2] Update documentation --- README.md | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 8b6e745..488b368 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,9 @@ __Options:__ - `raw` (*optional*, default: `true`) - Runs queries faster by returning plain objects instead of Sequelize models. - `events` (*optional*) - A list of [custom service events](https://docs.feathersjs.com/api/events.html#custom-events) sent by this service - `paginate` (*optional*) - A [pagination object](https://docs.feathersjs.com/api/databases/common.html#pagination) containing a `default` and `max` page size +- `multi` (*optional*) - Allow `create` with arrays and `update` and `remove` with `id` `null` to change multiple items. Can be `true` for all methods or an array of allowed methods (e.g. `[ 'remove', 'create' ]`) +- `operators` (*optional*) - A mapping from query syntax property names to to [Sequelize secure operators](http://docs.sequelizejs.com/manual/tutorial/querying.html) +- `whitelist` (*optional*) - A list of additional query parameters to allow (e..g `[ '$regex', '$geoNear' ]`). Default is the supported `operators` ### params.sequelize @@ -70,6 +73,27 @@ app.service('messages').hooks({ }); ``` +### operators + +Sequelize deprecated string based operators a while ago for security reasons. Starting at version 4.0.0 `feathers-sequelize` converts queries securely. If you want to support additional Sequelize operators, the `operators` service option can contain a mapping from query parameter name to Sequelize operator. By default supported are: + +``` +'$eq', +'$ne', +'$gte', +'$gt', +'$lte', +'$lt', +'$in', +'$nin', +'$like', +'$notLike', +'$iLike', +'$notILike', +'$or', +'$and' +``` + ## Sequelize `raw` queries By default, all `feathers-sequelize` operations will return `raw` data (using `raw: true` when querying the database). This results in faster execution and allows feathers-sequelize to interoperate with feathers-common hooks and other 3rd party integrations. However, this will bypass some of the "goodness" you get when using Sequelize as an ORM: @@ -480,8 +504,34 @@ In the unfortunate case where you must revert your app to a previous state, it i 1. Revert your code back to the previous state 1. Start your app +### Migrating + +`feathers-sequelize` 4.0.0 comes with important security and usability updates. + +> __Important:__ For general migration information to the new database adapter functionality see [crow.docs.feathersjs.com/migrating.html#database-adapters](https://crow.docs.feathersjs.com/migrating.html#database-adapters). + +The following breaking changes have been introduced: + +- All methods now take `params.sequelize` into account +- All methods allow additional query parameters +- Multiple updates are disabled by default (see the `multi` option) +- Upgraded to secure Sequelize operators (see the [operators](#operators) option) +- Errors no longer contain Sequelize specific information. The original Sequelize error can be retrieved on the server via: + +```js +const { ERROR } = require('feathers-sequelize'); + +try { + await sequelizeService.doSomethign(); +} catch(error) { + // error is a FeathersError + // Safely retrieve the Sequelize error + const sequelizeError = error.data[ERROR]; +} +``` + ## License -Copyright (c) 2017 +Copyright (c) 2019 Licensed under the [MIT license](LICENSE).