Skip to content

Commit

Permalink
Added missing permissions to Contributor & Editor
Browse files Browse the repository at this point in the history 
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).
  • Loading branch information
@allouis
allouis committed Mar 19, 2024
1 parent cdf4517 commit bc52c43
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 36 deletions.
20 changes: 20 additions & 0 deletions ...ore/server/data/migrations/versions/5.81/2024-03-18-16-20-add-missing-post-permissions.js
View file
@@ -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'
})
);
4 changes: 2 additions & 2 deletions ghost/core/core/server/data/schema/fixtures/fixtures.json
View file
Expand Up @@ -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"],
Expand All @@ -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"],
Expand Down
4 changes: 2 additions & 2 deletions ghost/core/test/integration/migrations/migration.test.js
View file
Expand Up @@ -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']);
Expand Down
2 changes: 1 addition & 1 deletion ghost/core/test/unit/server/data/schema/integrity.test.js
View file
Expand Up @@ -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';

Expand Down
54 changes: 25 additions & 29 deletions ghost/core/test/unit/server/models/post.test.js
View file
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) => {
Expand Down Expand Up @@ -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']);
Expand All @@ -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) => {
Expand All @@ -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) => {
Expand All @@ -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']);
Expand All @@ -594,7 +590,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () {
context,
unsafeAttrs,
testUtils.permissions.contributor,
false,
true,
true,
true
).then(() => {
Expand All @@ -619,7 +615,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () {
context,
unsafeAttrs,
testUtils.permissions.contributor,
false,
true,
true,
true
).then(() => {
Expand All @@ -644,7 +640,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () {
context,
unsafeAttrs,
testUtils.permissions.contributor,
false,
true,
true,
true
).then(() => {
Expand All @@ -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) => {
Expand Down Expand Up @@ -697,7 +693,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () {
context,
{},
testUtils.permissions.contributor,
false,
true,
true,
true
).then(() => {
Expand Down Expand Up @@ -726,7 +722,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () {
context,
{},
testUtils.permissions.contributor,
false,
true,
true,
true
).then(() => {
Expand Down Expand Up @@ -755,7 +751,7 @@ describe('Unit: models/post: uses database (@TODO: fix me)', function () {
context,
{},
testUtils.permissions.contributor,
false,
true,
true,
true
).then((result) => {
Expand Down
4 changes: 2 additions & 2 deletions ghost/core/test/utils/fixtures/fixtures.json
View file
Expand Up @@ -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"],
Expand All @@ -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"],
Expand Down

0 comments on commit bc52c43

Please sign in to comment.