From 72c010622b3594f5986b397872dc74f8b92a9d00 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Sun, 14 Aug 2022 16:38:20 +0200 Subject: [PATCH] FIX empty with hasRight and add phpunit to avoid this in future --- htdocs/accountancy/index.php | 2 +- htdocs/commande/card.php | 6 +++--- htdocs/document.php | 2 +- htdocs/user/card.php | 20 ++++++++++---------- htdocs/user/list.php | 14 +++++++------- htdocs/user/note.php | 2 +- htdocs/user/perms.php | 2 +- test/phpunit/CodingPhpTest.php | 13 ++++++++++++- 8 files changed, 36 insertions(+), 25 deletions(-) diff --git a/htdocs/accountancy/index.php b/htdocs/accountancy/index.php index 926097c1509..f6f86461d9b 100644 --- a/htdocs/accountancy/index.php +++ b/htdocs/accountancy/index.php @@ -49,7 +49,7 @@ if (empty($user->rights->accounting->mouvements->lire)) { if (empty($conf->comptabilite->enabled) && empty($conf->accounting->enabled) && empty($conf->asset->enabled) && empty($conf->intracommreport->enabled)) { accessforbidden(); } -if (empty($user->hasRight('compta', 'resultat', 'lire')) && empty($user->hasRight('accounting', 'comptarapport', 'lire')) && empty($user->hasRight('accounting', 'mouvements', 'lire')) && empty($user->hasRight('asset', 'read')) && empty($user->hasRight('intracommreport', 'read'))) { +if (!$user->hasRight('compta', 'resultat', 'lire') && !$user->hasRight('accounting', 'comptarapport', 'lire') && !$user->hasRight('accounting', 'mouvements', 'lire') && !$user->hasRight('asset', 'read') && !$user->hasRight('intracommreport', 'read')) { accessforbidden(); } diff --git a/htdocs/commande/card.php b/htdocs/commande/card.php index 16cb1d24b27..12e29ac9b2b 100644 --- a/htdocs/commande/card.php +++ b/htdocs/commande/card.php @@ -113,9 +113,9 @@ $usercancreate = $user->hasRight("commande", "creer"); $usercandelete = $user->hasRight("commande", "supprimer"); // Advanced permissions -$usercanclose = ((empty($conf->global->MAIN_USE_ADVANCED_PERMS) && !empty($usercancreate)) || (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && !empty($user->hasRight('commande', 'order_advance', 'close')))); -$usercanvalidate = ((empty($conf->global->MAIN_USE_ADVANCED_PERMS) && $usercancreate) || (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && !empty($user->hasRight('commande', 'order_advance', 'validate')))); -$usercancancel = ((empty($conf->global->MAIN_USE_ADVANCED_PERMS) && $usercancreate) || (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && !empty($user->hasRight('commande', 'order_advance', 'annuler')))); +$usercanclose = ((empty($conf->global->MAIN_USE_ADVANCED_PERMS) && !empty($usercancreate)) || (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && $user->hasRight('commande', 'order_advance', 'close'))); +$usercanvalidate = ((empty($conf->global->MAIN_USE_ADVANCED_PERMS) && $usercancreate) || (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && $user->hasRight('commande', 'order_advance', 'validate'))); +$usercancancel = ((empty($conf->global->MAIN_USE_ADVANCED_PERMS) && $usercancreate) || (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && $user->hasRight('commande', 'order_advance', 'annuler'))); $usercansend = (empty($conf->global->MAIN_USE_ADVANCED_PERMS) || $user->hasRight('commande', 'order_advance', 'send')); $usercangeneretedoc = (empty($conf->global->MAIN_USE_ADVANCED_PERMS) || $user->hasRight('commande', 'order_advance', 'generetedoc')); diff --git a/htdocs/document.php b/htdocs/document.php index e432d6a92f1..65e5cce7e5f 100644 --- a/htdocs/document.php +++ b/htdocs/document.php @@ -120,7 +120,7 @@ if ($user->socid > 0) { // For some module part, dir may be privates if (in_array($modulepart, array('facture_paiement', 'unpaid'))) { - if (empty($user->hasRight('societe', 'client', 'voir')) || $socid) { + if (!$user->hasRight('societe', 'client', 'voir') || $socid) { $original_file = 'private/'.$user->id.'/'.$original_file; // If user has no permission to see all, output dir is specific to user } } diff --git a/htdocs/user/card.php b/htdocs/user/card.php index c1dc297bfcb..139834c6a64 100644 --- a/htdocs/user/card.php +++ b/htdocs/user/card.php @@ -1165,7 +1165,7 @@ if ($action == 'create' || $action == 'adduserldap') { } // Categories - if (!empty($conf->categorie->enabled) && !empty($user->hasRight("categorie", "read"))) { + if (!empty($conf->categorie->enabled) && $user->hasRight("categorie", "read")) { print ''.$form->editfieldkey('Categories', 'usercats', '', $object, 0).''; $cate_arbo = $form->select_all_categories('user', null, 'parent', null, null, 1); print img_picto('', 'category', 'class="pictofixedwidth"').$form->multiselectarray('usercats', $cate_arbo, GETPOST('usercats', 'array'), 0, 0, 'maxwdith300 widthcentpercentminusx', 0, '90%'); @@ -1234,9 +1234,9 @@ if ($action == 'create' || $action == 'adduserldap') { print ''; print ''; - if ((!empty($conf->salaries->enabled) && !empty($user->hasRight("salaries", "read")) && in_array($id, $childids)) - || (!empty($conf->salaries->enabled) && !empty($user->hasRight("salaries", "readall"))) - || (!empty($conf->hrm->enabled) && !empty($user->hasRight("hrm", "employee", "read")))) { + if ((!empty($conf->salaries->enabled) && $user->hasRight("salaries", "read") && in_array($id, $childids)) + || (!empty($conf->salaries->enabled) && $user->hasRight("salaries", "readall")) + || (!empty($conf->hrm->enabled) && $user->hasRight("hrm", "employee", "read"))) { $langs->load("salaries"); // THM @@ -1541,8 +1541,8 @@ if ($action == 'create' || $action == 'adduserldap') { // Sensitive salary/value information if ((empty($user->socid) && in_array($id, $childids)) // A user can always see salary/value information for its subordinates - || (!empty($conf->salaries->enabled) && !empty($user->hasRight("salaries", "readall"))) - || (!empty($conf->hrm->enabled) && !empty($user->hasRight("hrm", "employee", "read")))) { + || (!empty($conf->salaries->enabled) && $user->hasRight("salaries", "readall")) + || (!empty($conf->hrm->enabled) && $user->hasRight("hrm", "employee", "read"))) { $langs->load("salaries"); // Salary @@ -1625,7 +1625,7 @@ if ($action == 'create' || $action == 'adduserldap') { } // Categories - if (!empty($conf->categorie->enabled) && !empty($user->hasRight("categorie", "read"))) { + if (!empty($conf->categorie->enabled) && $user->hasRight("categorie", "read")) { print ''.$langs->trans("Categories").''; print ''; print $form->showCategories($object->id, Categorie::TYPE_USER, 1); @@ -2567,7 +2567,7 @@ if ($action == 'create' || $action == 'adduserldap') { print ''; // Categories - if (!empty($conf->categorie->enabled) && !empty($user->hasRight("categorie", "read"))) { + if (!empty($conf->categorie->enabled) && $user->hasRight("categorie", "read")) { print ''.$form->editfieldkey('Categories', 'usercats', '', $object, 0).''; print ''; print img_picto('', 'category', 'class="pictofixedwidth"'); @@ -2712,8 +2712,8 @@ if ($action == 'create' || $action == 'adduserldap') { // Sensitive salary/value information if ((empty($user->socid) && in_array($id, $childids)) // A user can always see salary/value information for its subordinates - || (!empty($conf->salaries->enabled) && !empty($user->hasRight("salaries", "readall"))) - || (!empty($conf->hrm->enabled) && !empty($user->hasRight("hrm", "employee", "read")))) { + || (!empty($conf->salaries->enabled) && $user->hasRight("salaries", "readall")) + || (!empty($conf->hrm->enabled) && $user->hasRight("hrm", "employee", "read"))) { $langs->load("salaries"); // Salary diff --git a/htdocs/user/list.php b/htdocs/user/list.php index f0196789e25..443893fa096 100644 --- a/htdocs/user/list.php +++ b/htdocs/user/list.php @@ -131,7 +131,7 @@ $arrayfields = array( 'u.email'=>array('label'=>"EMail", 'checked'=>1, 'position'=>35), 'u.api_key'=>array('label'=>"ApiKey", 'checked'=>0, 'position'=>40, "enabled"=>(!empty($conf->api->enabled) && $user->admin)), 'u.fk_soc'=>array('label'=>"Company", 'checked'=>($contextpage == 'employeelist' ? 0 : 1), 'position'=>45), - 'u.salary'=>array('label'=>"Salary", 'checked'=>1, 'position'=>80, 'enabled'=>(!empty($conf->salaries->enabled) && !empty($user->hasRight("salaries", "readall")))), + 'u.salary'=>array('label'=>"Salary", 'checked'=>1, 'position'=>80, 'enabled'=>(!empty($conf->salaries->enabled) && $user->hasRight("salaries", "readall"))), 'u.datelastlogin'=>array('label'=>"LastConnexion", 'checked'=>1, 'position'=>100), 'u.datepreviouslogin'=>array('label'=>"PreviousConnexion", 'checked'=>0, 'position'=>110), 'u.datec'=>array('label'=>"DateCreation", 'checked'=>0, 'position'=>500), @@ -189,11 +189,11 @@ $error = 0; // Permission to list if ($mode == 'employee') { - if (empty($user->hasRight("salaries", "read"))) { + if (!$user->hasRight("salaries", "read")) { accessforbidden(); } } else { - if (empty($user->hasRight("user", "user", "read")) && empty($user->admin)) { + if (!$user->hasRight("user", "user", "read") && empty($user->admin)) { accessforbidden(); } } @@ -441,7 +441,7 @@ if ($search_categ == -2) { if ($search_warehouse > 0) { $sql .= " AND u.fk_warehouse = ".((int) $search_warehouse); } -if ($mode == 'employee' && empty($user->hasRight("salaries", "readall"))) { +if ($mode == 'employee' && !$user->hasRight("salaries", "readall")) { $sql .= " AND u.rowid IN (".$db->sanitize(join(',', $childids)).")"; } // Add where from extra fields @@ -939,9 +939,9 @@ while ($i < $imaxinloop) { $li = $object->getNomUrl(-1, '', 0, 0, 24, 1, 'login', '', 1); $canreadhrmdata = 0; - if ((!empty($conf->salaries->enabled) && !empty($user->hasRight("salaries", "read")) && in_array($obj->rowid, $childids)) - || (!empty($conf->salaries->enabled) && !empty($user->hasRight("salaries", "readall"))) - || (!empty($conf->hrm->enabled) && !empty($user->hasRight("hrm", "employee", "read")))) { + if ((!empty($conf->salaries->enabled) && $user->hasRight("salaries", "read") && in_array($obj->rowid, $childids)) + || (!empty($conf->salaries->enabled) && $user->hasRight("salaries", "readall")) + || (!empty($conf->hrm->enabled) && $user->hasRight("hrm", "employee", "read"))) { $canreadhrmdata = 1; } $canreadsecretapi = 0; diff --git a/htdocs/user/note.php b/htdocs/user/note.php index 2b1e02b1376..8269dfa93aa 100644 --- a/htdocs/user/note.php +++ b/htdocs/user/note.php @@ -138,7 +138,7 @@ if ($id) { } print ''; - $editenabled = (($action == 'edit') && !empty($user->hasRight("user", "user", "write"))); + $editenabled = (($action == 'edit') && $user->hasRight("user", "user", "write")); // Note print ''.$langs->trans("Note").''; diff --git a/htdocs/user/perms.php b/htdocs/user/perms.php index bfc234abce1..ef00c90d6cb 100644 --- a/htdocs/user/perms.php +++ b/htdocs/user/perms.php @@ -67,7 +67,7 @@ if (isset($user->socid) && $user->socid > 0) { } $feature2 = (($socid && $user->hasRight("user", "self", "write")) ? '' : 'user'); // A user can always read its own card if not advanced perms enabled, or if he has advanced perms, except for admin -if ($user->id == $id && (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && empty($user->hasRight("user", "self_advance", "readperms")) && empty($user->admin))) { +if ($user->id == $id && (!empty($conf->global->MAIN_USE_ADVANCED_PERMS) && !$user->hasRight("user", "self_advance", "readperms") && empty($user->admin))) { accessforbidden(); } diff --git a/test/phpunit/CodingPhpTest.php b/test/phpunit/CodingPhpTest.php index bff78d47b94..6388d3985e7 100644 --- a/test/phpunit/CodingPhpTest.php +++ b/test/phpunit/CodingPhpTest.php @@ -513,7 +513,7 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase $this->assertTrue($ok, 'Found a forbidden string sequence into '.$file['relativename'].' : name="token" value="\'.$_SESSION[..., you must use a newToken() instead of $_SESSION[\'newtoken\'].'); - // Test we don't have @var array( + // Test we don't have preg_grep with a param without preg_quote $ok=true; $matches=array(); preg_match_all('/preg_grep\(.*\$/', $filecontent, $matches, PREG_SET_ORDER); @@ -526,6 +526,17 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase $this->assertTrue($ok, 'Found a preg_grep with a param that is a $var but without preg_quote in file '.$file['relativename'].'.'); + // Test we don't have empty($user->hasRight + $ok=true; + $matches=array(); + preg_match_all('/empty\(\$user->hasRight/', $filecontent, $matches, PREG_SET_ORDER); + foreach ($matches as $key => $val) { + $ok=false; + break; + } + $this->assertTrue($ok, 'Found code empty($user->hasRight in file '.$file['relativename'].'. empty() must not be used with hasRight.'); + + // Test we don't have @var array( $ok=true; $matches=array();