From 2a5ad1c4ea18a001c4deca29068ee9bec8b4879f Mon Sep 17 00:00:00 2001 From: Jane Jeon Date: Thu, 2 Jan 2020 11:33:01 -0500 Subject: [PATCH] adds support for casl, closes #37 --- index.js | 151 +++++++++--------- lib/casl.js | 32 +++- lib/role-acl.js | 26 --- lib/role-acl@3.js | 34 ++++ package.json | 7 +- test/casl.test.js | 5 +- test/index.test.js | 13 -- test/{role-acl.test.js => role-acl@3.test.js} | 4 +- test/utils/plugin-test.js | 3 +- yarn.lock | 38 +++-- 10 files changed, 169 insertions(+), 144 deletions(-) delete mode 100644 lib/role-acl.js create mode 100644 lib/role-acl@3.js delete mode 100644 test/index.test.js rename test/{role-acl.test.js => role-acl@3.test.js} (90%) diff --git a/index.js b/index.js index 99416b2..d814659 100644 --- a/index.js +++ b/index.js @@ -1,23 +1,25 @@ const assert = require('http-assert') const isEmpty = obj => !Object.keys(obj || {}).length -const debug = require('debug')('objection-authorize') const pick = require('lodash.pick') const omit = require('lodash.omit') module.exports = (acl, library = 'role-acl', opts) => { if (!acl) throw new Error('acl is a required parameter!') - if (typeof library === 'object') + if (typeof library === 'object') { throw new Error( - 'With v3, objection-authorize now has the signature (acl, library, opts)' + 'objection-authorize@3 now has the signature (acl, library, opts)' ) + } const defaultOpts = { defaultRole: 'anonymous', unauthenticatedErrorCode: 401, unauthorizedErrorCode: 403, - resourceAugments: { true: true, false: false, undefined: undefined }, userFromResult: false, - contextKey: 'req' + // below are role-acl specific options + contextKey: 'req', + roleFromUser: user => user.role, + resourceAugments: { true: true, false: false, undefined: undefined } } opts = Object.assign(defaultOpts, opts) @@ -31,27 +33,20 @@ module.exports = (acl, library = 'role-acl', opts) => { // wrappers around acl, querybuilder, and model _checkAccess (action, body) { - // _checkAccess may be called outside of authorization context - if (!this._shouldCheckAccess) return - - debug('_checkAccess', action, body) - let { + const { _user: user, _resource: resource, _opts: opts, _action } = this.context() - body = body || resource + // allowed the specified action to override the default, inferred action action = _action || action - resource = this.modelClass().fromJson(resource, { - skipValidation: true - }) - const access = lib.getAccess(acl, user, resource, action, ctx, opts) + const access = lib.getAccess(acl, user, resource, action, body, opts) // authorize request assert( - lib.isAuthorized(access), + lib.isAuthorized(access, action, resource), user.role === opts.defaultRole ? opts.unauthenticatedErrorCode : opts.unauthorizedErrorCode @@ -60,125 +55,122 @@ module.exports = (acl, library = 'role-acl', opts) => { return access } + // convenience helper for insert/update/delete + _filterBody (action, body) { + if (!this._shouldCheckAccess) return body + + const access = this._checkAccess(action, body) + const { _resource: resource } = this.context() + + // there's no need to cache these fields because this isn't the read access. + const pickFields = lib.pickFields(access, action, resource) + const omitFields = lib.omitFields(access, action, resource) + + if (pickFields.length) body = pick(body, pickFields) + if (omitFields.length) body = omit(body, omitFields) + + return body + } + // insert/patch/update/delete are the "primitive" query actions. // All other methods like insertAndFetch or deleteById are built on these. // automatically checks if you can create this resource, and if yes, - // restricts the body object to only the fields they're allowed to set + // restricts the body object to only the fields they're allowed to set. insert (body) { - debug('insert', body) - if (this._shouldCheckAccess) - body = this._checkAccess('create', body).filter(body) + return super.insert(this._filterBody('create', body)) + } - return super.insert(body) + insertAndFetch (body) { + return super.insertAndFetch(this._filterBody('create', body)) } patch (body) { - debug('patch', body) - if (this._shouldCheckAccess) - body = this._checkAccess('update', body).filter(body) - - return super.patch(body) + return super.patch(this._filterBody('update', body)) } patchAndFetch (body) { - debug('patchAndFetch', body) - if (this._shouldCheckAccess) - body = this._checkAccess('update', body).filter(body) - - return super.patchAndFetch(body) + return super.patchAndFetch(this._filterBody('update', body)) } + // istanbul ignore next patchAndFetchById (id, body) { - debug('patchAndFetchById', id, body) - if (this._shouldCheckAccess) - body = this._checkAccess('update', body).filter(body) - - return super.patchAndFetchById(id, body) + return super.patchAndFetchById(id, this._filterBody('update', body)) } // istanbul ignore next update (body) { - debug('update', body) - if (this._shouldCheckAccess) - body = this._checkAccess('update', body).filter(body) - - return super.update(body) + return super.update(this._filterBody('update', body)) } // istanbul ignore next updateAndFetch (body) { - debug('updateAndFetch', body) - if (this._shouldCheckAccess) - body = this._checkAccess('update', body).filter(body) - - return super.updateAndFetch(body) + return super.updateAndFetch(this._filterBody('update', body)) } // istanbul ignore next updateAndFetchById (id, body) { - debug('updateAndFetchById', id, body) - if (this._shouldCheckAccess) - body = this._checkAccess('update', body).filter(body) - - return super.updateAndFetchById(id, body) + return super.updateAndFetchById(id, this._filterBody('update', body)) } delete (body) { - debug('delete', body) this._checkAccess('delete', body) - return super.delete() } + // istanbul ignore next deleteById (id, body) { - debug('deleteById', id, body) - - return this.findById(id).delete(body) + this._checkAccess('delete', body) + return super.deleteById(id) } // specify a custom action, which takes precedence over the "default" action. action (_action) { - debug('action', _action) this.mergeContext({ _action }) - return this } // result is always an array, so we figure out if we should look at the result // as a single object instead by looking at whether .first() was called or not. first () { - debug('first') this.mergeContext({ _first: true }) - return super.first() } // THE magic method that schedules the actual authorization logic to be called // later down the line when the query is built and is ready to be executed. authorize (user, resource, optOverride) { + resource = resource || this.context()._instance || {} + // wrap the resource to give it all the custom methods & properties + // defined in the associating model class (e.g. Post, User). + resource = this.modelClass().fromJson(resource, { + skipValidation: true + }) + this.mergeContext({ _user: Object.assign({ role: opts.defaultRole }, user), - _resource: resource || this.context()._instance || {}, + _resource: resource, _opts: Object.assign({}, opts, optOverride), _authorize: true }) // This is run AFTER the query has been completely built. // In other words, the query already checked create/update/delete access // by this point, and the only thing to check now is the read access, - // IF the resource is specified. Otherwise, it's delayed till the end! + // IF the resource is specified. + // Otherwise, we check the read access after the query has been run, on the + // query results as the resource. .runBefore(async (result, query) => { if (query.isFind() && !isEmpty(resource)) { const readAccess = query._checkAccess('read') - // store the read access just in case + // store the read access so that it can be reused after the query. query.mergeContext({ readAccess }) } return result }) .runAfter(async (result, query) => { - // there's no result object(s) to filter here + // If there's no result objects, we don't need to filter them. if (typeof result !== 'object' || !query._shouldCheckAccess) return result @@ -192,15 +184,19 @@ module.exports = (acl, library = 'role-acl', opts) => { _readAccess: readAccess } = query.context() - // set the resource as the result if it's still not set! + // Set the resource as the result if it's still not set! // Note, since the resource needs to be singular, it can only be done - // when there's only one result! - // We're trusting that if the query returns an array of results, + // when there's only one result - + // we're trusting that if the query returns an array of results, // then you've already filtered it according to the user's read access - // in the query (instead of relying on the ACL to do it) since it's costly! - if (isEmpty(resource)) { - if (!isArray) query.mergeContext({ _resource: result }) - else if (first) query.mergeContext({ _resource: result[0] }) + // in the query (instead of relying on the ACL to do it) since it's costly + // to check every single item in the result array... + if (isEmpty(resource) && (!isArray || first)) { + resource = isArray ? result[0] : result + resource = query.modelClass().fromJson(resource, { + skipValidation: true + }) + query.mergeContext({ _resource: resource }) } // after create/update operations, the returning result may be the requester @@ -209,7 +205,7 @@ module.exports = (acl, library = 'role-acl', opts) => { !isArray && opts.userFromResult ) { - // check if we the user is changed + // check if the user is changed const resultIsUser = typeof opts.userFromResult === 'function' ? opts.userFromResult(user, result) @@ -232,8 +228,8 @@ module.exports = (acl, library = 'role-acl', opts) => { // 1. arrays don't have toJSON() method, // 2. objection-visibility doesn't work without calling $formatJson() return isArray - ? result.map(model => model._filter(readAccess)) - : result._filter(readAccess) + ? result.map(model => model._filterModel(readAccess)) + : result._filterModel(readAccess) }) // for chaining @@ -242,9 +238,10 @@ module.exports = (acl, library = 'role-acl', opts) => { } return class extends Model { - _filter (access) { - const pickFields = lib.pickFields(access) - const omitFields = lib.omitFields(access) + // filter the model instance directly + _filterModel (readAccess) { + const pickFields = lib.pickFields(readAccess, 'read', this) + const omitFields = lib.omitFields(readAccess, 'read', this) if (pickFields.length) this.$pick(pickFields) if (omitFields.length) this.$omit(omitFields) diff --git a/lib/casl.js b/lib/casl.js index 4e1f891..45fa623 100644 --- a/lib/casl.js +++ b/lib/casl.js @@ -1,13 +1,29 @@ const { permittedFieldsOf } = require('@casl/ability/extra') -exports.getAccess = (acl, user, resource, action, opts) => - acl(user, resource, action) +// O(M) with regards to the number of rules to match. +// Note, however, that since we know what the resource is beforehand, +// we can cut down the number of rules that need to be encoded in the acl +// by a factor of n where n is the number of resources. +// This different approach of monolithic vs. functional acl between role-acl and casl +// results in a drastically better overall time complexity for casl. +exports.getAccess = (acl, user, resource, action, body, opts) => + acl(user, resource, action, body, opts) -exports.isAuthorized = (ability, action, subject) => - ability.can(action, subject) +exports.isAuthorized = (ability, action, resource) => + ability.can(action, resource) -exports.filter = (ability, body) => access.filter(body) +// While role-acl is able to "infer" that ['*', '!email'] + ['email'] = '*', casl cannot. +// In casl world, the same equation comes out to ['email'], so we need to provide ALL the fields +// for a given model when picking fields (remember, casl does not have any concept of "exclusions", +// e.g. '!email'). The model *itself* doesn't have all of the necessary field information; +// however, Objection has methods to fetch the fields by actually querying the database with knex +// and caching it; and here, we're just fetching the cached metadata and extracting the fields. +// Unfortunately, having to iterate thru every single field means we're incurring a fixed +// O(N) cost with regards to the number of fields. +exports.pickFields = (ability, action, resource) => + permittedFieldsOf(ability, action, resource, { + fieldsFrom: rule => + rule.fields || (resource.constructor.tableMetadata() || {}).columns + }) -exports.pickFields = (ability, action, subject) => permittedFieldsOf(ability) - -exports.omitFields = ability => [] +exports.omitFields = () => [] diff --git a/lib/role-acl.js b/lib/role-acl.js deleted file mode 100644 index c37bf36..0000000 --- a/lib/role-acl.js +++ /dev/null @@ -1,26 +0,0 @@ -exports.getAccess = (acl, user, resource, action, body, opts) => - acl - .can(user.role) - .execute(action) - .context( - Object.assign( - {}, - { [opts.contextKey]: { user, body } }, - opts.resourceAugments, - resource - ) - ) - .on(resource.constructor.name) - -exports.isAuthorized = access => access.granted - -exports.filter = (access, body) => access.filter(body) - -exports.pickFields = access => - access.attributes.filter(field => field !== '*' && !field.startsWith('!')) || - [] - -exports.omitFields = access => - access.attributes - .filter(field => field.startsWith('!')) - .map(field => field.substr(1)) || [] diff --git a/lib/role-acl@3.js b/lib/role-acl@3.js new file mode 100644 index 0000000..963ed76 --- /dev/null +++ b/lib/role-acl@3.js @@ -0,0 +1,34 @@ +// Because role-acl expects all necessary information to stored in a single context object +// when checking access, we have to take everything we have - req.user, req.body, true, false, +// and the resource - and merge it into one. +// Thus, this is a fixed O(NM) cost with regards to N: the number of fields in the resource, +// and M: the number of rules to match. +exports.getAccess = (acl, user, resource, action, body, opts) => + acl + .can(opts.roleFromUser(user)) // role + .execute(action) + .context( + Object.assign( + {}, + { [opts.contextKey]: { user, body } }, + opts.resourceAugments, + resource + ) + ) + .on(resource.constructor.name) + +exports.isAuthorized = access => access.granted + +// We need to separate out the fields to pick and omit from. +// Thus, it's a fixed O(N) cost (again). +// However, we currently have to go over the list (of properties) *twice*, +// once for filtering pickfields, once for omitFields. +// TODO: figure out a way to do this in one pass! +exports.pickFields = access => + access.attributes.filter(field => field !== '*' && !field.startsWith('!')) || + [] + +exports.omitFields = access => + access.attributes + .filter(field => field.startsWith('!')) + .map(field => field.substr(1)) || [] diff --git a/package.json b/package.json index 5c84aa1..b992573 100644 --- a/package.json +++ b/package.json @@ -29,14 +29,10 @@ "singleQuote": true }, "dependencies": { - "debug": "^4.1.1", "http-assert": "^1.4.1", "lodash.omit": "^4.5.0", "lodash.pick": "^4.4.0" }, - "peerDependencies": { - "objection": "1.X" - }, "devDependencies": { "@casl/ability": "^3.2.0", "@types/jest": "^24.0.25", @@ -50,7 +46,8 @@ "objection": "^1.6.11", "objection-visibility": "^0.4.0", "prettier-standard": "^16.1.0", - "role-acl": "<4.0.0", + "role-acl-3": "npm:role-acl@<4.0.0", + "role-acl-4": "npm:role-acl@>=4.3.2", "sqlite3": "^4.1.1" }, "keywords": [ diff --git a/test/casl.test.js b/test/casl.test.js index fdefb94..8f67ab7 100644 --- a/test/casl.test.js +++ b/test/casl.test.js @@ -1,8 +1,7 @@ const pluginTest = require('./utils/plugin-test') -const { AbilityBuilder, Ability } = require('@casl/ability') +const { AbilityBuilder } = require('@casl/ability') -// TODO: do I need to put the deny before or after the allow? -function acl (user, resource, action, ctx) { +function acl (user, resource, action, body, opts) { return AbilityBuilder.define((allow, forbid) => { allow('read', 'User') forbid('read', 'User', ['email', 'secrethiddenfield']) diff --git a/test/index.test.js b/test/index.test.js deleted file mode 100644 index 437ec0b..0000000 --- a/test/index.test.js +++ /dev/null @@ -1,13 +0,0 @@ -const BaseModel = require('./utils/base-model') -const plugin = require('..') - -describe('objection-authorize', () => { - test('requires acl', () => { - expect(() => plugin()(BaseModel)).toThrow() - plugin({})(BaseModel) - }) - - test('specify library', () => { - expect(() => plugin({}, 'blah')(BaseModel)).toThrow() - }) -}) diff --git a/test/role-acl.test.js b/test/role-acl@3.test.js similarity index 90% rename from test/role-acl.test.js rename to test/role-acl@3.test.js index b4c8e6c..af3418e 100644 --- a/test/role-acl.test.js +++ b/test/role-acl@3.test.js @@ -1,5 +1,5 @@ const pluginTest = require('./utils/plugin-test') -const RoleAcl = require('role-acl') +const RoleAcl = require('role-acl-3') // these are some sample grants that you might use for your app in regards to user rights const anonymous = { @@ -44,4 +44,4 @@ const user = { ] } -pluginTest(new RoleAcl({ user, anonymous }), 'role-acl') +pluginTest(new RoleAcl({ user, anonymous }), 'role-acl@3') diff --git a/test/utils/plugin-test.js b/test/utils/plugin-test.js index a9e8344..a30a9e2 100644 --- a/test/utils/plugin-test.js +++ b/test/utils/plugin-test.js @@ -15,13 +15,14 @@ module.exports = (acl, library) => { describe(`${library} plugin`, () => { beforeAll(async () => { - return knex.schema.createTable(User.tableName, table => { + await knex.schema.createTable(User.tableName, table => { table.increments() table.text('username') table.text('email') table.text('role') table.text('secrethiddenfield') }) + await User.fetchTableMetadata() }) let testUser diff --git a/yarn.lock b/yarn.lock index a8e918a..ac2f185 100644 --- a/yarn.lock +++ b/yarn.lock @@ -150,11 +150,11 @@ to-fast-properties "^2.0.0" "@casl/ability@^3.2.0": - version "3.2.0" - resolved "https://registry.yarnpkg.com/@casl/ability/-/ability-3.2.0.tgz#b723abb17202619874dcf13bf912f8d597ba47a7" - integrity sha512-MoU+fm8Bfx37DCM2r7BtkyYTy3T6x+2d9zaVESe4y+Q7BKZsWeyKLhF8Yh+fZHBcAC5RDupmk88ckoBSHpiOtQ== + version "3.4.0" + resolved "https://registry.yarnpkg.com/@casl/ability/-/ability-3.4.0.tgz#06c9f2df0bbb257b3deecb05885e731420cca780" + integrity sha512-aJodlkXamBRNnb25m0xuibU6StiyWZzz7EFivqLKO5kmfLDAwrIklysa1w4K/Exi1mRt+lfOp7pSb4QtuAXx+A== dependencies: - sift "^7.0.1" + sift "^9.0.4" "@cnakazawa/watch@^1.0.3": version "1.0.3" @@ -3452,6 +3452,11 @@ locate-path@^5.0.0: dependencies: p-locate "^4.1.0" +lodash.clonedeep@^4.5.0: + version "4.5.0" + resolved "https://registry.yarnpkg.com/lodash.clonedeep/-/lodash.clonedeep-4.5.0.tgz#e23f3f9c4f8fbdde872529c1071857a086e5ccef" + integrity sha1-4j8/nE+Pvd6HJSnBBxhXoIblzO8= + lodash.omit@^4.5.0: version "4.5.0" resolved "https://registry.yarnpkg.com/lodash.omit/-/lodash.omit-4.5.0.tgz#6eb19ae5a1ee1dd9df0b969e66ce0b7fa30b5e60" @@ -4840,7 +4845,7 @@ rimraf@^3.0.0: dependencies: glob "^7.1.3" -role-acl@<4.0.0: +"role-acl-3@npm:role-acl@<4.0.0": version "3.3.7" resolved "https://registry.yarnpkg.com/role-acl/-/role-acl-3.3.7.tgz#24354857df6f3444934a9ce1194a3107e585537c" integrity sha512-bWY4T2/JKVMVjRrVccAvka7tqqaghzCSNk3evoo4pzzUrlYprKdn2MpiFZsBAt3QpODzUsejgMpJ8XFyAItA+Q== @@ -4849,6 +4854,16 @@ role-acl@<4.0.0: matcher "^1.0.0" notation "^1.3.5" +"role-acl-4@npm:role-acl@>=4.3.2": + version "4.3.2" + resolved "https://registry.yarnpkg.com/role-acl/-/role-acl-4.3.2.tgz#1283ff6d17934772bb490feecd58c16abeb03f13" + integrity sha512-pRAxmb2U7g4xh9UuwHieoXxcDXPWgrpEU8+83VZu2J5pqSsfpuXC7YoqYNdGWTIjo8S2vYFGHntl55VfyIYL3A== + dependencies: + jsonpath-plus "^0.18.0" + lodash.clonedeep "^4.5.0" + matcher "^1.0.0" + notation "^1.3.5" + rsvp@^4.8.4: version "4.8.5" resolved "https://registry.yarnpkg.com/rsvp/-/rsvp-4.8.5.tgz#c8f155311d167f68f21e168df71ec5b083113734" @@ -4984,10 +4999,15 @@ shellwords@^0.1.1: resolved "https://registry.yarnpkg.com/shellwords/-/shellwords-0.1.1.tgz#d6b9181c1a48d397324c84871efbcfc73fc0654b" integrity sha512-vFwSUfQvqybiICwZY5+DAWIPLKsWO31Q91JSKl3UYv+K5c2QRPzn0qzec6QPu1Qc9eHYItiP3NdJqNVqetYAww== -sift@^7.0.1: - version "7.0.1" - resolved "https://registry.yarnpkg.com/sift/-/sift-7.0.1.tgz#47d62c50b159d316f1372f8b53f9c10cd21a4b08" - integrity sha512-oqD7PMJ+uO6jV9EQCl0LrRw1OwsiPsiFQR5AR30heR+4Dl7jBBbDLnNvWiak20tzZlSE1H7RB30SX/1j/YYT7g== +sift@^9.0.4: + version "9.0.4" + resolved "https://registry.yarnpkg.com/sift/-/sift-9.0.4.tgz#9f0767230cc7d156684cd152a3f0cc7389d15a08" + integrity sha512-mXYg5X7rMxBRG7CTOKVigbKMUzidLy2T95LwsYKTqn+BfYFxBzdyrMhKWWwiEcJViVZr2TiI6JyaSlNUVsGUfQ== + +sigmund@^1.0.1: + version "1.0.1" + resolved "https://registry.yarnpkg.com/sigmund/-/sigmund-1.0.1.tgz#3ff21f198cad2175f9f3b781853fd94d0d19b590" + integrity sha1-P/IfGYytIXX587eBhT/ZTQ0ZtZA= signal-exit@^3.0.0, signal-exit@^3.0.2: version "3.0.2"