Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-02-24 23:48:13 -05:00
Code Issues Activity

Added missing permissions to Contributor & Editor (#19881)

Browse source 
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).
This commit is contained in:
Fabien 'egg' O'Carroll 2024-03-20 20:36:07 +07:00 • committed by GitHub
parent 38f8e05a3e
commit 7cc65c18cc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 52 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
ghost/core/core/server/data/migrations/versions/5.81/2024-03-18-16-20-add-missing-post-permissions.js Normal file
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
ghost/core/core/server/data/schema/fixtures/fixtures.json
View file

@ -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"],

4
ghost/core/test/integration/migrations/migration.test.js
View file

@ -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']);

2
ghost/core/test/unit/server/data/schema/integrity.test.js
View file

@ -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';

54
ghost/core/test/unit/server/models/post.test.js
View file

@ -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) => {

4
ghost/core/test/utils/fixtures/fixtures.json
View file

@ -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"],