Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-01-06 22:40:14 -05:00
Code Issues Activity

🐛 Fixed idempotentcy of addPermissionToRole util (#13685)

Browse source 
refs https://github.com/TryGhost/Team/issues/1178

The "up" migration that this util generates correctly throws if the
pre-requisite data cannot be found in the database. The "down" migration
however was incorrectly mirroring this behaviour of throwing - which
meant that it wasn't idempotent, as it does not require a permission or
role to existing if it wants to move relations between them.
This commit is contained in:
Fabien 'egg' O'Carroll 2021-11-01 10:27:50 +01:00 committed by GitHub
parent 46277b6718
commit b36d0cc1c4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 8 additions and 32 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
core/server/data/migrations/utils.js
View file

@ -191,14 +191,8 @@ function addPermissionToRole(config) {
}).first();
if (!permission) {
throw new errors.GhostError({
message: tpl(messages.permissionRoleActionError, {
action: 'remove',
permission: config.permission,
role: config.role,
resource: 'permission'
})
});
logging.warn(`Removing permission(${config.permission}) from role(${config.role}) - Permission not found.`);
return;
}
const role = await connection('roles').where({
@ -206,14 +200,8 @@ function addPermissionToRole(config) {
}).first();
if (!role) {
throw new errors.GhostError({
message: tpl(messages.permissionRoleActionError, {
action: 'remove',
permission: config.permission,
role: config.role,
resource: 'role'
})
});
logging.warn(`Removing permission(${config.permission}) from role(${config.role}) - Role not found.`);
return;
}
const existingRelation = await connection('permissions_roles').where({

16
test/unit/server/data/migrations/utils.test.js
View file

@ -380,7 +380,7 @@ describe('migrations/utils/permissions', function () {
}
});
it('Throws when permission cannot be found in down migration', async function () {
it('Does not throw when permission cannot be found in down migration', async function () {
const knex = await setupDb();
const migration = utils.addPermissionToRole({
@ -393,13 +393,7 @@ describe('migrations/utils/permissions', function () {
.where('name', '=', 'Permission Name')
.del();
try {
await runDownMigration(knex, migration);
should.fail('addPermissionToRole down migration did not throw');
} catch (err) {
should.equal(err instanceof errors.GhostError, true);
err.message.should.equal('Cannot remove permission(Permission Name) with role(Role Name) - permission does not exist');
}
});
it('Throws when role cannot be found', async function () {
@ -419,7 +413,7 @@ describe('migrations/utils/permissions', function () {
}
});
it('Throws when role cannot be found in down migration', async function () {
it('Does not throw when role cannot be found in down migration', async function () {
const knex = await setupDb();
const migration = utils.addPermissionToRole({
@ -432,13 +426,7 @@ describe('migrations/utils/permissions', function () {
.where('name', '=', 'Role Name')
.del();
try {
await runDownMigration(knex, migration);
should.fail('addPermissionToRole down migration did not throw');
} catch (err) {
should.equal(err instanceof errors.GhostError, true);
err.message.should.equal('Cannot remove permission(Permission Name) with role(Role Name) - role does not exist');
}
});
});
});