diff --git a/core/control/RequestHandler.php b/core/control/RequestHandler.php index f4e25e4ab..4996223bd 100644 --- a/core/control/RequestHandler.php +++ b/core/control/RequestHandler.php @@ -166,24 +166,31 @@ class RequestHandler extends ViewableData { if($action == 'index') return true; if($allowedActions) { + // convert all keys and values to lowercase for easier comparison (only if not set as boolean) foreach($allowedActions as $key => $value) { - $newAllowedActions[strtolower($key)] = strtolower($value); + $newAllowedActions[strtolower($key)] = (is_bool($value)) ? $value : strtolower($value); } - $allowedActions = $newAllowedActions; - if(isset($allowedActions[$action])) { - $test = $allowedActions[$action]; - - if($test === true) { - return true; - } elseif(substr($test, 0, 2) == '->') { - return $this->{substr($test, 2)}(); - } elseif(Permission::check($test)) { + // check for specific action rules first, and fall back to global rules defined by asterisk + foreach(array($action,'*') as $actionOrAll) { + // check if specific action is set + if(isset($allowedActions[$actionOrAll])) { + $test = $allowedActions[$actionOrAll]; + if($test === true) { + // Case 1: TRUE should always allow access + return true; + } elseif(substr($test, 0, 2) == '->') { + // Case 2: Determined by custom method with "->" prefix + return $this->{substr($test, 2)}(); + } elseif(Permission::check($test)) { + // Case 3: Value is a permission code to check the current member against + return true; + } + } elseif((($key = array_search($actionOrAll, $allowedActions)) !== false) && is_numeric($key)) { + // Case 4: Allow numeric array notation (search for array value as action instead of key) return true; } - } elseif((($key = array_search($action, $allowedActions)) !== false) && is_numeric($key)) { - return true; } } diff --git a/tests/ControllerTest.php b/tests/ControllerTest.php index 10f6e644e..ecb4b57d4 100644 --- a/tests/ControllerTest.php +++ b/tests/ControllerTest.php @@ -36,9 +36,20 @@ class ControllerTest extends SapphireTest { $response = Director::test("ControllerTest_SecuredController/adminonly"); $this->assertEquals(403, $response->getStatusCode()); - // test that a controller without a specified allowed_actions allows actions through $response = Director::test('ControllerTest_UnsecuredController/stringaction'); - $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals(200, $response->getStatusCode(), + "test that a controller without a specified allowed_actions allows actions through" + ); + + $response = Director::test("ControllerTest_FullSecuredController/adminonly"); + $this->assertEquals(403, $response->getStatusCode(), + "Actions can be globally disallowed by using asterisk (*) instead of a method name" + ); + + $response = Director::test("ControllerTest_FullSecuredController/unsecuredaction"); + $this->assertEquals(200, $response->getStatusCode(), + "Actions can be overridden to be allowed if globally disallowed by using asterisk (*)" + ); } /** @@ -111,4 +122,20 @@ class ControllerTest_SecuredController extends Controller { } } +class ControllerTest_FullSecuredController extends Controller { + + static $allowed_actions = array( + "*" => "ADMIN", + 'unsecuredaction' => true, + ); + + function adminonly() { + return "You must be an admin!"; + } + + function unsecuredaction() { + return "Allowed for everybody"; + } +} + class ControllerTest_UnsecuredController extends ControllerTest_SecuredController {} \ No newline at end of file