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

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

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

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 <a href='psi_element://Subsite'>Subsite</a> 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.

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

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,24 +392,19 @@ 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(
+            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
@@ -416,7 +413,6 @@ class Subsite extends DataObject
             // 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();
+        $accessibleSubsites = ArrayList::create();
+        $subsites = self::all_sites($includeMainSite, $mainSiteTitle);
+        foreach ($subsites as $subsite) {
+            // Exclude subsites with no title
+            if (empty($subsite->Title)) {
+                continue;
             }
 
-        /** @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')"
-            );
+            /** @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);
 
-        if (!$subsites && $rolesSubsites) {
-            return $rolesSubsites;
+                    return Permission::checkMember($member, $permCode);
+                });
+
+            if ($canAccess === false) {
+                // Explicitly denied access
+                continue;
             }
 
-        $subsites = new ArrayList($subsites->toArray());
-
-        if ($rolesSubsites) {
-            foreach ($rolesSubsites as $subsite) {
-                if (!$subsites->find('ID', $subsite->ID)) {
-                    $subsites->push($subsite);
-                }
-            }
+            $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;
     }
 
     /**

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,48 +33,71 @@ 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());
+        $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'");
@@ -82,6 +107,17 @@ class LeftAndMainSubsitesTest extends FunctionalTest
             '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()

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