Skip to content
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

"Permission denied" alerts for limited access policy Users #12568

Closed
pixelchutes opened this issue Aug 19, 2015 · 26 comments
Closed

"Permission denied" alerts for limited access policy Users #12568

pixelchutes opened this issue Aug 19, 2015 · 26 comments

Comments

@pixelchutes
Copy link
Contributor

Summary

Everything functioning as desired in MODX 2.3.5 with a custom "customer service" user role, tied to a custom access policy with limited permissions (enough to change basic user profile fields / passwords, etc), but settings permission is not granted, as User Settings / System Settings access is not desired.

Upgrading to MODX 2.4.0-pl, and now a "Permission denied." alert is shown on each page load when attempting to update a user.

Step to reproduce

Create a User, assigned to a custom role tied to a custom policy without settings permission access. Login as the new user and attempt to edit another User in the system.

Observed behavior

Permission Denied alert is now prompted on every page load when editing a user once the following processor is called:

  • system/settings/getAreas

Code: 200 OK
{"success":false,"message":"Permission denied!","total":0,"data":[],"object":[]}

alert

Expected behavior

Prior to the 2.4.0 upgrade, these processors did not alert the permission denied error each time. This was just fine since the access policy did not grant settings permission, and "failed silently", allowing the customer service user to administer users without issue, to the extent their access allowed. User settings grid was empty (by design).

Now a confusing "permission denied" prompt is alerted by simply loading the page...

Hopefully there is a way to suppress this warning / error message, as it is important to be able to provide limited access controls in the Manager (e.g. to only manage users) without having to grant access to settings (System Settings!) to do so.

Environment

MODX 2.4.0-pl, Chrome 46

@pixelchutes
Copy link
Contributor Author

Ok, I've finally traced it down to 0ba1005 which modified manager/assets/modext/widgets/core/modx.combo.js in #12354

Specifically, it is stemming from this fireEvent call when the settings panel is empty (per missing settings permission)

The combo doesn't load (e.g. "fails" silently), and works as before...until the fireEvent is invoked on a combo with no data store.

@theboxer any ideas on how to account for both scenarios here?

@pixelchutes
Copy link
Contributor Author

It appears one way to solve this specific scenario is to prevent the entire "Settings" tab from rendering if no settings access is granted...not sure this would cover combos in other areas, unfortunately.

@oori
Copy link
Contributor

oori commented Sep 18, 2015

Actually, the user edit form depends on both "settings" and "namespaces" permissions. as required by the settings grid, and the namespace combo "modx-combo-namespace".

This is wrong, it makes no sense to allow manager users to edit namespaces nor system settings, just because they require to edit users. (and that's not the only case)

I was about to send a PR with a solution, but I'm not convinced. The current MODx approach would be to expand "settings" and "namespaces" to multiple permissions. e.g.: namespace_view, _delete, _edit, _new, _save. (related with each processor's duty).

BUT:

  1. This feels bloated.. make little sense here, as "namespaces" and "settings" are really not granular, it's nothing or all (as is today, "settings/namespaces"), except other stuff depend on their getlist processor..
  2. A more generic thought is needed to standardise the permissions. as of today, there are different naming, e.g.: "new_document, save_document" VS. "policy_new, policy_save"
  3. Permission clusters/dependencies concept is missing, but practically exists, for example: user form depends on "settings", and various other forms depends on "namespaces". The existing permissions model doesn't resolve this, it should either be added (or at least - such dependencies documented)

Perhaps another opinion about "permissions 2.5/3.0" would help? @opengeek @theboxer

@brescianicri
Copy link

Then if i want create and editor user that manager the user i must do access permission of "settings" and "namespaces" ??
I found this is very wrong.

@pixelchutes
Copy link
Contributor Author

@oori @brescianicri Completely agree. 😞

I'm hopeful that a solution can be presented to at least restore this critical functionality to MODX 2.4.x-series.

I'm not sure of the implications of reverting this line of code...could be an option? @theboxer can you comment for us? Maybe there's an additional check that could account for combos who's store can't be populated as a result of no permissions, preventing the fireEvent (and subsequent Permission Denied prompt)?

While ideal, any kind of improvement or overhaul to permissions for 2.5.x seems less likely than for 3.x. It would definitely require the attention of someone more familiar on the matter.

@smashstack-kris
Copy link

we're having the same problem on sites recently running 2.4.2 ... when setting up new users in the manager, this error fires off when viewing their user info:

Code: 200 OK
{"success":false,"message":"Permission denied!","total":0,"data":[],"object":[]}

@oori
Copy link
Contributor

oori commented Jan 12, 2016

@pixelchutes reverting that line of code wouldn't do it. the issue is in the processors (getlist specifically)
as I commented earlier, it is rather simple to fix, but I really think we need a clustered/dependencies permission model (see my 3rd comment above). any solution would require modifying various processors, so I guess better resolve the new concept before we do the foot work. until than - people just have to give these permissive permissions as a stop gap solution.

