apostrophecms / apostrophecms/apostrophe
3.17: Module editRole and publishRole aren't correctly followed
- Dominant language
- JavaScript
- Stars
- 4.6k
- Forks
- 650
- Avg merge
- 19h 21m
- Merged PRs (30d)
- 23
Description
## To Reproduce
1. Create a custom piece-type
2. Set the following module options:
```JavaScript
editRole: 'contributor',
publishRole: 'contributor',
```
3. Contributors will not be able to publish, despite setting the `publishRole` to contributor
## Expected behavior
`editRole` and `publishRole` options on a module should be followed by the permissions logic
## Describe the bug
There are several related issues in the way permissions are handled, in and around [this section of the permissions module](https://github.com/apostrophecms/apostrophe/blob/adab563624ea05ef68ee23cc188ad9a7ddd58f37/modules/%40apostrophecms/permission/index.js#L89).
The feedback I got on my initial report in the Discord channel from @boutell was as follows:
> Hi @ytilis , those currently work well enough to allow image pieces to be published by contributors (implicitly), and to restrict editing of users to admins. I haven’t dug into the issue you were reporting but I wouldn’t say those options don’t work at all. I also wouldn’t say they’re guaranteed to work for every possible UI situation that could come up as a result of experimenting with them at the moment.
And this was a good question, since I'm positive those flags aren't actually working correctly, so I did a bunch of digging to find out why they appeared to be working in those scenarios, and here's what I've found, which does a better job explaining the issue than my original report:
### Why it appears to work for images
Publishing Images works for contributors purely because of the `autopublish` flag. There's a code comment [here on the doc-type module here](https://github.com/apostrophecms/apostrophe/blob/adab563624ea05ef68ee23cc188ad9a7ddd58f37/modules/%40apostrophecms/doc-type/index.js#L687/) that says:
> If `options.autopublishing` is true, then the `edit` permission is sufficient, otherwise the `publish` permission is checked for
What this means is that by setting `editRole: 'contributor',` on `@apostrophecms/image`, you can in fact upload/publish images, BUT, if you then try to then *edit* an uploaded image, you can't due to [the following code](https://github.com/apostrophecms/apostrophe/blob/adab563624ea05ef68ee23cc188ad9a7ddd58f37/modules/%40apostrophecms/permission/index.js#L82):
```JavaScript
if (manager && manager.options.editRole && (ranks[role] < ranks[manager.options.editRole])) {
return false;
} else if (mode === 'draft') {
return (role === 'contributor') || (role === 'editor');
} else {
return role === 'editor';
}
```
Since the initial image upload takes place in `draft` mode, autopublish's check that you can edit passes purely because contributors are allowed to edit drafts. But once it's published, even if publishRole and editRole are both set to contributor, the checks will still return false because after passing the module configured role checks, they're failing against the hardcoded roles in the subsequent elseif checks.
### Why it appears to work for users
This is actually a pretty interesting, and very complicated, scenario for a few reasons, and can easily illustrate this bug, because setting edit and publish roles to `editor` on `@apostrophecms/user` works as expected, but setting them to `contributor` doesn't.
The fundamental issue is that you can *raise* any permission to editor or above, since a higher module specified role will fail the initial if statement and immediately return false, but you can never lower any permission below editor for editing or publishing, because once they pass the initial if statement, the else-ifs will continue enforcing the default assumed permission model.
Additionally it should be noted that admins short-circuit this entire permissions check group, as [seen here](https://github.com/apostrophecms/apostrophe/blob/adab563624ea05ef68ee23cc188ad9a7ddd58f37/modules/%40apostrophecms/permission/index.js#L59) so the hard checks against roles never effect them.
## Proposed Solution
After looking more closely at this logic, I think my initial assumption and suggestion in Discord was actually incorrect. I assumed that the editRole and publishRole should've been the sole determiner, but I think this issue arose because the code was written to accommodate a lack of those options so it makes default assumptions. The trouble is those defaults always run when the module role check passes, effectively overriding the module configured permissions. My proposed solution is that if a publishRole, editRole, or viewRole is provided, the statement should simply return the role comparison, and if none is provided, then it should continue through the fallback else-ifs. So for example, the new editRole logic I mention above would instead look like this:
```JavaScript
if (manager && manager.options.editRole) {
return (ranks[role] >= ranks[manager.options.editRole]);
} else if (mode === 'draft') {
return (role === 'contributor') || (role === 'editor');
} else {
return role === 'editor';
}
```
Note the flipped `<` to `>=` and returning the comparison directly instead of `false`, thereby preventing the subsequent elseif checks from running.
A similar change would have to be made for publishRole and viewRole as well, which suffer from the same issues.
## Details
**Version of Node.js:**
v14.18.3
**Server Operating System:**
macOS Monterey v12.3.1
Contributor guide
Research direction
Start in modules/@apostrophecms/permission/index.js around the role checks, then compare the related autopublishing logic in modules/@apostrophecms/doc-type/index.js. Reproduce the custom piece-type scenario with contributor editRole and publishRole settings, and verify that configured editRole, publishRole, and viewRole values take precedence while the existing defaults remain for unset options.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- authorization, backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100