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

Check stack and global area permissions #7006

Closed
mlocati opened this issue Aug 24, 2018 · 11 comments
Closed

Check stack and global area permissions #7006

mlocati opened this issue Aug 24, 2018 · 11 comments
Assignees

Comments

@mlocati
Copy link
Contributor

mlocati commented Aug 24, 2018

When we check permissions of stacks and global areas, we should use the permission keys of the area category.
BTW in the core we currently often permission keys of the page category.

What does that imply?

  1. we need to create the Permission\Checker instance as $cp = new Checker(Area::get($c, STACKS_AREA_NAME)) instead of $cp = new Checker($c)
  2. we need to use the area permission keys, which doesn't match the page permission keys (for example: $cp->canEditAreaContents() instead of $cp->canEditPageContents()).

Here's the full list of page and area permission keys:

Page permissions Area permissions
add_subpage missing (can be used to control if a user can create localized stack versions)
approve_page_versions missing
delete_page missing (can be used to control if a user can delete a stack)
delete_page_versions missing
edit_page_contents edit_area_contents
edit_page_multilingual_settings nonsensical
edit_page_page_type nonsensical
edit_page_permissions edit_area_permissions
edit_page_properties edit_area_properties
edit_page_speed_settings nonsensical
edit_page_template nonsensical
edit_page_theme nonsensical
move_or_copy_page missing
preview_page_as_user nonsensical
schedule_page_contents_guest_access schedule_area_contents_guest_access
view_page view_area
view_page_in_sitemap nonsensical
view_page_versions missing
nonsensical add_block_to_area
nonsensical add_layout_to_area
nonsensical add_stack_to_area
nonsensical delete_area_contents
nonsensical edit_area_design

So, If it's ok for the core team, I'd add these area permission keys

  • add_subarea
  • approve_area_versions
  • delete_area
  • delete_area_versions
  • view_area_versions
  • move_or_copy_area
  • edit_area_properties

and I'd update the code accordingly.

@mlocati
Copy link
Contributor Author

mlocati commented Sep 5, 2018

#7029 fixes this.

@aembler
Copy link
Member

aembler commented Sep 10, 2018

I like the idea behind this – and I recognize it's a problem – but I don't know about some of the permissions here. Would it be better for us to just ignore area permissions entirely and use the permissions of the page that the stack comes from instead?

@mlocati
Copy link
Contributor Author

mlocati commented Sep 10, 2018

Well, stacks are meant to be used in multiple pages, right?

@aembler
Copy link
Member

aembler commented Sep 10, 2018

I don't mean the permissions of the page it's on – I mean instead of showing the area permissions when we click the permissions buttons – should we just show the page permissions, so those are the ones we're checking?

@aembler
Copy link
Member

aembler commented Sep 10, 2018

And by page permissions, I mean the permissions for the page that exists in the sitemap for that particular stack.

@mlocati
Copy link
Contributor Author

mlocati commented Sep 10, 2018

Yes, stacks are also pages.
Right now, we sometimes check the stack permissions as if they were pages, sometimes as if they were areas (and that's causing the errors this PR fixes).
This PR makes it so we always check the "area" permissions, but for sure we can switch to checking only "page" permissions. Just tell me which ones you prefer...

@mlocati
Copy link
Contributor Author

mlocati commented Sep 11, 2018

And by page permissions, I mean the permissions for the page that exists in the sitemap for that particular stack.

The problem is that stacks and global areas are both "pages" and "areas".
Sure, we can use "page" permissions (to check if we can see/approve their versions for example), but we also need "area" permissions (to check if we can add a block to them for example).

We have 3 solutions:

  1. use only "area" permissions; this requires adding new "area" permissions (it's what I've done in Fix stack access with advanced permissions #7029):
    1. add_subarea to check if a user can create a localized version of a stack)
    2. approve_area_versions to check if a user can approve a stack version
    3. delete_area to check if a user can delete a (localized) stack
    4. delete_area_versions to check if a user can delete a stack version
    5. view_area_versions to check if a user can view other stack versions
    6. move_or_copy_area to check if a user can duplicate a stack
    7. edit_area_properties to check if a user can rename a stack
  2. use only "page" permissions (like what seems to me you are suggesting): this requires adding new "page" permissions:
    1. add_stack_to_area
    2. add_block
  3. use both "page" and "area" permissions; this would require changes to let administrators specify at the same time the "page" permissions and the "area" permissions (which I guess would be quite a messy UI)

Just tell me the solution you prefer, and I'd implement it (I need it asap).

@aembler
Copy link
Member

aembler commented Sep 20, 2018

The tricky thing is that all the permissions we need currently exist – they are just split between pages and areas.

I wonder if we shouldn't just bite the bullet and create a new Stack permission response, with stack permission keys.

They would inherit their permissions from the page object by default, but if you overrode them on the stack you'd be able to determine who could edit the stack, and add the particular block types to them.

Let's brainstorm what permissions we would need, and what those keys would be named. They would all be stack-specific.

@mlocati
Copy link
Contributor Author

mlocati commented Sep 21, 2018

I just noticed another major issue (I just discovered it, the users didn't tell me that), strictly related to this.

Let's assume we have these advanced permissions set:
immagine

If I login as an editor, and edit a page (not a stack), the user is not able to add new blocks (the "Add blocks" dialog is empty):
immagine

That's because this line always returns false.

@mlocati
Copy link
Contributor Author

mlocati commented Sep 21, 2018

I'm going to close this issue, and reopen a new one with a more appropriate description.

@mlocati mlocati closed this as completed Sep 21, 2018
@mlocati
Copy link
Contributor Author

mlocati commented Sep 21, 2018

See #7086

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

Successfully merging a pull request may close this issue.

3 participants