From 64680df68f61b914e5728b8aae47fc8c1011bd30 Mon Sep 17 00:00:00 2001 From: Ryan Cramer Date: Fri, 14 Sep 2018 11:02:30 -0400 Subject: [PATCH] Refactoring of ProcessController and add support for access controlled methods that works with the existing moduleInfo['nav'][]['permission'] setting. Previously that permission was only used to determine whether to show the item in the nav, and you had to do your own separate permission check in the actual method implementation. This commit also moves all the WireException definitions to the main core/Exceptions file. --- wire/core/Exceptions.php | 42 ++++++- wire/core/PageFinder.php | 3 - wire/core/Process.php | 11 +- wire/core/ProcessController.php | 209 +++++++++++++++++++++----------- wire/core/SessionCSRF.php | 6 - 5 files changed, 181 insertions(+), 90 deletions(-) diff --git a/wire/core/Exceptions.php b/wire/core/Exceptions.php index 69d1f521..f1d9a25f 100644 --- a/wire/core/Exceptions.php +++ b/wire/core/Exceptions.php @@ -8,7 +8,7 @@ * This file is licensed under the MIT license * https://processwire.com/about/license/mit/ * - * ProcessWire 3.x, Copyright 2016 by Ryan Cramer + * ProcessWire 3.x, Copyright 2018 by Ryan Cramer * https://processwire.com * */ @@ -20,21 +20,51 @@ class WireException extends \Exception {} /** - * Triggered when access to a resource is not allowed + * Thrown when access to a resource is not allowed * */ class WirePermissionException extends WireException {} /** - * Triggered when a requested item does not exist and generates a fatal error + * Thrown when a requested page does not exist, or can be thrown manually to show the 404 page * */ class Wire404Exception extends WireException {} /** - * WireDatabaseException is the exception thrown by the Database class - * - * If you use this class without ProcessWire, change 'extends WireException' below to be just 'extends Exception' + * Thrown when ProcessWire is unable to connect to the database at boot * */ class WireDatabaseException extends WireException {} + +/** + * Thrown when cross site request forgery detected by SessionCSRF::validate() + * + */ +class WireCSRFException extends WireException {} + +/** + * Thrown when a requested Process or Process method is requested that doesn’t exist + * + */ +class ProcessController404Exception extends Wire404Exception { } + +/** + * Thrown when the user doesn’t have access to execute the requested Process or method + * + */ +class ProcessControllerPermissionException extends WirePermissionException { } + +/** + * Thrown by PageFinder when an error occurs trying to find pages + * + */ +class PageFinderException extends WireException { } + +/** + * Thrown by PageFinder when it detects an error in the syntax of a given page-finding selector + * + */ +class PageFinderSyntaxException extends PageFinderException { } + + diff --git a/wire/core/PageFinder.php b/wire/core/PageFinder.php index 13576edb..c7ee5f17 100644 --- a/wire/core/PageFinder.php +++ b/wire/core/PageFinder.php @@ -1,8 +1,5 @@ wire('modules')->getModuleInfo($processName, array('verbose' => false)); + $info = $this->wire('modules')->getModuleInfoVerbose($processName); + $this->processInfo = $info; if(!empty($info['permission'])) $permissionName = $info['permission']; $this->hasPermission($permissionName, true); // throws exception if no permission @@ -186,8 +183,6 @@ class ProcessController extends Wire { * * Note: an empty permission name is accessible only by the superuser * - * @todo: This may now be completely unnecessary since permission checking is built into Modules.php - * * @param string $permissionName * @param bool $throw Whether to throw an Exception if the user does not have permission * @return bool @@ -198,47 +193,112 @@ class ProcessController extends Wire { $user = $this->wire('user'); if($user->isSuperuser()) return true; if($permissionName && $user->hasPermission($permissionName)) return true; - if($throw) throw new ProcessControllerPermissionException("You don't have $permissionName permission"); + if($throw) { + throw new ProcessControllerPermissionException( + sprintf($this->_('You do not have “%s” permission'), $permissionName) + ); + } return false; } + /** + * Does user have permission for the given $method name in the current Process? + * + * @param string $method + * @param bool $throw Throw exception if not permission? + * @return bool + * @throws ProcessControllerPermissionException + * + */ + protected function hasMethodPermission($method, $throw = true) { + // i.e. executeHelloWorld => helloWorld + $urlSegment = $method; + if(strpos($method, 'execute') === 0) list(,$urlSegment) = explode('execute', $method, 2); + $urlSegment = $this->wire('sanitizer')->hyphenCase($urlSegment); + if(!$this->hasUrlSegmentPermission($urlSegment, $throw)) return false; + return true; + } + + /** + * Does user have permission for the given urlSegment in the current Process? + * + * @param string $urlSegment + * @param bool $throw Throw exception if not permission? + * @return bool + * @throws ProcessControllerPermissionException + * + */ + protected function hasUrlSegmentPermission($urlSegment, $throw = true) { + + if(empty($this->processInfo['nav']) || $this->wire('user')->isSuperuser()) return true; + $hasPermission = true; + $urlSegment = trim(strtolower($urlSegment), '.-_'); + + foreach($this->processInfo['nav'] as $navItem) { + if(empty($navItem['permission'])) continue; + $navSegment = strtolower(trim($navItem['url'], './')); + if(empty($navSegment)) continue; + if(strpos($navSegment, '/') !== false) list($navSegment,) = explode($navSegment, '/', 2); + $navSegmentAlt = str_replace('-', '', $navSegment); + if($urlSegment === $navSegment || $urlSegment === $navSegmentAlt) { + $hasPermission = $this->hasPermission($navItem['permission'], $throw); + break; + } + } + + return $hasPermission; + } + /** * Get the name of the method to execute with the Process * * @param Process @process * @return string + * @throws ProcessControllerPermissionException * */ public function getProcessMethodName(Process $process) { - $method = $this->processMethodName; + $forceFail = false; + $urlSegment1 = $this->wire('input')->urlSegment1; + $method = self::defaultProcessMethodName; + $sanitizer = $this->wire('sanitizer'); - if(!$method) { - $method = self::defaultProcessMethodName; - // urlSegment as given by ProcessPageView - $urlSegment1 = $this->input->urlSegment1; - if($urlSegment1 && !$this->user->isGuest()) { - if(strpos($urlSegment1, '-')) { - // urlSegment1 has multiple hyphenated parts: convert hello-world to HelloWorld - foreach(explode('-', $urlSegment1) as $v) $method .= ucfirst($v); - } else { - // just one part - $method .= ucfirst($urlSegment1); - } + if($this->processMethodName) { + // the method to use has been preset with the setProcessMethodName() function + $method = $this->processMethodName; + if($method !== self::defaultProcessMethodName) { + $this->hasMethodPermission($method); + } + + } else if(strlen($urlSegment1) && !$this->wire('user')->isGuest()) { + // determine requested method from urlSegment1 + // $urlSegment1 = trim($this->wire('sanitizer')->hyphenCase($urlSegment1, array('allow' => 'a-z0-9_')), '_'); + if(ctype_alpha($urlSegment1)) { + $methodName = ucfirst($urlSegment1); + $hyphenName = $urlSegment1; + } else { + $methodName = trim($sanitizer->pascalCase($urlSegment1, array('allowUnderscore' => true)), '_'); + $hyphenName = trim($sanitizer->hyphenCase($methodName, array('allowUnderscore' => true)), '_'); + } + if($hyphenName != strtolower($urlSegment1) && strtolower($methodName) != strtolower($urlSegment1)) { + // if urlSegment changed from sanitization, likely not in valid format + $forceFail = true; + } else { + // valid + $method .= $methodName; // execute => executeHelloWorld + $this->hasUrlSegmentPermission($hyphenName); } } - + + if($forceFail) return ''; if($method === 'executed') return ''; - $hookedMethod = "___$method"; - - if(method_exists($process, $method) - || method_exists($process, $hookedMethod) - || $process->hasHook($method . '()')) { - return $method; - } else { - return ''; - } + if(method_exists($process, $method)) return $method; + if(method_exists($process, "___$method")) return $method; + if($process->hasHook($method . '()')) return $method; + + return ''; } /** @@ -250,44 +310,53 @@ class ProcessController extends Wire { */ public function ___execute() { - $content = ''; - $method = ''; $debug = $this->wire('config')->debug; $breadcrumbs = $this->wire('breadcrumbs'); $headline = $this->wire('processHeadline'); $numBreadcrumbs = $breadcrumbs ? count($breadcrumbs) : null; - if($process = $this->getProcess()) { - if($method = $this->getProcessMethodName($this->process)) { - $className = $this->process->className(); - if($debug) Debug::timer("$className.$method()"); - $content = $this->process->$method(); - if($debug) Debug::saveTimer("$className.$method()"); - if($method != 'execute') { - // some method other than the main one - if(!is_null($numBreadcrumbs) && $numBreadcrumbs === count($breadcrumbs)) { - // process added no breadcrumbs, but there should be more - if($headline === $this->wire('processHeadline')) $process->headline(str_replace('execute', '', $method)); - $moduleInfo = $this->wire('modules')->getModuleInfo($process); - $href = substr($this->wire('input')->url(), -1) == '/' ? '../' : './'; - $process->breadcrumb($href, $moduleInfo['title']); - } - } - $this->process->executed($method); - } else { - throw new ProcessController404Exception("Unrecognized path"); - } + $process = $this->getProcess(); + + if(!$process) { + throw new ProcessController404Exception("Process does not exist: $this->processError"); + } - } else { - throw new ProcessController404Exception("The requested process does not exist - $this->processError"); + // determine method (throws ProcessControllerPermissionException if no access) + $method = $this->getProcessMethodName($process); + + if(!$method) { + throw new ProcessController404Exception("Unrecognized path"); } - if(empty($content) || is_bool($content)) { - $content = $this->process->getViewVars(); + // call method from Process (and time it if debug mode enabled) + $className = $process->className(); + if($debug) Debug::timer("$className.$method()"); + $content = $process->$method(); + if($debug) Debug::saveTimer("$className.$method()"); + + // setup breadcrumbs if in some method other than the main execute() method + if($method !== 'execute') { + // some method other than the main one + if(!is_null($numBreadcrumbs) && $numBreadcrumbs === count($breadcrumbs)) { + // process added no breadcrumbs, but there should be more + if($headline === $this->wire('processHeadline')) { + $process->headline(str_replace('execute', '', $method)); + } + $href = substr($this->wire('input')->url(), -1) == '/' ? '../' : './'; + $process->breadcrumb($href, $this->processInfo['title']); + } } + + // triggered "executed" (execute done) hook + $process->executed($method); + + if(empty($content) || is_bool($content)) { + $content = $process->getViewVars(); + } + if(is_array($content)) { // array of returned content indicates variables to send to a view - if(count($content) || $this->process->getViewFile()) { - $viewFile = $this->getViewFile($this->process, $method); + if(count($content) || $process->getViewFile()) { + $viewFile = $this->getViewFile($process, $method); if($viewFile) { // get output from a separate view file $template = $this->wire(new TemplateFile($viewFile)); diff --git a/wire/core/SessionCSRF.php b/wire/core/SessionCSRF.php index 9f071c93..ed47be3c 100644 --- a/wire/core/SessionCSRF.php +++ b/wire/core/SessionCSRF.php @@ -8,12 +8,6 @@ * */ -/** - * Exception triggered by SessionCSRF::validate() when CSRF detected - * - */ -class WireCSRFException extends WireException {} - /** * ProcessWire CSRF Protection *