From fcd3c6847b2f983bc16525db53056d808ad239b8 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 27 Sep 2017 12:12:53 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20=20Fixed=20author=20role=20permi?= =?UTF-8?q?ssion=20to=20change=20author=20(#9067)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐛 Fixed author role permission to change author no issue - To be able to fix this bug, we had to solve tasks from #9043 - This bug affects the private / undocumented API only - Author role users should not be allowed to change the author of a post --- core/server/api/posts.js | 11 +- core/server/models/post.js | 19 ++- core/test/functional/routes/api/posts_spec.js | 159 ++++++++++++++++-- 3 files changed, 166 insertions(+), 23 deletions(-) diff --git a/core/server/api/posts.js b/core/server/api/posts.js index e103b48ed1..3e60d41bde 100644 --- a/core/server/api/posts.js +++ b/core/server/api/posts.js @@ -12,6 +12,7 @@ var Promise = require('bluebird'), 'created_by', 'updated_by', 'published_by', 'author', 'tags', 'fields', 'next', 'previous', 'next.author', 'next.tags', 'previous.author', 'previous.tags' ], + unsafeAttrs = ['author_id'], posts; /** @@ -60,7 +61,7 @@ posts = { // Push all of our tasks into a `tasks` array in the correct order tasks = [ apiUtils.validate(docName, {opts: permittedOptions}), - apiUtils.handlePublicPermissions(docName, 'browse'), + apiUtils.handlePublicPermissions(docName, 'browse', unsafeAttrs), apiUtils.convertOptions(allowedIncludes, models.Post.allowedFormats), modelQuery ]; @@ -94,7 +95,7 @@ posts = { // Push all of our tasks into a `tasks` array in the correct order tasks = [ apiUtils.validate(docName, {attrs: attrs, opts: options.opts || []}), - apiUtils.handlePublicPermissions(docName, 'read'), + apiUtils.handlePublicPermissions(docName, 'read', unsafeAttrs), apiUtils.convertOptions(allowedIncludes, models.Post.allowedFormats), modelQuery ]; @@ -135,7 +136,7 @@ posts = { // Push all of our tasks into a `tasks` array in the correct order tasks = [ apiUtils.validate(docName, {opts: apiUtils.idDefaultOptions.concat(options.opts || [])}), - apiUtils.handlePermissions(docName, 'edit'), + apiUtils.handlePermissions(docName, 'edit', unsafeAttrs), apiUtils.convertOptions(allowedIncludes), modelQuery ]; @@ -182,7 +183,7 @@ posts = { // Push all of our tasks into a `tasks` array in the correct order tasks = [ apiUtils.validate(docName), - apiUtils.handlePermissions(docName, 'add'), + apiUtils.handlePermissions(docName, 'add', unsafeAttrs), apiUtils.convertOptions(allowedIncludes), modelQuery ]; @@ -229,7 +230,7 @@ posts = { // Push all of our tasks into a `tasks` array in the correct order tasks = [ apiUtils.validate(docName, {opts: apiUtils.idDefaultOptions}), - apiUtils.handlePermissions(docName, 'destroy'), + apiUtils.handlePermissions(docName, 'destroy', unsafeAttrs), apiUtils.convertOptions(allowedIncludes), deletePost ]; diff --git a/core/server/models/post.js b/core/server/models/post.js index 5785bc629a..182758916b 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -830,8 +830,23 @@ Post = ghostBookshelf.Model.extend({ }); } - if (postModel) { - // If this is the author of the post, allow it. + function isChanging(attr) { + return unsafeAttrs[attr] && unsafeAttrs[attr] !== postModel.get(attr); + } + + function actorIsAuthor(loadedPermissions) { + return loadedPermissions.user && _.some(loadedPermissions.user.roles, {name: 'Author'}); + } + + function isOwner() { + return unsafeAttrs.author_id && unsafeAttrs.author_id === context.user; + } + + if (actorIsAuthor(loadedPermissions) && action === 'edit' && isChanging('author_id')) { + hasUserPermission = false; + } else if (actorIsAuthor(loadedPermissions) && action === 'add') { + hasUserPermission = isOwner(); + } else if (postModel) { hasUserPermission = hasUserPermission || context.user === postModel.get('author_id'); } diff --git a/core/test/functional/routes/api/posts_spec.js b/core/test/functional/routes/api/posts_spec.js index f860657d3f..37dedd43ed 100644 --- a/core/test/functional/routes/api/posts_spec.js +++ b/core/test/functional/routes/api/posts_spec.js @@ -1321,25 +1321,120 @@ describe('Post API', function () { }); }); + describe('Add', function () { + it('can add own post', function (done) { + var post = { + title: 'Freshly added', + slug: 'freshly-added', + author_id: author.id + }; + + request.post(testUtils.API.getApiQuery('posts/')) + .set('Authorization', 'Bearer ' + authorAccessToken) + .send({posts: [post]}) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(201) + .end(function (err, res) { + if (err) { + return done(err); + } + + var postBody = res.body; + + res.headers['x-cache-invalidate'].should.eql('/p/' + postBody.posts[0].uuid + '/'); + should.exist(postBody); + + testUtils.API.checkResponse(postBody.posts[0], 'post'); + done(); + }); + }); + + it('CANNOT add post with other author ID', function (done) { + var post = { + title: 'Freshly added', + slug: 'freshly-added', + author_id: 1 + }; + + request.post(testUtils.API.getApiQuery('posts/')) + .set('Authorization', 'Bearer ' + authorAccessToken) + .send({posts: [post]}) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(403) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.exist(res.body.errors); + res.body.errors[0].errorType.should.eql('NoPermissionError'); + done(); + }); + }); + }); + describe('Edit', function () { - var postId; + var authorPostId; before(function () { - return testUtils - .createPost({ + return testUtils.createPost({ post: { title: 'Author\'s test post', - slug: 'author-post' - }, - author: author + slug: 'author-post', + author_id: author.id + } }) .then(function (post) { - postId = post.id; + authorPostId = post.id; }); }); it('can edit own post', function (done) { - request.get(testUtils.API.getApiQuery('posts/' + postId + '/?include=tags')) + request.get(testUtils.API.getApiQuery('posts/' + authorPostId + '/?include=tags')) + .set('Authorization', 'Bearer ' + authorAccessToken) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + var jsonResponse = res.body, + changedTitle = 'My new Title', + changedSlug = 'my-new-slug'; + + should.exist(jsonResponse.posts[0]); + jsonResponse.posts[0].title = changedTitle; + jsonResponse.posts[0].slug = changedSlug; + + request.put(testUtils.API.getApiQuery('posts/' + authorPostId + '/')) + .set('Authorization', 'Bearer ' + authorAccessToken) + .send(jsonResponse) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + var putBody = res.body; + res.headers['x-cache-invalidate'].should.eql('/*'); + should.exist(putBody); + putBody.posts[0].title.should.eql(changedTitle); + putBody.posts[0].slug.should.eql(changedSlug); + + testUtils.API.checkResponse(putBody.posts[0], 'post'); + done(); + }); + }); + }); + + it('CANNOT change author of own post', function (done) { + request.get(testUtils.API.getApiQuery('posts/' + authorPostId + '/?include=tags')) .set('Authorization', 'Bearer ' + authorAccessToken) .expect('Content-Type', /json/) .expect('Cache-Control', testUtils.cacheRules.private) @@ -1357,24 +1452,56 @@ describe('Post API', function () { jsonResponse.posts[0].title = changedTitle; jsonResponse.posts[0].author = changedAuthor; - request.put(testUtils.API.getApiQuery('posts/' + postId + '/')) + request.put(testUtils.API.getApiQuery('posts/' + authorPostId + '/')) .set('Authorization', 'Bearer ' + authorAccessToken) .send(jsonResponse) .expect('Content-Type', /json/) .expect('Cache-Control', testUtils.cacheRules.private) - .expect(200) + .expect(403) .end(function (err, res) { if (err) { return done(err); } - var putBody = res.body; - res.headers['x-cache-invalidate'].should.eql('/*'); - should.exist(putBody); - putBody.posts[0].title.should.eql(changedTitle); - putBody.posts[0].author.should.eql(changedAuthor); + should.exist(res.body.errors); + res.body.errors[0].errorType.should.eql('NoPermissionError'); + done(); + }); + }); + }); - testUtils.API.checkResponse(putBody.posts[0], 'post'); + it('CANNOT become author of other post', function (done) { + request.get(testUtils.API.getApiQuery('posts/' + testUtils.DataGenerator.Content.posts[0].id + '/?include=tags')) + .set('Authorization', 'Bearer ' + authorAccessToken) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + var jsonResponse = res.body, + changedTitle = 'My new Title', + changedAuthor = author.id; + + should.exist(jsonResponse.posts[0]); + jsonResponse.posts[0].title = changedTitle; + jsonResponse.posts[0].author = changedAuthor; + + request.put(testUtils.API.getApiQuery('posts/' + testUtils.DataGenerator.Content.posts[0].id + '/')) + .set('Authorization', 'Bearer ' + authorAccessToken) + .send(jsonResponse) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(403) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.exist(res.body.errors); + res.body.errors[0].errorType.should.eql('NoPermissionError'); done(); }); });