From b468d6dbe203a2339b9417686d732d9646a085d7 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Tue, 26 Sep 2017 17:06:14 +0100 Subject: [PATCH] Support for attribute-based permissions (#9025) refs #8602 - Add the wiring to pass attributes around the permission system - Allows us to get access to the important "unsafe" attributes that are changing - E.g. status for posts - This can then be used to determine whether a user has permission to perform an attribute-based action - E.g. publish a post (change status) --- core/server/api/utils.js | 6 +- core/server/models/post.js | 2 +- core/server/models/role.js | 2 +- core/server/models/subscriber.js | 2 +- core/server/models/user.js | 2 +- core/server/permissions/index.js | 5 +- core/test/unit/api/utils_spec.js | 83 ++++++++++++++++++++++++ core/test/unit/permissions/index_spec.js | 21 +++++- 8 files changed, 114 insertions(+), 9 deletions(-) diff --git a/core/server/api/utils.js b/core/server/api/utils.js index 08a56652bc..a31aaf5ac5 100644 --- a/core/server/api/utils.js +++ b/core/server/api/utils.js @@ -194,9 +194,10 @@ utils = { * ## Handle Permissions * @param {String} docName * @param {String} method (browse || read || edit || add || destroy) + * * @param {Array} unsafeAttrNames - attribute names (e.g. post.status) that could change the outcome * @returns {Function} */ - handlePermissions: function handlePermissions(docName, method) { + handlePermissions: function handlePermissions(docName, method, unsafeAttrNames) { var singular = docName.replace(/s$/, ''); /** @@ -206,7 +207,8 @@ utils = { * @returns {Object} options */ return function doHandlePermissions(options) { - var permsPromise = permissions.canThis(options.context)[method][singular](options.id); + var unsafeAttrObject = unsafeAttrNames && _.has(options, 'data.[' + docName + '][0]') ? _.pick(options.data[docName][0], unsafeAttrNames) : {}, + permsPromise = permissions.canThis(options.context)[method][singular](options.id, unsafeAttrObject); return permsPromise.then(function permissionGranted() { return options; diff --git a/core/server/models/post.js b/core/server/models/post.js index f4c58c5de2..5785bc629a 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -810,7 +810,7 @@ Post = ghostBookshelf.Model.extend({ }); }), - permissible: function permissible(postModelOrId, action, context, loadedPermissions, hasUserPermission, hasAppPermission) { + permissible: function permissible(postModelOrId, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission) { var self = this, postModel = postModelOrId, origArgs; diff --git a/core/server/models/role.js b/core/server/models/role.js index 4e57a787bb..34a21a1522 100644 --- a/core/server/models/role.js +++ b/core/server/models/role.js @@ -41,7 +41,7 @@ Role = ghostBookshelf.Model.extend({ return options; }, - permissible: function permissible(roleModelOrId, action, context, loadedPermissions, hasUserPermission, hasAppPermission) { + permissible: function permissible(roleModelOrId, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission) { var self = this, checkAgainst = [], origArgs; diff --git a/core/server/models/subscriber.js b/core/server/models/subscriber.js index 6feb716041..570d5b57da 100644 --- a/core/server/models/subscriber.js +++ b/core/server/models/subscriber.js @@ -59,7 +59,7 @@ Subscriber = ghostBookshelf.Model.extend({ return options; }, - permissible: function permissible(postModelOrId, action, context, loadedPermissions, hasUserPermission, hasAppPermission) { + permissible: function permissible(postModelOrId, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission) { // CASE: external is only allowed to add and edit subscribers if (context.external) { if (['add', 'edit'].indexOf(action) !== -1) { diff --git a/core/server/models/user.js b/core/server/models/user.js index 521c7357d2..f80e82271a 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -583,7 +583,7 @@ User = ghostBookshelf.Model.extend({ }); }, - permissible: function permissible(userModelOrId, action, context, loadedPermissions, hasUserPermission, hasAppPermission) { + permissible: function permissible(userModelOrId, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission) { var self = this, userModel = userModelOrId, origArgs; diff --git a/core/server/permissions/index.js b/core/server/permissions/index.js index 77a59b79cf..22da2acf93 100644 --- a/core/server/permissions/index.js +++ b/core/server/permissions/index.js @@ -45,8 +45,9 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c // Create the 'handler' for the object type; // the '.post()' in canThis(user).edit.post() - objTypeHandlers[objType] = function (modelOrId) { + objTypeHandlers[objType] = function (modelOrId, unsafeAttrs) { var modelId; + unsafeAttrs = unsafeAttrs || {}; // If it's an internal request, resolve immediately if (context.internal) { @@ -105,7 +106,7 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c // Offer a chance for the TargetModel to override the results if (TargetModel && _.isFunction(TargetModel.permissible)) { return TargetModel.permissible( - modelId, actType, context, loadedPermissions, hasUserPermission, hasAppPermission + modelId, actType, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission ); } diff --git a/core/test/unit/api/utils_spec.js b/core/test/unit/api/utils_spec.js index f9ef05ab6a..09525bf18a 100644 --- a/core/test/unit/api/utils_spec.js +++ b/core/test/unit/api/utils_spec.js @@ -607,7 +607,90 @@ describe('API Utils', function () { .then(function (res) { permsStub.callCount.should.eql(1); testStub.callCount.should.eql(1); + testStub.firstCall.args.length.should.eql(2); testStub.firstCall.args[0].should.eql(5); + testStub.firstCall.args[1].should.eql({}); + + res.should.eql(testObj); + + done(); + }) + .catch(done); + }); + + it('should ignore unsafe attrs if none are provided', function (done) { + var testStub = sandbox.stub().returns(new Promise.resolve()), + permsStub = sandbox.stub(permissions, 'canThis', function () { + return { + testing: { + test: testStub + } + }; + }), + permsFunc = apiUtils.handlePermissions('tests', 'testing', ['foo']), + testObj = {data: {tests: [{}]}, id: 5}; + + permsFunc(testObj) + .then(function (res) { + permsStub.callCount.should.eql(1); + testStub.callCount.should.eql(1); + testStub.firstCall.args.length.should.eql(2); + testStub.firstCall.args[0].should.eql(5); + testStub.firstCall.args[1].should.eql({}); + + res.should.eql(testObj); + + done(); + }) + .catch(done); + }); + + it('should ignore unsafe attrs if they are provided but not present', function (done) { + var testStub = sandbox.stub().returns(new Promise.resolve()), + permsStub = sandbox.stub(permissions, 'canThis', function () { + return { + testing: { + test: testStub + } + }; + }), + permsFunc = apiUtils.handlePermissions('tests', 'testing', ['foo']), + testObj = {foo: 'bar', id: 5}; + + permsFunc(testObj) + .then(function (res) { + permsStub.callCount.should.eql(1); + testStub.callCount.should.eql(1); + testStub.firstCall.args.length.should.eql(2); + testStub.firstCall.args[0].should.eql(5); + testStub.firstCall.args[1].should.eql({}); + + res.should.eql(testObj); + + done(); + }) + .catch(done); + }); + + it('should pass through unsafe attrs if they DO exist', function (done) { + var testStub = sandbox.stub().returns(new Promise.resolve()), + permsStub = sandbox.stub(permissions, 'canThis', function () { + return { + testing: { + test: testStub + } + }; + }), + permsFunc = apiUtils.handlePermissions('tests', 'testing', ['foo']), + testObj = {data: {tests: [{foo: 'bar'}]}, id: 5}; + + permsFunc(testObj) + .then(function (res) { + permsStub.callCount.should.eql(1); + testStub.callCount.should.eql(1); + testStub.firstCall.args.length.should.eql(2); + testStub.firstCall.args[0].should.eql(5); + testStub.firstCall.args[1].should.eql({foo: 'bar'}); res.should.eql(testObj); diff --git a/core/test/unit/permissions/index_spec.js b/core/test/unit/permissions/index_spec.js index 4235be6f80..9d5e75f3bb 100644 --- a/core/test/unit/permissions/index_spec.js +++ b/core/test/unit/permissions/index_spec.js @@ -1,4 +1,4 @@ -var should = require('should'), +var should = require('should'), // jshint ignore:line sinon = require('sinon'), testUtils = require('../../utils'), Promise = require('bluebird'), @@ -594,6 +594,16 @@ describe('Permissions', function () { }) .catch(function (err) { permissibleStub.callCount.should.eql(1); + permissibleStub.firstCall.args.should.have.lengthOf(7); + + permissibleStub.firstCall.args[0].should.eql(1); + permissibleStub.firstCall.args[1].should.eql('edit'); + permissibleStub.firstCall.args[2].should.be.an.Object(); + permissibleStub.firstCall.args[3].should.be.an.Object(); + permissibleStub.firstCall.args[4].should.be.an.Object(); + permissibleStub.firstCall.args[5].should.be.true(); + permissibleStub.firstCall.args[6].should.be.true(); + effectiveUserStub.callCount.should.eql(1); err.message.should.eql('Hello World!'); done(); @@ -618,6 +628,15 @@ describe('Permissions', function () { .post({id: 1}) // tag id in model syntax .then(function (res) { permissibleStub.callCount.should.eql(1); + permissibleStub.firstCall.args.should.have.lengthOf(7); + permissibleStub.firstCall.args[0].should.eql(1); + permissibleStub.firstCall.args[1].should.eql('edit'); + permissibleStub.firstCall.args[2].should.be.an.Object(); + permissibleStub.firstCall.args[3].should.be.an.Object(); + permissibleStub.firstCall.args[4].should.be.an.Object(); + permissibleStub.firstCall.args[5].should.be.true(); + permissibleStub.firstCall.args[6].should.be.true(); + effectiveUserStub.callCount.should.eql(1); should.not.exist(res); done();