From bc52c43c50bd0ea8acdd9a4b05d3dd7715cd588c Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Mon, 18 Mar 2024 16:46:12 -0400 Subject: [PATCH] Added missing permissions to Contributor & Editor ref ENG-728 ref https://linear.app/tryghost/issue/ENG-728 This is NOT a functionality change. The Post#permissible method unit tests have been updated to pass `true` as `hasUserPermission` and we can see that the permission functionality remains the same. The permissible method of the post model is responsible for removing permission based on the data that is being modified, but the permissions module is setup to allow the permissible method to grant permission - this means that we call permissible, even if the current actor doesn't have permission, this results in code that is hard to understand and manage. We are going to be instead returning early if an actor does not have permission, this will allow permissible method signatures to be greatly simplified (removing the need for hasUserPermission, hasApiKeyPermission & hasMemberPermission arguments). --- ...3-18-16-20-add-missing-post-permissions.js | 20 +++++++ .../server/data/schema/fixtures/fixtures.json | 4 +- .../integration/migrations/migration.test.js | 4 +- .../unit/server/data/schema/integrity.test.js | 2 +- .../core/test/unit/server/models/post.test.js | 54 +++++++++---------- ghost/core/test/utils/fixtures/fixtures.json | 4 +- 6 files changed, 52 insertions(+), 36 deletions(-) create mode 100644 ghost/core/core/server/data/migrations/versions/5.81/2024-03-18-16-20-add-missing-post-permissions.js diff --git a/ghost/core/core/server/data/migrations/versions/5.81/2024-03-18-16-20-add-missing-post-permissions.js b/ghost/core/core/server/data/migrations/versions/5.81/2024-03-18-16-20-add-missing-post-permissions.js new file mode 100644 index 000000000000..dd8bf0a863bc --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/5.81/2024-03-18-16-20-add-missing-post-permissions.js @@ -0,0 +1,20 @@ +const {combineTransactionalMigrations, addPermissionToRole} = require('../../utils'); + +module.exports = combineTransactionalMigrations( + addPermissionToRole({ + permission: 'Edit posts', + role: 'Author' + }), + addPermissionToRole({ + permission: 'Edit posts', + role: 'Contributor' + }), + addPermissionToRole({ + permission: 'Delete posts', + role: 'Author' + }), + addPermissionToRole({ + permission: 'Delete posts', + role: 'Contributor' + }) +); diff --git a/ghost/core/core/server/data/schema/fixtures/fixtures.json b/ghost/core/core/server/data/schema/fixtures/fixtures.json index c9478e628b4c..e6dd4b8d017a 100644 --- a/ghost/core/core/server/data/schema/fixtures/fixtures.json +++ b/ghost/core/core/server/data/schema/fixtures/fixtures.json @@ -929,7 +929,7 @@ "recommendation": ["browse", "read"] }, "Author": { - "post": ["browse", "read", "add"], + "post": ["browse", "read", "edit", "add", "destroy"], "setting": ["browse", "read"], "slug": "all", "tag": ["browse", "read", "add"], @@ -946,7 +946,7 @@ "recommendation": ["browse", "read"] }, "Contributor": { - "post": ["browse", "read", "add"], + "post": ["browse", "read", "edit", "add", "destroy"], "setting": ["browse", "read"], "slug": "all", "tag": ["browse", "read"], diff --git a/ghost/core/test/integration/migrations/migration.test.js b/ghost/core/test/integration/migrations/migration.test.js index 1a5fc87a853e..0da8cf8d35b4 100644 --- a/ghost/core/test/integration/migrations/migration.test.js +++ b/ghost/core/test/integration/migrations/migration.test.js @@ -114,9 +114,9 @@ describe('Migrations', function () { permissions.should.havePermission('Browse posts', ['Administrator', 'Editor', 'Author', 'Contributor', 'Admin Integration']); permissions.should.havePermission('Read posts', ['Administrator', 'Editor', 'Author', 'Contributor', 'Admin Integration']); - permissions.should.havePermission('Edit posts', ['Administrator', 'Editor', 'Admin Integration']); + permissions.should.havePermission('Edit posts', ['Administrator', 'Editor', 'Author', 'Contributor', 'Admin Integration']); permissions.should.havePermission('Add posts', ['Administrator', 'Editor', 'Author', 'Contributor', 'Admin Integration']); - permissions.should.havePermission('Delete posts', ['Administrator', 'Editor', 'Admin Integration']); + permissions.should.havePermission('Delete posts', ['Administrator', 'Editor', 'Author', 'Contributor', 'Admin Integration']); permissions.should.havePermission('Publish posts', ['Administrator', 'Editor', 'Admin Integration', 'Scheduler Integration']); permissions.should.havePermission('Browse settings', ['Administrator', 'Editor', 'Author', 'Contributor', 'Admin Integration']); diff --git a/ghost/core/test/unit/server/data/schema/integrity.test.js b/ghost/core/test/unit/server/data/schema/integrity.test.js index fea8db9b1e5e..25404d89b315 100644 --- a/ghost/core/test/unit/server/data/schema/integrity.test.js +++ b/ghost/core/test/unit/server/data/schema/integrity.test.js @@ -36,7 +36,7 @@ const validateRouteSettings = require('../../../../../core/server/services/route describe('DB version integrity', function () { // Only these variables should need updating const currentSchemaHash = '34a9fa4dc1223ef6c45f8ed991d25de5'; - const currentFixturesHash = '4db87173699ad9c9d8a67ccab96dfd2d'; + const currentFixturesHash = '347f16d80a8379814b718b388e9016ca'; const currentSettingsHash = '5c957ceb48c4878767d7d3db484c592d'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01'; diff --git a/ghost/core/test/unit/server/models/post.test.js b/ghost/core/test/unit/server/models/post.test.js index 9be8599febfd..bb96bece61b3 100644 --- a/ghost/core/test/unit/server/models/post.test.js +++ b/ghost/core/test/unit/server/models/post.test.js @@ -392,17 +392,15 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, - false, - true + true, + true, + false ).then(() => { done(new Error('Permissible function should have rejected.')); }).catch((error) => { error.should.be.an.instanceof(errors.NoPermissionError); - should(mockPostObj.get.called).be.false(); - should(mockPostObj.related.calledOnce).be.true(); done(); - }); + }).catch(done); }); it('rejects if changing visibility', function (done) { @@ -422,17 +420,15 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, - false, - true + true, + true, + false ).then(() => { done(new Error('Permissible function should have rejected.')); }).catch((error) => { error.should.be.an.instanceof(errors.NoPermissionError); - should(mockPostObj.get.called).be.false(); - should(mockPostObj.related.calledOnce).be.true(); done(); - }); + }).catch(done); }); it('rejects if changing authors.0', function (done) { @@ -451,9 +447,9 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, true, - true + true, + false ).then(() => { done(new Error('Permissible function should have rejected.')); }).catch((error) => { @@ -481,9 +477,9 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, true, - true + true, + false ).then((result) => { should.exist(result); should(result.excludedAttrs).deepEqual(['authors', 'tags']); @@ -510,9 +506,9 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, true, - true + true, + false ).then(() => { done(new Error('Permissible function should have rejected.')); }).catch((error) => { @@ -539,9 +535,9 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, true, - true + true, + false ).then(() => { done(new Error('Permissible function should have rejected.')); }).catch((error) => { @@ -568,9 +564,9 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, true, - true + true, + false ).then((result) => { should.exist(result); should(result.excludedAttrs).deepEqual(['authors', 'tags']); @@ -594,7 +590,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, + true, true, true ).then(() => { @@ -619,7 +615,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, + true, true, true ).then(() => { @@ -644,7 +640,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, + true, true, true ).then(() => { @@ -669,7 +665,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, unsafeAttrs, testUtils.permissions.contributor, - false, + true, true, true ).then((result) => { @@ -697,7 +693,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, {}, testUtils.permissions.contributor, - false, + true, true, true ).then(() => { @@ -726,7 +722,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, {}, testUtils.permissions.contributor, - false, + true, true, true ).then(() => { @@ -755,7 +751,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () { context, {}, testUtils.permissions.contributor, - false, + true, true, true ).then((result) => { diff --git a/ghost/core/test/utils/fixtures/fixtures.json b/ghost/core/test/utils/fixtures/fixtures.json index 6218bdf21857..c6f10459ba05 100644 --- a/ghost/core/test/utils/fixtures/fixtures.json +++ b/ghost/core/test/utils/fixtures/fixtures.json @@ -1105,7 +1105,7 @@ "recommendation": ["browse", "read"] }, "Author": { - "post": ["browse", "read", "add"], + "post": ["browse", "read", "edit", "add", "destroy"], "setting": ["browse", "read"], "slug": "all", "tag": ["browse", "read", "add"], @@ -1122,7 +1122,7 @@ "recommendation": ["browse", "read"] }, "Contributor": { - "post": ["browse", "read", "add"], + "post": ["browse", "read", "edit", "add", "destroy"], "setting": ["browse", "read"], "slug": "all", "tag": ["browse", "read"],