Clean code. Add permission test after action = something.

Dolibarr · Sep 30, 2024 · d7d8f87 · d7d8f87
1 parent 61e8382
commit d7d8f87
Show file tree

Hide file tree

Showing 18 changed files with 68 additions and 53 deletions.
diff --git a/htdocs/accountancy/bookkeeping/balance.php b/htdocs/accountancy/bookkeeping/balance.php
@@ -128,6 +128,7 @@
 	accessforbidden();
 }
 
+
 /*
  * Action
  */
@@ -169,10 +170,6 @@
 		$filter['t.doc_date<='] = $search_date_end;
 		$param .= '&date_endmonth=' . GETPOSTINT('date_endmonth') . '&date_endday=' . GETPOSTINT('date_endday') . '&date_endyear=' . GETPOSTINT('date_endyear');
 	}
-	if (!empty($search_doc_date)) {
-		$filter['t.doc_date'] = $search_doc_date;
-		$param .= '&doc_datemonth=' . GETPOSTINT('doc_datemonth') . '&doc_dateday=' . GETPOSTINT('doc_dateday') . '&doc_dateyear=' . GETPOSTINT('doc_dateyear');
-	}
 	if (!empty($search_accountancy_code_start)) {
 		if ($type == 'sub') {
 			$filter['t.subledger_account>='] = $search_accountancy_code_start;
@@ -207,7 +204,7 @@
 	}
 }
 
