Permalink
Browse files

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
  • Loading branch information...
1 parent 7efae6b commit 5fd55a50f207408fb07a5b46970a3705aca24507 Hamish Friedlander committed Feb 18, 2013
Showing with 413 additions and 206 deletions.
  1. +101 −70 control/RequestHandler.php
  2. +75 −0 docs/en/changelogs/3.1.0.md
  3. +24 −5 docs/en/topics/controller.md
  4. +213 −131 tests/control/ControllerTest.php
View
@@ -231,16 +231,37 @@ public function handleRequest(SS_HTTPRequest $request, DataModel $model) {
}
/**
- * Get a unified array of allowed actions on this controller (if such data is available) from both the controller
- * ancestry and any extensions.
+ * Get a array of allowed actions defined on this controller,
+ * 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');
+ }
- $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,35 +271,48 @@ public function allowedActions() {
}
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
*/
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
- // key or value. Note that if the action is numeric, then keys are not
- // searched for actions to prevent actual array keys being recognised
- // as actions.
+ // Check if the action is defined in the allowed actions of any ancestry class
+ // 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 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 || $isWildcard) return true;
+ if($isKey || $isValue) return true;
}
- if(!is_array($actions) || !$this->config()->get('allowed_actions',
- Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES)) {
-
+ $actionsWithoutExtra = $this->config()->get(
+ 'allowed_actions',
+ Config::UNINHERITED | Config::EXCLUDE_EXTRA_SOURCES
+ );
+ if(!is_array($actions) || !$actionsWithoutExtra) {
if($action != 'init' && $action != 'run' && method_exists($this, $action)) return true;
}
@@ -291,69 +325,66 @@ public function hasAction($action) {
*/
public function checkAccessAction($action) {
$actionOrigCasing = $action;
- $action = strtolower($action);
- $allowedActions = $this->allowedActions();
+ $action = strtolower($action);
- if($allowedActions) {
- // 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 || $test === 1 || $test === '1') {
- // Case 1: TRUE should always allow access
- return true;
- } elseif(substr($test, 0, 2) == '->') {
- // Case 2: 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);
- } else {
- // Case 3: Value is a permission code to check the current member against
- return Permission::check($test);
- }
-
- } elseif((($key = array_search($actionOrAll, $allowedActions, true)) !== false) && is_numeric($key)) {
- // Case 4: Allow numeric array notation (search for array value as action instead of key)
- return true;
- }
+ $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();
}
- }
-
- // 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'
- );
- }
+ $allowedActions = $this->allowedActions($definingClass);
- // 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'));
+ // 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 {
- throw new Exception("method_exists() true but ReflectionClass can't find method");
+ // Value is a permission code to check the current member against
+ $isAllowed = Permission::check($test);
}
- } else if(!$this->hasMethod($action)){
- // Return true so that a template can handle this action
- return true;
+ } 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
+ $isAllowed = true;
}
-
- return false;
+
+ return $isAllowed;
}
/**
@@ -19,13 +19,88 @@
* 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
* Support for [Composer](http://getcomposer.org) dependency manager (also works with 3.0)
## 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.
@@ -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
- * A wildcard (`*`) can be used to define access control for all methods (incl. methods on parent classes)
- * Specific method entries in `$allowed_actions` overrule any `*` settings
+ * An action named "index" is whitelisted by default,
+ unless allowed_actions is defined as an empty array,
+ 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
Oops, something went wrong.

2 comments on commit 5fd55a5

Owner

dhensby replied Feb 18, 2013

This has broken the 3.1 CMS branch as pages can't be saved/published.

Owner

chillu replied Feb 18, 2013

This was missed in between all the merges and different internal branches, sorry about that.
Was actually a bug which existed for months, but now just surfaced through the fixes on allowed_actions.
Fixed with 14dcc82

Please sign in to comment.