From fc73cd71bbec81650e99950ed680f11a0748a299 Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Fri, 18 Jan 2019 12:17:12 +0100 Subject: [PATCH] Updated permissions service to handle api keys (#9967) refs #9865 - Enabled the permissions module to lookup permissions based on an api_key id. - Updated the "can this" part of the permissions service to load permissions for any api key in the context, and correctly use that to determine whether an action is permissible. It also updates the permissible interface that models can implement to pass in the hasApiKeyPermissions param. --- core/server/services/permissions/can-this.js | 30 +++++++++++--- .../services/permissions/parse-context.js | 8 +++- core/server/services/permissions/providers.js | 18 ++++++++ core/server/translations/en.json | 3 ++ core/test/unit/api/v0.1/utils_spec.js | 6 +-- .../services/permissions/can-this_spec.js | 32 ++++++++++++++- .../permissions/parse-context_spec.js | 21 ++++++++++ .../services/permissions/providers_spec.js | 41 +++++++++++++++++++ 8 files changed, 148 insertions(+), 11 deletions(-) diff --git a/core/server/services/permissions/can-this.js b/core/server/services/permissions/can-this.js index 5bd9d0d70a..e0a94f890b 100644 --- a/core/server/services/permissions/can-this.js +++ b/core/server/services/permissions/can-this.js @@ -50,8 +50,10 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c return permissionLoad.then(function (loadedPermissions) { // Iterate through the user permissions looking for an affirmation var userPermissions = loadedPermissions.user ? loadedPermissions.user.permissions : null, + apiKeyPermissions = loadedPermissions.apiKey ? loadedPermissions.apiKey.permissions : null, appPermissions = loadedPermissions.app ? loadedPermissions.app.permissions : null, hasUserPermission, + hasApiKeyPermission, hasAppPermission, checkPermission = function (perm) { var permObjId; @@ -82,6 +84,14 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c hasUserPermission = _.some(userPermissions, checkPermission); } + // Check api key permissions if they were passed + hasApiKeyPermission = true; + if (!_.isNull(apiKeyPermissions)) { + // api key request have no user, but we want the user permissions checks to pass + hasUserPermission = true; + hasApiKeyPermission = _.some(apiKeyPermissions, checkPermission); + } + // Check app permissions if they were passed hasAppPermission = true; if (!_.isNull(appPermissions)) { @@ -91,11 +101,11 @@ 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, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission + modelId, actType, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission, hasApiKeyPermission ); } - if (hasUserPermission && hasAppPermission) { + if (hasUserPermission && hasApiKeyPermission && hasAppPermission) { return; } @@ -110,10 +120,11 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c CanThisResult.prototype.beginCheck = function (context) { var self = this, userPermissionLoad, + apiKeyPermissionLoad, appPermissionLoad, permissionsLoad; - // Get context.user and context.app + // Get context.user, context.api_key_id and context.app context = parseContext(context); if (actionsMap.empty()) { @@ -128,6 +139,14 @@ CanThisResult.prototype.beginCheck = function (context) { userPermissionLoad = Promise.resolve(null); } + // Kick off loading of api key permissions if necessary + if (context.api_key_id) { + apiKeyPermissionLoad = providers.apiKey(context.api_key_id); + } else { + // Resolve null if no context.api_key_id + apiKeyPermissionLoad = Promise.resolve(null); + } + // Kick off loading of app permissions if necessary if (context.app) { appPermissionLoad = providers.app(context.app); @@ -137,10 +156,11 @@ CanThisResult.prototype.beginCheck = function (context) { } // Wait for both user and app permissions to load - permissionsLoad = Promise.all([userPermissionLoad, appPermissionLoad]).then(function (result) { + permissionsLoad = Promise.all([userPermissionLoad, apiKeyPermissionLoad, appPermissionLoad]).then(function (result) { return { user: result[0], - app: result[1] + apiKey: result[1], + app: result[2] }; }); diff --git a/core/server/services/permissions/parse-context.js b/core/server/services/permissions/parse-context.js index acc1a9cd4a..27ac1c5829 100644 --- a/core/server/services/permissions/parse-context.js +++ b/core/server/services/permissions/parse-context.js @@ -3,7 +3,7 @@ * * Utility function, to expand strings out into objects. * @param {Object|String} context - * @return {{internal: boolean, external: boolean, user: integer|null, app: integer|null, public: boolean}} + * @return {{internal: boolean, external: boolean, user: integer|null, app: integer|null, public: boolean, api_key_id: ObjectId|null}} */ module.exports = function parseContext(context) { // Parse what's passed to canThis.beginCheck for standard user and app scopes @@ -11,6 +11,7 @@ module.exports = function parseContext(context) { internal: false, external: false, user: null, + api_key_id: null, app: null, public: true }; @@ -31,6 +32,11 @@ module.exports = function parseContext(context) { parsed.public = false; } + if (context && context.api_key_id) { + parsed.api_key_id = context.api_key_id; + parsed.public = false; + } + if (context && context.app) { parsed.app = context.app; parsed.public = false; diff --git a/core/server/services/permissions/providers.js b/core/server/services/permissions/providers.js index 3b05b7a2d6..bede6238ee 100644 --- a/core/server/services/permissions/providers.js +++ b/core/server/services/permissions/providers.js @@ -53,5 +53,23 @@ module.exports = { return {permissions: foundApp.related('permissions').models}; }); + }, + + apiKey(id) { + return models.ApiKey.findOne({id}, {withRelated: ['role', 'role.permissions']}) + .then((foundApiKey) => { + if (!foundApiKey) { + throw new common.errors.NotFoundError({ + message: common.i18n.t('errors.models.api_key.apiKeyNotFound') + }); + } + + // api keys have a belongs_to relationship to a role and no individual permissions + // so there's no need for permission deduplication + const permissions = foundApiKey.related('role').related('permissions').models; + const roles = [foundApiKey.toJSON().role]; + + return {permissions, roles}; + }); } }; diff --git a/core/server/translations/en.json b/core/server/translations/en.json index cfe0005ac5..2b3d0a22ed 100644 --- a/core/server/translations/en.json +++ b/core/server/translations/en.json @@ -270,6 +270,9 @@ "onlyOwnerCanTransferOwnerRole": "Only owners are able to transfer the owner role.", "onlyAdmCanBeAssignedOwnerRole": "Only administrators can be assigned the owner role." }, + "api_key": { + "apiKeyNotFound": "API Key not found" + }, "base": { "index": { "missingContext": "missing context" diff --git a/core/test/unit/api/v0.1/utils_spec.js b/core/test/unit/api/v0.1/utils_spec.js index c755322378..cdbe295849 100644 --- a/core/test/unit/api/v0.1/utils_spec.js +++ b/core/test/unit/api/v0.1/utils_spec.js @@ -608,7 +608,7 @@ describe('API Utils', function () { describe('handlePublicPermissions', function () { it('should return empty options if passed empty options', function (done) { apiUtils.handlePublicPermissions('tests', 'test')({}).then(function (options) { - options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null}}); + options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null, api_key_id: null}}); done(); }).catch(done); }); @@ -617,7 +617,7 @@ describe('API Utils', function () { var aPPStub = sandbox.stub(apiUtils, 'applyPublicPermissions').returns(Promise.resolve({})); apiUtils.handlePublicPermissions('tests', 'test')({}).then(function (options) { aPPStub.calledOnce.should.eql(true); - options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null}}); + options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null, api_key_id: null}}); done(); }).catch(done); }); @@ -633,7 +633,7 @@ describe('API Utils', function () { apiUtils.handlePublicPermissions('tests', 'test')({context: {user: 1}}).then(function (options) { cTStub.calledOnce.should.eql(true); cTMethodStub.test.test.calledOnce.should.eql(true); - options.should.eql({context: {app: null, external: false, internal: false, public: false, user: 1}}); + options.should.eql({context: {app: null, external: false, internal: false, public: false, user: 1, api_key_id: null}}); done(); }).catch(done); }); diff --git a/core/test/unit/services/permissions/can-this_spec.js b/core/test/unit/services/permissions/can-this_spec.js index 82d7292a1c..bf508fbea8 100644 --- a/core/test/unit/services/permissions/can-this_spec.js +++ b/core/test/unit/services/permissions/can-this_spec.js @@ -423,6 +423,32 @@ describe('Permissions', function () { }); }); + describe('API Key-based permissions', function () { + // TODO change to using fake models in tests! + // Permissions need to be NOT fundamentally baked into Ghost, but a separate module, at some point + // It can depend on bookshelf, but should NOT use hard coded model knowledge. + // We use the tag model here because it doesn't have permissible, once that changes, these tests must also change + it('With permissions: can edit non-specific tag (no permissible function on model)', function (done) { + const apiKeyProviderStub = sandbox.stub(providers, 'apiKey').callsFake(() => { + // Fake the response from providers.user, which contains permissions and roles + return Promise.resolve({ + permissions: models.Permissions.forge(testUtils.DataGenerator.Content.permissions).models, + // This should be JSON, so no need to run it through the model layer. 5 === admin api key + roles: [testUtils.DataGenerator.Content.roles[5]] + }); + }); + permissions.canThis({api_key_id: 123}) // api key context + .edit + .tag({id: 1}) // tag id in model syntax + .then(function (res) { + apiKeyProviderStub.callCount.should.eql(1); + should.not.exist(res); + done(); + }) + .catch(done); + }); + }); + describe('App-based permissions (requires user as well)', function () { // @TODO: revisit this - do we really need to have USER permissions AND app permissions? it('No permissions: cannot edit tag with app only (no permissible function on model)', function (done) { @@ -553,7 +579,7 @@ describe('Permissions', function () { }) .catch(function (err) { permissibleStub.callCount.should.eql(1); - permissibleStub.firstCall.args.should.have.lengthOf(7); + permissibleStub.firstCall.args.should.have.lengthOf(8); permissibleStub.firstCall.args[0].should.eql(1); permissibleStub.firstCall.args[1].should.eql('edit'); @@ -562,6 +588,7 @@ describe('Permissions', function () { permissibleStub.firstCall.args[4].should.be.an.Object(); permissibleStub.firstCall.args[5].should.be.true(); permissibleStub.firstCall.args[6].should.be.true(); + permissibleStub.firstCall.args[7].should.be.true(); userProviderStub.callCount.should.eql(1); err.message.should.eql('Hello World!'); @@ -587,7 +614,7 @@ 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.should.have.lengthOf(8); permissibleStub.firstCall.args[0].should.eql(1); permissibleStub.firstCall.args[1].should.eql('edit'); permissibleStub.firstCall.args[2].should.be.an.Object(); @@ -595,6 +622,7 @@ describe('Permissions', function () { permissibleStub.firstCall.args[4].should.be.an.Object(); permissibleStub.firstCall.args[5].should.be.true(); permissibleStub.firstCall.args[6].should.be.true(); + permissibleStub.firstCall.args[7].should.be.true(); userProviderStub.callCount.should.eql(1); should.not.exist(res); diff --git a/core/test/unit/services/permissions/parse-context_spec.js b/core/test/unit/services/permissions/parse-context_spec.js index 93e43d325a..dc095fc5a0 100644 --- a/core/test/unit/services/permissions/parse-context_spec.js +++ b/core/test/unit/services/permissions/parse-context_spec.js @@ -8,6 +8,7 @@ describe('Permissions', function () { internal: false, external: false, user: null, + api_key_id: null, app: null, public: true }); @@ -15,6 +16,7 @@ describe('Permissions', function () { internal: false, external: false, user: null, + api_key_id: null, app: null, public: true }); @@ -25,6 +27,7 @@ describe('Permissions', function () { internal: false, external: false, user: null, + api_key_id: null, app: null, public: true }); @@ -32,6 +35,7 @@ describe('Permissions', function () { internal: false, external: false, user: null, + api_key_id: null, app: null, public: true }); @@ -42,6 +46,18 @@ describe('Permissions', function () { internal: false, external: false, user: 1, + api_key_id: null, + app: null, + public: false + }); + }); + + it('should return api_key_id if api_key_id provided', function () { + parseContext({api_key_id: 1}).should.eql({ + internal: false, + external: false, + user: null, + api_key_id: 1, app: null, public: false }); @@ -52,6 +68,7 @@ describe('Permissions', function () { internal: false, external: false, user: null, + api_key_id: null, app: 5, public: false }); @@ -62,6 +79,7 @@ describe('Permissions', function () { internal: true, external: false, user: null, + api_key_id: null, app: null, public: false }); @@ -70,6 +88,7 @@ describe('Permissions', function () { internal: true, external: false, user: null, + api_key_id: null, app: null, public: false }); @@ -80,6 +99,7 @@ describe('Permissions', function () { internal: false, external: true, user: null, + api_key_id: null, app: null, public: false }); @@ -88,6 +108,7 @@ describe('Permissions', function () { internal: false, external: true, user: null, + api_key_id: null, app: null, public: false }); diff --git a/core/test/unit/services/permissions/providers_spec.js b/core/test/unit/services/permissions/providers_spec.js index f2d64b557c..3e59aca58e 100644 --- a/core/test/unit/services/permissions/providers_spec.js +++ b/core/test/unit/services/permissions/providers_spec.js @@ -174,6 +174,47 @@ describe('Permission Providers', function () { }); }); + describe('API Key', function () { + it('errors if api_key cannot be found', function (done) { + let findApiKeySpy = sandbox.stub(models.ApiKey, 'findOne'); + findApiKeySpy.returns(new Promise.resolve()); + providers.apiKey(1) + .then(() => { + done(new Error('Should have thrown an api key not found error')); + }) + .catch((err) => { + findApiKeySpy.callCount.should.eql(1); + err.errorType.should.eql('NotFoundError'); + done(); + }); + }); + it('can load api_key with role, and role.permissions', function (done) { + const findApiKeySpy = sandbox.stub(models.ApiKey, 'findOne').callsFake(function () { + const fakeApiKey = models.ApiKey.forge(testUtils.DataGenerator.Content.api_keys[0]); + const fakeAdminRole = models.Role.forge(testUtils.DataGenerator.Content.roles[0]); + const fakeAdminRolePermissions = models.Permissions.forge(testUtils.DataGenerator.Content.permissions); + fakeAdminRole.relations = { + permissions: fakeAdminRolePermissions + }; + fakeApiKey.relations = { + role: fakeAdminRole + }; + fakeApiKey.withRelated = ['role', 'role.permissions']; + return Promise.resolve(fakeApiKey); + }); + providers.apiKey(1).then((res) => { + findApiKeySpy.callCount.should.eql(1); + res.should.be.an.Object().with.properties('permissions', 'roles'); + res.roles.should.be.an.Array().with.lengthOf(1); + res.permissions[0].should.be.an.Object().with.properties('attributes', 'id'); + res.roles[0].should.be.an.Object().with.properties('id', 'name', 'description'); + res.permissions[0].should.be.instanceOf(models.Base.Model); + res.roles[0].should.not.be.instanceOf(models.Base.Model); + done(); + }).catch(done); + }); + }); + describe('App', function () { // @TODO make this consistent or sane or something! // Why is this an empty array, when the success is an object?