FIX: Ensure that actions inferred from templates with the "_action" suffix also respect allowed_actions.

FIX: Ensure SSViewer::hasTemplate() is aware of themes.

To do this, RequestHandler::definingClassForAction() has been created, splitting out the code that looks up the class that defines a given action into its own method.  This is then overridden in Controller to look at templates.

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 {
@@ -319,6 +321,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.
 	 *

control/RequestHandler.php

@@ -372,6 +372,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.
 	 * It will interrogate {@link self::$allowed_actions} to determine this.
@@ -382,17 +398,9 @@ 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);	
 
 		// check if specific action is set
@@ -405,8 +413,7 @@ class RequestHandler extends ViewableData {
 			} 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($this, $method), $arguments);
-					$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);
@@ -432,10 +439,6 @@ class RequestHandler extends ViewableData {
 		if(!$isDefined && ($action == 'index' || empty($action))) {
 			$isAllowed = true;
 		}
-		} else {
-			// Doesn't have method, set to true so that a template can handle this action
-			$isAllowed = true;
-		}
 
 		return $isAllowed;
 	}

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,6 +363,7 @@ class ControllerTest_Controller extends Controller implements TestOnly {
 		'methodaction',
 		'stringaction',
 		'redirectbacktest',
+		'templateaction'
 	);
 
 	public function methodaction() {
@@ -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() {}

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;