From f1db583fb4f8dbe67cb4fc33103edecd0cfbe530 Mon Sep 17 00:00:00 2001 From: Simon Welsh Date: Mon, 23 Jul 2012 17:44:37 +1200 Subject: [PATCH 1/2] NEW Allow arguments to be passed to allowed_action checkers This allows arguments to be passed along in an $allowed_actions deceleration of the form 'action' => '->method' in the same way that arguments can be passed to extension constructors when adding them using $extensions or Object::add_extension. I.e. 'action' => '->checkerMethod(false, 7, 2, "yesterday") would call the checkerMethod method with the boolean false the numbers 7 and 2 and the string "yesterday" as its arguments. --- control/RequestHandler.php | 3 +- docs/en/topics/controller.md | 42 +++++++++++++++++++++++++++ tests/control/RequestHandlingTest.php | 21 ++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/control/RequestHandler.php b/control/RequestHandler.php index 27fa5b3d1..6fa4a1cc6 100644 --- a/control/RequestHandler.php +++ b/control/RequestHandler.php @@ -283,7 +283,8 @@ class RequestHandler extends ViewableData { return true; } elseif(substr($test, 0, 2) == '->') { // Case 2: Determined by custom method with "->" prefix - return $this->{substr($test, 2)}(); + list($method, $arguments) = Object::parse_class_spec(substr($test, 2)); + return call_user_func_array(array($this, $method), $arguments); } else { // Case 3: Value is a permission code to check the current member against return Permission::check($test); diff --git a/docs/en/topics/controller.md b/docs/en/topics/controller.md index 633a14216..243e922e2 100644 --- a/docs/en/topics/controller.md +++ b/docs/en/topics/controller.md @@ -113,6 +113,48 @@ will are assumed to be relative to the site-root. The `redirect()` method takes an optional HTTP status code, either `301` for permanent redirects, or `302` for temporary redirects (default). +## Access control + +You can also limit access to actions on a controller using the static `$allowed_actions` array. This allows you to always allow an action, or restrict it to a specific permission or to call a method that checks if the action is allowed. + +For example, the default `Controller::$allowed_actions` is + static $allowed_actions = array( + 'handleAction', + 'handleIndex', + ); +which allows the `handleAction` and `handleIndex` methods to be called via a URL. + +To allow any action on your controller to be called you can either leave your `$allowed_actions` array empty or not have one at all. This is the default behaviour, however it is not recommended as it allows anything on your controller to be called via a URL, including view-specific methods. + +The recommended approach is to explicitly state the actions that can be called via a URL. Any action not in the `$allowed_actions` array, excluding the default `index` method, is then unable to be called. + +To always allow an action to be called, you can either add the name of the action to the array or add a value of `true` to the array, using the name of the method as its index. For example + static $allowed_actions = array( + 'MyAwesomeAction', + 'MyOtherAction' => true + ); + + +To require that the current user has a certain permission before being allowed to call an action you add the action to the array as an index with the value being the permission code that user must have. For example + static $allowed_actions = array( + 'MyAwesomeAction', + 'MyOtherAction' => true, + 'MyLimitedAction' => 'CMS_ACCESS_CMSMain', + 'MyAdminAction' => 'ADMIN' + ); + +If neither of these are enough to decide if an action should be called, you can have the check use a method. The method must be on the controller class and return true if the action is allowed or false if it isn't. To do this add the action to the array as an index with the value being the name of the method to called preceded by '->'. You are able to pass static arguments to the method in much the same way as you can with extensions. Strings are enclosed in quotes, numeric values are written as numbers and true and false are written as true and false. For example + static $allowed_actions = array( + 'MyAwesomeAction', + 'MyOtherAction' => true, + 'MyLimitedAction' => 'CMS_ACCESS_CMSMain', + 'MyAdminAction' => 'ADMIN', + 'MyRestrictedAction' => '->myCheckerMethod("MyRestrictedAction", false, 42)', + 'MyLessRestrictedAction' => '->myCheckerMethod' + ); + +In this example, `MyAwesomeAction` and `MyOtherAction` are always allowed, `MyLimitedAction` requires access to the CMS for the current user and `MyAdminAction` requires the current user to be an admin. `MyRestrictedAction` calls the method `myCheckerMethod`, passing in the string "MyRestrictedAction", the boolean false and the number 42. `MyLessRestrictedAction` simply calls the method `myCheckerMethod` with no arguments. + ## API Documentation `[api:Controller]` diff --git a/tests/control/RequestHandlingTest.php b/tests/control/RequestHandlingTest.php index 78587bd07..f5e37e45b 100644 --- a/tests/control/RequestHandlingTest.php +++ b/tests/control/RequestHandlingTest.php @@ -126,6 +126,13 @@ class RequestHandlingTest extends FunctionalTest { $response = Director::test("RequestHandlingTest_AllowedController/extendedMethod"); $this->assertEquals("extendedMethod", $response->getBody()); + /* This action has been blocked by an argument to a method */ + $response = Director::test('RequestHandlingTest_AllowedController/blockMethod'); + $this->assertEquals(403, $response->getStatusCode()); + + /* Whereas this one has been allowed by a method without an argument */ + $response = Director::test('RequestHandlingTest_AllowedController/allowMethod'); + $this->assertEquals('allowMethod', $response->getBody()); } public function testHTTPException() { @@ -411,6 +418,8 @@ class RequestHandlingTest_AllowedController extends Controller implements TestOn static $allowed_actions = array( 'failoverMethod', // part of the failover object 'extendedMethod', // part of the RequestHandlingTest_ControllerExtension object + 'blockMethod' => '->provideAccess(false)', + 'allowMethod' => '->provideAccess', ); static $extensions = array( @@ -426,6 +435,18 @@ class RequestHandlingTest_AllowedController extends Controller implements TestOn function index($request) { return "This is the controller"; } + + function provideAccess($access = true) { + return $access; + } + + function blockMethod($request) { + return 'blockMethod'; + } + + function allowMethod($request) { + return 'allowMethod'; + } } /** From 78fdcc580bf89ea63ad6cb977bc45b8f73f7af53 Mon Sep 17 00:00:00 2001 From: Simon Welsh Date: Sat, 30 Jun 2012 22:37:58 +1200 Subject: [PATCH 2/2] API DataList->leftJoin()/innerJoin() args no longer escaped The table name in the join was being escaped, though table names aren't escaped anywhere else. This breaks namespaced classes, which rely on unescaped backslashes. --- model/DataList.php | 4 ++-- model/SQLQuery.php | 2 +- tests/model/DataListTest.php | 8 ++++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/model/DataList.php b/model/DataList.php index 2b7211483..4c8496606 100644 --- a/model/DataList.php +++ b/model/DataList.php @@ -533,7 +533,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab /** * Return a new DataList instance with an inner join clause added to this list's query. * - * @param string $table Table name (unquoted) + * @param string $table Table name (unquoted and as escaped SQL) * @param string $onClause Escaped SQL statement, e.g. '"Table1"."ID" = "Table2"."ID"' * @param string $alias - if you want this table to be aliased under another name * @return DataList @@ -547,7 +547,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab /** * Return a new DataList instance with a left join clause added to this list's query. * - * @param string $table Table name (unquoted) + * @param string $table Table name (unquoted and as escaped SQL) * @param string $onClause Escaped SQL statement, e.g. '"Table1"."ID" = "Table2"."ID"' * @param string $alias - if you want this table to be aliased under another name * @return DataList diff --git a/model/SQLQuery.php b/model/SQLQuery.php index 55f934473..3bb2c8dd3 100644 --- a/model/SQLQuery.php +++ b/model/SQLQuery.php @@ -874,7 +874,7 @@ class SQLQuery { else $filter = "(" . implode(") AND (", $join['filter']) . ")"; $aliasClause = ($alias != $join['table']) ? " AS \"" . Convert::raw2sql($alias) . "\"" : ""; - $this->from[$alias] = strtoupper($join['type']) . " JOIN \"" . Convert::raw2sql($join['table']) . "\"$aliasClause ON $filter"; + $this->from[$alias] = strtoupper($join['type']) . " JOIN \"" . $join['table'] . "\"$aliasClause ON $filter"; } } diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index be2dea797..d5c743c47 100644 --- a/tests/model/DataListTest.php +++ b/tests/model/DataListTest.php @@ -85,6 +85,14 @@ class DataListTest extends SapphireTest { $list->leftJoin('DataObjectTest_Team', '"DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"', 'Team'); $expected = 'SELECT DISTINCT "DataObjectTest_TeamComment"."ClassName", "DataObjectTest_TeamComment"."Created", "DataObjectTest_TeamComment"."LastEdited", "DataObjectTest_TeamComment"."Name", "DataObjectTest_TeamComment"."Comment", "DataObjectTest_TeamComment"."TeamID", "DataObjectTest_TeamComment"."ID", CASE WHEN "DataObjectTest_TeamComment"."ClassName" IS NOT NULL THEN "DataObjectTest_TeamComment"."ClassName" ELSE '.$db->prepStringForDB('DataObjectTest_TeamComment').' END AS "RecordClassName" FROM "DataObjectTest_TeamComment" LEFT JOIN "DataObjectTest_Team" AS "Team" ON "DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"'; $this->assertEquals($expected, $list->sql()); + + // Test with namespaces (with non-sensical join, but good enough for testing) + $list = DataObjectTest_TeamComment::get(); + $list->leftJoin('DataObjectTest\NamespacedClass', '"DataObjectTest\NamespacedClass"."ID" = "DataObjectTest_TeamComment"."ID"'); + $expected = 'SELECT DISTINCT "DataObjectTest_TeamComment"."ClassName", "DataObjectTest_TeamComment"."Created", "DataObjectTest_TeamComment"."LastEdited", "DataObjectTest_TeamComment"."Name", "DataObjectTest_TeamComment"."Comment", "DataObjectTest_TeamComment"."TeamID", "DataObjectTest_TeamComment"."ID", CASE WHEN "DataObjectTest_TeamComment"."ClassName" IS NOT NULL THEN "DataObjectTest_TeamComment"."ClassName" ELSE '.$db->prepStringForDB('DataObjectTest_TeamComment').' END AS "RecordClassName" ' . + 'FROM "DataObjectTest_TeamComment" ' . + 'LEFT JOIN "DataObjectTest\NamespacedClass" ON "DataObjectTest\NamespacedClass"."ID" = "DataObjectTest_TeamComment"."ID"'; + $this->assertEquals($expected, $list->sql(), 'Retains backslashes in namespaced classes'); } function testToNestedArray() {