Fix test on permissions

htdocs/asset/accountancy_codes.php

@@ -116,7 +116,7 @@ if (empty($reshook)) {
 		$action = '';
 	}
 
-	if ($action == "update") {
+	if ($action == "update" && $permissiontoadd) {
 		$assetaccountancycodes->setAccountancyCodesFromPost();
 
 		$result = $assetaccountancycodes->updateAccountancyCodes($user, $object->id);

htdocs/asset/card.php

@@ -135,7 +135,7 @@ if (empty($reshook)) {
 			setEventMessages($object->error, $object->errors, 'errors');
 		}
 		$action = '';
-	} elseif ($action == "add") {
+	} elseif ($action == "add" && $permissiontoadd) {
 		$object->supplier_invoice_id = GETPOSTINT('supplier_invoice_id');
 	}
 

htdocs/asset/depreciation_options.php

@@ -110,7 +110,7 @@ if (empty($reshook)) {
 		$action = '';
 	}
 
-	if ($action == "update") {
+	if ($action == "update" && $permissiontoadd) {
 		$result = $assetdepreciationoptions->setDeprecationOptionsFromPost();
 		if ($result > 0) {
 			$result = $assetdepreciationoptions->updateDeprecationOptions($user, $object->id);

htdocs/asset/model/accountancy_codes.php

@@ -108,7 +108,7 @@ if (empty($reshook)) {
 		$action = '';
 	}
 
-	if ($action == "update") {
+	if ($action == "update" && $permissiontoadd) {
 		$assetaccountancycodes->setAccountancyCodesFromPost();
 
 		$result = $assetaccountancycodes->updateAccountancyCodes($user, 0, $object->id);

htdocs/asset/model/depreciation_options.php

@@ -110,7 +110,7 @@ if (empty($reshook)) {
 		$action = '';
 	}
 
-	if ($action == "update") {
+	if ($action == "update" && $permissiontoadd) {
 		$result = $assetdepreciationoptions->setDeprecationOptionsFromPost(1);
 		if ($result > 0) {
 			$result = $assetdepreciationoptions->updateDeprecationOptions($user, 0, $object->id);

htdocs/barcode/codeinit.php

@@ -96,7 +96,7 @@ if (getDolGlobalString('BARCODE_THIRDPARTY_ADDON_NUM')) {
 	}
 }
 
-if ($action == 'initbarcodethirdparties') {
+if ($action == 'initbarcodethirdparties' && $user->hasRight('societe', 'lire')) {
 	if (!is_object($modBarCodeThirdparty)) {
 		$error++;
 		setEventMessages($langs->trans("NoBarcodeNumberingTemplateDefined"), null, 'errors');
@@ -202,7 +202,7 @@ if (getDolGlobalString('BARCODE_PRODUCT_ADDON_NUM')) {
 	}
 }
 
-if ($action == 'initbarcodeproducts') {
+if ($action == 'initbarcodeproducts' && $user->hasRight('produit', 'lire')) {
 	if (!is_object($modBarCodeProduct)) {
 		$error++;
 		setEventMessages($langs->trans("NoBarcodeNumberingTemplateDefined"), null, 'errors');

htdocs/barcode/printsheet.php

@@ -122,7 +122,7 @@ if (empty($reshook)) {
 		}
 	}
 
-	if ($action == 'builddoc') {
+	if ($action == 'builddoc' && $user->hasRight('barcode', 'read')) {
 		$result = 0;
 		$error = 0;
 

htdocs/bom/bom_net_needs.php

@@ -115,13 +115,6 @@ if (empty($reshook)) {
 			}
 		}
 	}
-
-	$TChildBom = array();
-	if ($action == 'treeview') {
-		$object->getNetNeedsTree($TChildBom, 1);
-	} else {
-		$object->getNetNeeds($TChildBom, 1);
-	}
 }
 
 
@@ -134,9 +127,18 @@ $formfile = new FormFile($db);
 
 $title = $langs->trans('BOM');
 $help_url ='EN:Module_BOM';
+
 llxHeader('', $title, $help_url, '', 0, 0, '', '', '', 'mod-bom page-net_needs');
 
 
+$TChildBom = array();
+if ($action == 'treeview') {
+	$object->getNetNeedsTree($TChildBom, 1);
+} else {
+	$object->getNetNeeds($TChildBom, 1);
+}
+
+
 // Part to show record
 if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) {
 	$head = bomPrepareHead($object);

htdocs/bookmarks/card.php

@@ -64,8 +64,8 @@ $permissiontodelete = $user->hasRight('bookmark', 'supprimer');
  * Actions
  */
 
-if ($action == 'add' || $action == 'addproduct' || $action == 'update') {
-	if ($action == 'update') {
+if (($action == 'add' || $action == 'addproduct' || $action == 'update') && $permissiontoadd) {
+	if ($action == 'update') {	// Test on permission already done
 		$invertedaction = 'edit';
 	} else {
 		$invertedaction = 'create';
@@ -81,7 +81,7 @@ if ($action == 'add' || $action == 'addproduct' || $action == 'update') {
 		exit;
 	}
 
-	if ($action == 'update') {
+	if ($action == 'update') {	// Test on permission already done
 		$object->fetch(GETPOSTINT("id"));
 	}
 	// Check if null because user not admin can't set an user and send empty value here.
@@ -106,7 +106,7 @@ if ($action == 'add' || $action == 'addproduct' || $action == 'update') {
 	if (!$error) {
 		$object->favicon = 'none';
 
-		if ($action == 'update') {
+		if ($action == 'update') {	// Test on permission already done
 			$res = $object->update();
 		} else {
 			$res = $object->create();

htdocs/categories/card.php

@@ -100,6 +100,7 @@ $error = 0;
 /*
  *	Actions
  */
+
 $parameters = array('socid' => $socid, 'origin' => $origin, 'catorigin' => $catorigin, 'type' => $type, 'urlfrom' => $urlfrom, 'backtopage' => $backtopage, 'label' => $label, 'description' => $description, 'color' => $color, 'position' => $position, 'visible' => $visible, 'parent' => $parent);
 // Note that $action and $object may be modified by some hooks
 $reshook = $hookmanager->executeHooks('doActions', $parameters, $object, $action);
@@ -174,10 +175,8 @@ if (empty($reshook)) {
 			}
 		}
 	}
-	// Confirm action
-	if (($action == 'add' || $action == 'confirmed') && $user->hasRight('categorie', 'creer')) {
 	// Action confirmation of creation category
-		if ($action == 'confirmed') {
+	if ($action == 'confirmed' && $user->hasRight('categorie', 'creer')) {
 		if ($urlfrom) {
 			header("Location: ".$urlfrom);
 			exit;
@@ -207,9 +206,9 @@ if (empty($reshook)) {
 		header("Location: ".DOL_URL_ROOT.'/categories/viewcat.php?id='.$result.'&type='.$type);
 		exit;
 	}
-	}
 }
 
+
 /*
  * View
  */

htdocs/categories/photos.php

@@ -48,9 +48,6 @@ if ($id == '' && $label == '') {
 	exit();
 }
 
-// Security check
-$result = restrictedArea($user, 'categorie', $id, '&category');
-
 $object = new Categorie($db);
 $result = $object->fetch($id, $label);
 if ($result <= 0) {
@@ -67,9 +64,16 @@ $upload_dir = $conf->categorie->multidir_output[$object->entity];
 
 $hookmanager->initHooks(array('categorycard'));
 
+// Security check
+$result = restrictedArea($user, 'categorie', $id, '&category');
+
+$permissiontoadd = $user->hasRight('categorie', 'creer');
+
+
 /*
  * Actions
  */
+
 $parameters = array('id' => $id,  'label' => $label, 'confirm' => $confirm, 'type' => $type, 'uploaddir' => $upload_dir, 'sendfile' => (GETPOST("sendit") ? true : false));
 // Note that $action and $object may be modified by some hooks
 $reshook = $hookmanager->executeHooks('doActions', $parameters, $object, $action);
@@ -96,11 +100,11 @@ if (empty($reshook)) {
 		}
 	}
 
-	if ($action == 'confirm_delete' && GETPOST("file") && $confirm == 'yes' && $user->hasRight('categorie', 'creer')) {
+	if ($action == 'confirm_delete' && GETPOST("file") && $confirm == 'yes' && $permissiontoadd) {
 		$object->delete_photo($upload_dir."/".GETPOST("file"));
 	}
 
-	if ($action == 'addthumb' && GETPOST("file")) {
+	if ($action == 'addthumb' && GETPOST("file") && $permissiontoadd) {
 		$object->addThumbs($upload_dir."/".GETPOST("file"));
 	}
 }

htdocs/categories/traduction.php

@@ -46,9 +46,6 @@ if ($id == '' && $label == '') {
 	exit();
 }
 
-// Security check
-$result = restrictedArea($user, 'categorie', $id, '&category');
-
 $object = new Categorie($db);
 $result = $object->fetch($id, $label);
 if ($result <= 0) {
@@ -61,6 +58,11 @@ if (is_numeric($type)) {
 	$type = Categorie::$MAP_ID_TO_CODE[$type];   // For backward compatibility
 }
 
+// Security check
+$result = restrictedArea($user, 'categorie', $id, '&category');
+
+$permissiontoadd = $user->hasRight('categorie', 'creer');
+
 
 /*
  * Actions
@@ -75,9 +77,7 @@ if ($cancel == $langs->trans("Cancel")) {
 
 
 // validation of addition
-if ($action == 'vadd' &&
-$cancel != $langs->trans("Cancel") &&
-($user->hasRight('categorie', 'creer'))) {
+if ($action == 'vadd' && $cancel != $langs->trans("Cancel") && $permissiontoadd) {
 	$object->fetch($id);
 	$current_lang = $langs->getDefaultLang();
 
@@ -124,9 +124,7 @@ $cancel != $langs->trans("Cancel") &&
 }
 
 // validation of the edition
-if ($action == 'vedit' &&
-$cancel != $langs->trans("Cancel") &&
-($user->hasRight('categorie', 'creer'))) {
+if ($action == 'vedit' && $cancel != $langs->trans("Cancel") && $permissiontoadd) {
 	$object->fetch($id);
 	$current_lang = $langs->getDefaultLang();
 

htdocs/collab/index.php

@@ -76,7 +76,8 @@ if (empty($action)) {
 	$action = 'preview';
 }
 
-
+$permissiontoadd = $user->hasRight('collab', 'read');
+$permissiontodelete = $user->hasRight('collab', 'delete');
 
 
 /*
@@ -92,7 +93,7 @@ if (GETPOST('refreshpage')) {
 
 
 // Add a collab page
-if ($action == 'add') {
+if ($action == 'add' && $permissiontoadd) {
 	$db->begin();
 
 	$objectpage->title = GETPOST('WEBSITE_TITLE');
@@ -125,7 +126,7 @@ if ($action == 'add') {
 }
 
 // Update page
-if ($action == 'delete') {
+if ($action == 'delete' && $permissiontodelete) {
 	$db->begin();
 
 	$res = $object->fetch(0, $website);

htdocs/comm/action/card.php

@@ -185,10 +185,10 @@ if (empty($reshook) && (GETPOST('removedassigned') || GETPOST('removedassigned')
 
 	$_SESSION['assignedtouser'] = json_encode($tmpassigneduserids);
 	$donotclearsession = 1;
-	if ($action == 'add') {
+	if ($action == 'add' && $usercancreate) {
 		$action = 'create';
 	}
-	if ($action == 'update') {
+	if ($action == 'update' && $usercancreate) {
 		$action = 'edit';
 	}
 
@@ -212,10 +212,10 @@ if (empty($reshook) && (GETPOST('removedassignedresource') || GETPOST('removedas
 
 	$_SESSION['assignedtoresource'] = json_encode($tmpassignedresourceids);
 	$donotclearsessionresource = 1;
-	if ($action == 'add') {
+	if ($action == 'add' && $usercancreate) {
 		$action = 'create';
 	}
-	if ($action == 'update') {
+	if ($action == 'update' && $usercancreate) {
 		$action = 'edit';
 	}
 
@@ -234,10 +234,10 @@ if (empty($reshook) && (GETPOST('addassignedtouser') || GETPOST('updateassignedt
 		$_SESSION['assignedtouser'] = json_encode($assignedtouser);
 	}
 	$donotclearsession = 1;
-	if ($action == 'add') {
+	if ($action == 'add' && $usercancreate) {
 		$action = 'create';
 	}
-	if ($action == 'update') {
+	if ($action == 'update' && $usercancreate) {
 		$action = 'edit';
 	}
 
@@ -256,10 +256,10 @@ if (empty($reshook) && (GETPOST('addassignedtoresource') || GETPOST('updateassig
 		$_SESSION['assignedtoresource'] = json_encode($assignedtoresource);
 	}
 	$donotclearsession = 1;
-	if ($action == 'add') {
+	if ($action == 'add' && $usercancreate) {
 		$action = 'create';
 	}
-	if ($action == 'update') {
+	if ($action == 'update' && $usercancreate) {
 		$action = 'edit';
 	}
 
@@ -274,7 +274,7 @@ if (empty($reshook) && $action == 'classin' && ($user->hasRight('agenda', 'allac
 }
 
 // Action clone object
-if (empty($reshook) && $action == 'confirm_clone' && $confirm == 'yes') {
+if (empty($reshook) && $action == 'confirm_clone' && $confirm == 'yes' && $usercancreate) {
 	if (1 == 0 && !GETPOST('clone_content') && !GETPOST('clone_receivers')) {
 		setEventMessages($langs->trans("NoCloneOptionsSpecified"), null, 'errors');
 	} else {
@@ -297,7 +297,7 @@ if (empty($reshook) && $action == 'confirm_clone' && $confirm == 'yes') {
 }
 
 // Add event
-if (empty($reshook) && $action == 'add') {
+if (empty($reshook) && $action == 'add' && $usercancreate) {
 	$error = 0;
 
 	if (empty($backtopage)) {
@@ -735,10 +735,8 @@ if (empty($reshook) && $action == 'add') {
 	}
 }
 
-/*
- * Action update event
- */
-if (empty($reshook) && $action == 'update') {
+// Action update event
+if (empty($reshook) && $action == 'update' && $usercancreate) {
 	if (empty($cancel)) {
 		$fulldayevent = GETPOST('fullday');
 		$aphour = GETPOSTINT('aphour');

htdocs/product/card.php

@@ -201,7 +201,7 @@ if ($object->id > 0) {
 $hookmanager->initHooks(array('productcard', 'globalcard'));
 
 // Permissions
-$usercanread   = (($object->type == Product::TYPE_PRODUCT && $user->hasRight('produit', 'read')) || ($object->type == Product::TYPE_SERVICE && $user->hasRight('service', 'lire')));
+$usercanread   = (($object->type == Product::TYPE_PRODUCT && $user->hasRight('produit', 'lire')) || ($object->type == Product::TYPE_SERVICE && $user->hasRight('service', 'lire')));
 $usercancreate = (($object->type == Product::TYPE_PRODUCT && $user->hasRight('produit', 'creer')) || ($object->type == Product::TYPE_SERVICE && $user->hasRight('service', 'creer')));
 $usercandelete = (($object->type == Product::TYPE_PRODUCT && $user->hasRight('produit', 'supprimer')) || ($object->type == Product::TYPE_SERVICE && $user->hasRight('service', 'supprimer')));
 

test/phpunit/CodingPhpTest.php

@@ -650,7 +650,11 @@ class CodingPhpTest extends CommonClassTest
 
 			preg_match_all('/if\s*\(\s*\$action\s*==\s*[\'"][a-z]+[\'"].*/', $filecontentaction, $matches, PREG_SET_ORDER);
 			foreach ($matches as $key => $val) {
-				if (!preg_match('/\$user->hasR/', $val[0]) && !preg_match('/\$permission/', $val[0]) && !preg_match('/\$usercan/', $val[0])) {
+				if (!preg_match('/\$user->hasR/', $val[0])
+					&& !preg_match('/\$permission/', $val[0])
+					&& !preg_match('/\$usercan/', $val[0])
+					&& !preg_match('/\$canedit/', $val[0])
+					&& !preg_match('/already done/', $val[0])) {
 					$ok = false;
 					print "Line: ".$val[0]."\n";
 					break;