Skip to content

Commit

Permalink
🐛 Fixed edit permission of the common article by multiple authors (#1…
Browse files Browse the repository at this point in the history 
…0214)

closes #10212
  • Loading branch information
@kacperduras @naz
kacperduras authored and naz committed Dec 3, 2018
1 parent 7aa8251 commit 7c1840f
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion core/server/models/relations/authors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ module.exports.extendModel = function extendModel(Post, Posts, ghostBookshelf) {
}

function isCurrentOwner() {
return context.user === postModel.related('authors').models[0].id;
return postModel.related('authors').models.map(author => author.id).includes(context.user);

This comment has been minimized.

Sign in to view

Copy link
@kirrg001

kirrg001 Dec 3, 2018

Contributor

Is this fix correct? 🤔

e.g.

            } else if (isContributor && isDestroy) {
                hasUserPermission = isCurrentOwner();

Case: If the logged in user is a contributor and he wants to destroy the post.
Behaviour: The contributor can delete a post as secondary author.

Feels like a behaviour change? I have not tested it.

IMO we need to functions here: isPrimaryAuthor and isAnyAuthor.

This comment has been minimized.

Sign in to view

Copy link
@naz

naz Dec 3, 2018

Contributor

Wasn't sure if we even want to allow that, there's no distinction between primary/co-authors in our permissions system atm - https://docs.ghost.org/faq/managing-your-team/#user-role-capabilities. Based on that table, this specific case should be always false for a contributor, no matter if he's a co-author or primary author. Not sure where the 'source of truth' for these rules is :)

This comment has been minimized.

Sign in to view

Copy link
@kirrg001

kirrg001 Dec 3, 2018

Contributor

Contribs can delete their own posts if draft (#9314). That means the authors relation checks if

  • are you a contrib? yes/no?
  • you want to delete? yes/no?
  • is that your post (primary author)? yes/no?

This comment has been minimized.

Sign in to view

Copy link
@naz

naz Dec 3, 2018

Contributor

is that your post (primary author)? yes/no?

This bit was a source of confusion for me as it's not described clearly anywhere what is the definition of 'your post' (#10234):

Specifically there should be a clarification on whether there's a difference between user being a co-author or a primary author.

This comment has been minimized.

Sign in to view

Copy link
@kirrg001

kirrg001 Dec 3, 2018

Contributor

Agree, we could specify the behaviour a bit better with the help of #10234.

But for now: i think this change introduced a bug and changed a behaviour?

The fn is even called isCurrentOwner, which is now very confusing 🤪

The current owner was always the author_id in the post model (https://github.com/TryGhost/Ghost/blame/3e295bee7e6520cd46410f5f0c39e346d093a7e9/core/server/models/post.js#L710). And the author_id is the primary author.

This comment has been minimized.

Sign in to view

Copy link
@naz

naz Dec 4, 2018

Contributor

@kirrg001 I'll create an issue+PR with a quick fix for this permission mixup. Just to summarize: we revert the change to only allow editing to primary authors for 'Contributors' and allow editing as a co-author for Authors? :)
}

if (isContributor && isEdit) {
Expand Down

0 comments on commit 7c1840f

Please sign in to comment.