Proper error handling for permissible implementations

no issue

- currently if you would like to edit a resource (e.g. post) and you pass an invalid model id, the following happens
  - permission check calls `Post.permissible`
  - the Post could not find the post, but ignored it and returned `userPermissions:true`
  - then the model layer is queried again and figured out that the post does not exist
- A: there is no need to query the model twice
- B: we needed proper error handling for post and role model
This commit is contained in:
kirrg001 2018-02-21 16:59:48 +01:00
parent 49c815b2f1
commit e01b61dcf4
6 changed files with 34 additions and 12 deletions

View File

@ -238,7 +238,9 @@ posts = {
return Post.findOne(data, fetchOpts).then(function () {
return Post.destroy(options).return(null);
}).catch(Post.NotFoundError, function () {
throw new common.errors.NotFoundError({message: common.i18n.t('errors.api.posts.postNotFound')});
throw new common.errors.NotFoundError({
message: common.i18n.t('errors.api.posts.postNotFound')
});
});
}

View File

@ -234,6 +234,10 @@ utils = {
return Promise.reject(err);
}
if (common.errors.utils.isIgnitionError(err)) {
return Promise.reject(err);
}
return Promise.reject(new common.errors.GhostError({
err: err
}));

View File

@ -668,12 +668,19 @@ Post = ghostBookshelf.Model.extend({
origArgs = _.toArray(arguments).slice(1);
// Get the actual post model
return this.findOne({id: postModelOrId, status: 'all'}).then(function then(foundPostModel) {
// Build up the original args but substitute with actual model
var newArgs = [foundPostModel].concat(origArgs);
return this.findOne({id: postModelOrId, status: 'all'})
.then(function then(foundPostModel) {
if (!foundPostModel) {
throw new common.errors.NotFoundError({
message: common.i18n.t('errors.models.posts.postNotFound')
});
}
return self.permissible.apply(self, newArgs);
});
// Build up the original args but substitute with actual model
var newArgs = [foundPostModel].concat(origArgs);
return self.permissible.apply(self, newArgs);
});
}
function isChanging(attr) {

View File

@ -52,12 +52,19 @@ Role = ghostBookshelf.Model.extend({
origArgs = _.toArray(arguments).slice(1);
// Get the actual role model
return this.findOne({id: roleModelOrId, status: 'all'}).then(function then(foundRoleModel) {
// Build up the original args but substitute with actual model
var newArgs = [foundRoleModel].concat(origArgs);
return this.findOne({id: roleModelOrId, status: 'all'})
.then(function then(foundRoleModel) {
if (!foundRoleModel) {
throw new common.errors.NotFoundError({
message: common.i18n.t('errors.models.role.roleNotFound')
});
}
return self.permissible.apply(self, newArgs);
});
// Build up the original args but substitute with actual model
var newArgs = [foundRoleModel].concat(origArgs);
return self.permissible.apply(self, newArgs);
});
}
if (action === 'assign' && loadedPermissions.user) {

View File

@ -205,6 +205,7 @@
"notEnoughPermission": "You do not have permission to perform this action"
},
"post": {
"postNotFound": "Post not found.",
"untitled": "(Untitled)",
"valueCannotBeBlank": "Value in {key} cannot be blank.",
"isAlreadyPublished": "Your post is already published, please reload your page.",
@ -217,6 +218,7 @@
}
},
"role": {
"roleNotFound": "Role not found",
"notEnoughPermission": "You do not have permission to perform this action"
},
"settings": {

View File

@ -506,7 +506,7 @@ describe('API Utils', function () {
});
it('should handle an unknown rejection', function (done) {
var testStub = sandbox.stub().returns(new Promise.reject()),
var testStub = sandbox.stub().returns(new Promise.reject(new Error('not found'))),
permsStub = sandbox.stub(permissions, 'canThis').callsFake(function () {
return {
testing: {