-
Notifications
You must be signed in to change notification settings - Fork 584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
3.17: Module editRole and publishRole aren't correctly followed #3719
Comments
UpdateI wrote a project-level module to patch the issue I described above, and wound up running into another permissions issue downstream. The MongoDB criteria defined in the permissions module restrict database edit queries to draft states only. This means that even with both edit and publish permission on the module level, contributors cannot actually edit content after publishing it, because they will get a database error from MongoDB due to the criteria restriction. For example, trying to edit a published post won't return a document for the query in the publish function here because contributors are blocked from searching for documents they have edit rights to which aren't in the draft state. So when they trigger a publish due to their module-level permissions, it doesn't see the existing published document, resulting in an attempt to insert a new document with the same ID, which throws a Sorry if that explanation is difficult to follow, I admit I'm not the best at troubleshooting MongoDB query logic so this took me a bit of trial and error to diagnose. |
Thanks for the clarification, I appreciate it. |
So to fix this issue for our project in the short-term, I created a local override at // Copied from the original file, since it's not exported
const ranks = {
guest: 0,
contributor: 1,
editor: 2,
admin: 3,
};
const checkRank = (userRole, minimumRole) => {
return ranks[userRole] >= ranks[minimumRole];
};
module.exports = {
extendMethods(self) {
return {
// Patches https://github.com/apostrophecms/apostrophe/issues/3719#issue-1209207160
can(_super, req, action, docOrType, mode) {
const defaultPermissions = _super(req, action, docOrType, mode);
// Below vars use the same logic at the beginning of the default super function
mode = mode || req.mode;
const role = req.user && req.user.role;
const type = docOrType && (docOrType.type || docOrType);
const manager = type && self.apos.doc.getManager(type);
// Explicitly recheck the module role level for the action if it exists
const minimumRole = manager && manager.options[`${action}Role`];
if (minimumRole) {
return checkRank(role, minimumRole);
}
// Otherwise use the default return
return defaultPermissions;
},
// Patches https://github.com/apostrophecms/apostrophe/issues/3719#issuecomment-1104457929
criteria(_super, req, action) {
const defaultCriteria = _super(req, action);
// If there are no mode retrictions, we're fine with what we've already got
if (!defaultCriteria.aposMode || !req.route) {
return defaultCriteria;
}
// Only place to get the type right now is from the url as far as I can tell
const apiPrefix = '/api/v1/';
const { path } = req.route;
const type = path.slice(
path.indexOf(apiPrefix) + apiPrefix.length,
path.indexOf('/:_id'),
);
const manager = type && self.apos.doc.getManager(type);
const role = req.user && req.user.role;
if (action === 'edit') {
if (role === 'contributor') {
// Explicitly recheck the module publish role to see if we can edit those
const minimumRole = manager && manager.options.publishRole;
if (minimumRole && checkRank(role, minimumRole)) {
delete defaultCriteria.aposMode;
}
}
}
return defaultCriteria;
},
};
},
}; Not sure if there's a better way to get which piece-type is being checked, as the only place I was seeing it in the request was in the url itself. Any suggestions are welcome, as to be honest I'm not entirely happy with the way I'm handling the db criteria change right now. |
Thank you for sharing this. We are just a few iterations out from
implementing our permissions groups module so that will be the right time
for us to look more closely at it.
…On Fri, Apr 22, 2022 at 5:42 PM Yury Tilis ***@***.***> wrote:
So to fix this issue for our project in the short-term, I created a local
override at ***@***.***/permission/index.js with the following
content:
// Copied from the original file, since it's not exportedconst ranks = {
guest: 0,
contributor: 1,
editor: 2,
admin: 3,};
const checkRank = (userRole, minimumRole) => {
return ranks[userRole] >= ranks[minimumRole];};
module.exports = {
extendMethods(self) {
return {
// Patches #3719 (comment)
can(_super, req, action, docOrType, mode) {
const defaultPermissions = _super(req, action, docOrType, mode);
// Below vars use the same logic at the beginning of the default super function
mode = mode || req.mode;
const role = req.user && req.user.role;
const type = docOrType && (docOrType.type || docOrType);
const manager = type && self.apos.doc.getManager(type);
// Explicitly recheck the module role level for the action if it exists
const minimumRole = manager && manager.options[`${action}Role`];
if (minimumRole) {
return checkRank(role, minimumRole);
}
// Otherwise use the default return
return defaultPermissions;
},
// Patches #3719 (comment)
criteria(_super, req, action) {
const defaultCriteria = _super(req, action);
// If there are no mode retrictions, we're fine with what we've already got
if (!defaultCriteria.aposMode || !req.route) {
return defaultCriteria;
}
// Only place to get the type right now is from the url as far as I can tell
const apiPrefix = '/api/v1/';
const { path } = req.route;
const type = path.slice(
path.indexOf(apiPrefix) + apiPrefix.length,
path.indexOf('/:_id'),
);
const manager = type && self.apos.doc.getManager(type);
const role = req.user && req.user.role;
if (action === 'edit') {
if (role === 'contributor') {
// Explicitly recheck the module publish role to see if we can edit those
const minimumRole = manager && manager.options.publishRole;
if (minimumRole && checkRank(role, minimumRole)) {
delete defaultCriteria.aposMode;
}
}
}
return defaultCriteria;
},
};
},};
Not sure if there's a better way to get which piece-type is being checked,
as the only place I was seeing it in the request was in the url itself. Any
suggestions are welcome, as to be honest I'm not entirely happy with the
way I'm handling the db criteria change right now.
—
Reply to this email directly, view it on GitHub
<#3719 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAH27JBI3KAJQSEGZ736OLVGMMNFANCNFSM5T25Q72Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER
APOSTROPHECMS | apostrophecms.com | he/him/his
|
Hey @boutell , can you assign me this issue so that I can get started working on it ? |
@itsajay1029 there's quite a bit of work here already on this ticket, I think it might make more sense for us to internally review what @ytilis has done. We have also shipped |
To Reproduce
publishRole
to contributorExpected behavior
editRole
andpublishRole
options on a module should be followed by the permissions logicDescribe the bug
There are several related issues in the way permissions are handled, in and around this section of the permissions module.
The feedback I got on my initial report in the Discord channel from @boutell was as follows:
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 that says: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: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 tocontributor
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 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:
Note the flipped
<
to>=
and returning the comparison directly instead offalse
, 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
The text was updated successfully, but these errors were encountered: