API Tighten up allowed_actions

allowed_actions is now only allowed to reference public methods defined
on the same Controller as the allowed_actions static, and
the wildcard "*" has been deprecated

control/RequestHandler.php

@@ -231,16 +231,37 @@ class RequestHandler extends ViewableData {
 	}
 	
 	/**
-	 * Get a unified array of allowed actions on this controller (if such data is available) from both the controller
+	 * Get a array of allowed actions defined on this controller,
-	 * ancestry and any extensions.
+	 * any parent classes or extensions.
 	 * 
+	 * Caution: Since 3.1, allowed_actions definitions only apply
+	 * to methods on the controller they're defined on,
+	 * so it is recommended to use the $class argument
+	 * when invoking this method.
+	 *
+	 * @param String $limitToClass
 	 * @return array|null
 	 */
-	public function allowedActions() {
+	public function allowedActions($limitToClass = null) {
-
+		if($limitToClass) {
+			$actions = Config::inst()->get(
+				$limitToClass, 
+				'allowed_actions', 
+				Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES
+			);
+		} else {
 			$actions = Config::inst()->get(get_class($this), 'allowed_actions');
+		}
+
+		if(is_array($actions)) {
+			if(array_key_exists('*', $actions)) {
+				Deprecation::notice(
+					'3.0', 
+					'Wildcards (*) are no longer valid in $allowed_actions due their ambiguous '
+					. ' and potentially insecure behaviour. Please define all methods explicitly instead.'
+				);
+			}
 
-		if($actions) {
 			// convert all keys and values to lowercase to 
 			// allow for easier comparison, unless it is a permission code
 			$actions = array_change_key_case($actions, CASE_LOWER);
@@ -250,11 +271,15 @@ class RequestHandler extends ViewableData {
 			}
 
 			return $actions;
+		} else {
+			return null;
 		}
 	}
 	
 	/**
-	 * Checks if this request handler has a specific action (even if the current user cannot access it).
+	 * Checks if this request handler has a specific action, 
+	 * even if the current user cannot access it.
+	 * Includes class ancestry and extensions in the checks.
 	 *
 	 * @param string $action
 	 * @return bool
@@ -262,23 +287,32 @@ class RequestHandler extends ViewableData {
 	public function hasAction($action) {
 		if($action == 'index') return true;
 
+		// Don't allow access to any non-public methods (inspect instance plus all extensions)
+		$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($action);
+			if(!$m || !$m->isPublic()) return false;
+		}
+		
 		$action  = strtolower($action);
 		$actions = $this->allowedActions();
 
-		// Check if the action is defined in the allowed actions as either a
+		// Check if the action is defined in the allowed actions of any ancestry class
-		// key or value. Note that if the action is numeric, then keys are not
+		// as either a key or value. Note that if the action is numeric, then keys are not
-		// searched for actions to prevent actual array keys being recognised
+		// searched for actions to prevent actual array keys being recognised as actions.
-		// as actions.
 		if(is_array($actions)) {
 			$isKey   = !is_numeric($action) && array_key_exists($action, $actions);
 			$isValue = in_array($action, $actions, true);
-			$isWildcard = (in_array('*', $actions) && $this->checkAccessAction($action));
+			if($isKey || $isValue) return true;
-			if($isKey || $isValue || $isWildcard) return true;
 		}
 
-		if(!is_array($actions) || !$this->config()->get('allowed_actions',
+		$actionsWithoutExtra = $this->config()->get(
-				Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES)) {
+			'allowed_actions',
-
+			Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES
+		);
+		if(!is_array($actions) || !$actionsWithoutExtra) {
 			if($action != 'init' && $action != 'run' && method_exists($this, $action)) return true;
 		}
 		
@@ -292,68 +326,65 @@ class RequestHandler extends ViewableData {
 	public function checkAccessAction($action) {
 		$actionOrigCasing = $action;
 		$action = strtolower($action);
-		$allowedActions    = $this->allowedActions();
 
-		if($allowedActions)  {
+		$isAllowed = false;
-			// check for specific action rules first, and fall back to global rules defined by asterisk
+		$isDefined = false;
-			foreach(array($action,'*') as $actionOrAll) {
+		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();	
+			}
+
+			$allowedActions = $this->allowedActions($definingClass);	
+
 			// check if specific action is set
-				if(isset($allowedActions[$actionOrAll])) {
+			if(isset($allowedActions[$action])) {
-					$test = $allowedActions[$actionOrAll];
+				$isDefined = true;
+				$test = $allowedActions[$action];
 				if($test === true || $test === 1 || $test === '1') {
-						// Case 1: TRUE should always allow access
+					// TRUE should always allow access
-						return true;
+					$isAllowed = true;
 				} elseif(substr($test, 0, 2) == '->') {
-						// Case 2: Determined by custom method with "->" prefix
+					// Determined by custom method with "->" prefix
 					list($method, $arguments) = Object::parse_class_spec(substr($test, 2));
-						return call_user_func_array(array($this, $method), $arguments);
+					$definingClassInst = Injector::inst()->get($definingClass);
+					$isAllowed = call_user_func_array(array($definingClassInst, $method), $arguments);
 				} else {
-						// Case 3: Value is a permission code to check the current member against
+					// Value is a permission code to check the current member against
-						return Permission::check($test);
+					$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;
 			}
 
-				} elseif((($key = array_search($actionOrAll, $allowedActions, true)) !== false) && is_numeric($key)) {
+			// If we don't have a match in allowed_actions,
-					// Case 4: Allow numeric array notation (search for array value as action instead of key)
+			// whitelist the 'index' action as well as undefined actions.
-					return true;
+			if(!$isDefined && ($action == 'index' || empty($action))) {
+				$isAllowed = true;
 			}
-			}
-		}
-		
-		// If we get here an the action is 'index', then it hasn't been specified, which means that
-		// it should be allowed.
-		if($action == 'index' || empty($action)) return true;
-
-		// Get actions defined for this specific class
-		$relevantActions = null;
-		if($allowedActions !== null && method_exists($this, $action)) {
-			$r = new ReflectionClass(get_class($this));
-			$m = $r->getMethod($actionOrigCasing);
-			$relevantActions = Object::uninherited_static(
-				$m->getDeclaringClass()->getName(), 
-				'allowed_actions'
-			);
-		}
-
-		// If no allowed_actions are provided on in the whole inheritance chain,
-		// or they aren't provided on the specific class...
-		if($allowedActions === null || $relevantActions === null) {
-			// ... only let through actions that aren't handled by
-			// magic methods we test this by calling the unmagic method_exists. 
-			if(method_exists($this, $action)) {
-				$r = new ReflectionClass(get_class($this));
-				if($r->hasMethod($actionOrigCasing)) {
-					$m = $r->getMethod($actionOrigCasing);
-					return ($m && is_subclass_of($m->getDeclaringClass()->getName(), 'RequestHandler'));
 		} else {
-					throw new Exception("method_exists() true but ReflectionClass can't find method");
+			// Doesn't have method, set to true so that a template can handle this action
-				}
+			$isAllowed = true;
-			} else if(!$this->hasMethod($action)){
-				// Return true so that a template can handle this action
-				return true;
-			}
 		}
 
-		return false;
+		return $isAllowed;
 	}
 	
 	/**

docs/en/changelogs/3.1.0.md

@@ -19,6 +19,9 @@
  * Behaviour testing support through [Behat](http://behat.org), with CMS test coverage 
    (see the [SilverStripe Behat Extension]() for details)
  * Removed legacy table APIs (e.g. `TableListField`), use GridField instead
+ * Deny URL access if `Controller::$allowed_actions` is undefined
+ * Removed support for "*" rules in `Controller::$allowed_actions`
+ * Removed support for overriding rules on parent classes through `Controller::$allowed_actions`
  * Editing of relation table data (`$many_many_extraFields`) in `GridField`
  * Optional integration with ImageMagick as a new image manipulation backend
  * Support for PHP 5.4's built-in webserver
@@ -26,6 +29,78 @@
 
 ## Upgrading
 
+### Deny URL access if `Controller::$allowed_actions` is undefined or empty array
+
+In order to make controller access checks more consistent and easier to
+understand, the routing will require definition of `$allowed_actions`
+on your own `Controller` subclasses if they contain any actions
+accessible through URLs, or any forms.
+
+	:::php
+	class MyController extends Controller {
+		// This action is now denied because no $allowed_actions are specified
+		public function myaction($request) {}
+	}
+
+Please review all rules governing allowed actions in the
+["controller" topic](/topics/controller).
+
+### Removed support for "*" rules in `Controller::$allowed_actions`
+
+The wildcard ('*') character allowed to define fallback rules
+in case they weren't explicitly defined. This caused a lot of confusion,
+particularly around inherited rules. We've decided to remove the feature,
+you'll need to specificy each accessible action individually.
+
+	:::php
+	class MyController extends Controller {
+		public static $allowed_actions = array('*' => 'ADMIN');
+		// Always denied because not explicitly listed in $allowed_actions
+		public function myaction($request) {}
+		// Always denied because not explicitly listed in $allowed_actions
+		public function myotheraction($request) {}
+	}
+
+Please review all rules governing allowed actions in the
+["controller" topic](/topics/controller).
+
+### Removed support for overriding rules on parent classes through `Controller::$allowed_actions`
+
+Since 3.1, the `$allowed_actions` definitions only apply
+to methods defined on the class they're also defined on.
+Overriding inherited access definitions is no longer possible.
+
+	:::php
+	class MyController extends Controller {
+		public static $allowed_actions = array('myaction' => 'ADMIN');
+		public function myaction($request) {}
+	}
+	class MySubController extends MyController {
+		// No longer works
+		public static $allowed_actions = array('myaction' => 'CMS_ACCESS_CMSMAIN');
+	}
+
+This also applies for custom implementations of `handleAction()` and `handleRequest()`,
+which now have to be listed in the `$allowed_actions` specifically.
+It also restricts `Extension` classes applied to controllers, which now
+can only grant or deny access or methods they define themselves.
+
+New approach with the [Config API](/topics/configuration)
+
+	:::php
+	class MySubController extends MyController {
+		public function init() {
+			parent::init();
+
+			Config::inst()->update('MyController', 'allowed_actions',
+				array('myaction' => 'CMS_ACCESS_CMSMAIN')
+			);
+		}
+	}
+
+Please review all rules governing allowed actions in the
+["controller" topic](/topics/controller).
+
 ### Grouped CMS Buttons
 
 The CMS buttons are now grouped, in order to hide minor actions by default and declutter the interface.

docs/en/topics/controller.md

@@ -83,17 +83,21 @@ way to perform checks against permission codes or custom logic.
 
 There's a couple of rules guiding these checks:
 
- * Each controller is only responsible for access control on the methods it defines
+ * Each class is only responsible for access control on the methods it defines
+ * If `$allowed_actions` is defined as an empty array, no actions are allowed
+ * If `$allowed_actions` is undefined, all public methods on the specific class are allowed 
+   (not recommended)
+ * Access checks on parent classes need to be overwritten via the Config API
+ * Only public methods can be made accessible
  * If a method on a parent class is overwritten, access control for it has to be redefined as well
- * An action named "index" is whitelisted by default
+ * An action named "index" is whitelisted by default, 
- * A wildcard (`*`) can be used to define access control for all methods (incl. methods on parent classes)
+   unless allowed_actions is defined as an empty array,
- * Specific method entries in `$allowed_actions` overrule any `*` settings
+   or the action is specifically restricted in there.
  * Methods returning forms also count as actions which need to be defined
  * Form action methods (targets of `FormAction`) should NOT be included in `$allowed_actions`,
    they're handled separately through the form routing (see the ["forms" topic](/topics/forms))
  * `$allowed_actions` can be defined on `Extension` classes applying to the controller.
 
-
 If the permission check fails, SilverStripe will return a "403 Forbidden" HTTP status.
 
 ### Through the action
@@ -158,6 +162,21 @@ through `/fastfood/drivethrough/` to use the same order function.
 	        'drivethrough/$Action/$ID/$Name' => 'order'
 	    );
 
+## Access Control
+
+### Through $allowed_actions
+
+ * If `$allowed_actions` is undefined, `null` or `array()`, no actions are accessible
+ * Each class is only responsible for access control on the methods it defines
+ * Access checks on parent classes need to be overwritten via the Config API
+ * Only public methods can be made accessible
+ * If a method on a parent class is overwritten, access control for it has to be redefined as well
+ * An action named "index" is whitelisted by default
+ * Methods returning forms also count as actions which need to be defined
+ * Form action methods (targets of `FormAction`) should NOT be included in `$allowed_actions`,
+   they're handled separately through the form routing (see the ["forms" topic](/topics/forms))
+ * `$allowed_actions` can be defined on `Extension` classes applying to the controller.
+
 ## URL Patterns
 
 The `[api:RequestHandler]` class will parse all rules you specify against the 

tests/control/ControllerTest.php

@@ -6,6 +6,12 @@ class ControllerTest extends FunctionalTest {
 
 	protected $autoFollowRedirection = false;
 
+	protected $requiredExtensions = array(
+		'ControllerTest_AccessBaseController' => array(
+			'ControllerTest_AccessBaseControllerExtension'
+		)
+	);
+	
 	public function testDefaultAction() {
 		/* For a controller with a template, the default action will simple run that template. */
 		$response = $this->get("ControllerTest_Controller/");
@@ -38,27 +44,69 @@ class ControllerTest extends FunctionalTest {
 	public function testAllowedActions() {
 		$adminUser = $this->objFromFixture('Member', 'admin');
 
-		$response = $this->get("ControllerTest_AccessBaseController/unsecuredaction");
+		$response = $this->get("ControllerTest_UnsecuredController/");
 		$this->assertEquals(200, $response->getStatusCode(),
-			'Access granted on action without $allowed_actions on defining controller'
+			'Access granted on index action without $allowed_actions on defining controller, ' .
+			'when called without an action in the URL'
 		);
 
-		$response = $this->get("ControllerTest_AccessBaseController/onlysecuredinsubclassaction");
+		$response = $this->get("ControllerTest_UnsecuredController/index");
+		$this->assertEquals(200, $response->getStatusCode(),
+			'Access granted on index action without $allowed_actions on defining controller, ' .
+			'when called with an action in the URL'
+		);
+
+		$response = $this->get("ControllerTest_UnsecuredController/method1");
 		$this->assertEquals(200, $response->getStatusCode(),
 			'Access granted on action without $allowed_actions on defining controller, ' .
-			'even when action is secured in subclasses'
+			'when called without an action in the URL'
 		);
 
-		$response = $this->get("ControllerTest_AccessSecuredController/onlysecuredinsubclassaction");
+		$response = $this->get("ControllerTest_AccessBaseController/");
+		$this->assertEquals(200, $response->getStatusCode(),
+			'Access granted on index with empty $allowed_actions on defining controller, ' .
+			'when called without an action in the URL'
+		);
+
+		$response = $this->get("ControllerTest_AccessBaseController/index");
+		$this->assertEquals(200, $response->getStatusCode(),
+			'Access granted on index with empty $allowed_actions on defining controller, ' .
+			'when called with an action in the URL'
+		);
+		
+		$response = $this->get("ControllerTest_AccessBaseController/method1");
 		$this->assertEquals(403, $response->getStatusCode(),
-			'Access denied on action with $allowed_actions on defining controller, ' .
+			'Access denied on action with empty $allowed_actions on defining controller'
-			'even if action is unsecured on parent class'
+		);
+
+		$response = $this->get("ControllerTest_AccessBaseController/method2");
+		$this->assertEquals(403, $response->getStatusCode(),
+			'Access denied on action with empty $allowed_actions on defining controller, ' .
+			'even when action is allowed in subclasses (allowed_actions don\'t inherit)'
+		);
+
+		$response = $this->get("ControllerTest_AccessSecuredController/");
+		$this->assertEquals(200, $response->getStatusCode(),
+			'Access granted on index with non-empty $allowed_actions on defining controller, ' .
+			'even when index isn\'t specifically mentioned in there'
+		);
+		
+		$response = $this->get("ControllerTest_AccessSecuredController/method1");
+		$this->assertEquals(403, $response->getStatusCode(),
+			'Access denied on action which is only defined in parent controller, ' .
+			'even when action is allowed in currently called class (allowed_actions don\'t inherit)'
+		);
+
+		$response = $this->get("ControllerTest_AccessSecuredController/method2");
+		$this->assertEquals(200, $response->getStatusCode(),
+			'Access grante on action originally defined with empty $allowed_actions on parent controller, ' .
+			'because it has been redefined in the subclass'
 		);
 
 		$response = $this->get("ControllerTest_AccessSecuredController/adminonly");
 		$this->assertEquals(403, $response->getStatusCode(),
 			'Access denied on action with $allowed_actions on defining controller, ' .
-			'when action is not defined on any parent classes'
+			'when restricted by unmatched permission code'
 		);
 
 		$response = $this->get("ControllerTest_AccessSecuredController/aDmiNOnlY");
@@ -67,49 +115,65 @@ class ControllerTest extends FunctionalTest {
 			'regardless of capitalization'
 		);
 		
-		// TODO Change this API
+		$response = $this->get('ControllerTest_AccessSecuredController/protectedmethod');
-		$response = $this->get('ControllerTest_AccessUnsecuredSubController/unsecuredaction');
-		$this->assertEquals(200, $response->getStatusCode(), 
-			"Controller without a specified allowed_actions allows its own actions through"
-		);
-
-		$response = $this->get('ControllerTest_AccessUnsecuredSubController/adminonly');
-		$this->assertEquals(403, $response->getStatusCode(), 
-			"Controller without a specified allowed_actions still disallows actions defined on parents"
-		);
-		
-		$response = $this->get("ControllerTest_AccessAsteriskSecuredController/index");
-		$this->assertEquals(403, $response->getStatusCode(),
-			"Actions can be globally disallowed by using asterisk (*) for index method"
-		);
-		
-		$response = $this->get("ControllerTest_AccessAsteriskSecuredController/onlysecuredinsubclassaction");
 		$this->assertEquals(404, $response->getStatusCode(), 
-			"Actions can be globally disallowed by using asterisk (*) instead of a method name, " .
+			"Access denied to protected method even if its listed in allowed_actions"
-			"in which case they'll be marked as 'not found'"
-		);
-		
-		$response = $this->get("ControllerTest_AccessAsteriskSecuredController/unsecuredaction");
-		$this->assertEquals(200, $response->getStatusCode(),
-			"Actions can be overridden to be allowed if globally disallowed by using asterisk (*)"
 		);
 				
 		$this->session()->inst_set('loggedInAs', $adminUser->ID);
 		$response = $this->get("ControllerTest_AccessSecuredController/adminonly");
-		$this->assertEquals(
+		$this->assertEquals(200, $response->getStatusCode(), 
-			200, 
-			$response->getStatusCode(), 
 			"Permission codes are respected when set in \$allowed_actions"
 		);
+		$this->session()->inst_set('loggedInAs', null);
 
-		$response = $this->get("ControllerTest_AccessUnsecuredSubController/adminonly");
+		$response = $this->get('ControllerTest_AccessBaseController/extensionmethod1');
 		$this->assertEquals(200, $response->getStatusCode(), 
-			"Actions can be globally disallowed by using asterisk (*) instead of a method name"
+			"Access granted to method defined in allowed_actions on extension, " .
+			"where method is also defined on extension"
 		);
 
+		$response = $this->get('ControllerTest_AccessSecuredController/extensionmethod1');
+		$this->assertEquals(200, $response->getStatusCode(), 
+			"Access granted to method defined in allowed_actions on extension, " .
+			"where method is also defined on extension, even when called in a subclass"
+		);
+
+		$response = $this->get('ControllerTest_AccessBaseController/extensionmethod2');
+		$this->assertEquals(404, $response->getStatusCode(), 
+			"Access denied to method not defined in allowed_actions on extension, " .
+			"where method is also defined on extension"
+		);
+
+		$response = $this->get('ControllerTest_IndexSecuredController/');
+		$this->assertEquals(403, $response->getStatusCode(), 
+			"Access denied when index action is limited through allowed_actions, " .
+			"and doesn't satisfy checks, and action is empty"
+		);
+
+		$response = $this->get('ControllerTest_IndexSecuredController/index');
+		$this->assertEquals(403, $response->getStatusCode(), 
+			"Access denied when index action is limited through allowed_actions, " .
+			"and doesn't satisfy checks"
+		);
+
+		$this->session()->inst_set('loggedInAs', $adminUser->ID);
+		$response = $this->get('ControllerTest_IndexSecuredController/');
+		$this->assertEquals(200, $response->getStatusCode(), 
+			"Access granted when index action is limited through allowed_actions, " .
+			"and does satisfy checks"
+		);
 		$this->session()->inst_set('loggedInAs', null);
 	}
 
+	/**
+	 * @expectedException PHPUnit_Framework_Error
+	 * @expectedExceptionMessage Wildcards (*) are no longer valid
+	 */
+	public function testWildcardAllowedActions() {
+		$this->get('ControllerTest_AccessWildcardSecuredController');
+	}
+
 	/**
 	 * Test Controller::join_links()
 	 */
@@ -161,20 +225,60 @@ class ControllerTest extends FunctionalTest {
 	 */
 	public function testHasAction() {
 		$controller = new ControllerTest_HasAction();
+		$unsecuredController = new ControllerTest_HasAction_Unsecured();
+		$securedController = new ControllerTest_AccessSecuredController();
 		
-		$this->assertFalse($controller->hasAction('1'), 'Numeric actions do not slip through.');
+		$this->assertFalse(
-		//$this->assertFalse($controller->hasAction('lowercase_permission'),
+			$controller->hasAction('1'), 
-		//'Lowercase permission does not slip through.');
+			'Numeric actions do not slip through.'
-		$this->assertFalse($controller->hasAction('undefined'), 'undefined actions do not exist');
+		);
-		$this->assertTrue($controller->hasAction('allowed_action'), 'allowed actions are recognised');
+		//$this->assertFalse(
-		$this->assertTrue($controller->hasAction('template_action'), 'action-specific templates are recognised');
+		//	$controller->hasAction('lowercase_permission'),
-		
+		//	'Lowercase permission does not slip through.'
-		$unsecured = new ControllerTest_HasAction_Unsecured();
+		//);
+		$this->assertFalse(
+			$controller->hasAction('undefined'), 
+			'undefined actions do not exist'
+		);
+		$this->assertTrue(
+			$controller->hasAction('allowed_action'), 
+			'allowed actions are recognised'
+		);
+		$this->assertTrue(
+			$controller->hasAction('template_action'), 
+			'action-specific templates are recognised'
+		);
 		
 		$this->assertTrue (
-			$unsecured->hasAction('defined_action'),
+			$unsecuredController->hasAction('defined_action'),
 			'Without an allowed_actions, any defined methods are recognised as actions'
 		);
+
+		$this->assertTrue(
+			$securedController->hasAction('adminonly'),
+			'Method is generally visible even if its denied via allowed_actions'
+		);
+
+		$this->assertFalse(
+			$securedController->hasAction('protectedmethod'),
+			'Method is not visible when protected, even if its defined in allowed_actions'
+		);
+
+		$this->assertTrue(
+			$securedController->hasAction('extensionmethod1'),
+			'Method is visible when defined on an extension and part of allowed_actions'
+		);
+
+		$this->assertFalse(
+			$securedController->hasAction('internalextensionmethod'),
+			'Method is not visible when defined on an extension, but not part of allowed_actions'
+		);
+
+		$this->assertFalse(
+			$securedController->hasAction('protectedextensionmethod'),
+			'Method is not visible when defined on an extension, part of allowed_actions, ' .
+			'but with protected visibility'
+		);
 	}
 
 	/* Controller::BaseURL no longer exists, but was just a direct call to Director::BaseURL, so not sure what this
@@ -237,8 +341,15 @@ class ControllerTest extends FunctionalTest {
  * Simple controller for testing
  */
 class ControllerTest_Controller extends Controller implements TestOnly {
+	
 	public $Content = "default content";
 
+	public static $allowed_actions = array(
+		'methodaction',
+		'stringaction',
+		'redirectbacktest',
+	);
+	
 	public function methodaction() {
 		return array(
 			"Content" => "methodaction content"
@@ -254,111 +365,82 @@ class ControllerTest_Controller extends Controller implements TestOnly {
 	}
 }
 
-/**
+class ControllerTest_UnsecuredController extends Controller implements TestOnly {
- * Allowed actions flattened:
- * - unsecuredaction: *
- * - onlysecuredinsubclassaction: *
- * - unsecuredinparentclassaction: *
- * - unsecuredinsubclassaction: *
- */
-class ControllerTest_AccessBaseController extends Controller implements TestOnly {
-	// Accessible by all
-	public function unsecuredaction() {
-		return 'unsecuredaction was called.';
-	}
 
-	// Accessible by all
+	// Not defined, allow access to all
-	public function onlysecuredinsubclassaction() {
+	// static $allowed_actions = array();
-		return 'onlysecuredinsubclass was called.';
-	}
 	
-	// Accessible by all
+	// Granted for all
-	public function unsecuredinparentclassaction() {
+	public function method1() {}
-		return 'unsecuredinparentclassaction was called.';
-	}
 
-	// Accessible by all
+	// Granted for all
-	public function unsecuredinsubclassaction() {
+	public function method2() {}
-		return 'unsecuredinsubclass was called.';
+}
-	}
+
+class ControllerTest_AccessBaseController extends Controller implements TestOnly {
+
+	static $allowed_actions = array();
+	
+	// Denied for all
+	public function method1() {}
+
+	// Denied for all
+	public function method2() {}
 }
 
-/**
- * Allowed actions flattened:
- * - unsecuredaction: *
- * - onlysecuredinsubclassaction: ADMIN (parent: *)
- * - unsecuredinparentclassaction: *
- * - unsecuredinsubclassaction: (none) (parent: *)
- * - adminonly: ADMIN
- */
 class ControllerTest_AccessSecuredController extends ControllerTest_AccessBaseController implements TestOnly {
 	
 	static $allowed_actions = array(
-		"onlysecuredinsubclassaction" => 'ADMIN',
+		"method1", // denied because only defined in parent
+		"method2" => true, // granted because its redefined
 		"adminonly" => "ADMIN",
+		"protectedmethod" => true, // denied because its protected
 	);
 
-	// Accessible by ADMIN only
+	public function method2() {}
-	public function onlysecuredinsubclassaction() {
-		return 'onlysecuredinsubclass was called.';
-	}
 		
-	// Not accessible, since overloaded but not mentioned in $allowed_actions
+	public function adminonly() {}
-	public function unsecuredinsubclassaction() {
-		return 'unsecuredinsubclass was called.';
-	}
 
-	// Accessible by all, since only defined in parent class
+	protected function protectedmethod()  {}
-	//public function unsecuredinparentclassaction() {
 
-	// Accessible by ADMIN only
-	public function adminonly() {
-		return "You must be an admin!";
-	}
 }
 
-/**
+class ControllerTest_AccessWildcardSecuredController extends ControllerTest_AccessBaseController implements TestOnly {
- * Allowed actions flattened:
- * - unsecuredaction: *
- * - onlysecuredinsubclassaction: ADMIN (parent: *)
- * - unsecuredinparentclassaction: ADMIN (parent: *)
- * - unsecuredinsubclassaction: (none) (parent: *)
- * - adminonly: ADMIN (parent: ADMIN)
- */
-class ControllerTest_AccessAsteriskSecuredController extends ControllerTest_AccessBaseController implements TestOnly {
 	
 	static $allowed_actions = array(
-		"*" => "ADMIN",
+		"*" => "ADMIN", // should throw exception
-		'unsecuredaction' => true,
 	);
 	
-	// Accessible by ADMIN only
-	public function adminonly() {
-		return "You must be an admin!";
-	}
-	
-	// Accessible by all
-	public function unsecuredaction() {
-		return "Allowed for everybody";
-	}
 }
 
-/**
+class ControllerTest_IndexSecuredController extends ControllerTest_AccessBaseController implements TestOnly {
- * Allowed actions flattened:
- * - unsecuredaction: *
- * - onlysecuredinsubclassaction: ADMIN (parent: *)
- * - unsecuredinparentclassaction: *
- * - unsecuredinsubclassaction: (none) (parent: *)
- * - adminonly: ADMIN
- */
-class ControllerTest_AccessUnsecuredSubController extends ControllerTest_AccessSecuredController implements TestOnly {
 	
-	// No $allowed_actions defined here
+	static $allowed_actions = array(
+		"index" => "ADMIN",
+	);
+	
+}
+
+class ControllerTest_AccessBaseControllerExtension extends Extension implements TestOnly {
+
+	static $allowed_actions = array(
+		"extensionmethod1" => true, // granted because defined on this class
+		"method1" => true, // ignored because method not defined on this class
+		"method2" => true, // ignored because method not defined on this class
+		"protectedextensionmethod" => true, // ignored because method is protected
+	);
+
+	// Allowed for all
+	public function extensionmethod1() {}
+
+	// Denied for all, not defined
+	public function extensionmethod2() {}
+
+	// Denied because its protected
+	protected function protectedextensionmethod() {}
+
+	public function internalextensionmethod() {}
 
-	// Accessible by ADMIN only, defined in parent class
-	public function onlysecuredinsubclassaction() {
-		return 'onlysecuredinsubclass was called.';
-	}
 }
 
 class ControllerTest_HasAction extends Controller {