Permalink
Browse files

BUG Refactor the access checks and initial subsite redirections.

Remove the special AJAX handling to simplify the code. Now redirection
will be forced on any request that changes the subsite to re-synchronise
with the frontend.

Introduce canAccess method, and add it to alternateAccessCheck to make
sure this subsite-specific chceck is also done in situations that are
not captured by onBeforeInit.
  • Loading branch information...
1 parent e6f054f commit 58b926af25b722297694aa8590ccf0b04840e85e @mateusz mateusz committed Dec 4, 2013
Showing with 74 additions and 44 deletions.
  1. +7 −0 code/SubsiteXHRController.php
  2. +64 −41 code/extensions/LeftAndMainSubsites.php
  3. +3 −3 tests/SubsiteAdminFunctionalTest.php
@@ -16,6 +16,13 @@ public function canView($member = null) {
return false;
}
+ /**
+ * Similar as above, but for the LeftAndMainSubsites - allow access if user allowed into the CMS at all.
+ */
+ public function canAccess() {
+ if (Subsite::all_accessible_sites()->count()>0) return true;
+ }
+
public function getResponseNegotiator() {
$negotiator = parent::getResponseNegotiator();
$self = $this;
@@ -160,20 +160,45 @@ public function shouldChangeSubsite($adminClass, $recordSubsiteID, $currentSubsi
}
/**
- * Do some pre-flight checks if a subsite switch is needed.
- * We redirect the user to something accessible if the current section/subsite is forbidden.
+ * Check if the current controller is accessible for this user on this subsite.
+ */
+ function canAccess() {
+ // Admin can access everything, no point in checking.
+ $member = Member::currentUser();
+ if($member && Permission::checkMember($member, 'ADMIN')) return true;
+
+ // Check if we have access to current section on the current subsite.
+ $accessibleSites = $this->owner->sectionSites($member);
+ if ($accessibleSites->count() && $accessibleSites->find('ID', Subsite::currentSubsiteID())) {
+ // Current section can be accessed on the current site, all good.
+ return true;
+ }
+
+ return false;
+ }
+
+ /**
+ * Prevent accessing disallowed resources. This happens after onBeforeInit has executed,
+ * so all redirections should've already taken place.
+ */
+ public function alternateAccessCheck() {
+ return $this->owner->canAccess();
+ }
+
+ /**
+ * Redirect the user to something accessible if the current section/subsite is forbidden.
+ *
+ * This is done via onBeforeInit as it needs to be done before the LeftAndMain::init has a
+ * chance to forbids access via alternateAccessCheck.
+ *
+ * If we need to change the subsite we force the redirection to /admin/ so the frontend is
+ * fully re-synchronised with the internal session. This is better than risking some panels
+ * showing data from another subsite.
*/
public function onBeforeInit() {
// We are accessing the CMS, so we need to let Subsites know we will be using the session.
Subsite::$use_session_subsiteid = true;
- // Do not attempt to redirect for AJAX calls. The proper security checks are done on specific objects
- // (by Subsite augmentations of Member and Group). Also the only good time to change the subsite in the CMS
- // is upon initial load - otherwise we risk the internal state becoming mismatched with the interface.
- if ($this->owner->request->isAjax()) {
- return;
- }
-
// FIRST, check if we need to change subsites due to the URL.
// Automatically redirect the session to appropriate subsite when requesting a record.
@@ -182,7 +207,11 @@ public function onBeforeInit() {
if($record && isset($record->SubsiteID) && is_numeric($record->SubsiteID)) {
if ($this->shouldChangeSubsite($this->owner->class, $record->SubsiteID, Subsite::currentSubsiteID())) {
+ // Update current subsite in session
Subsite::changeSubsite($record->SubsiteID);
+
+ //Redirect to clear the current page
+ return $this->owner->redirect('admin/');
}
}
@@ -203,47 +232,41 @@ public function onBeforeInit() {
// SECOND, check if we need to change subsites due to lack of permissions.
- // If we can view current URL there is nothing to do.
- if ($this->owner->canView()) {
- return;
- }
-
- // Admin can access everything, no point in checking.
- $member = Member::currentUser();
- if($member && Permission::checkMember($member, 'ADMIN')) return;
+ if (!$this->owner->canAccess()) {
- // Check if we have access to current section on the current subsite.
- $accessibleSites = $this->owner->sectionSites($member);
- if ($accessibleSites->count() && $accessibleSites->find('ID', Subsite::currentSubsiteID())) {
- // Current section can be accessed on the current site, all good.
- return;
- }
+ $member = Member::currentUser();
- // If the current section is not accessible, try at least to stick to the same subsite.
- $menu = CMSMenu::get_menu_items();
- foreach($menu as $candidate) {
- if($candidate->controller && $candidate->controller!=$this->owner->class) {
+ // Current section is not accessible, try at least to stick to the same subsite.
+ $menu = CMSMenu::get_menu_items();
+ foreach($menu as $candidate) {
+ if($candidate->controller && $candidate->controller!=$this->owner->class) {
- $accessibleSites = singleton($candidate->controller)->sectionSites(true, 'Main site', $member);
- if ($accessibleSites->count() && $accessibleSites->find('ID', Subsite::currentSubsiteID())) {
- // Section is accessible, redirect there.
- $this->owner->redirect(singleton($candidate->controller)->Link());
- return;
+ $accessibleSites = singleton($candidate->controller)->sectionSites(true, 'Main site', $member);
+ if ($accessibleSites->count() && $accessibleSites->find('ID', Subsite::currentSubsiteID())) {
+ // Section is accessible, redirect there.
+ return $this->owner->redirect(singleton($candidate->controller)->Link());
+ }
}
}
- }
- // Finally, if no section is available, move to any other permitted subsite.
- foreach($menu as $candidate) {
- if($candidate->controller) {
- $accessibleSites = singleton($candidate->controller)->sectionSites(true, 'Main site', $member);
- if ($accessibleSites->count()) {
- Subsite::changeSubsite($accessibleSites->First()->ID);
- $this->owner->redirect(singleton($candidate->controller)->Link());
- return;
+ // If no section is available, look for other accessible subsites.
+ foreach($menu as $candidate) {
+ if($candidate->controller) {
+ $accessibleSites = singleton($candidate->controller)->sectionSites(true, 'Main site', $member);
+ if ($accessibleSites->count()) {
+ Subsite::changeSubsite($accessibleSites->First()->ID);
+ return $this->owner->redirect(singleton($candidate->controller)->Link());
+ }
}
}
+
+ // We have not found any accessible section or subsite. User should be denied access.
+ return Security::permissionFailure($this->owner);
+
}
+
+ // Current site is accessible. Allow through.
+ return;
}
function augmentNewSiteTreeItem(&$item) {
@@ -69,17 +69,17 @@ function testAdminIsRedirectedToObjectsSubsite() {
Subsite::changeSubsite(0);
$this->getAndFollowAll("admin/pages/edit/show/$subsite1Home->ID");
$this->assertEquals(Subsite::currentSubsiteID(), $subsite1Home->SubsiteID, 'Loading an object switches the subsite');
- $this->assertRegExp("#^admin/pages/edit/show/$subsite1Home->ID.*#", $this->mainSession->lastUrl(), 'Lands on the correct section');
+ $this->assertRegExp("#^admin/pages.*#", $this->mainSession->lastUrl(), 'Lands on the correct section');
Config::inst()->update('CMSPageEditController', 'treats_subsite_0_as_global', true);
Subsite::changeSubsite(0);
$this->getAndFollowAll("admin/pages/edit/show/$subsite1Home->ID");
$this->assertEquals(Subsite::currentSubsiteID(), $subsite1Home->SubsiteID, 'Loading a non-main-site object still switches the subsite if configured with treats_subsite_0_as_global');
- $this->assertRegExp("#^admin/pages/edit/show/$subsite1Home->ID.*#", $this->mainSession->lastUrl(), 'Lands on the correct section');
+ $this->assertRegExp("#^admin/pages.*#", $this->mainSession->lastUrl(), 'Lands on the correct section');
$this->getAndFollowAll("admin/pages/edit/show/$mainSubsitePage->ID");
$this->assertNotEquals(Subsite::currentSubsiteID(), $mainSubsitePage->SubsiteID, 'Loading a main-site object does not change the subsite if configured with treats_subsite_0_as_global');
- $this->assertRegExp("#^admin/pages/edit/show/$mainSubsitePage->ID.*#", $this->mainSession->lastUrl(), 'Lands on the correct section');
+ $this->assertRegExp("#^admin/pages.*#", $this->mainSession->lastUrl(), 'Lands on the correct section');
Config::inst()->unnest();
}

0 comments on commit 58b926a

Please sign in to comment.