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.

2025-08-08 07:47:00 +02:00 · 2017-12-13 10:37:39 -05:00
parent bac60dc340
commit 9b10232b73
7 changed files with 325 additions and 101 deletions
--- 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
 * 
--- 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
 * 
--- 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;
 	}

--- 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
@@ -747,26 +747,41 @@ 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 = '';
+		
 		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); 
-
-			return $user; 
+			$fail = false;

 		} 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';
 		}
 		
-		return null; 
+		if($fail) {
+			$this->loginFailure($name, $failReason);
+			$user = 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; 
 	}

 	/**
--- 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);
 		}
 		
 	}
--- 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('./'); 
+				}
 			}
 		}

--- 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();
@@ -43,6 +51,12 @@ class ProcessUser extends ProcessPageType {
 		$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'); 
 	}
@@ -50,6 +64,9 @@ class ProcessUser extends ProcessPageType {
 	/**
 	 * 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);
@@ -146,27 +177,42 @@ 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'); 
+	/**
+	 * 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;
+	}

-		$this->addHookAfter('ProcessPageEdit::buildForm', $this, 'hookPageEditBuildForm'); 
-		
-		$out = parent::___executeEdit();
-
+	/**
+	 * 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;
@@ -208,8 +286,15 @@ class ProcessUser extends ProcessPageType {
 		}
 	}

+	/**
+	 * 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,7 +303,13 @@ 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(!$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;
 		
-		// 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")); 
+		/** @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)) {
+		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
-			$changedUser = $page;
-			$pages->uncache($page, array('shallow' => true));
-			$originalUser = $this->wire('users')->get($page->id); // get a fresh, unmodified copy
-			if($originalUser->id) {
+		// user-admin-all permission is installed and user doesn't have it
+		// check that the role assignments are valid
 		
-				/** @var PagePermissions $pagePermissions */
-				$pagePermissions = $this->wire('modules')->get('PagePermissions');
-				$removedRoles = array();
+		/** @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;
 		
-				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));
-						}
-					}
-				}
+		/** @var PagePermissions $pagePermissions */
+		$pagePermissions = $this->wire('modules')->get('PagePermissions');
+		$removedRoles = array();

-				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);
+		}
 	}
 	
 }