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

Consider reworking default access rules for editing pages. #3014

Open
2 tasks done
amolswnz opened this issue Oct 14, 2024 · 6 comments
Open
2 tasks done

Consider reworking default access rules for editing pages. #3014

amolswnz opened this issue Oct 14, 2024 · 6 comments

Comments

@amolswnz
Copy link
Contributor

Module version(s) affected

5.2.0

Description

Users with "View any page" can publish, unpublish, archive, edit any page.

How to reproduce

  1. Create a Group
  2. In Permissions tab select "Access to 'Pages' section"
  3. Select "View any page"
  4. Create a user in this group
  5. Login with the newly created user
  6. User is able to do all functions on pages which have same permissions as "Edit any page"

Possible Solution

This code seems to be the culprit in SiteTree.php

public function canEdit($member = null)
{
    ...
    // Check inherited permissions
    return static::getPermissionChecker()
        ->canEdit($this->ID, $member);

Additional Context

No response

Validations

  • Check that there isn't already an issue that reports the same bug
  • Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)
@andrewandante
Copy link
Contributor

andrewandante commented Oct 16, 2024

I have tested and reproduced - you don't even need the View any page permission, you only need the Access to 'Pages' section permission and you have full edit rights on the pages.

Only having the View any page permission does correctly prevent your access though.

Edit: have just tested with files, the problem is not present there.

@andrewandante
Copy link
Contributor

andrewandante commented Oct 16, 2024

Tracing this through, I think the default permission set is to allow anyone you can log-in to the CMS to be able to edit pages on the site:

image

So falling through the canEdit() checks, it falls back to:

  return static::getPermissionChecker()
        ->canEdit($this->ID, $member);

which then eventually falls back to:

SiteConfig::current_site_config()->canEditPages($member);

which goes to Logged-In Users as the default type, set here: https://github.com/silverstripe/silverstripe-siteconfig/blob/5/code/SiteConfig.php#L49

class SiteConfig extends DataObject implements PermissionProvider, TemplateGlobalProvider
{
    private static $db = [
        "Title" => "Varchar(255)",
        "Tagline" => "Varchar(255)",
        "CanViewType" => "Enum('Anyone, LoggedInUsers, OnlyTheseUsers, OnlyTheseMembers', 'Anyone')",
        "CanEditType" => "Enum('LoggedInUsers, OnlyTheseUsers, OnlyTheseMembers', 'LoggedInUsers')",
        "CanCreateTopLevelType" => "Enum('LoggedInUsers, OnlyTheseUsers, OnlyTheseMembers', 'LoggedInUsers')",
    ];

So - this is working as intended, it seems - though the implication is that VIEW ANY PAGE without explicit EDIT ANY PAGE permission is that they should not be able to edit, where in fact, by default, they can. In essence, the VIEW ANY PAGE permission doesn't do anything, and in fact disguises the true access a user has, by default. This is the Bad Thing™️

In my mind there are a few options here:

a) change the default CanEditType to a group, set it to content-authors
b) explicitly check against users who have only the VIEW ANY PAGE permission and deny them
c) remove the SiteConfig thing entirely

There are probably others, but that's just off the top of my head.

In the meantime, the workaround is to change that top-level setting so that it's not inherited down.

@GuySartorelli
Copy link
Member

Thank you for the investigation @andrewandante! As you noted, this is working as currently intended - though it's not the first time someone has reported this, so it's clearly a source of confusion and perhaps it's time we think about changing how this works.

@amolswnz If this wasn't working as intended, it would have been a security problem and should have been reported through the security process - please read that so you know what to do next time.

@GuySartorelli
Copy link
Member

For now I'm going to change the title of this so that anyone who stumbles across it doesn't freak out.

@GuySartorelli GuySartorelli changed the title Bug: Users with "View any page" have all the permissions same as "Edit any page" Consider reworking default access rules for editing pages. Oct 16, 2024
@andrewandante
Copy link
Contributor

I wonder if there's a way to rethink how permissions are represented on a page.

Even with the "inherited from above" permission, it's not that obvious actually who has permissions on that page.

Would be cool to see some sort of icon, or descriptive line, that tells the users exactly what the "computed" access is for the page. Something along the lines of:

  • "This page is only viewable and editable by administrators"
  • "This page is editable by anyone with CMS access (inherited from root)"
  • "This page is only editable by select users (configured on this page) and viewable by anyone with CMS access (inherited from root)"

I'm pretty sure that's hard to surface though 😅

@amolswnz
Copy link
Contributor Author

SiteConfig "Who can edit pages on this site?" is a double up on permissions. If we select "Only these groups", it lists "Groups with global edit permissions" which is saying that all groups with "Edit" permissions can edit pages on site 🔁

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