From 33e50a3bf16e75c49b245987d30c8f45034ff2db Mon Sep 17 00:00:00 2001 From: Andrew Paxley Date: Tue, 17 Oct 2023 13:53:34 +1300 Subject: [PATCH] ENH add granular dev url permissions --- src/Dev/BuildTask.php | 1 - src/Dev/DevBuildController.php | 38 +++++++++++++++++++++++- src/Dev/DevConfigController.php | 40 ++++++++++++++++++++++++-- src/Dev/DevelopmentAdmin.php | 51 ++++++++++++++++----------------- src/Dev/TaskRunner.php | 39 +++++++++++++------------ 5 files changed, 121 insertions(+), 48 deletions(-) diff --git a/src/Dev/BuildTask.php b/src/Dev/BuildTask.php index 10ca23adfb1..9b2659c53f0 100644 --- a/src/Dev/BuildTask.php +++ b/src/Dev/BuildTask.php @@ -7,7 +7,6 @@ use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Extensible; use SilverStripe\Core\Injector\Injectable; -use SilverStripe\Security\Permission; /** * Interface for a generic build task. Does not support dependencies. This will simply diff --git a/src/Dev/DevBuildController.php b/src/Dev/DevBuildController.php index 5f7a949fd7b..c22f4e091f2 100644 --- a/src/Dev/DevBuildController.php +++ b/src/Dev/DevBuildController.php @@ -7,8 +7,11 @@ use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\ORM\DatabaseAdmin; +use SilverStripe\Security\Permission; +use SilverStripe\Security\PermissionProvider; +use SilverStripe\Security\Security; -class DevBuildController extends Controller +class DevBuildController extends Controller implements PermissionProvider { private static $url_handlers = [ @@ -19,6 +22,15 @@ class DevBuildController extends Controller 'build' ]; + protected function init() + { + parent::init(); + + if (!$this->canInit()) { + Security::permissionFailure($this); + } + } + public function build(HTTPRequest $request): HTTPResponse { if (Director::is_cli()) { @@ -39,4 +51,28 @@ public function build(HTTPRequest $request): HTTPResponse return $response; } } + + public function canInit() + { + return ( + Director::isDev() + // We need to ensure that DevelopmentAdminTest can simulate permission failures when running + // "dev/tasks" from CLI. + || (Director::is_cli() && DevelopmentAdmin::config()->get('allow_all_cli')) + || Permission::check("ADMIN") + || Permission::check("CAN_DEV_BUILD") + ); + } + + public function providePermissions() + { + return [ + 'CAN_DEV_BUILD' => [ + 'name' => _t(__CLASS__ . '.CAN_DEV_BUILD_DESCRIPTION', 'Can execute a Dev/Build'), + 'help' => _t(__CLASS__ . '.CAN_DEV_BUILD_HELP', 'Can execute a Dev/Build (/dev/build).'), + 'category' => _t(__CLASS__ . 'PERMISSIONS_CATEGORY', 'Dev/Build permissions'), + 'sort' => 100 + ], + ]; + } } diff --git a/src/Dev/DevConfigController.php b/src/Dev/DevConfigController.php index 3b5d4886920..46e05c548ff 100644 --- a/src/Dev/DevConfigController.php +++ b/src/Dev/DevConfigController.php @@ -7,13 +7,16 @@ use SilverStripe\Control\HTTPResponse; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; -use Symfony\Component\Yaml\Yaml; use SilverStripe\Core\Injector\Injector; +use SilverStripe\Security\Permission; +use SilverStripe\Security\PermissionProvider; +use SilverStripe\Security\Security; +use Symfony\Component\Yaml\Yaml; /** * Outputs the full configuration. */ -class DevConfigController extends Controller +class DevConfigController extends Controller implements PermissionProvider { /** @@ -32,6 +35,15 @@ class DevConfigController extends Controller 'audit', ]; + protected function init() + { + parent::init(); + + if (!$this->canInit()) { + Security::permissionFailure($this); + } + } + /** * Note: config() method is already defined, so let's just use index() * @@ -129,6 +141,30 @@ public function audit() return $this->getResponse()->setBody($body); } + public function canInit() + { + return ( + Director::isDev() + // We need to ensure that DevelopmentAdminTest can simulate permission failures when running + // "dev/tasks" from CLI. + || (Director::is_cli() && DevelopmentAdmin::config()->get('allow_all_cli')) + || Permission::check("ADMIN") + || Permission::check("CAN_DEV_CONFIG") + ); + } + + public function providePermissions() + { + return [ + 'CAN_DEV_CONFIG' => [ + 'name' => _t(__CLASS__ . '.CAN_DEV_CONFIG_DESCRIPTION', 'Can see Dev/Config'), + 'help' => _t(__CLASS__ . '.CAN_DEV_CONFIG_HELP', 'Can see Dev/Config (/dev/config).'), + 'category' => _t(__CLASS__ . 'PERMISSIONS_CATEGORY', 'Dev/Config permissions'), + 'sort' => 100 + ], + ]; + } + /** * Returns all the keys of a multi-dimensional array while maintining any nested structure * diff --git a/src/Dev/DevelopmentAdmin.php b/src/Dev/DevelopmentAdmin.php index 854aa32efa8..91829eb0020 100644 --- a/src/Dev/DevelopmentAdmin.php +++ b/src/Dev/DevelopmentAdmin.php @@ -84,26 +84,8 @@ protected function init() if (static::config()->get('deny_non_cli') && !Director::is_cli()) { return $this->httpError(404); } - - // Special case for dev/build: Defer permission checks to DatabaseAdmin->init() (see #4957) - $requestedDevBuild = (stripos($this->getRequest()->getURL() ?? '', 'dev/build') === 0) - && (stripos($this->getRequest()->getURL() ?? '', 'dev/build/defaults') === false); - - // We allow access to this controller regardless of live-status or ADMIN permission only - // if on CLI. Access to this controller is always allowed in "dev-mode", or of the user is ADMIN. - $allowAllCLI = static::config()->get('allow_all_cli'); - $canAccess = ( - $requestedDevBuild - || Director::isDev() - || (Director::is_cli() && $allowAllCLI) - // Its important that we don't run this check if dev/build was requested - || Permission::check("ADMIN") - ); - - - - if (!$canAccess) { + if (!$this->canViewAll()) { Security::permissionFailure($this); return; } @@ -169,9 +151,6 @@ public function runRegisteredController(HTTPRequest $request) } } - - - /* * Internal methods */ @@ -186,8 +165,12 @@ protected static function get_links() $reg = Config::inst()->get(static::class, 'registered_controllers'); foreach ($reg as $registeredController) { if (isset($registeredController['links'])) { - foreach ($registeredController['links'] as $url => $desc) { - $links[$url] = $desc; + // Check access to controller + $controllerSingleton = Injector::inst()->get($registeredController['controller']); + if (!$controllerSingleton->hasMethod('canInit') || $controllerSingleton->canInit()) { + foreach ($registeredController['links'] as $url => $desc) { + $links[$url] = $desc; + } } } } @@ -207,8 +190,6 @@ protected function getRegisteredController($baseUrlPart) } - - /* * Unregistered (hidden) actions */ @@ -262,4 +243,22 @@ public function errors() { $this->redirect("Debug_"); } + + protected function canViewAll() + { + // Special case for dev/build: Defer permission checks to DatabaseAdmin->init() (see #4957) + $requestedDevBuild = (stripos($this->getRequest()->getURL() ?? '', 'dev/build') === 0) + && (stripos($this->getRequest()->getURL() ?? '', 'dev/build/defaults') === false); + + // We allow access to this controller regardless of live-status or ADMIN permission only + // if on CLI. Access to this controller is always allowed in "dev-mode", or of the user is ADMIN. + $allowAllCLI = static::config()->get('allow_all_cli'); + return ( + $requestedDevBuild + || Director::isDev() + || (Director::is_cli() && $allowAllCLI) + // Its important that we don't run this check if dev/build was requested + || Permission::check("ADMIN") + ); + } } diff --git a/src/Dev/TaskRunner.php b/src/Dev/TaskRunner.php index c320abec5f2..0ce6e05448e 100644 --- a/src/Dev/TaskRunner.php +++ b/src/Dev/TaskRunner.php @@ -44,18 +44,8 @@ protected function init() { parent::init(); - if (!$this->canViewAllTasks()) { - $canAccessAny = false; - foreach ($this->getTaskList() as $task) { - if ($task->hasMethod('canView') && $task->canView()) { - $canAccessAny = true; - break; - } - } - - if (!$canAccessAny) { - Security::permissionFailure($this); - } + if (!$this->canInit()) { + Security::permissionFailure($this); } } @@ -208,7 +198,7 @@ protected function canViewTask($class) } $task = Injector::inst()->get($class); - if ($task->hasMethod('canView') && $task->canView()) { + if (!$task->hasMethod('canView') || $task->canView()) { return true; } @@ -251,16 +241,29 @@ protected function addCssToHeader($header) return $header; } + + public function canInit() + { + if ($this->canViewAllTasks()) { + return true; + } + foreach ($this->getTaskList() as $task) { + if (!$task->hasMethod('canView') || $task->canView()) { + return true; + } + } + return false; + } - public function providePermissions() { + public function providePermissions() + { return [ 'BUILDTASK_CAN_RUN' => [ - 'name' => _t(__CLASS__.'.BUILDTASK_CAN_RUN_DESCRIPTION', 'Can view and run Build Tasks'), - 'help' => _t(__CLASS__.'.BUILDTASK_CAN_RUN_HELP', 'Can view and run Build Tasks via the endpoint (/dev/tasks).'), - 'category' => _t(__CLASS__.'PERMISSIONS_CATEGORY', 'Task permissions'), + 'name' => _t(__CLASS__ . '.BUILDTASK_CAN_RUN_DESCRIPTION', 'Can view and run Build Tasks'), + 'help' => _t(__CLASS__ . '.BUILDTASK_CAN_RUN_HELP', 'Can view and run Build Tasks via the endpoint (/dev/tasks).'), + 'category' => _t(__CLASS__ . 'PERMISSIONS_CATEGORY', 'Task permissions'), 'sort' => 100 ], ]; - } }