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

Moving a page with no permission #3382

Closed
arkhi opened this issue Dec 12, 2020 · 10 comments
Closed

Moving a page with no permission #3382

arkhi opened this issue Dec 12, 2020 · 10 comments
Assignees

Comments

@arkhi
Copy link
Contributor

arkhi commented Dec 12, 2020

I will write a few different Issues that may or may not be related. I’m just not sure yet about how permissions are working with Grav 1.7, so please bear with me. :)

With this user config on this vagrant machine and Grav v1.7.0-rc.19 with the following plugins:

  • Admin Panel v1.10.0-rc.19
  • Email v3.1.0
  • Error v1.7.1
  • Flex Objects v1.0.0-rc.19
  • Form v4.2.0
  • Login v3.3.8
  • Markdown Notices v1.1.0
  • Problems v2.0.3

admin can log in with Admin4dm1n.


  1. I login with limited (password: Limit3d!).

  2. I go to the only page I have read access to (01.page-1).

  3. I click on the Move button.

  4. In the modal that pops up, I select 02.page-2 as the parent.

  5. I press the Continue button.

  6. Some things happened:

    • I am redirected to a 404 page (/admin/pages/page-0/page-1) with an error message:

      Save Failed: file_get_contents(/vagrant/user/pages/03.page-0/02.page-2/999999.page-1/default.md): failed to open stream: No such file or directory

    • 01.page-1 was deleted (although I have no permission to delete page 1).

    • 01.page-1 was moved inside 02.page-2 (although I have no permission to access or create pages within 02.page-2).

    • 01.page-1 folder index is renamed 999999.page-1

I don’t know what happened for sure. One thing that strikes me is that pages the currently logged in user has no access to are being listed.

Not sure if that could be of any help: I made a plugin (grav-plugin-admin-pages-permissions) a few months ago for Grav 1.6 which only lists the tree of accessible pages based on CRUD permissions. Keep in mind I am no PHP developer and that the inner logic of Grav code is not my forte at all, so the plugin does what it can to… work within its context.

I also had no real performance issues as the website does not have many pages. All those loops seem to me to not be a good thing.

Let me know if I can be of any help.

@arkhi arkhi changed the title Moving a page with no Delete permission and no Moving a page with no Delete permission Dec 12, 2020
@mahagr mahagr self-assigned this Dec 14, 2020
@mahagr
Copy link
Member

mahagr commented Jun 8, 2021

@arkhi Can you please re-test this as I think that most of it have already been fixed?

@arkhi
Copy link
Contributor Author

arkhi commented Jun 9, 2021

Sure!

I did the following and:

  • 01.page-1 folder index is not renamed 999999.page-1 any more. 👍
  • The rest still happens, with the error being slightly different:

    Failed to save entry: file_get_contents(/vagrant/user/pages/03.page-0/02.page-2/01.page-1/default.md): failed to open stream: No such file or directory


  1. Download the latest version from getgrav.org,
  2. Unpack the zip file.
  3. Overwrite files with the content of user.zip.

@mahagr
Copy link
Member

mahagr commented Jun 10, 2021

I'm not getting the error, but the page is being moved...

@mahagr
Copy link
Member

mahagr commented Jun 10, 2021

Fixed similar ACL bug when creating a new page or folder:

trilbymedia/grav-plugin-flex-objects@8edf759

@mahagr mahagr transferred this issue from getgrav/grav-plugin-admin Jun 10, 2021
@mahagr mahagr added the bug label Jun 10, 2021
@mahagr
Copy link
Member

mahagr commented Jun 10, 2021

@mahagr mahagr added the fixed label Jun 10, 2021
@mahagr
Copy link
Member

mahagr commented Jun 10, 2021

I was not able to reproduce your error, but now the ACL checks should be correct.

Note that I ended up checking update permission for the page itself and create permission for the new parent page. I don't think that delete permission for the old parent should be checked as the page will still exist after moving it.

@mahagr mahagr changed the title Moving a page with no Delete permission Moving a page with no permission Jun 10, 2021
@arkhi
Copy link
Contributor Author

arkhi commented Jun 10, 2021

There might be a misunderstanding.

Can a page be moved if it can’t be deleted? My understanding is that moving implies deleting to be able to change its location.

@arkhi
Copy link
Contributor Author

arkhi commented Jun 10, 2021

So for what I understand of ACL, the page should not be able to be moved at all since its rules do not allow limited to delete it.

@mahagr
Copy link
Member

mahagr commented Jun 14, 2021

I compared the permissions to what Joomla has and it also allows move without delete permission. Technically you're not deleting the page, but moving it into another location. Based on that I'm checking edit permission for the page and create permission from the new parent page to see if the user can touch the new location.

I don't think there's a right way to do it and I have no preference on which checks are being made. You can also think of moving as copy (or create) + delete...

What is your reasoning why there needs to be delete check?

CC @rhukster @w00fz

@arkhi
Copy link
Contributor Author

arkhi commented Jun 14, 2021

Interesting… Thanks for checking on Joomla.

My understanding is that moving implies deleting to be able to change its location.

What is your reasoning why there needs to be delete check?

My reasoning is exactly what you mentioned: “moving = copy (or create) + delete”. :)

My goal here is to know what is to be expected; just making sure things work as intended. If the result matches documented expectations, all’s good and people can act accordingly.

I think Grav not checking for deletion would be worth documenting if it’s the agreed upon decision (or I missed that part of the documentation).

@mahagr mahagr closed this as completed Mar 22, 2022
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

2 participants