commit acd7e5601ac6bc8b079bd6ea7f8b637a5ec89b5f
Author: jim-p <jimp@netgate.com>
Date:   Tue Aug 27 13:19:30 2019 -0400

    User & Group Manager: Improve Deny Config Write Handling. Fixes #9259
    
    * Denies all changes if a user has the Deny Config Write privilege.
    Previously it only denied the config write but some OS operations were
    performed.
    * Sets an input error so the user is notified that their attempt failed.
    * Hides the add and delete buttons so read only users don't see the
    option to perform those actions (but are still denied if they submit the
    form through other means)

diff --git a/src/usr/local/www/system_groupmanager.php b/src/usr/local/www/system_groupmanager.php
index 4463ed4d66..ec8d9cc1d5 100644
--- a/src/usr/local/www/system_groupmanager.php
+++ b/src/usr/local/www/system_groupmanager.php
@@ -62,7 +62,21 @@ function admin_groups_sort() {
 	usort($a_group, "cpusercmp");
 }
 
-if ($_POST['act'] == "delgroup") {
+/*
+ * Check user privileges to test if the user is allowed to make changes.
+ * Otherwise users can end up in an inconsistent state where some changes are
+ * performed and others denied. See https://redmine.pfsense.org/issues/9259
+ */
+phpsession_begin();
+$guiuser = getUserEntry($_SESSION['Username']);
+$read_only = (is_array($guiuser) && userHasPrivilege($guiuser, "user-config-readonly"));
+phpsession_end();
+
+if (!empty($_POST) && $read_only) {
+	$input_errors = array(gettext("Insufficient privileges to make the requested change (read only)."));
+}
+
+if (($_POST['act'] == "delgroup") && !$read_only) {
 
 	if (!isset($id) || !isset($_REQUEST['groupname']) ||
 	    !isset($a_group[$id]) ||
@@ -86,7 +100,7 @@ if ($_POST['act'] == "delgroup") {
 	syslog($logging_level, "{$logging_prefix}: {$savemsg}");
 }
 
-if ($_POST['act'] == "delpriv") {
+if (($_POST['act'] == "delpriv") && !$read_only) {
 
 	if (!isset($id) || !isset($a_group[$id])) {
 		pfSenseHeader("system_groupmanager.php");
@@ -126,7 +140,7 @@ if ($act == "edit") {
 	}
 }
 
-if (isset($_POST['dellall_x'])) {
+if (isset($_POST['dellall_x']) && !$read_only) {
 
 	$del_groups = $_POST['delete_check'];
 	$deleted_groups = array();
@@ -155,7 +169,7 @@ if (isset($_POST['dellall_x'])) {
 	}
 }
 
-if (isset($_POST['save'])) {
+if (isset($_POST['save']) && !$read_only) {
 	unset($input_errors);
 	$pconfig = $_POST;
 
@@ -265,7 +279,7 @@ if (isset($_POST['save'])) {
 }
 
 function build_priv_table() {
-	global $a_group, $id;
+	global $a_group, $id, $read_only;
 
 	$privhtml = '<div class="table-responsive">';
 	$privhtml .=	'<table class="table table-striped table-hover table-condensed">';
@@ -289,7 +303,9 @@ function build_priv_table() {
 			$user_has_root_priv = true;
 		}
 		$privhtml .=			'</td>';
-		$privhtml .=			'<td><a class="fa fa-trash" title="' . gettext('Delete Privilege') . '"	href="system_groupmanager.php?act=delpriv&amp;groupid=' . $id . '&amp;privid=' . $i . '" usepost></a></td>';
+		if (!$read_only) {
+			$privhtml .=			'<td><a class="fa fa-trash" title="' . gettext('Delete Privilege') . '"	href="system_groupmanager.php?act=delpriv&amp;groupid=' . $id . '&amp;privid=' . $i . '" usepost></a></td>';
+		}
 		$privhtml .=		'</tr>';
 
 	}
@@ -310,7 +326,9 @@ function build_priv_table() {
 	$privhtml .= '</div>';
 
 	$privhtml .= '<nav class="action-buttons">';
-	$privhtml .=	'<a href="system_groupmanager_addprivs.php?groupid=' . $id . '" class="btn btn-success"><i class="fa fa-plus icon-embed-btn"></i>' . gettext("Add") . '</a>';
+	if (!$read_only) {
+		$privhtml .=	'<a href="system_groupmanager_addprivs.php?groupid=' . $id . '" class="btn btn-success"><i class="fa fa-plus icon-embed-btn"></i>' . gettext("Add") . '</a>';
+	}
 	$privhtml .= '</nav>';
 
 	return($privhtml);
@@ -383,7 +401,7 @@ if (!($act == "new" || $act == "edit")) {
 						</td>
 						<td>
 							<a class="fa fa-pencil" title="<?=gettext("Edit group"); ?>" href="?act=edit&amp;groupid=<?=$i?>"></a>
-							<?php if ($group['scope'] != "system"): ?>
+							<?php if (($group['scope'] != "system") && !$read_only): ?>
 								<a class="fa fa-trash"	title="<?=gettext("Delete group")?>" href="?act=delgroup&amp;groupid=<?=$i?>&amp;groupname=<?=$group['name']?>" usepost></a>
 							<?php endif;?>
 						</td>
@@ -398,10 +416,12 @@ if (!($act == "new" || $act == "edit")) {
 </div>
 
 <nav class="action-buttons">
+	<?php if (!$read_only): ?>
 	<a href="?act=new" class="btn btn-success btn-sm">
 		<i class="fa fa-plus icon-embed-btn"></i>
 		<?=gettext("Add")?>
 	</a>
+	<?php endif; ?>
 </nav>
 <?php
 	include('foot.inc');
diff --git a/src/usr/local/www/system_groupmanager_addprivs.php b/src/usr/local/www/system_groupmanager_addprivs.php
index 120e23e60f..82ab33f09f 100644
--- a/src/usr/local/www/system_groupmanager_addprivs.php
+++ b/src/usr/local/www/system_groupmanager_addprivs.php
@@ -61,7 +61,21 @@ if (!is_array($a_group['priv'])) {
 $spriv_list = $priv_list;
 uasort($spriv_list, "compare_by_name");
 
-if ($_POST['save']) {
+/*
+ * Check user privileges to test if the user is allowed to make changes.
+ * Otherwise users can end up in an inconsistent state where some changes are
+ * performed and others denied. See https://redmine.pfsense.org/issues/9259
+ */
+phpsession_begin();
+$guiuser = getUserEntry($_SESSION['Username']);
+$read_only = (is_array($guiuser) && userHasPrivilege($guiuser, "user-config-readonly"));
+phpsession_end();
+
+if (!empty($_POST) && $read_only) {
+	$input_errors = array(gettext("Insufficient privileges to make the requested change (read only)."));
+}
+
+if ($_POST['save'] && !$read_only) {
 
 	unset($input_errors);
 	$pconfig = $_POST;
diff --git a/src/usr/local/www/system_usermanager.php b/src/usr/local/www/system_usermanager.php
index 43efec4e6c..45f417da26 100644
--- a/src/usr/local/www/system_usermanager.php
+++ b/src/usr/local/www/system_usermanager.php
@@ -83,7 +83,21 @@ if (isset($id) && $a_user[$id]) {
 	$pconfig['disabled'] = isset($a_user[$id]['disabled']);
 }
 
-if ($_POST['act'] == "deluser") {
+/*
+ * Check user privileges to test if the user is allowed to make changes.
+ * Otherwise users can end up in an inconsistent state where some changes are
+ * performed and others denied. See https://redmine.pfsense.org/issues/9259
+ */
+phpsession_begin();
+$guiuser = getUserEntry($_SESSION['Username']);
+$read_only = (is_array($guiuser) && userHasPrivilege($guiuser, "user-config-readonly"));
+phpsession_end();
+
+if (!empty($_POST) && $read_only) {
+	$input_errors = array(gettext("Insufficient privileges to make the requested change (read only)."));
+}
+
+if (($_POST['act'] == "deluser") && !$read_only) {
 
 	if (!isset($_POST['username']) || !isset($a_user[$id]) || ($_POST['username'] != $a_user[$id]['name'])) {
 		pfSenseHeader("system_usermanager.php");
@@ -125,7 +139,7 @@ if ($_POST['act'] == "deluser") {
 
 }
 
-if (isset($_POST['dellall'])) {
+if (isset($_POST['dellall']) && !$read_only) {
 
 	$del_users = $_POST['delete_check'];
 	$deleted_users = array();
@@ -155,7 +169,7 @@ if (isset($_POST['dellall'])) {
 	}
 }
 
-if ($_POST['act'] == "delcert") {
+if (($_POST['act'] == "delcert") && !$read_only) {
 
 	if (!$a_user[$id]) {
 		pfSenseHeader("system_usermanager.php");
@@ -171,7 +185,7 @@ if ($_POST['act'] == "delcert") {
 	$_POST['act'] = "edit";
 }
 
-if ($_POST['act'] == "delprivid") {
+if (($_POST['act'] == "delprivid") && !$read_only) {
 	$privdeleted = $priv_list[$a_user[$id]['priv'][$_POST['privid']]]['name'];
 	unset($a_user[$id]['priv'][$_POST['privid']]);
 	local_user_set($a_user[$id]);
@@ -181,7 +195,7 @@ if ($_POST['act'] == "delprivid") {
 	$_POST['act'] = "edit";
 }
 
-if ($_POST['save']) {
+if ($_POST['save'] && !$read_only) {
 	unset($input_errors);
 	$pconfig = $_POST;
 
@@ -475,7 +489,7 @@ if ($_POST['save']) {
 }
 
 function build_priv_table() {
-	global $a_user, $id;
+	global $a_user, $id, $read_only;
 
 	$privhtml = '<div class="table-responsive">';
 	$privhtml .=	'<table class="table table-striped table-hover table-condensed">';
@@ -508,7 +522,7 @@ function build_priv_table() {
 		}
 		$privhtml .=			'</td>';
 		$privhtml .=			'<td>';
-		if (!$group) {
+		if (!$group && !$read_only) {
 			$privhtml .=			'<a class="fa fa-trash no-confirm icon-pointer" title="' . gettext('Delete Privilege') . '" id="delprivid' . $i . '"></a>';
 		}
 
@@ -536,14 +550,16 @@ function build_priv_table() {
 	$privhtml .= '</div>';
 
 	$privhtml .= '<nav class="action-buttons">';
-	$privhtml .=	'<a href="system_usermanager_addprivs.php?userid=' . $id . '" class="btn btn-success"><i class="fa fa-plus icon-embed-btn"></i>' . gettext("Add") . '</a>';
+	if (!$read_only) {
+		$privhtml .=	'<a href="system_usermanager_addprivs.php?userid=' . $id . '" class="btn btn-success"><i class="fa fa-plus icon-embed-btn"></i>' . gettext("Add") . '</a>';
+	}
 	$privhtml .= '</nav>';
 
 	return($privhtml);
 }
 
 function build_cert_table() {
-	global $a_user, $id;
+	global $a_user, $id, $read_only;
 
 	$certhtml = '<div class="table-responsive">';
 	$certhtml .=	'<table class="table table-striped table-hover table-condensed">';
@@ -568,8 +584,10 @@ function build_cert_table() {
 			$certhtml .=		'<td>' . htmlspecialchars($cert['descr']) . $revokedstr . '</td>';
 			$certhtml .=		'<td>' . htmlspecialchars($ca['descr']) . '</td>';
 			$certhtml .=		'<td>';
-			$certhtml .=			'<a id="delcert' . $i .'" class="fa fa-trash no-confirm icon-pointer" title="';
-			$certhtml .=			gettext('Remove this certificate association? (Certificate will not be deleted)') . '"></a>';
+			if (!$read_only) {
+				$certhtml .=			'<a id="delcert' . $i .'" class="fa fa-trash no-confirm icon-pointer" title="';
+				$certhtml .=			gettext('Remove this certificate association? (Certificate will not be deleted)') . '"></a>';
+			}
 			$certhtml .=		'</td>';
 			$certhtml .=	'</tr>';
 			$i++;
@@ -582,7 +600,9 @@ function build_cert_table() {
 	$certhtml .= '</div>';
 
 	$certhtml .= '<nav class="action-buttons">';
-	$certhtml .=	'<a href="system_certmanager.php?act=new&amp;userid=' . $id . '" class="btn btn-success"><i class="fa fa-plus icon-embed-btn"></i>' . gettext("Add") . '</a>';
+	if (!$read_only) {
+		$certhtml .=	'<a href="system_certmanager.php?act=new&amp;userid=' . $id . '" class="btn btn-success"><i class="fa fa-plus icon-embed-btn"></i>' . gettext("Add") . '</a>';
+	}
 	$certhtml .= '</nav>';
 
 	return($certhtml);
@@ -659,7 +679,7 @@ foreach ($a_user as $i => $userent):
 						<td><?=implode(",", local_user_get_groups($userent))?></td>
 						<td>
 							<a class="fa fa-pencil" title="<?=gettext("Edit user"); ?>" href="?act=edit&amp;userid=<?=$i?>"></a>
-<?php if (($userent['scope'] != "system") && ($userent['name'] != $_SESSION['Username'])): ?>
+<?php if (($userent['scope'] != "system") && ($userent['name'] != $_SESSION['Username']) && !$read_only): ?>
 							<a class="fa fa-trash"	title="<?=gettext("Delete user")?>" href="?act=deluser&amp;userid=<?=$i?>&amp;username=<?=$userent['name']?>" usepost></a>
 <?php endif; ?>
 						</td>
@@ -671,6 +691,8 @@ foreach ($a_user as $i => $userent):
 	</div>
 </div>
 <nav class="action-buttons">
+	<?php if (!$read_only): ?>
+
 	<a href="?act=new" class="btn btn-sm btn-success">
 		<i class="fa fa-plus icon-embed-btn"></i>
 		<?=gettext("Add")?>
@@ -680,6 +702,7 @@ foreach ($a_user as $i => $userent):
 		<i class="fa fa-trash icon-embed-btn"></i>
 		<?=gettext("Delete")?>
 	</button>
+	<?php endif; ?>
 
 </nav>
 </form>
diff --git a/src/usr/local/www/system_usermanager_addprivs.php b/src/usr/local/www/system_usermanager_addprivs.php
index af8596e19f..611263ef1c 100644
--- a/src/usr/local/www/system_usermanager_addprivs.php
+++ b/src/usr/local/www/system_usermanager_addprivs.php
@@ -58,7 +58,21 @@ if (!is_array($a_user['priv'])) {
 $spriv_list = $priv_list;
 uasort($spriv_list, "compare_by_name");
 
-if ($_POST['save']) {
+/*
+ * Check user privileges to test if the user is allowed to make changes.
+ * Otherwise users can end up in an inconsistent state where some changes are
+ * performed and others denied. See https://redmine.pfsense.org/issues/9259
+ */
+phpsession_begin();
+$guiuser = getUserEntry($_SESSION['Username']);
+$read_only = (is_array($guiuser) && userHasPrivilege($guiuser, "user-config-readonly"));
+phpsession_end();
+
+if (!empty($_POST) && $read_only) {
+	$input_errors = array(gettext("Insufficient privileges to make the requested change (read only)."));
+}
+
+if ($_POST['save'] && !$read_only) {
 	unset($input_errors);
 	$pconfig = $_POST;
 
