From 7f28c32427ca5364ebfbe6a0cf347bb33f7debed Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 24 Aug 2018 10:12:05 +1200 Subject: [PATCH 01/10] FIX Pages now correctly duplicate children across subsites --- src/Extensions/SiteTreeSubsites.php | 86 +++++++++++++++++------------ tests/php/SiteTreeSubsitesTest.php | 43 ++++++++++----- 2 files changed, 78 insertions(+), 51 deletions(-) diff --git a/src/Extensions/SiteTreeSubsites.php b/src/Extensions/SiteTreeSubsites.php index 488fef6..6c7e55f 100644 --- a/src/Extensions/SiteTreeSubsites.php +++ b/src/Extensions/SiteTreeSubsites.php @@ -2,7 +2,6 @@ namespace SilverStripe\Subsites\Extensions; -use Page; use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; @@ -170,57 +169,71 @@ class SiteTreeSubsites extends DataExtension } /** - * Does the basic duplication, but doesn't write anything - * this means we can subclass this easier and do more complex - * relation duplication. + * Does the basic duplication, but doesn't write anything this means we can subclass this easier and do more + * complex relation duplication. + * + * Note that when duplicating including children, everything is written. + * + * @param Subsite|int $subsiteID + * @param bool $includeChildren + * @return SiteTree */ - public function duplicateToSubsitePrep($subsiteID) + public function duplicateToSubsitePrep($subsiteID, $includeChildren) { if (is_object($subsiteID)) { $subsiteID = $subsiteID->ID; } - $oldSubsite = SubsiteState::singleton()->getSubsiteId(); - if ($subsiteID) { - Subsite::changeSubsite($subsiteID); - } else { - $subsiteID = $oldSubsite; + return SubsiteState::singleton() + ->withState(function (SubsiteState $newState) use ($subsiteID, $includeChildren) { + $newState->setSubsiteId($subsiteID); + + /** @var SiteTree $page */ + $page = $this->owner; + + try { + $originalFilter = Subsite::$disable_subsite_filter; + Subsite::disable_subsite_filter(true); + + return $includeChildren ? $page->duplicateWithChildren() : $page->duplicate(false); + } finally { + Subsite::disable_subsite_filter($originalFilter); + } + }); + } + + /** + * When duplicating a page, assign the current subsite ID from the state + */ + public function onBeforeDuplicate() + { + $subsiteId = SubsiteState::singleton()->getSubsiteId(); + if ($subsiteId !== null) { + $this->owner->SubsiteID = $subsiteId; } - // doesn't write as we need to reset the SubsiteID, ParentID etc - $clone = $this->owner->duplicate(false); - $clone->CheckedPublicationDifferences = $clone->AddedToStage = true; - $subsiteID = ($subsiteID ? $subsiteID : $oldSubsite); - $clone->SubsiteID = $subsiteID; - // We have no idea what the parentID should be, so as a workaround use the url-segment and subsite ID - if ($this->owner->Parent()) { - $parentSeg = $this->owner->Parent()->URLSegment; - $newParentPage = Page::get()->filter('URLSegment', $parentSeg)->first(); - if ($newParentPage) { - $clone->ParentID = $newParentPage->ID; - } else { - // reset it to the top level, so the user can decide where to put it - $clone->ParentID = 0; - } - } - // MasterPageID is here for legacy purposes, to satisfy the subsites_relatedpages module - $clone->MasterPageID = $this->owner->ID; - return $clone; } /** * Create a duplicate of this page and save it to another subsite - * @param $subsiteID int|Subsite The Subsite to copy to, or its ID + * + * @param Subsite|int $subsiteID The Subsite to copy to, or its ID + * @param boolean $includeChildren Whether to duplicate child pages too + * @return SiteTree The duplicated page */ - public function duplicateToSubsite($subsiteID = null) + public function duplicateToSubsite($subsiteID = null, $includeChildren = false) { - $clone = $this->owner->duplicateToSubsitePrep($subsiteID); + $clone = $this->owner->duplicateToSubsitePrep($subsiteID, $includeChildren); $clone->invokeWithExtensions('onBeforeDuplicateToSubsite', $this->owner); - $clone->write(); + + if (!$includeChildren) { + // Write the new page if "include children" is false, because it is written by default when it's true. + $clone->write(); + } + // Deprecated: manually duplicate any configured relationships $clone->duplicateSubsiteRelations($this->owner); - // new extension hooks which happens after write, - // onAfterDuplicate isn't reliable due to - // https://github.com/silverstripe/silverstripe-cms/issues/1253 + $clone->invokeWithExtensions('onAfterDuplicateToSubsite', $this->owner); + return $clone; } @@ -231,6 +244,7 @@ class SiteTreeSubsites extends DataExtension * It may be that some relations are not diostinct to sub site so can stay * whereas others may need to be duplicated * + * @param SiteTree $originalPage */ public function duplicateSubsiteRelations($originalPage) { diff --git a/tests/php/SiteTreeSubsitesTest.php b/tests/php/SiteTreeSubsitesTest.php index 17131ae..efb16f8 100644 --- a/tests/php/SiteTreeSubsitesTest.php +++ b/tests/php/SiteTreeSubsitesTest.php @@ -359,28 +359,41 @@ class SiteTreeSubsitesTest extends BaseSubsiteTest $this->assertEquals('important-page', $mainSubsiteImportantPage->URLSegment); } - public function testCopySubsiteWithChildren() + /** + * @param bool $withChildren + * @param int $expectedChildren + * @dataProvider duplicateToSubsiteProvider + */ + public function testDuplicateToSubsite($withChildren, $expectedChildren) { - $page = $this->objFromFixture('Page', 'about'); + /** @var SiteTree $page */ + $page = $this->objFromFixture(Page::class, 'about'); + /** @var Subsite $newSubsite */ $newSubsite = $this->objFromFixture(Subsite::class, 'subsite1'); - $moved = $page->duplicateToSubsite($newSubsite->ID, true); - $this->assertEquals($moved->SubsiteID, $newSubsite->ID, 'Ensure returned records are on new subsite'); - $this->assertEquals( - $moved->AllChildren()->count(), - $page->AllChildren()->count(), - 'All pages are copied across' + /** @var SiteTree $duplicatedPage */ + $duplicatedPage = $page->duplicateToSubsite($newSubsite->ID, $withChildren); + $this->assertInstanceOf(SiteTree::class, $duplicatedPage, 'A new page is returned'); + + $this->assertEquals($newSubsite->ID, $duplicatedPage->SubsiteID, 'Ensure returned records are on new subsite'); + + $this->assertCount(1, $page->AllChildren()); + $this->assertCount( + $expectedChildren, + $duplicatedPage->AllChildren(), + 'Duplicated page also duplicates children' ); } - public function testCopySubsiteWithoutChildren() + /** + * @return array[] + */ + public function duplicateToSubsiteProvider() { - $page = $this->objFromFixture('Page', 'about'); - $newSubsite = $this->objFromFixture(Subsite::class, 'subsite2'); - - $moved = $page->duplicateToSubsite($newSubsite->ID, false); - $this->assertEquals($moved->SubsiteID, $newSubsite->ID, 'Ensure returned records are on new subsite'); - $this->assertEquals($moved->AllChildren()->count(), 0, 'All pages are copied across'); + return [ + [true, 1], + [false, 0], + ]; } public function testIfSubsiteThemeIsSetToThemeList() From e8a72e1c3341a90b9a3b440a47ef672611970aa4 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 24 Aug 2018 10:30:27 +1200 Subject: [PATCH 02/10] FIX Duplicate page's parent IDs are now assumed or zeroed after duplication --- src/Extensions/SiteTreeSubsites.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/Extensions/SiteTreeSubsites.php b/src/Extensions/SiteTreeSubsites.php index 6c7e55f..ecef3a5 100644 --- a/src/Extensions/SiteTreeSubsites.php +++ b/src/Extensions/SiteTreeSubsites.php @@ -2,6 +2,7 @@ namespace SilverStripe\Subsites\Extensions; +use Page; use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; @@ -192,12 +193,30 @@ class SiteTreeSubsites extends DataExtension $page = $this->owner; try { + // We have no idea what the ParentID should be, but it shouldn't be the same as it was since + // we're now in a different subsite. As a workaround use the url-segment and subsite ID. + if ($page->Parent()) { + $parentSeg = $page->Parent()->URLSegment; + $newParentPage = Page::get()->filter('URLSegment', $parentSeg)->first(); + $originalParentID = $page->ParentID; + if ($newParentPage) { + $page->ParentID = (int) $newParentPage->ID; + } else { + // reset it to the top level, so the user can decide where to put it + $page->ParentID = 0; + } + } + + // Disable query filtering by subsite during actual duplication $originalFilter = Subsite::$disable_subsite_filter; Subsite::disable_subsite_filter(true); return $includeChildren ? $page->duplicateWithChildren() : $page->duplicate(false); } finally { Subsite::disable_subsite_filter($originalFilter); + + // Re-set the original parent ID for the current page + $page->ParentID = $originalParentID; } }); } From 6af985420f896063694fc4f5aacfcc3c08c8a3a9 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 24 Aug 2018 16:58:34 +1200 Subject: [PATCH 03/10] FIX CMS permission checks for subsite are now handled in the state context We now check the subsite state for the context and validate it against the current member's group permissions using the SilverStripe ORM relationships instead of using SQL queries. More granular permission checks e.g. canView etc are still up to data models to define and handle. --- src/Controller/SubsiteXHRController.php | 14 +- src/Extensions/GroupSubsites.php | 8 +- src/Extensions/LeftAndMainSubsites.php | 120 ++++++++++------- src/Extensions/SiteTreeSubsites.php | 6 + src/Model/Subsite.php | 167 +++++++----------------- tests/php/LeftAndMainSubsitesTest.php | 114 ++++++++++------ tests/php/SiteTreeSubsitesTest.php | 65 +++++---- 7 files changed, 241 insertions(+), 253 deletions(-) diff --git a/src/Controller/SubsiteXHRController.php b/src/Controller/SubsiteXHRController.php index c758d53..9016f6b 100644 --- a/src/Controller/SubsiteXHRController.php +++ b/src/Controller/SubsiteXHRController.php @@ -27,25 +27,13 @@ class SubsiteXHRController extends LeftAndMain return true; } - if (Subsite::all_accessible_sites()->count() > 0) { + if (Subsite::all_accessible_sites(true, 'Main site', $member)->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(); diff --git a/src/Extensions/GroupSubsites.php b/src/Extensions/GroupSubsites.php index 2e583f1..4df1521 100644 --- a/src/Extensions/GroupSubsites.php +++ b/src/Extensions/GroupSubsites.php @@ -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', new OptionsetField( + $fields->addFieldToTab('Root.Subsites', OptionsetField::create( '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', new CheckboxSetField( + $fields->addFieldToTab('Root.Subsites', CheckboxSetField::create( 'Subsites', '', $subsiteMap )); } else { if (sizeof($subsiteMap) <= 1) { - $fields->addFieldToTab('Root.Subsites', new ReadonlyField( + $fields->addFieldToTab('Root.Subsites', ReadonlyField::create( 'SubsitesHuman', _t(__CLASS__ . '.ACCESSRADIOTITLE', 'Give this group access to'), reset($subsiteMap) )); } else { - $fields->addFieldToTab('Root.Subsites', new CheckboxSetField( + $fields->addFieldToTab('Root.Subsites', CheckboxSetField::create( 'Subsites', _t(__CLASS__ . '.ACCESSRADIOTITLE', 'Give this group access to'), $subsiteMap diff --git a/src/Extensions/LeftAndMainSubsites.php b/src/Extensions/LeftAndMainSubsites.php index 760623f..a4facee 100644 --- a/src/Extensions/LeftAndMainSubsites.php +++ b/src/Extensions/LeftAndMainSubsites.php @@ -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,14 +65,13 @@ class LeftAndMainSubsites extends LeftAndMainExtension * * @param bool $includeMainSite * @param string $mainSiteTitle - * @param null $member - * @return ArrayList of Subsite instances. - * instances. + * @param Member|int|null $member + * @return ArrayList List of Subsite instances */ public function sectionSites($includeMainSite = true, $mainSiteTitle = 'Main site', $member = null) { - if ($mainSiteTitle == 'Main site') { - $mainSiteTitle = _t('Subsites.MainSiteTitle', 'Main site'); + if ($mainSiteTitle === 'Main site') { + $mainSiteTitle = _t('SilverStripe\\Subsites\\Model\\Subsite.MainSiteTitle', 'Main site'); } // Rationalise member arguments @@ -86,49 +85,38 @@ class LeftAndMainSubsites extends LeftAndMainExtension $member = DataObject::get_by_id(Member::class, $member); } - // 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)); - } - } else { - // Check overriden - all subsites accessible. - return Subsite::all_sites(); + $accessibleSubsites = ArrayList::create(); + $subsites = Subsite::all_sites($includeMainSite, $mainSiteTitle); + + // Check whether we have any subsites + if (!$subsites->exists()) { + 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; + foreach ($subsites as $subsite) { + /** @var Subsite $subsite */ + $canAccess = SubsiteState::singleton() + ->withState(function (SubsiteState $newState) use ($subsite, $member) { + $newState->setSubsiteId($subsite->ID); - // Retain Subsite objects for later. - $sitesArray[$site->ID] = $site; + return $this->canAccess($member); + }); + + if ($canAccess === false) { + // Explicitly denied + continue; } + $accessibleSubsites->push($subsite); } - // 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; + return $accessibleSubsites; } /* * 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() { @@ -138,6 +126,7 @@ class LeftAndMainSubsites extends LeftAndMainExtension /* * Generates a list of subsites with the data needed to * produce a dropdown site switcher + * * @return ArrayList */ @@ -215,11 +204,17 @@ 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_get_args() ? func_get_arg(0) : null; + // Admin can access everything, no point in checking. - $member = Security::getCurrentUser(); + $member = $passedMember ?: Security::getCurrentUser(); if ($member && (Permission::checkMember($member, 'ADMIN') // 'Full administrative rights' || Permission::checkMember($member, 'CMS_ACCESS_LeftAndMain') // 'Access to all CMS sections' @@ -228,18 +223,55 @@ class LeftAndMainSubsites extends LeftAndMainExtension return true; } - // 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()); + // 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; + } + } + + // If we know that the user is not allowed in this subsite, explicitly say this + if (!$allowedInSubsite) { + return false; + } + + // Delegate to core + return null; } /** * Prevent accessing disallowed resources. This happens after onBeforeInit has executed, * so all redirections should've already taken place. + * + * @return false|null */ public function alternateAccessCheck() { - return $this->owner->canAccess(); + if ($this->owner->canAccess() === false) { + return false; + } } /** @@ -331,7 +363,7 @@ class LeftAndMainSubsites extends LeftAndMainExtension // SECOND, check if we need to change subsites due to lack of permissions. - if (!$this->owner->canAccess()) { + if ($this->owner->canAccess() === false) { $member = Security::getCurrentUser(); // Current section is not accessible, try at least to stick to the same subsite. diff --git a/src/Extensions/SiteTreeSubsites.php b/src/Extensions/SiteTreeSubsites.php index ecef3a5..b1f3f74 100644 --- a/src/Extensions/SiteTreeSubsites.php +++ b/src/Extensions/SiteTreeSubsites.php @@ -3,6 +3,7 @@ namespace SilverStripe\Subsites\Extensions; use Page; +use SilverStripe\CMS\Controllers\CMSPagesController; use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; @@ -322,6 +323,11 @@ 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'); diff --git a/src/Model/Subsite.php b/src/Model/Subsite.php index 229604d..704e23d 100644 --- a/src/Model/Subsite.php +++ b/src/Model/Subsite.php @@ -3,6 +3,7 @@ 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; @@ -28,6 +29,7 @@ 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; @@ -372,7 +374,7 @@ class Subsite extends DataObject if ($includeMainSite) { $subsites = $subsites->toArray(); - $mainSite = new Subsite(); + $mainSite = Subsite::create(); $mainSite->Title = $mainSiteTitle; array_unshift($subsites, $mainSite); @@ -382,7 +384,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. * @@ -390,32 +392,26 @@ 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) { - $accessibleSites = singleton($candidate->controller)->sectionSites( - $includeMainSite, - $mainSiteTitle, - $member - ); - - // Replace existing keys so no one site appears twice. - $subsites->merge($accessibleSites); + if (!$candidate->controller) { + continue; } + + // 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(); @@ -427,11 +423,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 $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 + * @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 */ public static function accessible_sites( $permCode, @@ -439,107 +435,42 @@ class Subsite extends DataObject $mainSiteTitle = 'Main site', $member = null ) { - - // 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) . "'"; - } + $memberCacheKey = is_object($member) ? $member->ID : 0; // Cache handling - $cacheKey = $SQL_codes . '-' . $member->ID . '-' . $includeMainSite . '-' . $mainSiteTitle; + $cacheKey = json_encode($permCode) . '-' . $memberCacheKey . '-' . $includeMainSite . '-' . $mainSiteTitle; if (isset(self::$cache_accessible_sites[$cacheKey])) { return self::$cache_accessible_sites[$cacheKey]; } - /** @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')" - ); - - if (!$subsites) { - $subsites = new ArrayList(); - } - - /** @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')" - ); - - 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); - } + $accessibleSubsites = ArrayList::create(); + $subsites = self::all_sites($includeMainSite, $mainSiteTitle); + foreach ($subsites as $subsite) { + // Exclude subsites with no title + if (empty($subsite->Title)) { + continue; } + + /** @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 ($includeMainSite) { - if (!is_array($permCode)) { - $permCode = [$permCode]; - } - if (self::hasMainSitePermission($member, $permCode)) { - $subsites = $subsites->toArray(); + self::$cache_accessible_sites[$cacheKey] = $accessibleSubsites; - $mainSite = new Subsite(); - $mainSite->Title = $mainSiteTitle; - array_unshift($subsites, $mainSite); - $subsites = ArrayList::create($subsites); - } - } - - self::$cache_accessible_sites[$cacheKey] = $subsites; - - return $subsites; + return $accessibleSubsites; } /** diff --git a/tests/php/LeftAndMainSubsitesTest.php b/tests/php/LeftAndMainSubsitesTest.php index 09df18f..5be30e9 100644 --- a/tests/php/LeftAndMainSubsitesTest.php +++ b/tests/php/LeftAndMainSubsitesTest.php @@ -7,8 +7,10 @@ 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; @@ -31,57 +33,91 @@ class LeftAndMainSubsitesTest extends FunctionalTest return $obj; } - public function testSectionSites() + /** + * @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') { - $member = $this->objFromFixture(Member::class, 'subsite1member'); + $member = $this->objFromFixture(Member::class, $identifier); - $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.'); + /** @var CMSMain|LeftAndMainSubsites $cmsmain */ + $cmsMain = Injector::inst()->create($className); + $subsites = $cmsMain->sectionSites(true, 'Main site', $member); + $this->$assertion($expected, $subsites, $message); } - public function testAccessChecksDontChangeCurrentSubsite() + /** + * @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) { $this->logInAs('admin'); - $ids = []; - $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; + /** @var Subsite $subsite */ + $subsite = $this->objFromFixture(Subsite::class, $identifier); + $id = $subsite->ID; // Enable session-based subsite tracking. SubsiteState::singleton()->setUseSessions(true); - foreach ($ids as $id) { - Subsite::changeSubsite($id); - $this->assertEquals($id, SubsiteState::singleton()->getSubsiteId()); + Subsite::changeSubsite($id); + $this->assertEquals($id, SubsiteState::singleton()->getSubsiteId(), 'Subsite ID is in the state'); - $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.' - ); - } + $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'], + ]; } public function testShouldChangeSubsite() diff --git a/tests/php/SiteTreeSubsitesTest.php b/tests/php/SiteTreeSubsitesTest.php index efb16f8..6efe3e7 100644 --- a/tests/php/SiteTreeSubsitesTest.php +++ b/tests/php/SiteTreeSubsitesTest.php @@ -112,46 +112,41 @@ class SiteTreeSubsitesTest extends BaseSubsiteTest $this->assertEquals($expected_path, $path); } - public function testCanEditSiteTree() + /** + * @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) { - $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'); + $this->logInAs($memberIdentifier); - // Cant pass member as arguments to canEdit() because of GroupSubsites - $this->logInAs($admin); + /** @var Page $page */ + $page = $this->objFromFixture(Page::class, $pageIdentifier); - $this->assertTrue( - (bool)$subsite1page->canEdit(), - 'Administrators can edit all subsites' - ); + if (is_string($subsiteIdentifier)) { + $subsiteIdentifier = $this->idFromFixture(Subsite::class, $subsiteIdentifier); + } + Subsite::changeSubsite($subsiteIdentifier); - // @todo: Workaround because GroupSubsites->augmentSQL() is relying on session state - Subsite::changeSubsite($subsite1); + $this->assertSame($expected, (bool) $page->canEdit(), $message); + } - $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' - ); + /** + * @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'], + ]; } /** From 7681634cb2add840c7a067ca26b2f5fcfc020fad Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 27 Aug 2018 10:04:57 +1200 Subject: [PATCH 04/10] Remove irrelevant check for subsites list size, use func_num_args() and add break to loop --- src/Extensions/LeftAndMainSubsites.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Extensions/LeftAndMainSubsites.php b/src/Extensions/LeftAndMainSubsites.php index a4facee..d643d7e 100644 --- a/src/Extensions/LeftAndMainSubsites.php +++ b/src/Extensions/LeftAndMainSubsites.php @@ -88,11 +88,6 @@ class LeftAndMainSubsites extends LeftAndMainExtension $accessibleSubsites = ArrayList::create(); $subsites = Subsite::all_sites($includeMainSite, $mainSiteTitle); - // Check whether we have any subsites - if (!$subsites->exists()) { - return $accessibleSubsites; - } - foreach ($subsites as $subsite) { /** @var Subsite $subsite */ $canAccess = SubsiteState::singleton() @@ -211,7 +206,7 @@ class LeftAndMainSubsites extends LeftAndMainExtension public function canAccess() { // Allow us to accept a Member object passed in as an argument without breaking semver - $passedMember = func_get_args() ? func_get_arg(0) : null; + $passedMember = func_num_args() ? func_get_arg(0) : null; // Admin can access everything, no point in checking. $member = $passedMember ?: Security::getCurrentUser(); @@ -249,6 +244,7 @@ class LeftAndMainSubsites extends LeftAndMainExtension $groupSubsiteIds = $group->Subsites()->column('ID'); if (in_array($currentSubsiteId, $groupSubsiteIds)) { $allowedInSubsite = true; + break; } } From 313d22ffcacab76723c7105a5bf46b48e1b738a4 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 7 Sep 2018 11:03:51 +0200 Subject: [PATCH 05/10] FIX Only continue delegation when DB exceptions are caused by no database selected This prevents the middleware from interrupting legitimate database exceptions from being propagated. --- src/Middleware/InitStateMiddleware.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Middleware/InitStateMiddleware.php b/src/Middleware/InitStateMiddleware.php index b5abf0c..4b7cd30 100644 --- a/src/Middleware/InitStateMiddleware.php +++ b/src/Middleware/InitStateMiddleware.php @@ -43,8 +43,12 @@ class InitStateMiddleware implements HTTPMiddleware return $delegate($request); } catch (DatabaseException $ex) { - // Database is not ready - return $delegate($request); + $message = $ex->getMessage(); + if (strpos($message, 'No database selected') !== false) { + // Database is not ready, ignore and continue + return $delegate($request); + } + throw $ex; } finally { // Persist to the session if using the CMS if ($state->getUseSessions()) { From bb226a0652cce663615fbc56d78db5f44f2f415b Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 14 Sep 2018 09:42:29 +0200 Subject: [PATCH 06/10] FIX Remove duplicate Configuration tabs when creating a new subsite --- src/Forms/GridFieldSubsiteDetailFormItemRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Forms/GridFieldSubsiteDetailFormItemRequest.php b/src/Forms/GridFieldSubsiteDetailFormItemRequest.php index 275368d..f6775aa 100644 --- a/src/Forms/GridFieldSubsiteDetailFormItemRequest.php +++ b/src/Forms/GridFieldSubsiteDetailFormItemRequest.php @@ -42,7 +42,7 @@ class GridFieldSubsiteDetailFormItemRequest extends GridFieldDetailForm_ItemRequ $templateArray ); $templateDropdown->setEmptyString('(' . _t('Subsite.NOTEMPLATE', 'No template') . ')'); - $form->Fields()->addFieldToTab('Root.Configuration', $templateDropdown); + $form->Fields()->addFieldToTab('Root.Main', $templateDropdown); } return $form; From 1e458ef03d5e0e330ce99ca3df523d15a22d443e Mon Sep 17 00:00:00 2001 From: DorsetDigital Date: Wed, 17 Oct 2018 23:20:20 +0100 Subject: [PATCH 07/10] Change source of admin URL in getIsAdmin() As per #394 Change direct call to the AdminRootController config setting, using instead the admin_url() method on the class which provides detection via the Director rules, and the fallback to the config setting. --- src/Middleware/InitStateMiddleware.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Middleware/InitStateMiddleware.php b/src/Middleware/InitStateMiddleware.php index b5abf0c..abec0ff 100644 --- a/src/Middleware/InitStateMiddleware.php +++ b/src/Middleware/InitStateMiddleware.php @@ -62,7 +62,7 @@ class InitStateMiddleware implements HTTPMiddleware public function getIsAdmin(HTTPRequest $request) { $adminPaths = static::config()->get('admin_url_paths'); - $adminPaths[] = AdminRootController::config()->get('url_base') . '/'; + $adminPaths[] = AdminRootController::admin_url(); $currentPath = rtrim($request->getURL(), '/') . '/'; foreach ($adminPaths as $adminPath) { if (substr($currentPath, 0, strlen($adminPath)) === $adminPath) { From e52fe41a237c88d6e98333d036f9b64722bffd5e Mon Sep 17 00:00:00 2001 From: bergice Date: Thu, 18 Oct 2018 18:41:07 +1300 Subject: [PATCH 08/10] BUG: Fix `MigrateFileTask` not migrating files for subsites Fixes #352 --- _config/middleware.yml | 2 ++ src/Tasks/SubsiteMigrateFileTask.php | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 src/Tasks/SubsiteMigrateFileTask.php diff --git a/_config/middleware.yml b/_config/middleware.yml index beb0a93..5cbf9ca 100644 --- a/_config/middleware.yml +++ b/_config/middleware.yml @@ -8,3 +8,5 @@ SilverStripe\Core\Injector\Injector: properties: Middlewares: SubsitesStateMiddleware: %$SilverStripe\Subsites\Middleware\InitStateMiddleware + SilverStripe\Dev\Tasks\MigrateFileTask: + class: SilverStripe\Subsites\Tasks\SubsiteMigrateFileTask diff --git a/src/Tasks/SubsiteMigrateFileTask.php b/src/Tasks/SubsiteMigrateFileTask.php new file mode 100644 index 0000000..ac6dcd6 --- /dev/null +++ b/src/Tasks/SubsiteMigrateFileTask.php @@ -0,0 +1,20 @@ + Date: Thu, 18 Oct 2018 11:03:16 +0200 Subject: [PATCH 09/10] Automated linting fix --- src/Tasks/SubsiteMigrateFileTask.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Tasks/SubsiteMigrateFileTask.php b/src/Tasks/SubsiteMigrateFileTask.php index ac6dcd6..c00c776 100644 --- a/src/Tasks/SubsiteMigrateFileTask.php +++ b/src/Tasks/SubsiteMigrateFileTask.php @@ -2,7 +2,6 @@ namespace SilverStripe\Subsites\Tasks; - use SilverStripe\Dev\Tasks\MigrateFileTask; use SilverStripe\Subsites\Model\Subsite; From 7cc86199e7140c9584660e603bc4e4eb48652150 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Fri, 19 Oct 2018 12:00:50 +1300 Subject: [PATCH 10/10] Revert "FIX CMS permission checks for subsite are now handled in the state context" --- src/Controller/SubsiteXHRController.php | 14 +- src/Extensions/GroupSubsites.php | 8 +- src/Extensions/LeftAndMainSubsites.php | 122 +++++++---------- src/Extensions/SiteTreeSubsites.php | 6 - src/Model/Subsite.php | 167 +++++++++++++++++------- tests/php/LeftAndMainSubsitesTest.php | 114 ++++++---------- tests/php/SiteTreeSubsitesTest.php | 65 ++++----- 7 files changed, 256 insertions(+), 240 deletions(-) diff --git a/src/Controller/SubsiteXHRController.php b/src/Controller/SubsiteXHRController.php index 9016f6b..c758d53 100644 --- a/src/Controller/SubsiteXHRController.php +++ b/src/Controller/SubsiteXHRController.php @@ -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(); diff --git a/src/Extensions/GroupSubsites.php b/src/Extensions/GroupSubsites.php index 4df1521..2e583f1 100644 --- a/src/Extensions/GroupSubsites.php +++ b/src/Extensions/GroupSubsites.php @@ -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 diff --git a/src/Extensions/LeftAndMainSubsites.php b/src/Extensions/LeftAndMainSubsites.php index d643d7e..760623f 100644 --- a/src/Extensions/LeftAndMainSubsites.php +++ b/src/Extensions/LeftAndMainSubsites.php @@ -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 Subsite 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. diff --git a/src/Extensions/SiteTreeSubsites.php b/src/Extensions/SiteTreeSubsites.php index b1f3f74..ecef3a5 100644 --- a/src/Extensions/SiteTreeSubsites.php +++ b/src/Extensions/SiteTreeSubsites.php @@ -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'); diff --git a/src/Model/Subsite.php b/src/Model/Subsite.php index 704e23d..229604d 100644 --- a/src/Model/Subsite.php +++ b/src/Model/Subsite.php @@ -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; } /** diff --git a/tests/php/LeftAndMainSubsitesTest.php b/tests/php/LeftAndMainSubsitesTest.php index 5be30e9..09df18f 100644 --- a/tests/php/LeftAndMainSubsitesTest.php +++ b/tests/php/LeftAndMainSubsitesTest.php @@ -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() diff --git a/tests/php/SiteTreeSubsitesTest.php b/tests/php/SiteTreeSubsitesTest.php index 6efe3e7..efb16f8 100644 --- a/tests/php/SiteTreeSubsitesTest.php +++ b/tests/php/SiteTreeSubsitesTest.php @@ -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' + ); } /**