diff --git a/control/Controller.php b/control/Controller.php index cda5774bd..b07c804c0 100644 --- a/control/Controller.php +++ b/control/Controller.php @@ -265,10 +265,12 @@ class Controller extends RequestHandler implements TemplateGlobalProvider { */ public function getViewer($action) { // Hard-coded templates - if($this->templates[$action]) { + if(isset($this->templates[$action]) && $this->templates[$action]) { $templates = $this->templates[$action]; - } else if($this->templates['index']) { + + } else if(isset($this->templates['index']) && $this->templates['index']) { $templates = $this->templates['index']; + } else if($this->template) { $templates = $this->template; } else { @@ -318,6 +320,23 @@ class Controller extends RequestHandler implements TemplateGlobalProvider { return $returnURL; } + + /** + * Return the class that defines the given action, so that we know where to check allowed_actions. + * Overrides RequestHandler to also look at defined templates + */ + protected function definingClassForAction($action) { + $definingClass = parent::definingClassForAction($action); + if($definingClass) return $definingClass; + + $class = get_class($this); + while($class != 'RequestHandler') { + $templateName = strtok($class, '_') . '_' . $action; + if(SSViewer::hasTemplate($templateName)) return $class; + + $class = get_parent_class($class); + } + } /** * Returns TRUE if this controller has a template that is specifically designed to handle a specific action. diff --git a/control/RequestHandler.php b/control/RequestHandler.php index 17b366679..a6db29fe4 100644 --- a/control/RequestHandler.php +++ b/control/RequestHandler.php @@ -371,6 +371,22 @@ class RequestHandler extends ViewableData { return false; } + + /** + * Return the class that defines the given action, so that we know where to check allowed_actions. + */ + protected function definingClassForAction($actionOrigCasing) { + $action = strtolower($actionOrigCasing); + + $definingClass = null; + $insts = array_merge(array($this), (array)$this->getExtensionInstances()); + foreach($insts as $inst) { + if(!method_exists($inst, $action)) continue; + $r = new ReflectionClass(get_class($inst)); + $m = $r->getMethod($actionOrigCasing); + return $m->getDeclaringClass()->getName(); + } + } /** * Check that the given action is allowed to be called from a URL. @@ -382,58 +398,45 @@ class RequestHandler extends ViewableData { $isAllowed = false; $isDefined = false; - if($this->hasMethod($actionOrigCasing) || !$action || $action == 'index') { - // Get actions for this specific class (without inheritance) - $definingClass = null; - $insts = array_merge(array($this), (array)$this->getExtensionInstances()); - foreach($insts as $inst) { - if(!method_exists($inst, $action)) continue; - $r = new ReflectionClass(get_class($inst)); - $m = $r->getMethod($actionOrigCasing); - $definingClass = $m->getDeclaringClass()->getName(); - } + + // Get actions for this specific class (without inheritance) + $definingClass = $this->definingClassForAction($actionOrigCasing); + $allowedActions = $this->allowedActions($definingClass); - $allowedActions = $this->allowedActions($definingClass); - - // check if specific action is set - if(isset($allowedActions[$action])) { - $isDefined = true; - $test = $allowedActions[$action]; - if($test === true || $test === 1 || $test === '1') { - // TRUE should always allow access - $isAllowed = true; - } elseif(substr($test, 0, 2) == '->') { - // Determined by custom method with "->" prefix - list($method, $arguments) = Object::parse_class_spec(substr($test, 2)); - $definingClassInst = Injector::inst()->get($definingClass); - $isAllowed = call_user_func_array(array($definingClassInst, $method), $arguments); - } else { - // Value is a permission code to check the current member against - $isAllowed = Permission::check($test); - } - } elseif( - is_array($allowedActions) - && (($key = array_search($action, $allowedActions, true)) !== false) - && is_numeric($key) - ) { - // Allow numeric array notation (search for array value as action instead of key) - $isDefined = true; - $isAllowed = true; - } elseif(is_array($allowedActions) && !count($allowedActions)) { - // If defined as empty array, deny action - $isAllowed = false; - } elseif($allowedActions === null) { - // If undefined, allow action + // check if specific action is set + if(isset($allowedActions[$action])) { + $isDefined = true; + $test = $allowedActions[$action]; + if($test === true || $test === 1 || $test === '1') { + // TRUE should always allow access $isAllowed = true; + } elseif(substr($test, 0, 2) == '->') { + // Determined by custom method with "->" prefix + list($method, $arguments) = Object::parse_class_spec(substr($test, 2)); + $isAllowed = call_user_func_array(array($this, $method), $arguments); + } else { + // Value is a permission code to check the current member against + $isAllowed = Permission::check($test); } + } elseif( + is_array($allowedActions) + && (($key = array_search($action, $allowedActions, true)) !== false) + && is_numeric($key) + ) { + // Allow numeric array notation (search for array value as action instead of key) + $isDefined = true; + $isAllowed = true; + } elseif(is_array($allowedActions) && !count($allowedActions)) { + // If defined as empty array, deny action + $isAllowed = false; + } elseif($allowedActions === null) { + // If undefined, allow action + $isAllowed = true; + } - // If we don't have a match in allowed_actions, - // whitelist the 'index' action as well as undefined actions. - if(!$isDefined && ($action == 'index' || empty($action))) { - $isAllowed = true; - } - } else { - // Doesn't have method, set to true so that a template can handle this action + // If we don't have a match in allowed_actions, + // whitelist the 'index' action as well as undefined actions. + if(!$isDefined && ($action == 'index' || empty($action))) { $isAllowed = true; } diff --git a/tests/control/ControllerTest.php b/tests/control/ControllerTest.php index 2ff8c6c38..40bc0ad0c 100644 --- a/tests/control/ControllerTest.php +++ b/tests/control/ControllerTest.php @@ -100,10 +100,24 @@ class ControllerTest extends FunctionalTest { $response = $this->get("ControllerTest_AccessSecuredController/method2"); $this->assertEquals(200, $response->getStatusCode(), - 'Access grante on action originally defined with empty $allowed_actions on parent controller, ' . + 'Access granted on action originally defined with empty $allowed_actions on parent controller, ' . 'because it has been redefined in the subclass' ); + $response = $this->get("ControllerTest_AccessSecuredController/templateaction"); + $this->assertEquals(403, $response->getStatusCode(), + 'Access denied on action with $allowed_actions on defining controller, ' . + 'if action is not a method but rather a template discovered by naming convention' + ); + + $this->session()->inst_set('loggedInAs', $adminUser->ID); + $response = $this->get("ControllerTest_AccessSecuredController/templateaction"); + $this->assertEquals(200, $response->getStatusCode(), + 'Access granted for logged in admin on action with $allowed_actions on defining controller, ' . + 'if action is not a method but rather a template discovered by naming convention' + ); + $this->session()->inst_set('loggedInAs', null); + $response = $this->get("ControllerTest_AccessSecuredController/adminonly"); $this->assertEquals(403, $response->getStatusCode(), 'Access denied on action with $allowed_actions on defining controller, ' . @@ -349,8 +363,9 @@ class ControllerTest_Controller extends Controller implements TestOnly { 'methodaction', 'stringaction', 'redirectbacktest', + 'templateaction' ); - + public function methodaction() { return array( "Content" => "methodaction content" @@ -395,7 +410,7 @@ class ControllerTest_AccessSecuredController extends ControllerTest_AccessBaseCo "method1", // denied because only defined in parent "method2" => true, // granted because its redefined "adminonly" => "ADMIN", - "protectedmethod" => true, // denied because its protected + 'templateaction' => 'ADMIN' ); public function method2() {} diff --git a/view/SSViewer.php b/view/SSViewer.php index 858691e97..220d59d87 100644 --- a/view/SSViewer.php +++ b/view/SSViewer.php @@ -717,8 +717,14 @@ class SSViewer { public static function hasTemplate($templates) { $manifest = SS_TemplateLoader::instance()->getManifest(); + if(Config::inst()->get('SSViewer', 'theme_enabled')) { + $theme = Config::inst()->get('SSViewer', 'theme'); + } else { + $theme = null; + } + foreach ((array) $templates as $template) { - if ($manifest->getTemplate($template)) return true; + if ($manifest->getCandidateTemplate($template, $theme)) return true; } return false;