From 152bbeb90a9ad54509d0e9be90b0fadb7f8464b8 Mon Sep 17 00:00:00 2001
From: Laurent Destailleur <eldy@destailleur.fr>
Date: Tue, 21 Mar 2023 18:27:32 +0100
Subject: [PATCH] Securize and enhance objectonoff for external modules

---
 htdocs/core/ajax/objectonoff.php              | 57 +++++++++++++++----
 htdocs/core/class/commonobject.class.php      |  3 +-
 htdocs/core/lib/ajax.lib.php                  |  5 +-
 htdocs/core/lib/security.lib.php              | 10 ++--
 .../template/class/myobject.class.php         |  4 +-
 5 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/htdocs/core/ajax/objectonoff.php b/htdocs/core/ajax/objectonoff.php
index 77cd3234cd8..54344da699e 100644
--- a/htdocs/core/ajax/objectonoff.php
+++ b/htdocs/core/ajax/objectonoff.php
@@ -18,7 +18,7 @@
 /**
  *       \file       htdocs/core/ajax/objectonoff.php
  *       \brief      File to set status for an object
- *       			 This Ajax service is called when option MAIN_DIRECT_STATUS_UPDATE is set.
+ *       			 This Ajax service is oftenly called when option MAIN_DIRECT_STATUS_UPDATE is set.
  */
 
 if (!defined('NOTOKENRENEWAL')) {
@@ -53,15 +53,34 @@ $format = 'int';
 
 $object = new GenericObject($db);
 
-$tablename = $element;
-if ($tablename == 'websitepage') {
-	$tablename = 'website_page';
-}
+$tmparray = explode('@', $element);
+if (empty($tmparray[1])) {
+	$subelement = '';
 
-$object->table_element = $tablename;
+	$object->module = $element;
+	$object->element = $element;
+	$object->table_element = $element;
+
+	// Special case for compatibility
+	if ($object->table_element == 'websitepage') {
+		$object->table_element = 'website_page';
+	}
+} else {
+	$element = $tmparray[0];
+	$subelement = $tmparray[1];
+
+	$object->module = $element;
+	$object->element = $subelement;
+	$object->table_element = $object->module.'_'.$object->element;
+}
 $object->id = $id;
 $object->fields[$field] = array('type' => $format, 'enabled' => 1);
 
+$module = $object->module;
+$element = $object->element;
+
+//var_dump($object->module); var_dump($object->element); var_dump($object->table_element);
+
 // Security check
 if (!empty($user->socid)) {
 	$socid = $user->socid;
@@ -70,12 +89,20 @@ if (!empty($user->socid)) {
 //$user->hasRight('societe', 'lire') = 0;$user->rights->fournisseur->lire = 0;
 //restrictedArea($user, 'societe', $id);
 
-if (in_array($field, array('status'))) {
-	restrictedArea($user, $element, $id);
+// We check permission.
+// Check is done on $user->rights->element->create or $user->rights->element->subelement->create (because $action = 'set')
+if (preg_match('/status$/', $field)) {
+	$module = $object->module;
+	$element = $object->element;
+	$usesublevelpermission = ($module != $element ? $element : '');
+	if ($usesublevelpermission && !isset($user->rights->$module->$element)) {	// There is no permission on object defined, we will check permission on module directly
+		$usesublevelpermission = '';
+	}
+	restrictedArea($user, $object->module, $object, $object->table_element, $usesublevelpermission);
 } elseif ($element == 'product' && in_array($field, array('tosell', 'tobuy', 'tobatch'))) {	// Special case for products
-	restrictedArea($user, 'produit|service', $id, 'product&product', '', '', 'rowid');
+	restrictedArea($user, 'produit|service', $object, 'product&product', '', '', 'rowid');
 } else {
-	httponly_accessforbidden("Bad value for combination of parameters element/field.");
+	httponly_accessforbidden("Bad value for combination of parameters element/field.");	// This includes the exit.
 }
 
 
@@ -89,7 +116,7 @@ print '<!-- Ajax page called with url '.dol_escape_htmltag($_SERVER["PHP_SELF"])
 
 // Registering new values
 if (($action == 'set') && !empty($id)) {
-	$triggerkey = strtoupper($element).'_UPDATE';
+	$triggerkey = strtoupper(($module != $element ? $module.'_' : '').$element).'_UPDATE';
 	// Special case
 	if ($triggerkey == 'SOCIETE_UPDATE') {
 		$triggerkey = 'COMPANY_MODIFY';
@@ -98,5 +125,11 @@ if (($action == 'set') && !empty($id)) {
 		$triggerkey = 'PRODUCT_MODIFY';
 	}
 
-	$object->setValueFrom($field, $value, $tablename, $id, $format, '', $user, $triggerkey);
+	$result = $object->setValueFrom($field, $value, $object->table_element, $id, $format, '', $user, $triggerkey);
+
+	if ($result < 0) {
+		print $object->error;
+		http_response_code(500);
+		exit;
+	}
 }
diff --git a/htdocs/core/class/commonobject.class.php b/htdocs/core/class/commonobject.class.php
index c85f66ca2c6..7d3c1030678 100644
--- a/htdocs/core/class/commonobject.class.php
+++ b/htdocs/core/class/commonobject.class.php
@@ -2122,6 +2122,8 @@ abstract class CommonObject
 
 		$error = 0;
 
+		dol_syslog(__METHOD__, LOG_DEBUG);
+
 		$this->db->begin();
 
 		$sql = "UPDATE ".$this->db->prefix().$table." SET ";
@@ -2146,7 +2148,6 @@ abstract class CommonObject
 
 		$sql .= " WHERE ".$id_field." = ".((int) $id);
 
-		dol_syslog(__METHOD__, LOG_DEBUG);
 		$resql = $this->db->query($sql);
 		if ($resql) {
 			if ($trigkey) {
diff --git a/htdocs/core/lib/ajax.lib.php b/htdocs/core/lib/ajax.lib.php
index fde2a988cf7..909124f5f5f 100644
--- a/htdocs/core/lib/ajax.lib.php
+++ b/htdocs/core/lib/ajax.lib.php
@@ -710,6 +710,7 @@ function ajax_object_onoff($object, $code, $field, $text_on, $text_off, $input =
 	if (empty($htmlname)) {
 		$htmlname = $code;
 	}
+	//var_dump($object->module); var_dump($object->element);
 
 	$out = '<script>
         $(function() {
@@ -722,7 +723,7 @@ function ajax_object_onoff($object, $code, $field, $text_on, $text_off, $input =
                     action: \'set\',
                     field: \''.dol_escape_js($field).'\',
                     value: \'1\',
-                    element: \''.dol_escape_js($object->element).'\',
+                    element: \''.dol_escape_js(((empty($object->module) || $object->module == $object->element) ? '' : $object->module.'@').$object->element).'\',
                     id: \''.$object->id.'\',
 					token: \''.currentToken().'\'
                 },
@@ -754,7 +755,7 @@ function ajax_object_onoff($object, $code, $field, $text_on, $text_off, $input =
                     action: \'set\',
                     field: \''.dol_escape_js($field).'\',
                     value: \'0\',
-                    element: \''.dol_escape_js($object->element).'\',
+                    element: \''.dol_escape_js(((empty($object->module) || $object->module == $object->element) ? '' : $object->module.'@').$object->element).'\',
                     id: \''.$object->id.'\',
 					token: \''.currentToken().'\'
                 },
diff --git a/htdocs/core/lib/security.lib.php b/htdocs/core/lib/security.lib.php
index 1f7b9404a7c..63202c693e4 100644
--- a/htdocs/core/lib/security.lib.php
+++ b/htdocs/core/lib/security.lib.php
@@ -485,7 +485,7 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '',
 				$readok = 0;
 				$nbko++;
 			}
-		} elseif (!empty($feature2)) { 													// This is for permissions on 2 levels
+		} elseif (!empty($feature2)) { 													// This is for permissions on 2 levels (module->object->read)
 			$tmpreadok = 1;
 			foreach ($feature2 as $subfeature) {
 				if ($subfeature == 'user' && $user->id == $objectid) {
@@ -504,7 +504,7 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '',
 				$readok = 0; // All tests are ko (we manage here the and, the or will be managed later using $nbko).
 				$nbko++;
 			}
-		} elseif (!empty($feature) && ($feature != 'user' && $feature != 'usergroup')) {		// This is permissions on 1 level
+		} elseif (!empty($feature) && ($feature != 'user' && $feature != 'usergroup')) {		// This is permissions on 1 level (module->read)
 			if (empty($user->rights->$feature->lire)
 				&& empty($user->rights->$feature->read)
 				&& empty($user->rights->$feature->run)) {
@@ -531,7 +531,7 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '',
 	// Check write permission from module (we need to know write permission to create but also to delete drafts record or to upload files)
 	$createok = 1;
 	$nbko = 0;
-	$wemustcheckpermissionforcreate = (GETPOST('sendit', 'alpha') || GETPOST('linkit', 'alpha') || in_array(GETPOST('action', 'aZ09'), array('create', 'update', 'add_element_resource', 'confirm_delete_linked_resource')) || GETPOST('roworder', 'alpha', 2));
+	$wemustcheckpermissionforcreate = (GETPOST('sendit', 'alpha') || GETPOST('linkit', 'alpha') || in_array(GETPOST('action', 'aZ09'), array('create', 'update', 'set', 'add_element_resource', 'confirm_delete_linked_resource')) || GETPOST('roworder', 'alpha', 2));
 	$wemustcheckpermissionfordeletedraft = ((GETPOST("action", "aZ09") == 'confirm_delete' && GETPOST("confirm", "aZ09") == 'yes') || GETPOST("action", "aZ09") == 'delete');
 
 	if ($wemustcheckpermissionforcreate || $wemustcheckpermissionfordeletedraft) {
@@ -576,7 +576,7 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '',
 					$createok = 0;
 					$nbko++;
 				}
-			} elseif (!empty($feature2)) {														// This is for permissions on one level
+			} elseif (!empty($feature2)) {													// This is for permissions on 2 levels (module->object->write)
 				foreach ($feature2 as $subfeature) {
 					if ($subfeature == 'user' && $user->id == $objectid && $user->rights->user->self->creer) {
 						continue; // User can edit its own card
@@ -599,7 +599,7 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '',
 						break;
 					}
 				}
-			} elseif (!empty($feature)) {												// This is for permissions on 2 levels ('creer' or 'write')
+			} elseif (!empty($feature)) {												// This is for permissions on 1 levels (module->write)
 				//print '<br>feature='.$feature.' creer='.$user->rights->$feature->creer.' write='.$user->rights->$feature->write; exit;
 				if (empty($user->rights->$feature->creer)
 				&& empty($user->rights->$feature->write)
diff --git a/htdocs/modulebuilder/template/class/myobject.class.php b/htdocs/modulebuilder/template/class/myobject.class.php
index 2a9b583b9e9..b54eff6a0e3 100644
--- a/htdocs/modulebuilder/template/class/myobject.class.php
+++ b/htdocs/modulebuilder/template/class/myobject.class.php
@@ -82,7 +82,7 @@ class MyObject extends CommonObject
 	 *  	'date', 'datetime', 'timestamp', 'duration',
 	 *  	'boolean', 'checkbox', 'radio', 'array',
 	 *  	'mail', 'phone', 'url', 'password', 'ip'
-	 *		Note: Filter must be a Dolibarr filter syntax string. Example: "(t.ref:like:'SO-%') or (t.date_creation:<:'20160101') or (t.status:!=:0) or (t.nature:is:NULL)"
+	 *		Note: Filter must be a Dolibarr Universal Filter syntax string. Example: "(t.ref:like:'SO-%') or (t.date_creation:<:'20160101') or (t.status:!=:0) or (t.nature:is:NULL)"
 	 *  'label' the translation key.
 	 *  'picto' is code of a picto to show before value in forms
 	 *  'enabled' is a condition when the field must be managed (Example: 1 or '$conf->global->MY_SETUP_PARAM' or 'isModEnabled("multicurrency")' ...)
@@ -260,7 +260,7 @@ class MyObject extends CommonObject
 		}
 
 		// Example to show how to set values of fields definition dynamically
-		/*if ($user->rights->mymodule->myobject->read) {
+		/*if ($user->hasRights->('mymodule', 'myobject', 'read')) {
 			$this->fields['myfield']['visible'] = 1;
 			$this->fields['myfield']['noteditable'] = 0;
 		}*/