Merge pull request #397 from silverstripe/revert-388-pulls/2.1/fix-role-permissions

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

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();

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

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 <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.

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');
 

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;
     }
 
     /**

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()

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'
+        );
     }
 
     /**