-if ($action == 'export_csv') {
+if ($action == 'export_csv' && $user->hasRight('accounting', 'mouvements', 'lire')) {
 	$sep = getDolGlobalString('ACCOUNTING_EXPORT_SEPARATORCSV');
 
 	$filename = 'balance';

diff --git a/htdocs/accountancy/closure/index.php b/htdocs/accountancy/closure/index.php
@@ -103,8 +103,8 @@
 }
 
 if (empty($reshook)) {
-	if (isset($current_fiscal_period) && $user->hasRight('accounting', 'fiscalyear', 'write')) {
-		if ($action == 'confirm_step_1' && $confirm == "yes") {
+	if (isset($current_fiscal_period)) {
+		if ($action == 'confirm_step_1' && $confirm == "yes" && $user->hasRight('accounting', 'fiscalyear', 'write')) {
 			$date_start = dol_mktime(0, 0, 0, GETPOSTINT('date_startmonth'), GETPOSTINT('date_startday'), GETPOSTINT('date_startyear'));
 			$date_end = dol_mktime(23, 59, 59, GETPOSTINT('date_endmonth'), GETPOSTINT('date_endday'), GETPOSTINT('date_endyear'));
 
@@ -119,7 +119,7 @@
 				setEventMessages($object->error, $object->errors, 'errors');
 				$action = '';
 			}
-		} elseif ($action == 'confirm_step_2' && $confirm == "yes") {
+		} elseif ($action == 'confirm_step_2' && $confirm == "yes" && $user->hasRight('accounting', 'fiscalyear', 'write')) {
 			$new_fiscal_period_id = GETPOSTINT('new_fiscal_period_id');
 			$separate_auxiliary_account = GETPOST('separate_auxiliary_account', 'aZ09');
 			$generate_bookkeeping_records = GETPOST('generate_bookkeeping_records', 'aZ09');
@@ -147,7 +147,7 @@
 					exit;
 				}
 			}
-		} elseif ($action == 'confirm_step_3' && $confirm == "yes") {
+		} elseif ($action == 'confirm_step_3' && $confirm == "yes" && $user->hasRight('accounting', 'fiscalyear', 'write')) {
 			$inventory_journal_id = GETPOSTINT('inventory_journal_id');
 			$new_fiscal_period_id = GETPOSTINT('new_fiscal_period_id');
 			$date_start = dol_mktime(0, 0, 0, GETPOSTINT('date_startmonth'), GETPOSTINT('date_startday'), GETPOSTINT('date_startyear'));

diff --git a/htdocs/compta/bank/releve.php b/htdocs/compta/bank/releve.php
@@ -190,7 +190,7 @@
  * Actions
  */
 
-if ($action == 'confirm_editbankreceipt' && !empty($oldbankreceipt) && !empty($newbankreceipt)) {
+if ($action == 'confirm_editbankreceipt' && !empty($oldbankreceipt) && !empty($newbankreceipt) && $user->hasRight('banque', 'consolidate')) {
 	// Test to check newbankreceipt does not exists yet
 	$sqltest = "SELECT b.rowid FROM ".MAIN_DB_PREFIX."bank as b, ".MAIN_DB_PREFIX."bank_account as ba";
 	$sqltest .= " WHERE b.fk_account = ba.rowid AND ba.entity = ".((int) $conf->entity);

diff --git a/htdocs/compta/paiement_charge.php b/htdocs/compta/paiement_charge.php
@@ -32,10 +32,11 @@
 // Load translation files required by the page
 $langs->loadLangs(array("banks", "bills", "compta"));
 
-$chid = GETPOSTINT("id");
 $action = GETPOST('action', 'aZ09');
+$confirm = GETPOST('confirm', 'alpha');
 $cancel = GETPOST('cancel');
 
+$chid = GETPOSTINT("id");
 $amounts = array();
 
 // Security check
@@ -51,7 +52,7 @@
  * Actions
  */
 
-if ($action == 'add_payment' || ($action == 'confirm_paiement' && $confirm == 'yes')) {
+if (($action == 'add_payment' || ($action == 'confirm_paiement' && $confirm == 'yes')) && $user->hasRight('tax', 'charges', 'creer')) {
 	$error = 0;
 
 	if ($cancel) {

diff --git a/htdocs/compta/paiement_vat.php b/htdocs/compta/paiement_vat.php
@@ -33,10 +33,11 @@
 // Load translation files required by the page
 $langs->loadLangs(array("banks", "bills"));
 
-$chid = GETPOSTINT("id");
 $action = GETPOST('action', 'alpha');
+$confirm = GETPOST('confirm', 'alpha');
 $cancel = GETPOST('cancel');
 
+$chid = GETPOSTINT("id");
 $amounts = array();
 
 // Security check
@@ -45,12 +46,14 @@
 	$socid = $user->socid;
 }
 
+$permissiontoadd = $user->hasRight('tax', 'charges', 'creer');
+
 
 /*
  * Actions
  */
 
-if ($action == 'add_payment' || ($action == 'confirm_paiement' && $confirm == 'yes')) {
+if (($action == 'add_payment' || ($action == 'confirm_paiement' && $confirm == 'yes')) && $permissiontoadd) {
 	$error = 0;
 
 	if ($cancel) {

diff --git a/htdocs/compta/sociales/class/paymentsocialcontribution.class.php b/htdocs/compta/sociales/class/paymentsocialcontribution.class.php
@@ -291,7 +291,6 @@ public function create($user, $closepaidcontrib = 0)
 	 */
 	public function fetch($id)
 	{
-		global $langs;
 		$sql = "SELECT";
 		$sql .= " t.rowid,";
 		$sql .= " t.fk_charge,";
@@ -301,7 +300,7 @@ public function fetch($id)
 		$sql .= " t.amount,";
 		$sql .= " t.fk_typepaiement,";
 		$sql .= " t.num_paiement as num_payment,";
-		$sql .= " t.note,";
+		$sql .= " t.note as note_private,";
 		$sql .= " t.fk_bank,";
 		$sql .= " t.fk_user_creat,";
 		$sql .= " t.fk_user_modif,";
@@ -330,7 +329,7 @@ public function fetch($id)
 				$this->fk_typepaiement = $obj->fk_typepaiement;
 				$this->num_payment = $obj->num_payment;
 				$this->num_paiement = $obj->num_payment;
-				$this->note_private = $obj->note;
+				$this->note_private = $obj->note_private;
 				$this->fk_bank = $obj->fk_bank;
 				$this->fk_user_creat = $obj->fk_user_creat;
 				$this->fk_user_modif = $obj->fk_user_modif;

diff --git a/htdocs/core/photos_resize.php b/htdocs/core/photos_resize.php
@@ -346,7 +346,7 @@
 	}
 }
 
-if ($action == 'confirm_resize' && GETPOSTISSET("file") && GETPOSTISSET("sizex") && GETPOSTISSET("sizey")) {
+if ($action == 'confirm_resize' && GETPOSTISSET("file") && GETPOSTISSET("sizex") && GETPOSTISSET("sizey")) {	// Test on permission already done
 	if (empty($dir)) {
 		dol_print_error(null, 'Bug: Value for $dir could not be defined.');
 		exit;

diff --git a/htdocs/don/payment/card.php b/htdocs/don/payment/card.php
@@ -54,13 +54,17 @@
 	}
 }
 
+$permissiontoread = $user->hasRight('don', 'lire');
+$permissiontoadd = $user->hasRight('don', 'creer');
+$permissiontodelete = $user->hasRight('don', 'supprimer');
+
 
 /*
  * Actions
  */
 
 // Delete payment
-if ($action == 'confirm_delete' && $confirm == 'yes' && $user->hasRight('don', 'supprimer')) {
+if ($action == 'confirm_delete' && $confirm == 'yes' && $permissiontodelete) {
 	$db->begin();
 
 	$result = $object->delete($user);
@@ -79,6 +83,7 @@
 /*
  * View
  */
+
 $title = $langs->trans("Payment");
 llxHeader('', $title, '', '', 0, 0, '', '', '', 'mod-donation page-payment_card');
 

diff --git a/htdocs/don/payment/payment.php b/htdocs/don/payment/payment.php
@@ -44,12 +44,16 @@
 
 $object = new Don($db);
 
+$permissiontoread = $user->hasRight('don', 'lire');
+$permissiontoadd = $user->hasRight('don', 'creer');
+$permissiontodelete = $user->hasRight('don', 'supprimer');
+
 
 /*
  * Actions
  */
 
-if ($action == 'add_payment') {
+if ($action == 'add_payment' && $permissiontoadd) {
 	$error = 0;
 
 	if ($cancel) {

diff --git a/htdocs/expensereport/payment/payment.php b/htdocs/expensereport/payment/payment.php
@@ -46,12 +46,14 @@
 	$socid = $user->socid;
 }
 
+$permissiontoadd = $user->hasRight('expensereport', 'creer');
+
 
 /*
  * Actions
  */
 
-if ($action == 'add_payment') {
+if ($action == 'add_payment' && $permissiontoadd) {
 	$error = 0;
 
 	if ($cancel) {

diff --git a/htdocs/fourn/facture/paiement.php b/htdocs/fourn/facture/paiement.php
@@ -110,6 +110,7 @@
 
 $arrayfields = array();
 
+$permissiontoadd = ($user->hasRight("fournisseur", "facture", "creer") || $user->hasRight("supplier_invoice", "creer"));
 
 
 /*
@@ -148,7 +149,7 @@
 }
 
 if (empty($reshook)) {
-	if ($action == 'add_paiement' || ($action == 'confirm_paiement' && $confirm == 'yes')) {
+	if (($action == 'add_paiement' || ($action == 'confirm_paiement' && $confirm == 'yes')) && $permissiontoadd) {
 		$error = 0;
 
 		$datepaye = dol_mktime(12, 0, 0, GETPOST('remonth'), GETPOST('reday'), GETPOST('reyear'));

diff --git a/htdocs/loan/payment/payment.php b/htdocs/loan/payment/payment.php
@@ -33,9 +33,11 @@
 
 $langs->loadLangs(array("bills", "loan"));
 
-$chid = GETPOSTINT('id');
 $action = GETPOST('action', 'aZ09');
+$confirm	= GETPOST('confirm', 'alpha');
 $cancel = GETPOST('cancel', 'alpha');
+
+$chid = GETPOSTINT('id');
 $datepaid = dol_mktime(12, 0, 0, GETPOSTINT('remonth'), GETPOSTINT('reday'), GETPOSTINT('reyear'));
 
 // Security check
@@ -84,12 +86,14 @@
 	}
 }
 
+$permissiontoadd = $user->hasRight('loan', 'write');
+
 
 /*
  * Actions
  */
 
-if ($action == 'add_payment') {
+if ($action == 'add_payment' && $permissiontoadd) {
 	$error = 0;
 
 	if ($cancel) {

diff --git a/htdocs/projet/activity/index.php b/htdocs/projet/activity/index.php
@@ -70,7 +70,7 @@
 	setEventMessages($hookmanager->error, $hookmanager->errors, 'errors');
 }
 if (empty($reshook)) {
-	if ($action == 'refresh_search_project_user') {
+	if ($action == 'refresh_search_project_user' && $user->hasRight('projet', 'lire')) {
 		$search_project_user = GETPOSTINT('search_project_user');
 		$tabparam = array("MAIN_SEARCH_PROJECT_USER_PROJECTSINDEX" => $search_project_user);
 

diff --git a/htdocs/projet/index.php b/htdocs/projet/index.php
@@ -73,7 +73,7 @@
 	setEventMessages($hookmanager->error, $hookmanager->errors, 'errors');
 }
 if (empty($reshook)) {
-	if ($action == 'refresh_search_project_user') {
+	if ($action == 'refresh_search_project_user' && $user->hasRight('projet', 'lire')) {
 		$search_project_user = GETPOSTINT('search_project_user');
 		$tabparam = array("MAIN_SEARCH_PROJECT_USER_PROJECTSINDEX" => $search_project_user);
 

diff --git a/htdocs/public/project/index.php b/htdocs/public/project/index.php
@@ -110,7 +110,7 @@
  */
 function llxHeaderVierge($title, $head = "", $disablejs = 0, $disablehead = 0, $arrayofjs = [], $arrayofcss = [])
 {
-	global $user, $conf, $langs, $mysoc;
+	global $conf, $langs, $mysoc;
 
 	top_htmlhead($head, $title, $disablejs, $disablehead, $arrayofjs, $arrayofcss); // Show html headers
 
@@ -190,8 +190,6 @@ function llxFooterVierge()
 }
 
 
-
-
 /*
  * View
  */
@@ -211,7 +209,6 @@ function llxFooterVierge()
 llxHeaderVierge($langs->trans("SuggestForm"));
 
 
-
 print '<span id="dolpaymentspan"></span>'."\n";
 print '<div class="center">'."\n";
 

diff --git a/htdocs/public/ticket/create_ticket.php b/htdocs/public/ticket/create_ticket.php
@@ -149,7 +149,7 @@
 		$action = 'create_ticket';
 	}
 
-	if ($action == 'create_ticket' && GETPOST('save', 'alpha')) {
+	if ($action == 'create_ticket' && GETPOST('save', 'alpha')) {	// Test on permission not required. This is a public form. Security is managed by mitigation.
 		$error = 0;
 		$origin_email = GETPOST('email', 'email');
 		if (empty($origin_email)) {

diff --git a/htdocs/public/ticket/list.php b/htdocs/public/ticket/list.php
@@ -99,6 +99,28 @@
 	exit;
 }
 
+
+/*
+ * View
+ */
+
+$form = new Form($db);
+$user_assign = new User($db);
+$user_create = new User($db);
+$formTicket = new FormTicket($db);
+
+if (!getDolGlobalString('TICKET_ENABLE_PUBLIC_INTERFACE')) {
+	print '<div class="error">'.$langs->trans('TicketPublicInterfaceForbidden').'</div>';
+	$db->close();
+	exit();
+}
+
+$arrayofjs = array();
+$arrayofcss = array(getDolGlobalString('TICKET_URL_PUBLIC_INTERFACE', '/public/ticket/').'css/styles.css.php');
+
+llxHeaderTicket($langs->trans("Tickets"), "", 0, 0, $arrayofjs, $arrayofcss);
+
+// Load the ticket from track_id
 if ($action == "view_ticketlist") {
 	$error = 0;
 	$display_ticket_list = false;
@@ -167,32 +189,12 @@
 		}
 	}
 
-	if ($error || $errors) {
+	if ($error) {
 		setEventMessages($object->error, $object->errors, 'errors');
 		$action = '';
 	}
 }
 
-/*
- * View
- */
-
-$form = new Form($db);
-$user_assign = new User($db);
-$user_create = new User($db);
-$formTicket = new FormTicket($db);
-
-if (!getDolGlobalString('TICKET_ENABLE_PUBLIC_INTERFACE')) {
-	print '<div class="error">'.$langs->trans('TicketPublicInterfaceForbidden').'</div>';
-	$db->close();
-	exit();
-}
-
-$arrayofjs = array();
-$arrayofcss = array(getDolGlobalString('TICKET_URL_PUBLIC_INTERFACE', '/public/ticket/').'css/styles.css.php');
-
-llxHeaderTicket($langs->trans("Tickets"), "", 0, 0, $arrayofjs, $arrayofcss);
-
 
 if ($action == "view_ticketlist") {
 	print '<div class="ticketpublicarealist ticketlargemargin centpercent">';

diff --git a/test/phpunit/CodingPhpTest.php b/test/phpunit/CodingPhpTest.php
@@ -662,7 +662,7 @@ public function testPHP($file)
 				$filecontentaction = $filecontent;
 			}
 
-			preg_match_all('/if.*\$action\s*==\s*[\'"][a-z\-]+[\'"].*$/si', $filecontentaction, $matches, PREG_SET_ORDER);
+			preg_match_all('/if.*\$action\s*==\s*[\'"][a-z\-_]+[\'"].*$/si', $filecontentaction, $matches, PREG_SET_ORDER);
 
 			foreach ($matches as $key => $val) {
 				if (!preg_match('/\$user->hasR/', $val[0])