From 9b10232b732bd8fc61911a7919a14faa162e4794 Mon Sep 17 00:00:00 2001 From: Ryan Cramer Date: Wed, 13 Dec 2017 10:37:39 -0500 Subject: [PATCH] Add ability to specify roles that aren't allowed to login, related to processwire/processwire-requests#140 plus while I was in there, did some re-working of login related code in Session class and user management code in ProcessUser.module. --- wire/config.php | 26 ++ wire/core/Config.php | 2 + wire/core/Sanitizer.php | 9 +- wire/core/Session.php | 92 ++++-- wire/core/WireInput.php | 6 +- .../Process/ProcessForgotPassword.module | 9 +- .../Process/ProcessUser/ProcessUser.module | 282 ++++++++++++++---- 7 files changed, 325 insertions(+), 101 deletions(-) diff --git a/wire/config.php b/wire/config.php index b8a0b618..b377f943 100644 --- a/wire/config.php +++ b/wire/config.php @@ -331,6 +331,24 @@ $config->sessionHistory = 0; */ $config->userAuthHashType = 'sha1'; +/** + * Names (string) or IDs (int) of roles that are not allowed to login + * + * Note that you must create these roles yourself in the admin. When a user has + * one of these named roles, $session->login() will not accept a login from them. + * This affects the admin login form and any other login forms that use ProcessWire’s + * session system. + * + * The default value specifies a role name of "login-disabled", meaning if you create + * a role with that name, and assign it to a user, that user will no longer be able + * to login. + * + * @var array + * + */ +$config->loginDisabledRoles = array( + 'login-disabled' +); /*** 4. TEMPLATE FILES **************************************************************************/ @@ -700,6 +718,14 @@ $config->protectCSRF = true; */ $config->maxUrlSegments = 4; +/** + * Maximum length for any individual URL segment (default=128) + * + * @var int + * + */ +$config->maxUrlSegmentLength = 128; + /** * Maximum URL/path slashes (depth) for request URLs * diff --git a/wire/core/Config.php b/wire/core/Config.php index 7c51db4e..b1dc86de 100644 --- a/wire/core/Config.php +++ b/wire/core/Config.php @@ -63,6 +63,7 @@ * @property bool $sessionChallenge Should login sessions have a challenge key? (for extra security, recommended) #pw-group-session * @property bool $sessionFingerprint Should login sessions be tied to IP and user agent? 0 or false: Fingerprint off. 1 or true: Fingerprint on with default/recommended setting (currently 10). 2: Fingerprint only the remote IP. 4: Fingerprint only the forwarded/client IP (can be spoofed). 8: Fingerprint only the useragent. 10: Fingerprint the remote IP and useragent (default). 12: Fingerprint the forwarded/client IP and useragent. 14: Fingerprint the remote IP, forwarded/client IP and useragent (all). #pw-group-session * @property int $sessionHistory Number of session entries to keep (default=0, which means off). #pw-group-session + * @property array $loginDisabledRoles Array of role name(s) or ID(s) of roles where login is disallowed. #pw-group-session * * @property string $prependTemplateFile PHP file in /site/templates/ that will be loaded before each page's template file (default=none) #pw-group-template-files * @property string $appendTemplateFile PHP file in /site/templates/ that will be loaded after each page's template file (default=none) #pw-group-template-files @@ -79,6 +80,7 @@ * @property string $pageNumUrlPrefix Prefix used for pagination URLs. Default is "page", resulting in "/page1", "/page2", etc. #pw-group-URLs * @property array $pageNumUrlPrefixes Multiple prefixes that may be used for detecting pagination (internal use, for multi-language) #pw-group-URLs * @property int $maxUrlSegments Maximum number of extra stacked URL segments allowed in a page's URL (including page numbers) #pw-group-URLs + * @property int $maxUrlSegmentLength Maximum length of any individual URL segment (default=128). #pw-group-URLs * @property int $maxUrlDepth Maximum URL/path slashes (depth) for request URLs. (Min=10, Max=60) #pw-group-URLs * @property string $wireInputOrder Order that variables with the $input API var are handled when you access $input->var. #pw-group-HTTP-and-input * diff --git a/wire/core/Sanitizer.php b/wire/core/Sanitizer.php index f5b9aa42..6353d4ef 100644 --- a/wire/core/Sanitizer.php +++ b/wire/core/Sanitizer.php @@ -449,15 +449,16 @@ class Sanitizer extends Wire { * #pw-group-pages * * @param string $value Value to sanitize + * @param int $maxLength Maximum number of characters allowed * @return string Sanitized value * */ - public function pageNameUTF8($value) { + public function pageNameUTF8($value, $maxLength = 128) { if(!strlen($value)) return ''; // if UTF8 module is not enabled then delegate this call to regular pageName sanitizer - if($this->wire('config')->pageNameCharset != 'UTF8') return $this->pageName($value); + if($this->wire('config')->pageNameCharset != 'UTF8') return $this->pageName($value, false, $maxLength); // we don't allow UTF8 page names to be prefixed with "xn-" if(strpos($value, 'xn-') === 0) $value = substr($value, 3); @@ -469,7 +470,7 @@ class Sanitizer extends Wire { $extras = array('.', '-', '_', ' ', ',', ';', ':', '(', ')', '!', '?', '&', '%', '$', '#', '@'); // proceed only if value has some non-ascii characters - if(ctype_alnum(str_replace($extras, '', $value))) return $this->pageName($value); + if(ctype_alnum(str_replace($extras, '', $value))) return $this->pageName($value, false, $maxLength); // validate that all characters are in our whitelist $whitelist = $this->wire('config')->pageNameWhitelist; @@ -515,6 +516,8 @@ class Sanitizer extends Wire { // trim off any remaining separators/extras $value = trim($value, '-_.'); + if(mb_strlen($value) > $maxLength) $value = mb_substr($value, 0, $maxLength); + return $value; } diff --git a/wire/core/Session.php b/wire/core/Session.php index 4ffa4bc6..47637b12 100644 --- a/wire/core/Session.php +++ b/wire/core/Session.php @@ -17,13 +17,13 @@ * * @see https://processwire.com/api/ref/session/ Session documentation * - * @method User login() login($name, $pass) Login the user identified by $name and authenticated by $pass. Returns the user object on successful login or null on failure. + * @method User login() login($name, $pass, $force = false) Login the user identified by $name and authenticated by $pass. Returns the user object on successful login or null on failure. * @method Session logout() logout() Logout the current user, and clear all session variables. * @method void redirect() redirect($url, $http301 = true) Redirect this session to the specified URL. * @method void init() Initialize session (called automatically by constructor) #pw-hooker * @method bool authenticate(User $user, $pass) #pw-hooker * @method bool isValidSession($userID) #pw-hooker - * @method bool allowLogin($name) #pw-hooker + * @method bool allowLogin($name, User $user = null) #pw-hooker * @method void loginSuccess(User $user) #pw-hooker * @method void loginFailure($name, $reason) #pw-hooker * @method void logoutSuccess(User $user) #pw-hooker @@ -746,27 +746,42 @@ class Session extends Wire implements \IteratorAggregate { * */ public function ___login($name, $pass, $force = false) { + + /** @var User|null $user */ + $user = null; + /** @var Sanitizer $sanitizer */ + $sanitizer = $this->wire('sanitizer'); + /** @var Users $users */ + $users = $this->wire('users'); + /** @var int $guestUserID */ + $guestUserID = $this->wire('config')->guestUserPageID; + + $fail = true; + $failReason = ''; - $user = null; if(is_object($name) && $name instanceof User) { $user = $name; $name = $user->name; } else { - $name = $this->wire('sanitizer')->pageNameUTF8($name); + $name = $sanitizer->pageNameUTF8($name); } - - if(!$this->allowLogin($name)) { - $this->loginFailure($name, "User is not allowed to login"); - return null; - } - + + if(!strlen($name)) return null; + if(is_null($user)) { - $user = strlen($name) ? $this->wire('users')->get("name=$name") : null; + $user = $users->get('name=' . $sanitizer->selectorValue($name)); } - if( $user && $user->id - && $user->id != $this->wire('config')->guestUserPageID - && ($force === true || $this->authenticate($user, $pass))) { + if(!$user || !$user->id) { + $failReason = 'Unknown user'; + + } else if($user->id == $guestUserID) { + $failReason = 'Guest user may not login'; + + } else if(!$this->allowLogin($name, $user)) { + $failReason = 'Login not allowed'; + + } else if($force === true || $this->authenticate($user, $pass)) { $this->trackChange('login', $this->wire('user'), $user); session_regenerate_id(true); @@ -780,7 +795,8 @@ class Session extends Wire implements \IteratorAggregate { $this->set('_user', 'challenge', $challenge); $secure = $this->config->sessionCookieSecure ? (bool) $this->config->https : false; // set challenge cookie to last 30 days (should be longer than any session would feasibly last) - setcookie(session_name() . '_challenge', $challenge, time()+60*60*24*30, '/', $this->config->sessionCookieDomain, $secure, true); // PR #1264 + setcookie(session_name() . '_challenge', $challenge, time()+60*60*24*30, '/', + $this->config->sessionCookieDomain, $secure, true); } if($this->config->sessionFingerprint) { @@ -791,21 +807,19 @@ class Session extends Wire implements \IteratorAggregate { $this->wire('user', $user); $this->get('CSRF')->resetAll(); $this->loginSuccess($user); + $fail = false; - return $user; - } else { - if(!$user || !$user->id) { - $reason = "Unknown user: $name"; - } else if($user->id == $this->wire('config')->guestUserPageID) { - $reason = "Guest user may not login"; - } else { - $reason = "Invalid password"; - } - $this->loginFailure($name, $reason); + // authentication failed + $failReason = 'Invalid password'; + } + + if($fail) { + $this->loginFailure($name, $failReason); + $user = null; } - return null; + return $user; } /** @@ -857,12 +871,34 @@ class Session extends Wire implements \IteratorAggregate { * #pw-hooker * * @param string $name User login name + * @param User|null $user User object * @return bool True if allowed to login, false if not (hooks may modify this) * */ - public function ___allowLogin($name) { + public function ___allowLogin($name, $user = null) { + $allow = true; + if(!strlen($name)) return false; + if(!$user || !$user instanceof User) { + $name = $this->wire('sanitizer')->pageNameUTF8($name); + $user = $this->wire('users')->get("name=" . $this->wire('sanitizer')->selectorValue($name)); + if(!$user || !$user->id) return false; + } + $xroles = $this->wire('config')->loginDisabledRoles; + if(!is_array($xroles) && !empty($xroles)) $xroles = array($xroles); if($name) {} - return true; + if($user) { + if($user->isUnpublished()) { + $allow = false; + } else if(is_array($xroles)) { + foreach($xroles as $xrole) { + if($user->hasRole($xrole)) { + $allow = false; + break; + } + } + } + } + return $allow; } /** diff --git a/wire/core/WireInput.php b/wire/core/WireInput.php index a321e347..c397688f 100644 --- a/wire/core/WireInput.php +++ b/wire/core/WireInput.php @@ -278,6 +278,8 @@ class WireInput extends Wire { */ public function setUrlSegment($num, $value) { $num = (int) $num; + $maxLength = $this->wire('config')->maxUrlSegmentLength; + if($maxLength < 1) $maxLength = 128; if(is_null($value)) { // unset $n = 0; @@ -289,10 +291,10 @@ class WireInput extends Wire { $this->urlSegments = $urlSegments; } else if($this->wire('config')->pageNameCharset == 'UTF8') { // set UTF8 - $this->urlSegments[$num] = $this->wire('sanitizer')->pageNameUTF8($value); + $this->urlSegments[$num] = $this->wire('sanitizer')->pageNameUTF8($value, $maxLength); } else { // set ascii - $this->urlSegments[$num] = $this->wire('sanitizer')->name($value); + $this->urlSegments[$num] = $this->wire('sanitizer')->name($value, false, $maxLength); } } diff --git a/wire/modules/Process/ProcessForgotPassword.module b/wire/modules/Process/ProcessForgotPassword.module index 9d66bfb0..87c17e93 100644 --- a/wire/modules/Process/ProcessForgotPassword.module +++ b/wire/modules/Process/ProcessForgotPassword.module @@ -139,8 +139,13 @@ class ProcessForgotPassword extends Process implements ConfigurableModule { /** @var User $user */ $user = $this->users->get("name=" . $this->sanitizer->selectorValue($name)); if($user && $user->id && $user->email && !$user->isUnpublished()) { - // user was found, send them an email with reset link - $this->step2_sendEmail($user); + if($this->wire('session')->allowLogin($name, $user)) { + // user was found, send them an email with reset link + $this->step2_sendEmail($user); + } else { + $this->error($this->_('Specified account is not allowed to login so password may not be reset.')); + $this->wire('session')->redirect('./'); + } } } diff --git a/wire/modules/Process/ProcessUser/ProcessUser.module b/wire/modules/Process/ProcessUser/ProcessUser.module index 420716c4..10782fbf 100644 --- a/wire/modules/Process/ProcessUser/ProcessUser.module +++ b/wire/modules/Process/ProcessUser/ProcessUser.module @@ -27,11 +27,19 @@ class ProcessUser extends ProcessPageType { ); } + /** + * Construct and set default config values + * + */ public function __construct() { $this->set("maxAjaxQty", 25); return parent::__construct(); } + /** + * Init and prepare for execute methods + * + */ public function init() { $this->wire('pages')->addHookBefore('save', $this, 'hookPageSave'); parent::init(); @@ -42,13 +50,22 @@ class ProcessUser extends ProcessPageType { $roles->label = $this->_('Roles'); $roles->description = $this->_("User will inherit the permissions assigned to each role. You may assign multiple roles to a user. When accessing a page, the user will only inherit permissions from the roles that are also assigned to the page's template."); // Roles description } - + + /** + * Determine whether Lister should be used or not + * + * @return bool + * + */ protected function useLister() { return $this->wire('user')->hasPermission('page-lister'); } /** * Output JSON list of navigation items for this (intended to for ajax use) + * + * @param array $options + * @return string|array * */ public function ___executeNavJSON(array $options = array()) { @@ -81,6 +98,12 @@ class ProcessUser extends ProcessPageType { return parent::___executeNavJSON($options); } + /** + * Hook to ProcessPageLister::execute method to adjust selector to show specific roles + * + * @param HookEvent $event + * + */ public function hookListerExecute($event) { $role = (int) $this->wire('session')->get($this, 'listerRole'); @@ -96,6 +119,14 @@ class ProcessUser extends ProcessPageType { $lister->defaultSelector = $defaultSelector; } + /** + * Get settings to be used for Lister + * + * @param ProcessPageLister $lister + * @param string $selector + * @return array + * + */ protected function getListerSettings(ProcessPageLister $lister, $selector) { $settings = parent::getListerSettings($lister, $selector); @@ -145,28 +176,43 @@ class ProcessUser extends ProcessPageType { return $settings; } - - public function ___executeEdit() { - - $user = $this->wire('user'); - - if(!$user->isSuperuser()) { - // prevent showing superuser role at all - $this->addHookAfter('InputfieldPage::getSelectablePages', $this, 'hookGetSelectablePages'); - } - - $this->addHookAfter('ProcessPageEdit::buildForm', $this, 'hookPageEditBuildForm'); - - $out = parent::___executeEdit(); + /** + * Get the Page being edited, when applicable + * + * @return NullPage|Page + * + */ + public function getPage() { + $page = parent::getPage(); + if($page->id && !$page->get('_rolesPrevious') && $this->wire('input')->post('roles') !== null) { + $page->setQuietly('_rolesPrevious', clone $page->roles); + } + return $page; + } + + /** + * Return array of roles editable by current user for user $page + * + * @param User $page + * @return array of role names indexed by role ID + * + */ + protected function getEditableRoles(User $page) { + /** @var User $user */ + $user = $this->wire('user'); + $superuser = $user->isSuperuser(); $editableRoles = array(); + foreach($this->wire('roles') as $role) { if($role->name == 'guest') continue; + // if non-superuser editing a user, don't allow them to assign new roles with user-admin permission, + // unless the user already has the role checked + if(!$superuser && $role->hasPermission('user-admin') && !$page->hasPermission('user-admin')) continue; $editableRoles[$role->id] = $role->name; } - - if(!$user->isSuperuser()) { - $page = $this->getPage(); + + if(!$superuser) { $userAdminAll = $this->wire('permissions')->get('user-admin-all'); if($userAdminAll->id && !$user->hasPermission($userAdminAll)) { foreach($editableRoles as $roleID => $roleName) { @@ -192,14 +238,46 @@ class ProcessUser extends ProcessPageType { */ } + return $editableRoles; + } + + /** + * Edit user + * + * @return array|string + * + */ + public function ___executeEdit() { + + /** @var User $user */ + $user = $this->wire('user'); + + if(!$user->isSuperuser()) { + // prevent showing superuser role at all + $this->addHookAfter('InputfieldPage::getSelectablePages', $this, 'hookGetSelectablePages'); + } + + $this->addHookAfter('ProcessPageEdit::buildForm', $this, 'hookPageEditBuildForm'); + + $out = parent::___executeEdit(); + + /** @var User $page Available only after executeEdit() */ + $page = $this->getPage(); + $this->wire('config')->js('ProcessUser', array( - 'editableRoles' => array_keys($editableRoles), + 'editableRoles' => array_keys($this->getEditableRoles($page)), 'notEditableAlert' => $this->_('You may not change this role'), )); return $out; } - + + /** + * Hook to InputfieldPage::getSelectablePages to target the "roles" field + * + * @param HookEvent $event + * + */ public function hookGetSelectablePages($event) { if($event->object->attr('name') != 'roles') return; $suRoleID = $this->wire('config')->superUserRolePageID; @@ -207,9 +285,16 @@ class ProcessUser extends ProcessPageType { if($role->id == $suRoleID) $event->return->remove($role); } } - + + /** + * Hook to ProcessPageEdit::buildForm to adjust User edit form before presenting to user + * + * @param HookEvent $event + * + */ public function hookPageEditBuildForm(HookEvent $event) { $form = $event->return; + /** @var InputfieldSelect $theme */ $theme = $form->getChildByName('admin_theme'); if(!$theme) return; if(!$theme->attr('value')) { @@ -218,8 +303,14 @@ class ProcessUser extends ProcessPageType { } /** - * Perform a security check to make sure that a non-superuser isn't assigning superuser access to themselves or someone else. + * Hook before Pages::save() + * + * Perform a security check to make sure that a non-superuser isn't assigning superuser access to + * themselves or someone else. Plus perform addition role add/remove checks. * + * @param HookEvent $event + * @throws WireException + * */ public function hookPageSave(HookEvent $event) { @@ -233,6 +324,8 @@ class ProcessUser extends ProcessPageType { $pages = $this->wire('pages'); $user = $this->wire('user'); + $superuser = $user->isSuperuser(); + $suRole = $this->wire('roles')->get($this->wire('config')->superUserRolePageID); // don't allow removal of the guest role if(!$page->roles->has("name=guest")) { @@ -251,61 +344,118 @@ class ProcessUser extends ProcessPageType { $page = $copy; // don't let superusers remove their superuser role - if($user->isSuperuser() && !$page->roles->has("name=superuser")) { + if($superuser && !$page->roles->has($suRole)) { throw new WireException($this->_("You may not remove the superuser role from yourself")); } } - // if they are superuser, then all is good, no need to continue - if($user->isSuperuser()) return; - - // if not then we need to do the check below: - $suRole = $this->wire('roles')->get($this->wire('config')->superUserRolePageID); - if($page->roles->has("name=superuser") || $page->roles->has($suRole)) { // unnecessarily redundant, but might as well - throw new WireException($this->_("You may not assign the superuser role")); + if(!$superuser) { + if($page->roles->has("name=superuser") || $page->roles->has($suRole)) { + throw new WireException($this->_("You may not assign the superuser role")); + } + $this->checkSaveRoles($user, $page); + $this->checkSaveUserAdminAll($user, $page); } - + } + + /** + * Perform a general check for saving the roles field, making sure added/removed roles are okay + * + * @param User $user + * @param User $page + * + */ + protected function checkSaveRoles(User $user, User $page) { + + if($user->isSuperuser()) return; + + /** @var PageArray $rolesPrevious Set to page by the ProcessUser::getPage() method */ + $rolesPrevious = $page->get('_rolesPrevious'); + if(!$rolesPrevious || ((string) $rolesPrevious) === ((string) $page->roles)) return; + + $editableRoles = $this->getEditableRoles($page); + $addedRoles = array(); + $removedRoles = array(); + + // determine added and removed roles + foreach($page->roles as $role) { + if(!$rolesPrevious->has($role)) $addedRoles[$role->id] = $role; + } + foreach($rolesPrevious as $role) { + if(!$page->roles->has($role)) $removedRoles[$role->id] = $role; + } + + // if any added or removed roles are not consistent with editable roles, then reverse the change + // this is not likely to ever occur but is here for redundancy + foreach($addedRoles as $role) { + if($role->name == 'guest') continue; + if(!isset($editableRoles[$role->id])) { + $page->roles->remove($role); + $this->error("Role $role->name may not be added"); + } + } + foreach($removedRoles as $role) { + if(!isset($editableRoles[$role->id])) { + $page->roles->add($role); + $this->error("Role $role->name may not be removed"); + } + } + } + + /** + * Perform checks for when "user-admin-all" permission is installed and user does not have it + * + * @param User $user + * @param User $page + * + */ + protected function checkSaveUserAdminAll(User $user, User $page) { + + if($user->isSuperuser()) return; + $userAdminAll = $this->wire('permissions')->get('user-admin-all'); - if($userAdminAll->id && !$user->hasPermission($userAdminAll)) { - - // user-admin-all permission is installed and user doesn't have it - // check that the role assignments are valid - $changedUser = $page; - $pages->uncache($page, array('shallow' => true)); - $originalUser = $this->wire('users')->get($page->id); // get a fresh, unmodified copy - if($originalUser->id) { + if(!$userAdminAll->id || $user->hasPermission($userAdminAll)) return; + + // user-admin-all permission is installed and user doesn't have it + // check that the role assignments are valid + + /** @var Pages $pages */ + $pages = $this->wire('pages'); + $changedUser = $page; + $pages->uncache($page, array('shallow' => true)); + $originalUser = $this->wire('users')->get($page->id); // get a fresh, unmodified copy + if(!$originalUser->id) return; + + /** @var PagePermissions $pagePermissions */ + $pagePermissions = $this->wire('modules')->get('PagePermissions'); + $removedRoles = array(); - /** @var PagePermissions $pagePermissions */ - $pagePermissions = $this->wire('modules')->get('PagePermissions'); - $removedRoles = array(); - - foreach($originalUser->roles as $role) { - if(!$changedUser->roles->has($role)) { - // role was removed - if(!$pagePermissions->userCanAssignRole($role)) { - $changedUser->roles->add($role); - $this->error(sprintf($this->_('You are not allowed to remove role: %s'), $role->name)); - } else { - $removedRoles[] = $role; - } - } - } - foreach($changedUser->roles as $role) { - if(!$originalUser->roles->has($role)) { - // role was added - if(!$pagePermissions->userCanAssignRole($role)) { - $changedUser->roles->remove($role); - $this->error(sprintf($this->_('You are not allowed to add role: %s'), $role->name)); - } - } - } - - if(count($removedRoles) && !$changedUser->editable()) { - $this->error($this->_('You removed role(s) that that will prevent your edit access to this user. Roles have been restored.')); - foreach($removedRoles as $role) $changedUser->roles->add($role); + foreach($originalUser->roles as $role) { + if(!$changedUser->roles->has($role)) { + // role was removed + if(!$pagePermissions->userCanAssignRole($role)) { + $changedUser->roles->add($role); + $this->error(sprintf($this->_('You are not allowed to remove role: %s'), $role->name)); + } else { + $removedRoles[] = $role; } } } + + foreach($changedUser->roles as $role) { + if(!$originalUser->roles->has($role)) { + // role was added + if(!$pagePermissions->userCanAssignRole($role)) { + $changedUser->roles->remove($role); + $this->error(sprintf($this->_('You are not allowed to add role: %s'), $role->name)); + } + } + } + + if(count($removedRoles) && !$changedUser->editable()) { + $this->error($this->_('You removed role(s) that that will prevent your edit access to this user. Roles have been restored.')); + foreach($removedRoles as $role) $changedUser->roles->add($role); + } } }