Revert "FIX CMS permission checks for subsite are now handled in the state context"

This commit is contained in:
Guy Marriott 2018-10-19 12:00:50 +13:00 committed by GitHub
parent 4323db52f0
commit 7cc86199e7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 256 additions and 240 deletions

View File

@ -27,13 +27,25 @@ class SubsiteXHRController extends LeftAndMain
return true;
}
if (Subsite::all_accessible_sites(true, 'Main site', $member)->count() > 0) {
if (Subsite::all_accessible_sites()->count() > 0) {
return true;
}
return false;
}
/**
* Allow access if user allowed into the CMS at all.
*/
public function canAccess()
{
// Allow if any cms access is available
return Permission::check([
'CMS_ACCESS', // Supported by 3.1.14 and up
'CMS_ACCESS_LeftAndMain'
]);
}
public function getResponseNegotiator()
{
$negotiator = parent::getResponseNegotiator();

View File

@ -90,7 +90,7 @@ class GroupSubsites extends DataExtension implements PermissionProvider
// Interface is different if you have the rights to modify subsite group values on
// all subsites
if (isset($subsiteMap[0])) {
$fields->addFieldToTab('Root.Subsites', OptionsetField::create(
$fields->addFieldToTab('Root.Subsites', new OptionsetField(
'AccessAllSubsites',
_t(__CLASS__ . '.ACCESSRADIOTITLE', 'Give this group access to'),
[
@ -100,20 +100,20 @@ class GroupSubsites extends DataExtension implements PermissionProvider
));
unset($subsiteMap[0]);
$fields->addFieldToTab('Root.Subsites', CheckboxSetField::create(
$fields->addFieldToTab('Root.Subsites', new CheckboxSetField(
'Subsites',
'',
$subsiteMap
));
} else {
if (sizeof($subsiteMap) <= 1) {
$fields->addFieldToTab('Root.Subsites', ReadonlyField::create(
$fields->addFieldToTab('Root.Subsites', new ReadonlyField(
'SubsitesHuman',
_t(__CLASS__ . '.ACCESSRADIOTITLE', 'Give this group access to'),
reset($subsiteMap)
));
} else {
$fields->addFieldToTab('Root.Subsites', CheckboxSetField::create(
$fields->addFieldToTab('Root.Subsites', new CheckboxSetField(
'Subsites',
_t(__CLASS__ . '.ACCESSRADIOTITLE', 'Give this group access to'),
$subsiteMap

View File

@ -5,9 +5,9 @@ namespace SilverStripe\Subsites\Extensions;
use SilverStripe\Admin\AdminRootController;
use SilverStripe\Admin\CMSMenu;
use SilverStripe\Admin\LeftAndMainExtension;
use SilverStripe\CMS\Controllers\CMSPageEditController;
use SilverStripe\CMS\Controllers\CMSPagesController;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\CMS\Controllers\CMSPageEditController;
use SilverStripe\Control\Controller;
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Convert;
@ -65,13 +65,14 @@ class LeftAndMainSubsites extends LeftAndMainExtension
*
* @param bool $includeMainSite
* @param string $mainSiteTitle
* @param Member|int|null $member
* @return ArrayList List of Subsite instances
* @param null $member
* @return ArrayList of <a href='psi_element://Subsite'>Subsite</a> instances.
* instances.
*/
public function sectionSites($includeMainSite = true, $mainSiteTitle = 'Main site', $member = null)
{
if ($mainSiteTitle === 'Main site') {
$mainSiteTitle = _t('SilverStripe\\Subsites\\Model\\Subsite.MainSiteTitle', 'Main site');
if ($mainSiteTitle == 'Main site') {
$mainSiteTitle = _t('Subsites.MainSiteTitle', 'Main site');
}
// Rationalise member arguments
@ -85,33 +86,49 @@ class LeftAndMainSubsites extends LeftAndMainExtension
$member = DataObject::get_by_id(Member::class, $member);
}
$accessibleSubsites = ArrayList::create();
$subsites = Subsite::all_sites($includeMainSite, $mainSiteTitle);
foreach ($subsites as $subsite) {
/** @var Subsite $subsite */
$canAccess = SubsiteState::singleton()
->withState(function (SubsiteState $newState) use ($subsite, $member) {
$newState->setSubsiteId($subsite->ID);
return $this->canAccess($member);
});
if ($canAccess === false) {
// Explicitly denied
continue;
// Collect permissions - honour the LeftAndMain::required_permission_codes, current model requires
// us to check if the user satisfies ALL permissions. Code partly copied from LeftAndMain::canView.
$codes = [];
$extraCodes = Config::inst()->get(get_class($this->owner), 'required_permission_codes');
if ($extraCodes !== false) {
if ($extraCodes) {
$codes = array_merge($codes, (array)$extraCodes);
} else {
$codes[] = sprintf('CMS_ACCESS_%s', get_class($this->owner));
}
$accessibleSubsites->push($subsite);
} else {
// Check overriden - all subsites accessible.
return Subsite::all_sites();
}
return $accessibleSubsites;
// Find subsites satisfying all permissions for the Member.
$codesPerSite = [];
$sitesArray = [];
foreach ($codes as $code) {
$sites = Subsite::accessible_sites($code, $includeMainSite, $mainSiteTitle, $member);
foreach ($sites as $site) {
// Build the structure for checking how many codes match.
$codesPerSite[$site->ID][$code] = true;
// Retain Subsite objects for later.
$sitesArray[$site->ID] = $site;
}
}
// Find sites that satisfy all codes conjuncitvely.
$accessibleSites = new ArrayList();
foreach ($codesPerSite as $siteID => $siteCodes) {
if (count($siteCodes) == count($codes)) {
$accessibleSites->push($sitesArray[$siteID]);
}
}
return $accessibleSites;
}
/*
* Returns a list of the subsites accessible to the current user.
* It's enough for any section to be accessible for the section to be included.
*
* @return ArrayList
*/
public function Subsites()
{
@ -121,7 +138,6 @@ class LeftAndMainSubsites extends LeftAndMainExtension
/*
* Generates a list of subsites with the data needed to
* produce a dropdown site switcher
*
* @return ArrayList
*/
@ -199,17 +215,11 @@ class LeftAndMainSubsites extends LeftAndMainExtension
/**
* Check if the current controller is accessible for this user on this subsite.
*
* @param Member|int|null $member Will be added as a concrete param in 3.x
* @return false|null False if a decision was explicitly made to deny access, otherwise null to delegate to core
*/
public function canAccess()
{
// Allow us to accept a Member object passed in as an argument without breaking semver
$passedMember = func_num_args() ? func_get_arg(0) : null;
// Admin can access everything, no point in checking.
$member = $passedMember ?: Security::getCurrentUser();
$member = Security::getCurrentUser();
if ($member
&& (Permission::checkMember($member, 'ADMIN') // 'Full administrative rights'
|| Permission::checkMember($member, 'CMS_ACCESS_LeftAndMain') // 'Access to all CMS sections'
@ -218,56 +228,18 @@ class LeftAndMainSubsites extends LeftAndMainExtension
return true;
}
// Check we have a member
if (!$member) {
return null;
}
// Check that some subsites exist first
if (!Subsite::get()->exists()) {
return null;
}
// Get the current subsite ID
$currentSubsiteId = SubsiteState::singleton()->getSubsiteId();
// Check against the current user's associated groups
$allowedInSubsite = false;
foreach ($member->Groups() as $group) {
// If any of the current user's groups have been given explicit access to the subsite, delegate to core
if ($group->AccessAllSubsites) {
$allowedInSubsite = true;
break;
}
// Check if any of the current user's groups have been given explicit access to the current subsite
$groupSubsiteIds = $group->Subsites()->column('ID');
if (in_array($currentSubsiteId, $groupSubsiteIds)) {
$allowedInSubsite = true;
break;
}
}
// If we know that the user is not allowed in this subsite, explicitly say this
if (!$allowedInSubsite) {
return false;
}
// Delegate to core
return null;
// Check if we have access to current section on the current subsite.
$accessibleSites = $this->owner->sectionSites(true, 'Main site', $member);
return $accessibleSites->count() && $accessibleSites->find('ID', SubsiteState::singleton()->getSubsiteId());
}
/**
* Prevent accessing disallowed resources. This happens after onBeforeInit has executed,
* so all redirections should've already taken place.
*
* @return false|null
*/
public function alternateAccessCheck()
{
if ($this->owner->canAccess() === false) {
return false;
}
return $this->owner->canAccess();
}
/**
@ -359,7 +331,7 @@ class LeftAndMainSubsites extends LeftAndMainExtension
// SECOND, check if we need to change subsites due to lack of permissions.
if ($this->owner->canAccess() === false) {
if (!$this->owner->canAccess()) {
$member = Security::getCurrentUser();
// Current section is not accessible, try at least to stick to the same subsite.

View File

@ -3,7 +3,6 @@
namespace SilverStripe\Subsites\Extensions;
use Page;
use SilverStripe\CMS\Controllers\CMSPagesController;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Control\Controller;
use SilverStripe\Control\Director;
@ -323,11 +322,6 @@ class SiteTreeSubsites extends DataExtension
return null;
}
// Check general subsite section access for CMS
if (CMSPagesController::singleton()->canAccess($member) === false) {
return false;
}
// Find the sites that this user has access to
$goodSites = Subsite::accessible_sites('CMS_ACCESS_CMSMain', true, 'all', $member)->column('ID');

View File

@ -3,7 +3,6 @@
namespace SilverStripe\Subsites\Model;
use SilverStripe\Admin\CMSMenu;
use SilverStripe\Admin\LeftAndMain;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Control\Director;
use SilverStripe\Core\Convert;
@ -29,7 +28,6 @@ use SilverStripe\Security\Group;
use SilverStripe\Security\Member;
use SilverStripe\Security\Permission;
use SilverStripe\Security\Security;
use SilverStripe\Subsites\Extensions\LeftAndMainSubsites;
use SilverStripe\Subsites\State\SubsiteState;
use SilverStripe\Versioned\Versioned;
use UnexpectedValueException;
@ -374,7 +372,7 @@ class Subsite extends DataObject
if ($includeMainSite) {
$subsites = $subsites->toArray();
$mainSite = Subsite::create();
$mainSite = new Subsite();
$mainSite->Title = $mainSiteTitle;
array_unshift($subsites, $mainSite);
@ -384,7 +382,7 @@ class Subsite extends DataObject
return $subsites;
}
/**
/*
* Returns an ArrayList of the subsites accessible to the current user.
* It's enough for any section to be accessible for the site to be included.
*
@ -392,26 +390,32 @@ class Subsite extends DataObject
*/
public static function all_accessible_sites($includeMainSite = true, $mainSiteTitle = 'Main site', $member = null)
{
// Rationalise member arguments
if (!$member) {
$member = Security::getCurrentUser();
}
if (!$member) {
return ArrayList::create();
}
if (!is_object($member)) {
$member = DataObject::get_by_id(Member::class, $member);
}
$subsites = ArrayList::create();
// Collect subsites for all sections.
$menu = CMSMenu::get_viewable_menu_items();
foreach ($menu as $candidate) {
if (!$candidate->controller) {
continue;
if ($candidate->controller) {
$accessibleSites = singleton($candidate->controller)->sectionSites(
$includeMainSite,
$mainSiteTitle,
$member
);
// Replace existing keys so no one site appears twice.
$subsites->merge($accessibleSites);
}
// Determine which subsites can access the current admin section
/** @var LeftAndMain|LeftAndMainSubsites $controller */
$controller = singleton($candidate->controller);
$accessibleSites = $controller->sectionSites(
$includeMainSite,
$mainSiteTitle,
$member
);
// Replace existing keys so no one site appears twice.
$subsites->merge($accessibleSites);
}
$subsites->removeDuplicates();
@ -423,11 +427,11 @@ class Subsite extends DataObject
* Return the subsites that the current user can access by given permission.
* Sites will only be included if they have a Title.
*
* @param array|string $permCode Either a single permission code or an array of permission codes.
* @param bool $includeMainSite If true, the main site will be included if appropriate.
* @param string $mainSiteTitle The label to give to the main site
* @param Member|int $member The member attempting to access the sites
* @return ArrayList List of {@link Subsite} instances
* @param $permCode array|string Either a single permission code or an array of permission codes.
* @param $includeMainSite bool If true, the main site will be included if appropriate.
* @param $mainSiteTitle string The label to give to the main site
* @param $member int|Member The member attempting to access the sites
* @return DataList|ArrayList of {@link Subsite} instances
*/
public static function accessible_sites(
$permCode,
@ -435,42 +439,107 @@ class Subsite extends DataObject
$mainSiteTitle = 'Main site',
$member = null
) {
$memberCacheKey = is_object($member) ? $member->ID : 0;
// Rationalise member arguments
if (!$member) {
$member = Member::currentUser();
}
if (!$member) {
return new ArrayList();
}
if (!is_object($member)) {
$member = DataObject::get_by_id(Member::class, $member);
}
// Rationalise permCode argument
if (is_array($permCode)) {
$SQL_codes = "'" . implode("', '", Convert::raw2sql($permCode)) . "'";
} else {
$SQL_codes = "'" . Convert::raw2sql($permCode) . "'";
}
// Cache handling
$cacheKey = json_encode($permCode) . '-' . $memberCacheKey . '-' . $includeMainSite . '-' . $mainSiteTitle;
$cacheKey = $SQL_codes . '-' . $member->ID . '-' . $includeMainSite . '-' . $mainSiteTitle;
if (isset(self::$cache_accessible_sites[$cacheKey])) {
return self::$cache_accessible_sites[$cacheKey];
}
$accessibleSubsites = ArrayList::create();
$subsites = self::all_sites($includeMainSite, $mainSiteTitle);
foreach ($subsites as $subsite) {
// Exclude subsites with no title
if (empty($subsite->Title)) {
continue;
}
/** @skipUpgrade */
$subsites = DataList::create(Subsite::class)
->where("\"Subsite\".\"Title\" != ''")
->leftJoin('Group_Subsites', '"Group_Subsites"."SubsiteID" = "Subsite"."ID"')
->innerJoin(
'Group',
'"Group"."ID" = "Group_Subsites"."GroupID" OR "Group"."AccessAllSubsites" = 1'
)
->innerJoin(
'Group_Members',
"\"Group_Members\".\"GroupID\"=\"Group\".\"ID\" AND \"Group_Members\".\"MemberID\" = $member->ID"
)
->innerJoin(
'Permission',
"\"Group\".\"ID\"=\"Permission\".\"GroupID\"
AND \"Permission\".\"Code\"
IN ($SQL_codes, 'CMS_ACCESS_LeftAndMain', 'ADMIN')"
);
/** @var Subsite $subsite */
$canAccess = SubsiteState::singleton()
->withState(function (SubsiteState $newState) use ($member, $subsite, $permCode) {
// Mock each individual subsite and run permission checks in it
$newState->setSubsiteId($subsite->ID);
return Permission::checkMember($member, $permCode);
});
if ($canAccess === false) {
// Explicitly denied access
continue;
}
$accessibleSubsites->push($subsite);
if (!$subsites) {
$subsites = new ArrayList();
}
self::$cache_accessible_sites[$cacheKey] = $accessibleSubsites;
/** @var DataList $rolesSubsites */
/** @skipUpgrade */
$rolesSubsites = DataList::create(Subsite::class)
->where("\"Subsite\".\"Title\" != ''")
->leftJoin('Group_Subsites', '"Group_Subsites"."SubsiteID" = "Subsite"."ID"')
->innerJoin(
'Group',
'"Group"."ID" = "Group_Subsites"."GroupID" OR "Group"."AccessAllSubsites" = 1'
)
->innerJoin(
'Group_Members',
"\"Group_Members\".\"GroupID\"=\"Group\".\"ID\" AND \"Group_Members\".\"MemberID\" = $member->ID"
)
->innerJoin('Group_Roles', '"Group_Roles"."GroupID"="Group"."ID"')
->innerJoin('PermissionRole', '"Group_Roles"."PermissionRoleID"="PermissionRole"."ID"')
->innerJoin(
'PermissionRoleCode',
"\"PermissionRole\".\"ID\"=\"PermissionRoleCode\".\"RoleID\"
AND \"PermissionRoleCode\".\"Code\"
IN ($SQL_codes, 'CMS_ACCESS_LeftAndMain', 'ADMIN')"
);
return $accessibleSubsites;
if (!$subsites && $rolesSubsites) {
return $rolesSubsites;
}
$subsites = new ArrayList($subsites->toArray());
if ($rolesSubsites) {
foreach ($rolesSubsites as $subsite) {
if (!$subsites->find('ID', $subsite->ID)) {
$subsites->push($subsite);
}
}
}
if ($includeMainSite) {
if (!is_array($permCode)) {
$permCode = [$permCode];
}
if (self::hasMainSitePermission($member, $permCode)) {
$subsites = $subsites->toArray();
$mainSite = new Subsite();
$mainSite->Title = $mainSiteTitle;
array_unshift($subsites, $mainSite);
$subsites = ArrayList::create($subsites);
}
}
self::$cache_accessible_sites[$cacheKey] = $subsites;
return $subsites;
}
/**

View File

@ -7,10 +7,8 @@ use SilverStripe\AssetAdmin\Controller\AssetAdmin;
use SilverStripe\CMS\Controllers\CMSMain;
use SilverStripe\CMS\Controllers\CMSPageEditController;
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\FunctionalTest;
use SilverStripe\Security\Member;
use SilverStripe\Subsites\Extensions\LeftAndMainSubsites;
use SilverStripe\Subsites\Model\Subsite;
use SilverStripe\Subsites\State\SubsiteState;
@ -33,91 +31,57 @@ class LeftAndMainSubsitesTest extends FunctionalTest
return $obj;
}
/**
* @dataProvider sectionSitesProvider
*
* @param string $identifier
* @param string $className
* @param array $expected
* @param string $message
* @param string $assertion
*/
public function testSectionSites($identifier, $className, $expected, $message, $assertion = 'assertListEquals')
public function testSectionSites()
{
$member = $this->objFromFixture(Member::class, $identifier);
$member = $this->objFromFixture(Member::class, 'subsite1member');
/** @var CMSMain|LeftAndMainSubsites $cmsmain */
$cmsMain = Injector::inst()->create($className);
$subsites = $cmsMain->sectionSites(true, 'Main site', $member);
$this->$assertion($expected, $subsites, $message);
$cmsmain = singleton(CMSMain::class);
$subsites = $cmsmain->sectionSites(true, 'Main site', $member);
$this->assertDOSEquals([
['Title' => 'Subsite1 Template']
], $subsites, 'Lists member-accessible sites for the accessible controller.');
$assetadmin = singleton(AssetAdmin::class);
$subsites = $assetadmin->sectionSites(true, 'Main site', $member);
$this->assertDOSEquals([], $subsites, 'Does not list any sites for forbidden controller.');
$member = $this->objFromFixture(Member::class, 'editor');
$cmsmain = singleton(CMSMain::class);
$subsites = $cmsmain->sectionSites(true, 'Main site', $member);
$this->assertDOSContains([
['Title' => 'Main site']
], $subsites, 'Includes the main site for members who can access all sites.');
}
/**
* @return array[]
*/
public function sectionSitesProvider()
{
return [
[
'subsite1member',
CMSMain::class,
[['Title' => 'Subsite1 Template']],
'Lists member-accessible sites for the accessible controller.',
],
[
'subsite1member',
AssetAdmin::class,
[[]],
'Does not list any sites for forbidden controller.',
],
[
'editor',
CMSMain::class,
[['Title' => 'Main site']],
'Includes the main site for members who can access all sites.',
'assertListContains',
],
];
}
/**
* @dataProvider accessChecksProvider
*
* @param string $identifier
*/
public function testAccessChecksDontChangeCurrentSubsite($identifier)
public function testAccessChecksDontChangeCurrentSubsite()
{
$this->logInAs('admin');
$ids = [];
/** @var Subsite $subsite */
$subsite = $this->objFromFixture(Subsite::class, $identifier);
$id = $subsite->ID;
$subsite1 = $this->objFromFixture(Subsite::class, 'domaintest1');
$subsite2 = $this->objFromFixture(Subsite::class, 'domaintest2');
$subsite3 = $this->objFromFixture(Subsite::class, 'domaintest3');
$ids[] = $subsite1->ID;
$ids[] = $subsite2->ID;
$ids[] = $subsite3->ID;
$ids[] = 0;
// Enable session-based subsite tracking.
SubsiteState::singleton()->setUseSessions(true);
Subsite::changeSubsite($id);
$this->assertEquals($id, SubsiteState::singleton()->getSubsiteId(), 'Subsite ID is in the state');
foreach ($ids as $id) {
Subsite::changeSubsite($id);
$this->assertEquals($id, SubsiteState::singleton()->getSubsiteId());
$left = new LeftAndMain();
$this->assertTrue($left->canView(), "Admin user can view subsites LeftAndMain with id = '$id'");
$this->assertEquals(
$id,
SubsiteState::singleton()->getSubsiteId(),
'The current subsite has not been changed in the process of checking permissions for admin user.'
);
}
/**
* @return array[]
*/
public function accessChecksProvider()
{
return [
['domaintest1'],
['domaintest3'],
['domaintest3'],
];
$left = new LeftAndMain();
$this->assertTrue($left->canView(), "Admin user can view subsites LeftAndMain with id = '$id'");
$this->assertEquals(
$id,
SubsiteState::singleton()->getSubsiteId(),
'The current subsite has not been changed in the process of checking permissions for admin user.'
);
}
}
public function testShouldChangeSubsite()

View File

@ -112,41 +112,46 @@ class SiteTreeSubsitesTest extends BaseSubsiteTest
$this->assertEquals($expected_path, $path);
}
/**
* @dataProvider canEditProvider
*
* @param string $memberIdentifier
* @param string $subsiteIdentifier
* @param string $pageIdentifier
* @param bool $expected
* @param string $message
*/
public function testCanEditSiteTree($memberIdentifier, $subsiteIdentifier, $pageIdentifier, $expected, $message)
public function testCanEditSiteTree()
{
$this->logInAs($memberIdentifier);
$admin = $this->objFromFixture(Member::class, 'admin');
$subsite1member = $this->objFromFixture(Member::class, 'subsite1member');
$subsite2member = $this->objFromFixture(Member::class, 'subsite2member');
$mainpage = $this->objFromFixture('Page', 'home');
$subsite1page = $this->objFromFixture('Page', 'subsite1_home');
$subsite2page = $this->objFromFixture('Page', 'subsite2_home');
$subsite1 = $this->objFromFixture(Subsite::class, 'subsite1');
$subsite2 = $this->objFromFixture(Subsite::class, 'subsite2');
/** @var Page $page */
$page = $this->objFromFixture(Page::class, $pageIdentifier);
// Cant pass member as arguments to canEdit() because of GroupSubsites
$this->logInAs($admin);
if (is_string($subsiteIdentifier)) {
$subsiteIdentifier = $this->idFromFixture(Subsite::class, $subsiteIdentifier);
}
Subsite::changeSubsite($subsiteIdentifier);
$this->assertTrue(
(bool)$subsite1page->canEdit(),
'Administrators can edit all subsites'
);
$this->assertSame($expected, (bool) $page->canEdit(), $message);
}
// @todo: Workaround because GroupSubsites->augmentSQL() is relying on session state
Subsite::changeSubsite($subsite1);
/**
* @return array[]
*/
public function canEditProvider()
{
return [
['admin', 0, 'subsite1_home', true, 'Administrators can edit all subsites'],
['subsite1member', 'subsite1', 'subsite1_home', true, 'Can edit on subsite if group belongs to subsite'],
['subsite2member', 'subsite1', 'subsite1_home', false, 'Cannot edit on subsite if group doesn\'t belong'],
['subsite2member', 0, 'home', false, 'Cannot edit pages on main site if not in correct group'],
];
$this->logInAs($subsite1member->ID);
$this->assertTrue(
(bool)$subsite1page->canEdit(),
'Members can edit pages on a subsite if they are in a group belonging to this subsite'
);
$this->logInAs($subsite2member->ID);
$this->assertFalse(
(bool)$subsite1page->canEdit(),
'Members cant edit pages on a subsite if they are not in a group belonging to this subsite'
);
// @todo: Workaround because GroupSubsites->augmentSQL() is relying on session state
Subsite::changeSubsite(0);
$this->assertFalse(
$mainpage->canEdit(),
'Members cant edit pages on the main site if they are not in a group allowing this'
);
}
/**