Role Permissions are not respected in Child Groups

[baukezwaan]

Affected Version

phpunit/phpunit            5.7.27             The PHP Unit Testing framework.
silverstripe-themes/simple dev-master 4d546a4 The SilverStripe simple theme (default SilverStripe 3 theme)
silverstripe/recipe-cms    1.1.0              SilverStripe recipe for fully featured page and asset content editing
silverstripe/recipe-plugin 1.2.0              Helper plugin to install SilverStripe recipes
silverstripe/subsites      2.0.1              Run multiple sites from a single SilverStripe install.

Tested on SilverStripe 4.0.3 and SilverStripe 4.1

Description

We tried to use roles in combination with sub-groups, but we notices these permissions are displayed correctly on the users panel in the Security tab. But are not giving the expected result.
More specifically: if we make a role 'Edit Pages' with the permission 'Access to Pages section' and set that role to the group 'Content Editors', and create a sub-group 'Content Editors Company A' under 'Content Editors'. The users in that group cannot access the pages section...

Steps to Reproduce

1. Create Role and set some Permissions
2. Add role to group, eg. 'Content Editors'
3. Create sub-group under 'Content Editors', eg. 'Content Editors Company A'
4. Add user to subgroup and try to login (you can't).

[robbieaverill]

Baseline: SilverStripe 4.2.x-dev

First step - try and reproduce without subsites:

• Create Edit Pages role with permission to edit pages
• Assign role to Content Authors group
• Create sub-group of Content Authors: Content Authors Company A
• Create new member and assign it to Content Authors Company A
• Check permissions tab indicates that edit pages permission is inherited from the Edit Pages role (on parent group)
• Log out and back in as new member in Company A
• Access to pages section ✅

Try and reproduce with subsites 2.1.x-dev:

• Create Edit Pages role with permission to edit pages
• Assign role to Content Authors group
• Create sub-group of Content Authors: Content Authors Company A
• Create new member and assign it to Content Authors Company A
• Check permissions tab indicates that edit pages permission is inherited from the Edit Pages role (on parent group)
• Log out and back in as new member in Company A
• Access to pages section 🔴

Reproduced

[robbieaverill]

Ok the problem is that LeftAndMainSubsites::canAccess returns false

[robbieaverill]

This is going to be nasty because of how much SQL the existing subsites code is generating to perform these checks. I'll summarise my thinking around how permissions should be working before I change anything:

Subsites should be an extension layer over the top of the ACL system we have for admin. It's currently being a bit too intrusive which is what's causing this bug.

We should delegate most of the actual permission checks to core while using subsite state modification to set the subsite context for which to check the member's permissions.

The primary concern of this permission check logic should be whether or not a user can log into the admin area for a given subsite. Individual permission checking should be done using core checks in the context of the current subsite:

The rest of the permission checks in this area should come after this precursor config i.e. "we validate that the user can access this subsite generally, so let's continue with 'can edit page', 'can view files' etc checks in this subsite now."

Groups are not specific to subsites, they're global. There are two situations:

1. the group has been given explicit access to all subsites via AccessAllSubsites boolean (default: true), or
2. the group has been given a specific list of groups that it can access.

Check logic should be:

1. If no subsites exist at all, delegate to core (do not interject at all*), otherwise;
2. Subsites exist, so check in the current subsite context:

• if any of the current user's groups have permission to all subsites via AccessAllSubsites, return true - "yes, this user can access this subsite" - this doesn't necessarily mean they can access the individual admin controller though and this still needs to be checked
• if any of the current user's groups (or group role) have permission to access the current admin controller, return true, otherwise return false

@ScopeyNZ @raissanorth would you mind sanity checking my thinking on this before I go further into it?

* Note that in the context where I said "do not interject at all" this might mean returning a list of "accessible subsites" that includes the "main site" which is not a physical Subsite object but represents the default site.

[ScopeyNZ]

I'm not that familiar with subsites - but wouldn't the logic be:

Subsites exist ---[YES]--> User has a role (through a group) 
						   that can access all subsites?
                          /                        \
                       [YES]                       [NO]
                       /                              \
                Delegate to core           User is allowed for this subsite?
                                             /                        \
                                          [YES]                       [NO]
                                          /								 \
									Delegate to core				Return false

In other words - keep it as simple as possible. Return false if the group is not in the "site whitelist" - delegate in all other cases and let other permissions be checked. Returning true should be avoided.

I might be misunderstanding.

I also really wanted to make a flow chart.

[robbieaverill]

Correct, we would only be returning null to delegate or false if we know for sure that the group doesn't have access to the current subsite

[robbieaverill]

Ok I've made a PR at #388 which I think fixes this issue

[robbieaverill]

Sorry, I've made a mistake with this. I need to reimplement some of the logic that was there.

Here's my thinking of how permission checks should work for subsites:

Get all available subsites

Method: Subsite::accessible_sites()
Purpose: return a list of subsites that are permissible with the given permission code and member

Logic:

• Loop member's groups
  • Group has AccessAllSubsites = 1, return Permission::checkMember() (allowed to be global), OR;
  • Loop subsites AND;
    • Link exists in Group_Subsites for the group ID and subsite ID, return Permission::checkMember() (allowed in subsite generally, so check specific permission code) (do this recursively for parent groups), OR;
    • No record is found, group is explicitly not allowed access to the subsite and false should be returned for the subsite. Don't bother checking the code.
• Return list of subsites that didn't return false from permission checking

This should keep the handling of group roles in Permission::checkMember() but deny access to groups that have explicitly been denied access to a subsite.

Get all available subsites for the current controller

Method: LeftAndMainSubsites::sectionSites()
Purpose: return a list of subsites that the current controller can be accessed in, given a member

Logic:

• Loop all subsites
• Check whether LeftAndMainSubsites::canAccess($member) returns true in the context of each subsite
• Return the result

Check whether a member can access the controller given the current subsite

Method: LeftAndMainSubsites::canAccess()
Purpose: Check whether the given member is allowed to access the current CMS controller given the current subsite

Logic:

• Re-use Subsite::accessible_sites(), but provide it the base permission code for the current controller (e.g. $instanceClass = get_class($this->owner); return $instanceClass::getRequiredPermissions();)

There's an opportunity to simplify and refactor here.

Return a list of subsites that a user can do ANYTHING in

Method: Subsite::all_accessible_sites
Purpose: Return a list of all subsites that a user can do anything in. For example, they may be able to edit pages in one, assets in another, taxonomy terms in a third, so all three should be returned.

Logic:

• Loop CMS menu items
• Call LeftAndMainSubsites::sectionSites() (which loops subsites and calls canAccess in each)
• Return merged list

This method is correct I think.

---

If we did the above we'd have a chain of each method into the other, while each provides their own benefit.

The original statement from me was: I think what we need to ensure is that subsites deliberately return false for permission checks where the member's group is not allowed in a certain subsite. From then on, a permission check for a certain code can be handled by core.

I need to rest my brain for a bit, but I think there's something about that that isn't working correctly at the moment.

Again, this module seems to be working correctly, but silverstripe/silverstripe-securityreport#38 points out that there's a bug in the public API since this code isn't working (it's returning all subsites, not just the one that the user's group is allowed to access):

$subsites = Subsite::accessible_sites(
    'SITETREE_EDIT_ALL',
    true,
    "Main site",
    $this->owner
);
return implode(', ', $subsites->column('Title'));

Context for above code where it's a bug: $this->owner is a Member that belongs to one Group, and that Group is only allowed to access one Subsite. The result of this call shows all subsites in it.

[robbieaverill]

We've had to revert the original PR because it didn't completely the fix the problem, and it introduced regressions which other modules are relying on.