Skip to content

Commit

Permalink
Added missing permissions to Contributor & Editor (#19881)
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 @royalfig
allouis authored and royalfig committed Mar 25, 2024
1 parent 0c437e4 commit c524e4c
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 = 'a489d615989eab1023d4b8af0ecee7fd';
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 c524e4c

Please sign in to comment.