@pixelchutes
Copy link
Contributor Author

Thanks @oori. In my testing, it was that line of code triggering the processor resulting in the "permission denied" alert.

You're absolutely right, the root of the problem runs much deeper.

@pixelchutes pixelchutes changed the title "Permission denied" alert when editing User (using limited access policy) "Permission denied" alerts for limited access policy Users Mar 3, 2016
@pixelchutes
Copy link
Contributor Author

I have updated the title of this issue to reflect the previous conversation, and the fact that this issue is present outside of editing Users. It can happen when editing Resources (I believe since 2.4.3?) and other content, too.

This is an impactful regression issue since 2.4.0 that deserves proper attention.

@Robert-Saiter
Copy link

I'm unfortunately new to all of this.. I have this issue on users who do not have full admin rights but should have enough to edit users. I have given the settings and namespaces permissions but the problem persists. As soon as you visit Manage -> Users the error pops up. Any thoughts?

Currently on an unmodified version of 2.4.2-pl (not sure if this problem has been addressed at all in another version but this conversation suggests it has not been).

@pixelchutes
Copy link
Contributor Author

This is a system wide error that was introduced in 2.4.0 release. There is currently no other solution other than downgrading back to 2.3.x series or hacking the core, which generally is not recommended / only a workaround.

@muzzwood
Copy link
Contributor

muzzwood commented Apr 2, 2016

I'm getting the same:

Code: 200 OK
{"success":false,"message":"Permission denied!","total":0,"data":[],"object":[]}

when clicking save on the resource edit screen. Save still works, you just get this pop up box.
MODX 2.4.3
Only occurs with users in a restricted group. Admin is fine.

@Mark-H
Copy link
Collaborator

Mark-H commented May 11, 2016

Not every "Code 200 OK permission denied" error has the same root cause. The problem in each case is the lack of a specific permission though. In the case of editing the resource it's probably either the class list or content types (on the resource settings tab) that doesn't have sufficient permissions.

I'm not sure what the best way to resolve #12568 (namespaces dropdown triggering the error) is either, though I'm leaning towards a new permission for listing the namespaces. If that permission is not available, don't show the combo.

The problem with not showing the combo is that users might not be able of switching namespaces to get to the setting they should be able of editing.

Other areas this might also affect include the lexicon management (which is what the modAccessNamespaces was introduced for) and system/context settings.

@pyrographics
Copy link

Is it possible to make the combo more conclusive as to which permission is causing it? Maybe use the debug system setting to control whether they are displayed or not.

@pixelchutes
Copy link
Contributor Author

@Mark-H All great points. Do you think we should re-open #13009?

I agree with the idea of new permissions like @oori mentioned, a need for more granularity vs. a single / global settings or namespaces to "rule them all".

@pixelchutes
Copy link
Contributor Author

pixelchutes commented May 11, 2016

Another note is that the "permission denied" errors have always been the case for limited access users like this, however before 2.4.0, they simply happened silently behind the scenes. Perhaps there is a shorter term fix (e.g. band-aid), reverting the behavior resulting in the actual alert / prompt.

@pyrographics
Copy link

Show them in the js console but don't display them on the screen. Developers like me who create limited permission accounts know we are opening up a configuration can of worms and don't need to pass more problems along to end users.

@pixelchutes
Copy link
Contributor Author

@pyrographics Exactly. That's how it actually worked through 2.3.x

@Mark-H
Copy link
Collaborator

Mark-H commented May 11, 2016

ACLs have a discovery problem where the UI is suboptimal and the error messages insufficient. Hiding the error messages in the console really doesn't help make ACLs better.

What if the permission denied error showed the name of the permission that was required? The errors seem pretty scattered across the core from a quick search, but adding a new lexicon for something that says "Permission denied! Your account requires [[+permission]] to complete this action, which you have not been granted." and updating the various processors to pass the required permission in there... wouldn't that help?

@isaacniebeling
Copy link

Displaying / logging the permission would be awesome. I've had a tough time tracking issues down occasionally. Bonus points for also displaying/logging the bit of the system that's making the request.

@pixelchutes
Copy link
Contributor Author

Totally agree. This would be incredibly helpful for debugging, or new users to MODX ACLs, eliminating much of the current guess-work that's often involved.

@pixelchutes
Copy link
Contributor Author

Suggested idea related to #12693

@OptimusCrime
Copy link
Contributor

Did this issue change from being a bug report on a missing ACL to becoming a suggestion for how to log/display error messages related to permissions? The original issue was solved in PR #13508.

@pixelchutes
Copy link
Contributor Author

Hmm...I don't think so? This started as a regression issue in 2.4.x, specifically when editing users and the admin specifically does not have settings access, they start receiving this error prompt on page load. It's always been about how to prevent the alert moving forward, without requiring full-blown access to site settings.

@OptimusCrime
Copy link
Contributor

Ah, yes. I see. Sorry about that.

@Ibochkarev
Copy link
Collaborator

Improved in #15402

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests