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

A user with update access can change its own ACL permissions #2094

Closed
arkhi opened this issue Mar 11, 2021 · 9 comments
Closed

A user with update access can change its own ACL permissions #2094

arkhi opened this issue Mar 11, 2021 · 9 comments
Assignees

Comments

@arkhi
Copy link
Contributor

arkhi commented Mar 11, 2021

This issue uses the same configuration as #2001. Versions used are:

  • Grav v1.7.7
  • Admin Panel v1.10.6
  • Email v3.1.1
  • Error v1.7.1
  • Flex Objects v1.0.3
  • Form v5.0.0
  • Login v3.4.1
  • Markdown Notices v1.1.0
  • Problems v2.0.3

As I understand ACL, a user should not be able to update its permissions except, maybe, for a super user. This is possible with Grav Admin plugin by following these steps:

  1. I login with limited (password: Limit3d!).
  2. I go to the only page I have read and update access to (01.page-1).
  3. I click on the Expert button.
  4. I update the value of permissions.groups.authors.delete from false to true.
  5. I save the page.
  6. I delete the page:
    • The page is deleted successfully with the message Directory Entry removed successfully.
    • I am redirected to /admin/pages/page-0 with a Error 403.
@mahagr mahagr self-assigned this Mar 12, 2021
@mahagr mahagr added the bug label Mar 12, 2021
@mahagr
Copy link
Member

mahagr commented Mar 18, 2021

@rhukster Should we disable expert mode from everyone else but super users? I'm asking this because expert mode saves YAML just like it was written into the text field without any other checks than YAML syntax errors and some XSS checks.

@arkhi
Copy link
Contributor Author

arkhi commented Mar 18, 2021

Although I like the idea of disabling the expert mode for some, maybe making it configurable with an access.edit_mode: (expert|false) like the existing PHP check in Page.php would be good?

@mahagr
Copy link
Member

mahagr commented Mar 19, 2021

Well, the main issue with the expert mode is that it gives you raw access to the page. That means you can see every header variable on the page and you can modify it, no questions asked. As it bypasses (almost) all the security checks, I am not sure when we should expose it to the user.

IMHO configuration option should not override ACL. That said, the check does need to be updated so that it has no effect when the user isn't allowed to see raw mode.

@arkhi
Copy link
Contributor Author

arkhi commented Mar 19, 2021

I had this problem with my Grav 1.6 pages-permissions plugin. I’m not a PHP developer so my solution might seem to work for me and not actually be good, but I tackled the issue by defining locked properties.

@mahagr
Copy link
Member

mahagr commented Mar 19, 2021

I do not like the locked properties approach for the same reason: users should not be able to see everything from the file. It also doesn't really work if the user can add an infinite amount of new properties which happen not to be listed in the locked list.

@arkhi
Copy link
Contributor Author

arkhi commented Mar 19, 2021

Fair enough. :)

@mahagr
Copy link
Member

mahagr commented Mar 19, 2021

 @rhukster @w00fz To fix this issue I propose that we allow raw edit only for super users and perhaps allow specifying a special group from the configuration..?

@w00fz
Copy link
Member

w00fz commented Mar 20, 2021

Sounds like the most logical solution for me.

@mahagr
Copy link
Member

mahagr commented Mar 31, 2021

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

3 participants