From ea543428914840ce60ea39eb4c9be5b0eccd9333 Mon Sep 17 00:00:00 2001 From: VIAL-GOUTEYRON Quentin Date: Wed, 19 Aug 2026 02:46:21 +0200 Subject: [PATCH] NEW Drag and drop a file on the 23 remaining cards (#39473) * FIX Drag and drop of a file reports a wrong error, or none at all The error handler of dragAndDropFileUpload() had three defects that all end on the user believing the file was attached when it was not. - The first assignment of the message was dead code, there was no return after the test on the http code 403, so a refusal was reported as a generic error. - The key ErrorUploadPermissionDenied it used exists in no language file, so the raw key was shown. - JSON.parse() was called with no try/catch on an answer that is not always a json, a fatal error of the endpoint or a request over post_max_size for example. The exception left the user on a page with no message at all. - An empty list of files was treated as a success, while it means the endpoint stored nothing. The value of PHP_SELF is also escaped before it is written into the 6 generated javascript strings. It holds the path info of the request on a server that accepts it, so it is a user input. dol_escape_js() is called with the mode that escapes a double quote by a double quote, the strings being delimited by double quotes, and the sequence 'fetchProject() with no guard, while the signature of the function accepts an object that is not a CommonObject, and even a null when a module is given. Such a caller gets a fatal error where it expects the error string. No core caller is in that case today, an external module or a hook can be. - The entity of the object may have no declared directory, an object shared by another entity for example. The undefined index returned a relative path, so the caller read or wrote under the web root. The current entity is used instead, and the fallback is logged because the directory is then not the one of the entity of the object, which matters for a caller that deletes files. - When the current entity has no declared directory either, the fallback returned the sub directory alone, again a relative path. The same error than for a module that declares no directory at all is now returned. Adds the sub directory of a partnership and of a stock transfer, which their own document tabs already read. * FIX getElementProperties answers wrong properties for 9 elements A customer payment, a supplier payment, a various payment, a stock transfer and the 4 objects of the hrm module had no properties at all, or wrong ones, so any caller that resolves a class, a table or a document directory from an element failed on them. - payment, payment_supplier and payment_various had no branch. The branch of a customer payment tests $elementType and not $element, because the rule on the elements named myobject_mysubobject rewrites $element to 'payment' for 'payment_salary' too, which is stored somewhere else. - job, position, skill and evaluation answered a wrong table (hrm_job_user for a position, and so on) and no sub directory, while their document tabs read one named after the element. - stocktransfer answered an empty classname, because it is not the ucfirst() of the element, so a caller doing new $classname($db) ended on a fatal error. - The sub directory was concatenated even when the module is disabled and the directory is empty, which answered a path at the root of the file system. - A contact and a conference are stored into a sub directory their tab reads. isModEnabled('invoice') is tested for a customer payment: there is no module named 'compta', so testing it was always false, while $conf->compta->payment is set unconditionally by Conf::setValues() and could not be used as a proxy. * FIX Access refused to everyone on 11 objects of the core restrictedArea() and checkUserAccessToObject() refuse the access to objects that no permission and no rule can match, whatever the user, an administrator included. - The hrm module declares no permission at its first level, only 'all', and the stocktransfer module only 'stocktransfer'. A check on the module itself therefore tests a permission that does not exist. The mapping is the same one as into User::hasRight(). - The module of an event organization declares no permission at all, its whole permission block being commented out on purpose, and its cards check the parent project instead. The feature is mapped onto that project, with the two guards the card has: an external user is refused, and so is a conference with no parent project, whose id of 0 would otherwise grant an access with no check on the record at all. - The default rule of checkUserAccessToObject() builds its sql on the columns entity and fk_soc of the table. llx_asset, llx_paiement, llx_paiementfourn and llx_workstation_workstation have no fk_soc, and llx_hrm_job, llx_hrm_job_user and llx_hrm_skill have neither. The sql failed, so the access was refused to every user this rule applies to. These tables are now checked on their entity only, which is what the $check rule already does for the same class of tables, and the 3 tables of hrm can be checked on nothing at all. The rule is selected on the table and not on the element, because $object is an id and not an object for most of the callers, the cards of an asset and of a workstation included, which are broken today for any user without the permission to see all third parties. - An external user is refused explicitly on those tables: none of these objects is linked to a third party, so the default rule refused him through a link that does not exist, and the rules that replace it do not look at the third party of the user at all. Measured on a vanilla instance with 5 profiles, an administrator, an internal user with every right, one without the permission to see all third parties, one that is not a sales representative of the third party of the object, and an external user: the 11 objects go from refused to granted for the internal users and stay refused for the external one, and the 26 other elements answer exactly the same for the 5 profiles. * FIX A file dropped on a card is lost, or reported as refused when it was stored FileUpload stores the file into a directory that the "Attached files" tab of the object never reads, so the user attaches a file that no screen will ever show, and nothing is indexed in database to find it back. Measured on real databases: 216487 thirdparties over 216887 and 157852 products over 280319 are in that case on the cards that already enable the drag and drop. - The directory of the object is now forged with get_exdir(), the way the tabs do: it always uses the id for a thirdparty, whose ref is a company name and is not unique, and it falls back on the id when the ref is empty. The sub directory of the module is read with getMultidirOutput(), which knows the elements that store their documents into one. That function does not return an empty string when it fails but a string starting with 'error-', so only an absolute path is accepted: writing into that string would create the files under the web root. - fetchObjectByElement() returns an object even when fetch() returned 0. The object was then not loaded, and the file was stored at the root of the directory of the module, out of any object. The constructor now throws, and the endpoint answers the error with the same json contract than a successful call so that the caller can show it, instead of a fatal error and an http 500. - An attachment of the same name was silently overwritten, dol_move_uploaded_file() being called with $allowoverwrite = 1 while the name was checked before the ref of the object was added as a prefix. The check is done again on the final name, and on the .noexe suffixed name too, which that function appends to an executable file. - An executable file was renamed with that .noexe suffix and then reported as an error, while it was correctly stored. - The endpoint called restrictedArea() with an empty feature when the element is unknown, and the loop of that function then takes no branch at all and grants the access with no check. It refuses before, with the same http code and the same message than a refusal, so that a user cannot tell an object that exists but is not allowed from an object that does not exist. The refusal of an external user on an object of another third party answered a message of its own, which allowed the same enumeration. Comes with the tests of the path resolution, of the fallback of get_exdir(), of the rejection of the error string of getMultidirOutput(), and of the file name deduplication. * FIX Remove the drop area from 13 cards where dropping a file is harmful Two distinct groups, both of them removing a drop area that only produces a result the user does not want. 7 pages have no "Attached files" tab at all, so a file dropped on them can be reached by no screen: a fiscal year and its info page, an accounting model, a webhook trigger history, an intracomm report, a bookcal calendar and its booking list. Measured: 4 of them accepted the upload and wrote an orphan file, the accounting model wrote it at the root of the directory of the module, out of any object, and the 2 bookcal ones answered an http 500. 6 cards print their tabs inside their edit form, so the drop area covered that form: a product, an expense report, a VAT payment, a social contribution, a loan and a salary. Dropping a file there reloads the page, which discards what the user is typing. A product and an expense report have a second call for the read only view, so the drop area is only removed from the call of the edit branch; the 4 others share a single call between both views, so the parameter is now conditional. * NEW Drag and drop a file on the 23 remaining cards Every card of the core that owns an "Attached files" tab can now receive a file by drag and drop, which was the case of 32 of them only. The 23 added here are an asset, an event, a various payment, a customer payment, a contact, a donation, a conference or booth, a shipment, a supplier payment, a leave request, an evaluation, a job, a position, a skill, a knowledge record, a manufacturing order, a partnership, a lot, a stock transfer, a task, a reception, a resource and a workstation. The tab bar of a leave request and of a resource is printed inside their edit form, so the drop area is not enabled there: dropping a file reloads the page, which would discard what the user is typing. The card of a supplier payment prints its tab bar even when the object was not loaded, so the drop area is only enabled when it is. * FIX getMultidirOutput refuses an entity with no directory instead of falling back [skip-claudemd] The previous revision of this PR fell back on the directory of the current entity when the entity of the object had none, with a LOG_WARNING. On a multicompany install that made a caller read, write and above all delete files in the directory of another entity. Refuse instead: the function already answers 'error-diroutput-not-defined-for-this-object' when the module declares no directory at all, so the caller has one behaviour to handle, not two. The entity is cast to int, which is what the array index needs and what silences the four PhanTypeMismatchDimFetchNullable this function reported. --------- Co-authored-by: Laurent Destailleur --- htdocs/accountancy/admin/fiscalyear_card.php | 2 +- htdocs/accountancy/admin/fiscalyear_info.php | 2 +- htdocs/accountancy/admin/template/card.php | 2 +- htdocs/asset/card.php | 2 +- htdocs/bookcal/booking_list.php | 2 +- htdocs/bookcal/calendar_card.php | 2 +- htdocs/comm/action/card.php | 2 +- htdocs/compta/bank/various_payment/card.php | 2 +- htdocs/compta/paiement/card.php | 2 +- htdocs/compta/sociales/card.php | 2 +- htdocs/compta/tva/card.php | 2 +- htdocs/contact/card.php | 2 +- htdocs/core/ajax/fileupload.php | 41 +- htdocs/core/class/fileupload.class.php | 98 +++- htdocs/core/lib/files.lib.php | 54 +- htdocs/core/lib/functions.lib.php | 97 +++- htdocs/core/lib/security.lib.php | 46 ++ htdocs/don/card.php | 2 +- .../conferenceorbooth_card.php | 2 +- htdocs/expedition/card.php | 2 +- htdocs/expensereport/card.php | 2 +- htdocs/fourn/paiement/card.php | 4 +- htdocs/holiday/card.php | 8 +- htdocs/hrm/evaluation_card.php | 2 +- htdocs/hrm/job_card.php | 2 +- htdocs/hrm/position_card.php | 2 +- htdocs/hrm/skill_card.php | 2 +- htdocs/intracommreport/card.php | 2 +- .../knowledgerecord_card.php | 2 +- htdocs/langs/en_US/errors.lang | 1 + htdocs/loan/card.php | 2 +- htdocs/mrp/mo_card.php | 2 +- htdocs/partnership/partnership_card.php | 2 +- htdocs/product/card.php | 2 +- htdocs/product/stock/productlot_card.php | 2 +- .../stocktransfer/stocktransfer_card.php | 2 +- htdocs/projet/tasks/task.php | 2 +- htdocs/reception/card.php | 2 +- htdocs/resource/card.php | 2 +- htdocs/salaries/card.php | 2 +- htdocs/webhook/triggerhistory_card.php | 2 +- htdocs/workstation/workstation_card.php | 2 +- test/phpunit/FileUploadTest.php | 516 ++++++++++++++++++ test/phpunit/FunctionsLibDragDropTest.php | 482 ++++++++++++++++ test/phpunit/FunctionsLibTest.php | 208 +++++++ 45 files changed, 1533 insertions(+), 90 deletions(-) create mode 100644 test/phpunit/FileUploadTest.php create mode 100644 test/phpunit/FunctionsLibDragDropTest.php diff --git a/htdocs/accountancy/admin/fiscalyear_card.php b/htdocs/accountancy/admin/fiscalyear_card.php index 6de0f000241..8c587b143c1 100644 --- a/htdocs/accountancy/admin/fiscalyear_card.php +++ b/htdocs/accountancy/admin/fiscalyear_card.php @@ -321,7 +321,7 @@ if (($id || $ref) && $action == 'edit') { if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) { $head = fiscalyear_prepare_head($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Fiscalyear"), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("Fiscalyear"), -1, $object->picto); $morehtmlref = ''; //$morehtmlref .= '
'; diff --git a/htdocs/accountancy/admin/fiscalyear_info.php b/htdocs/accountancy/admin/fiscalyear_info.php index 54217511698..2ebd074e995 100644 --- a/htdocs/accountancy/admin/fiscalyear_info.php +++ b/htdocs/accountancy/admin/fiscalyear_info.php @@ -78,7 +78,7 @@ if ($id) { $head = fiscalyear_prepare_head($object); - print dol_get_fiche_head($head, 'info', $langs->trans("Fiscalyear"), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'info', $langs->trans("Fiscalyear"), -1, $object->picto); $linkback = ''.$langs->trans("BackToList").''; diff --git a/htdocs/accountancy/admin/template/card.php b/htdocs/accountancy/admin/template/card.php index dfc26e15bd5..74d64c5c7db 100644 --- a/htdocs/accountancy/admin/template/card.php +++ b/htdocs/accountancy/admin/template/card.php @@ -397,7 +397,7 @@ if (($id || $code) && $action == 'edit') { if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) { $head = accountingTransactionTemplatePrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("BookkeepingTemplate"), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("BookkeepingTemplate"), -1, $object->picto); $formconfirm = ''; diff --git a/htdocs/asset/card.php b/htdocs/asset/card.php index fc6405f5453..8fcf3591850 100644 --- a/htdocs/asset/card.php +++ b/htdocs/asset/card.php @@ -263,7 +263,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $res = $object->fetch_optionals(); $head = assetPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Asset"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("Asset"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/bookcal/booking_list.php b/htdocs/bookcal/booking_list.php index f1ccc9ea740..8dd2d2b5980 100644 --- a/htdocs/bookcal/booking_list.php +++ b/htdocs/bookcal/booking_list.php @@ -126,7 +126,7 @@ llxHeader('', $title, $helpurl, '', 0, 0, '', '', '', 'mod-bookcal page-list'); if ($object->id > 0) { $head = calendarPrepareHead($object); - print dol_get_fiche_head($head, 'booking', $langs->trans("Calendar"), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'booking', $langs->trans("Calendar"), -1, $object->picto); $formconfirm = ''; diff --git a/htdocs/bookcal/calendar_card.php b/htdocs/bookcal/calendar_card.php index e320b75c18f..2344fd983db 100644 --- a/htdocs/bookcal/calendar_card.php +++ b/htdocs/bookcal/calendar_card.php @@ -287,7 +287,7 @@ if (($id || $ref) && $action == 'edit') { if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) { $head = calendarPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Calendar"), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("Calendar"), -1, $object->picto); $formconfirm = ''; diff --git a/htdocs/comm/action/card.php b/htdocs/comm/action/card.php index f891f4c38d9..954cbd10d3c 100644 --- a/htdocs/comm/action/card.php +++ b/htdocs/comm/action/card.php @@ -2659,7 +2659,7 @@ if ($id > 0 && $action != 'create') { print ''; } else { - print dol_get_fiche_head($head, 'card', $langs->trans("Action"), -1, 'action'); + print dol_get_fiche_head($head, 'card', $langs->trans("Action"), -1, 'action', 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/compta/bank/various_payment/card.php b/htdocs/compta/bank/various_payment/card.php index ebdd2d23f8f..bb4aa5f009f 100644 --- a/htdocs/compta/bank/various_payment/card.php +++ b/htdocs/compta/bank/various_payment/card.php @@ -639,7 +639,7 @@ if ($id) { print $form->formconfirm(dolBuildUrl($_SERVER["PHP_SELF"], ['id' => $object->id]), $langs->trans('DeleteVariousPayment'), $text, 'confirm_delete', '', '', 2); } - print dol_get_fiche_head($head, 'card', $langs->trans("VariousPayment"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("VariousPayment"), -1, $object->picto, 0, '', '', 0, '', 1); $morehtmlref = '
'; // Project diff --git a/htdocs/compta/paiement/card.php b/htdocs/compta/paiement/card.php index d72b08f95d2..2d0dfb10320 100644 --- a/htdocs/compta/paiement/card.php +++ b/htdocs/compta/paiement/card.php @@ -291,7 +291,7 @@ $form = new Form($db); $head = payment_prepare_head($object); -print dol_get_fiche_head($head, 'payment', $langs->trans("PaymentCustomerInvoice"), -1, 'payment'); +print dol_get_fiche_head($head, 'payment', $langs->trans("PaymentCustomerInvoice"), -1, 'payment', 0, '', '', 0, '', 1); // Confirmation of payment delete if ($action == 'delete') { diff --git a/htdocs/compta/sociales/card.php b/htdocs/compta/sociales/card.php index 141a3e85cef..5aeb7a4a758 100644 --- a/htdocs/compta/sociales/card.php +++ b/htdocs/compta/sociales/card.php @@ -513,7 +513,7 @@ if ($id > 0) { } - print dol_get_fiche_head($head, 'card', $langs->trans("SocialContribution"), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("SocialContribution"), -1, $object->picto, 0, '', '', 0, '', ($action == 'edit' ? 0 : 1)); // Print form confirm print $formconfirm; diff --git a/htdocs/compta/tva/card.php b/htdocs/compta/tva/card.php index bade10f8669..ae9450594c1 100644 --- a/htdocs/compta/tva/card.php +++ b/htdocs/compta/tva/card.php @@ -583,7 +583,7 @@ if ($id > 0) { $formconfirm = $hookmanager->resPrint; } - print dol_get_fiche_head($head, 'card', $langs->trans("VATPayment"), -1, 'payment', 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("VATPayment"), -1, 'payment', 0, '', '', 0, '', ($action == 'edit' ? 0 : 1)); // Print form confirm print $formconfirm; diff --git a/htdocs/contact/card.php b/htdocs/contact/card.php index 9fe9199906a..8a2e7ac1633 100644 --- a/htdocs/contact/card.php +++ b/htdocs/contact/card.php @@ -1355,7 +1355,7 @@ if (is_object($objcanvas) && $objcanvas->displayCanvasExists($action)) { // Show errors dol_htmloutput_errors(is_numeric($error) ? '' : $error, $errors); - print dol_get_fiche_head($head, 'card', $title, -1, 'contact'); + print dol_get_fiche_head($head, 'card', $title, -1, 'contact', 0, '', '', 0, '', 1); if ($action == 'create_user') { // Full firstname and lastname separated with a dot : firstname.lastname diff --git a/htdocs/core/ajax/fileupload.php b/htdocs/core/ajax/fileupload.php index 353bab4111c..c8e64b51501 100644 --- a/htdocs/core/ajax/fileupload.php +++ b/htdocs/core/ajax/fileupload.php @@ -56,6 +56,17 @@ $elementupload = $element; // Load object according to $id and $element $object = fetchObjectByElement($id, $element); +// fetchObjectByElement() returns an object even when the record was not found, and it returns a +// GenericObject with no module when the element is unknown. In both cases restrictedArea() is then called +// with an empty feature, and it grants the access without checking any permission, so we must stop here. +// Note: fetchObjectByElement() may also return an int instead of an object when the module is disabled. +// We answer the same http code and the same message than a refusal by restrictedArea() below, so that a +// user can't tell an object that exists but is not allowed from an object that does not exist. +if (!is_object($object) || empty($object->id) || empty($object->module)) { + dol_syslog("fileupload.php object ".$element." with id ".$id." was not found or its element is not supported", LOG_WARNING); + httponly_accessforbidden('Not allowed'); +} + $module = $object->module; $element = $object->element; @@ -69,14 +80,21 @@ if ($usesublevelpermission && !$user->hasRight($module, $element)) { // There is // Security check if (!empty($user->socid)) { $socid = $user->socid; + // socid is not declared on CommonObject, which is the type fetchObjectByElement() answers, and an object + // that has no third party does not own the property at all. if (property_exists($object, 'socid') && !empty($object->socid) && $socid != $object->socid) { // @phan-suppress-current-line PhanUndeclaredProperty - httponly_accessforbidden("Access on object not allowed for this external user."); // This includes the exit. + // Same message than every other refusal of this page: a distinct one tells an external user that the + // object exists but belongs to another third party, which lets him enumerate the records of the others. + dol_syslog("fileupload.php object ".$element." with id ".$id." belongs to another third party than the external user", LOG_WARNING); + httponly_accessforbidden('Not allowed'); // This includes the exit. } } $result = restrictedArea($user, $object->module, $object, $object->table_element, $usesublevelpermission, 'fk_soc', 'rowid', 0, 1); // Call with mode return if (!$result) { - httponly_accessforbidden('Not allowed by restrictArea (module='.$object->module.' table_element='.$object->table_element.')'); + // The module and the table are reported into the log only, they must not be disclosed to the caller + dol_syslog("fileupload.php not allowed by restrictedArea (module=".$object->module." table_element=".$object->table_element.")", LOG_WARNING); + httponly_accessforbidden('Not allowed'); } @@ -103,7 +121,24 @@ switch ($_SERVER['REQUEST_METHOD']) { break; */ case 'POST': - $upload_handler = new FileUpload(null, $id, $elementupload); + // The constructor throws an exception when the element does not support file uploading, or when the + // object was not found. Answer with the same json contract than post() so the caller can show the + // error, instead of letting a fatal error return an http code 500 with no usable content. + try { + $upload_handler = new FileUpload(null, $id, $elementupload); + } catch (Exception $e) { + // Same http code 200 and same content type negotiation than post(), the error is reported into the + // json content as the caller expects. Forcing 'application/json' would make jQuery parse the content + // by itself, and the JSON.parse() of the caller would then fail on an already parsed array. + if (isset($_SERVER['HTTP_ACCEPT']) && (strpos($_SERVER['HTTP_ACCEPT'], 'application/json') !== false)) { + header('Content-type: application/json'); + } else { + header('Content-type: text/plain'); + } + echo json_encode(array(array('name' => '', 'error' => $e->getMessage()))); + $db->close(); + exit; + } /*if (isset($_REQUEST['_method']) && $_REQUEST['_method'] === 'DELETE') { $file = GETPOST('file'); diff --git a/htdocs/core/class/fileupload.class.php b/htdocs/core/class/fileupload.class.php index 7e2b2f7f387..938fdf7ab42 100644 --- a/htdocs/core/class/fileupload.class.php +++ b/htdocs/core/class/fileupload.class.php @@ -56,10 +56,11 @@ class FileUpload * @param ?array{script_url?:string,upload_dir?:string,upload_url?:string,param_name?:string,delete_type?:string,max_file_size?:?int,min_file_size?:int,accept_file_types?:string,max_number_of_files?:?int,max_width?:?int,max_height?:?int,min_width?:int,min_height?:int,discard_aborted_uploads?:bool,image_versions?:array} $options Options array * @param int $fk_element ID of element * @param string $element Code of element + * @throws Exception If the object was not found, if the element does not support file + * uploading, or if the upload directory is missing or not writable */ public function __construct($options = null, $fk_element = null, $element = null) { - global $db; global $hookmanager; $hookmanager->initHooks(array('fileupload')); @@ -77,11 +78,6 @@ class FileUpload //print 'fileupload.class.php: element='.$element.' pathname='.$pathname.' filename='.$filename.' dir_output='.$dir_output."\n"; - if (empty($dir_output)) { - setEventMessage('The element '.$element.' is not supported for uploading file. dir_output is unknown.', 'errors'); - throw new Exception('The element '.$element.' is not supported for uploading file. dir_output is unknown.'); - } - $object_ref = 'UndefinedReference'; $object = null; // If pathname and filename are null then we can still upload files if we have specified upload_dir on $options @@ -89,36 +85,62 @@ class FileUpload // Get object from its id and type $object = fetchObjectByElement($fk_element, $element); - $object_ref = dol_sanitizeFileName($object->ref); + // fetchObjectByElement() also returns an object when the record was not found (fetch() returning 0), + // so we must check the object was really loaded. Without this, files would be stored at the root of + // the module directory, out of any object and out of any permission check on the object. + if (!is_object($object) || empty($object->id)) { + dol_syslog(get_class($this)."::__construct object ".$element." with id ".((int) $fk_element)." was not found", LOG_WARNING); + throw new Exception('objectnotfound'); + } + + // Directory of the module, including the sub directory used by some elements (/sending for a shipment, + // /commande for a supplier order, / for a task, ...). We must use the same directory than + // the one used by the "Attached files" tab of the object, otherwise the uploaded file is stored but + // never shown to the user. + // Note: getMultidirOutput() only knows the elements of its own switch, that is a minority of them. For + // all the others it does not return an empty string but the string + // 'error-diroutput-not-defined-for-this-object=x', and keeping the directory of getElementProperties() + // is then the nominal case, not a degraded one. So we only accept an absolute path: that string is a + // relative path, and writing into it would create the files under the web root. + $tmpdir = getMultidirOutput($object, $element); + if (!empty($tmpdir) && preg_match('/^([a-z]:)?[\\\\\/]/i', $tmpdir)) { + $dir_output = dol_sanitizePathName($tmpdir); + } // Add object reference as file name prefix if const MAIN_DISABLE_SUGGEST_REF_AS_PREFIX is not enabled if (!getDolGlobalInt('MAIN_DISABLE_SUGGEST_REF_AS_PREFIX')) { - $savingDocMask = $object_ref . '-__file__'; + $savingDocMask = dol_sanitizeFileName($object->ref).'-__file__'; } - // Special cases to forge $object_ref used to forge $upload_dir - if ($element == 'invoice_supplier') { - $object_ref = get_exdir($object->id, 2, 0, 0, $object, 'invoice_supplier').$object_ref; - } elseif ($element == 'project_task') { - $parentForeignKey = 'fk_project'; - $parentClass = 'Project'; - $parentElement = 'projet'; - $parentObject = 'project'; + // get_exdir() forges the directory of an object the way the "Attached files" tabs do: it always + // uses the id for a thirdparty (a thirdparty ref is a company name, so it is not unique), and it + // falls back on the id when the ref is empty. Using anything else here would store the file into + // a directory the tab never reads. + // Note that a few tabs sanitize the ref themselves instead of calling this function, so they have + // no fallback: on an object whose ref is empty in database, which the interface does not produce + // but old records may hold, they read the root of the directory of the module while we store + // under the id. Storing at the root would mix the files of every object of the module, so the + // fallback is kept and those tabs are the ones that should be fixed. + $object_ref = get_exdir(0, 0, 0, 1, $object, $element); - dol_include_once('/'.$parentElement.'/class/'.$parentObject.'.class.php'); - $parent = new $parentClass($db); - if ($object->$parentForeignKey !== null) { - $parent->fetch((int) $object->$parentForeignKey); - if (!empty($parent->socid)) { - $parent->fetch_thirdparty(); - } - $object->$parentObject = clone $parent; - } - - $object_ref = dol_sanitizeFileName($object->project->ref).'/'.$object_ref; + // For the modules storing their documents on several levels, get_exdir() returned the level + // directories only, so we must append the directory of the object itself. + if (in_array($element, array('invoice_supplier', 'supplier_invoice'))) { + $object_ref .= '/'.dol_sanitizeFileName($object->ref); } } + // Tested after the call to getMultidirOutput(), because some elements have no 'dir_output' returned by + // getElementProperties() while getMultidirOutput() is still able to resolve their output directory. + if (empty($dir_output)) { + dol_syslog(get_class($this)."::__construct element ".$element." is not supported for uploading file, dir_output is unknown", LOG_WARNING); + throw new Exception('elementnotsupported'); + } + + // Note: 'upload_url' is not always the url of the file stored into 'upload_dir', because document.php + // forges the path of the file with its own rules for each value of modulepart. It is currently not a + // problem because the only caller of this class (the drag and drop of a file on a card) does not use + // the url returned into the json. $this->options = array( 'script_url' => $_SERVER['PHP_SELF'], 'upload_dir' => $dir_output.'/'.$object_ref.'/', @@ -458,6 +480,20 @@ class FileUpload $file->name = $fileNameWithPrefix; } + // trimFileName() checked the name is not already used, but it did it before the reference of the + // object was added as a prefix above, so it compared a name that is not the one we store. We must + // check it again on the final name, otherwise uploading twice the same file silently overwrites + // the first one, because dol_move_uploaded_file() is called below with $allowoverwrite = 1. + // The .noexe suffix is appended by dol_move_uploaded_file() on an executable file, so we must also + // look for the suffixed name, otherwise such a file is never seen as already existing and it is + // overwritten at each upload. + if ($this->options['discard_aborted_uploads']) { + $tmppath = dol_sanitizePathName($this->options['upload_dir']); + while (dol_is_file($tmppath.dol_sanitizeFileName($file->name)) || dol_is_file($tmppath.dol_sanitizeFileName($file->name).'.noexe')) { + $file->name = $this->upcountName($file->name); + } + } + $file_path = dol_sanitizePathName($this->options['upload_dir']).dol_sanitizeFileName($file->name); $append_file = !$this->options['discard_aborted_uploads'] && dol_is_file($file_path) && $file->size > dol_filesize($file_path); @@ -470,6 +506,14 @@ class FileUpload } else { // TODO Replace this with a call of dol_add_file_process(... $mode=1) $result = dol_move_uploaded_file($uploaded_file, $file_path, 1, 0, 0, 0, 'userfile'); + + // A return of 2 means the file was stored with a .noexe suffix appended on its name. + // We must follow that renaming, otherwise the size check below is done on a file that + // does not exist, and we report an error on a file that was correctly stored. + if ($result == 2) { + $file->name .= '.noexe'; + $file_path .= '.noexe'; + } } } else { // Non-multipart uploads (PUT method support) diff --git a/htdocs/core/lib/files.lib.php b/htdocs/core/lib/files.lib.php index 42c914411d7..5be5993d72c 100644 --- a/htdocs/core/lib/files.lib.php +++ b/htdocs/core/lib/files.lib.php @@ -4048,6 +4048,14 @@ function dragAndDropFileUpload($htmlname) { global $object, $langs; + // Every generated javascript string that carries an interpolated value is delimited by a single quote, so + // dol_escape_js() is called with the mode 1 everywhere below: it escapes a single quote and leaves a double + // quote alone. The default mode would rewrite a double quote into an escaped single quote, which is safe + // inside a '...' string but silently alters the value. + // dol_escape_js() escapes the quotes but not ''.img_picto("", 'download').'
'.$langs->trans("DropFileToAddItToObject").'
'; $out .= "\n\n"; @@ -4056,7 +4064,7 @@ function dragAndDropFileUpload($htmlname) jQuery(document).ready(function() { var enterTargetDragDrop = null; - $("#'.$htmlname.'").addClass("cssDragDropArea"); + $(\'#'.$htmlname.'\').addClass(\'cssDragDropArea\'); $(".cssDragDropArea").on("dragenter", function(ev, ui) { var dataTransfer = ev.originalEvent.dataTransfer; @@ -4074,7 +4082,7 @@ function dragAndDropFileUpload($htmlname) console.log("dragAndDropFileUpload: We add class highlightDragDropArea") enterTargetDragDrop = ev.target; $(this).addClass("highlightDragDropArea"); - $("#'.$htmlname.'Message").removeClass("hidden"); + $(\'#'.$htmlname.'Message\').removeClass(\'hidden\'); ev.preventDefault(); }); @@ -4082,7 +4090,7 @@ function dragAndDropFileUpload($htmlname) // Going out of drop area. Remove Highlight if (enterTargetDragDrop == ev.target){ console.log("dragAndDropFileUpload: We remove class highlightDragDropArea") - $("#'.$htmlname.'Message").addClass("hidden"); + $(\'#'.$htmlname.'Message\').addClass(\'hidden\'); $(this).removeClass("highlightDragDropArea"); } }); @@ -4093,12 +4101,12 @@ function dragAndDropFileUpload($htmlname) }); $(".cssDragDropArea").on("drop", function(e) { - console.log("Trigger event file dropped. fk_element='.dol_escape_js((string) $object->id).' element='.dol_escape_js($object->element).'"); + console.log(\'Trigger event file dropped. fk_element='.dol_escape_js((string) $object->id, 1).' element='.dol_escape_js($object->element, 1).'\'); e.preventDefault(); fd = new FormData(); - fd.append("fk_element", "'.dol_escape_js((string) $object->id).'"); - fd.append("element", "'.dol_escape_js($object->element).'"); - fd.append("token", "'.currentToken().'"); + fd.append(\'fk_element\', \''.dol_escape_js((string) $object->id, 1).'\'); + fd.append(\'element\', \''.dol_escape_js($object->element, 1).'\'); + fd.append(\'token\', \''.currentToken().'\'); fd.append("action", "linkit"); var dataTransfer = e.originalEvent.dataTransfer; @@ -4112,7 +4120,7 @@ function dragAndDropFileUpload($htmlname) $(".cssDragDropArea").removeClass("highlightDragDropArea"); counterdragdrop = 0; $.ajax({ - url: "'.DOL_URL_ROOT.'/core/ajax/fileupload.php", + url: \''.DOL_URL_ROOT.'/core/ajax/fileupload.php\', type: "POST", processData: false, contentType: false, @@ -4121,7 +4129,16 @@ function dragAndDropFileUpload($htmlname) console.log("Uploaded.", arguments); /* arguments[0] is the json string of files */ /* arguments[1] is the value for variable "success", can be 0 or 1 */ - let listoffiles = JSON.parse(arguments[0]); + let listoffiles = []; + /* The answer is not the expected json when php stopped before answering, for example when + post_max_size was reached. Without this, the exception of JSON.parse() would leave the + user on a page with no message at all, thinking the file was added. */ + try { + listoffiles = JSON.parse(arguments[0]); + } catch (e) { + window.location.href = \''.$pageurl.'?id='.dol_escape_js((string) $object->id, 1).'&seteventmessages=ErrorUploadFileDragDrop:errors\'; + return; + } console.log(listoffiles); let nboferror = 0; for (let i = 0; i < listoffiles.length; i++) { @@ -4131,18 +4148,23 @@ function dragAndDropFileUpload($htmlname) } } console.log(nboferror); - if (nboferror > 0) { - window.location.href = "'.$_SERVER["PHP_SELF"].'?id='.dol_escape_js((string) $object->id).'&seteventmessages=ErrorOnAtLeastOneFileUpload:warnings"; + /* An empty list means no file was stored at all, so it is an error and not a success: + php empties $_FILES when post_max_size is reached. */ + if (listoffiles.length == 0) { + window.location.href = \''.$pageurl.'?id='.dol_escape_js((string) $object->id, 1).'&seteventmessages=ErrorUploadFileDragDrop:errors\'; + } else if (nboferror > 0) { + window.location.href = \''.$pageurl.'?id='.dol_escape_js((string) $object->id, 1).'&seteventmessages=ErrorOnAtLeastOneFileUpload:warnings\'; } else { - window.location.href = "'.$_SERVER["PHP_SELF"].'?id='.dol_escape_js((string) $object->id).'&seteventmessages=UploadFileDragDropSuccess:mesgs"; + window.location.href = \''.$pageurl.'?id='.dol_escape_js((string) $object->id, 1).'&seteventmessages=UploadFileDragDropSuccess:mesgs\'; } }, - error:function() { + error:function(jqXHR) { console.log("Error Uploading.", arguments) - if (arguments[0].status == 403) { - window.location.href = "'.$_SERVER["PHP_SELF"].'?id='.dol_escape_js((string) $object->id).'&seteventmessages=ErrorUploadPermissionDenied:errors"; + if (jqXHR.status == 403) { + window.location.href = \''.$pageurl.'?id='.dol_escape_js((string) $object->id, 1).'&seteventmessages=ErrorUploadFileDragDropPermissionDenied:errors\'; + } else { + window.location.href = \''.$pageurl.'?id='.dol_escape_js((string) $object->id, 1).'&seteventmessages=ErrorUploadFileDragDrop:errors\'; } - window.location.href = "'.$_SERVER["PHP_SELF"].'?id='.dol_escape_js((string) $object->id).'&seteventmessages=ErrorUploadFileDragDropPermissionDenied:errors"; }, }) }); diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index cbbd89b7e46..d8a62613fa4 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -187,6 +187,12 @@ function getMultidirOutput($object, $module = '', $forobject = 0, $mode = 'outpu $module = 'knowledgemanagement'; $subdirectory = '/knowledgerecord'; break; + case 'partnership': + $subdirectory = '/partnership'; + break; + case 'stocktransfer': + $subdirectory = '/stocktransfer'; + break; case 'commande_fournisseur': $module = 'fournisseur'; $subdirectory = '/commande'; @@ -207,10 +213,20 @@ function getMultidirOutput($object, $module = '', $forobject = 0, $mode = 'outpu case 'project_task': $module = 'projet'; - // Fetch the project to build the correct path - $object->fetchProject(); + // Fetch the project to build the correct path. The signature of this function accepts an object + // that is not a CommonObject, and even a null when a module is given, so we must not call a method + // that only a CommonObject owns without testing it exists. + if (is_object($object) && method_exists($object, 'fetchProject')) { + $object->fetchProject(); + } - $subdirectory = '/'.$object->project->ref; + // The ref must be sanitized with dol_sanitizeFileName() and not only with dol_sanitizePathName() + // done at the end of this function, because a project ref is a user input that may contain a '/', + // a ':' or an accented char. dol_sanitizePathName() keeps them, so we would not return the + // directory used by projet/tasks/document.php, that sanitizes the ref with dol_sanitizeFileName(). + if (!empty($object->project->ref)) { + $subdirectory = '/'.dol_sanitizeFileName($object->project->ref); + } break; case 'action': case 'actioncomm': @@ -226,7 +242,13 @@ function getMultidirOutput($object, $module = '', $forobject = 0, $mode = 'outpu if (isset($conf->$module) && property_exists($conf->$module, 'multidir_output')) { $s = ''; if ($mode != 'outputrel') { - $s = $conf->$module->multidir_output[(empty($object->entity) ? $conf->entity : $object->entity)] . $subdirectory; + // An entity with no directory declared used to return an undefined index, so a relative path + // that made the caller read or write under the web root. Answer the error instead. + $entity = (int) (empty($object->entity) ? $conf->entity : $object->entity); + if (!isset($conf->$module->multidir_output[$entity])) { + return 'error-diroutput-not-defined-for-this-object='.$module; + } + $s = $conf->$module->multidir_output[$entity].$subdirectory; } if ($forobject && $object->id > 0) { $s .= ($mode != 'outputrel' ? '/' : '') . get_exdir(0, 0, 0, 0, $object); @@ -246,7 +268,12 @@ function getMultidirOutput($object, $module = '', $forobject = 0, $mode = 'outpu } } elseif ($mode == 'temp') { if (isset($conf->$module) && property_exists($conf->$module, 'multidir_temp')) { - return dol_sanitizePathName($conf->$module->multidir_temp[(empty($object->entity) ? $conf->entity : $object->entity)]); + // Same guard as the 'output' mode above, see the comment there + $entity = (int) (empty($object->entity) ? $conf->entity : $object->entity); + if (!isset($conf->$module->multidir_temp[$entity])) { + return 'error-dirtemp-not-defined-for-this-object='.$module; + } + return dol_sanitizePathName($conf->$module->multidir_temp[$entity]); } elseif (isset($conf->$module) && property_exists($conf->$module, 'dir_temp')) { return dol_sanitizePathName($conf->$module->dir_temp); } else { @@ -15243,6 +15270,7 @@ function getElementProperties($elementType) $module = 'societe'; $subelement = 'contact'; $table_element = 'socpeople'; + $subdir = '/contact'; } elseif ($elementType == 'inventory') { $module = 'product'; $classpath = 'product/inventory/class'; @@ -15458,6 +15486,45 @@ function getElementProperties($elementType) $classfile = 'paymentsalary'; $classname = 'PaymentSalary'; $module = 'salaries'; + } elseif ($elementType == 'payment') { + $classpath = 'compta/paiement/class'; + $classfile = 'paiement'; + $classname = 'Paiement'; + $module = 'facture'; // A customer payment belongs to the invoice module, there is no 'compta' module + $element = 'payment'; + $subelement = 'payment'; + $table_element = 'paiement'; + } elseif ($elementType == 'payment_supplier') { + $classpath = 'fourn/class'; + $classfile = 'paiementfourn'; + $classname = 'PaiementFourn'; + $module = 'fournisseur'; + $element = 'payment_supplier'; + $subelement = 'payment_supplier'; + $table_element = 'paiementfourn'; + } elseif ($elementType == 'payment_various') { + $classpath = 'compta/bank/class'; + $classfile = 'paymentvarious'; + $classname = 'PaymentVarious'; + $module = 'bank'; // We need $conf->bank->dir_output and not $conf->banque->dir_output + $element = 'payment_various'; + $subelement = 'payment_various'; + $table_element = 'payment_various'; + } elseif ($elementType == 'stocktransfer') { + $classpath = 'product/stock/stocktransfer/class'; + $classfile = 'stocktransfer'; + $classname = 'StockTransfer'; // Not the ucfirst() of the element, so it must be set explicitly + $module = 'stocktransfer'; + $subelement = 'stocktransfer'; + $table_element = 'stocktransfer_stocktransfer'; + } elseif ($elementType == 'job' || $elementType == 'position' || $elementType == 'skill' || $elementType == 'evaluation') { + $classpath = 'hrm/class'; + $classfile = $elementType; + $classname = ucfirst($elementType); + $module = 'hrm'; + $subelement = $elementType; + $table_element = ($elementType == 'position' ? 'hrm_job_user' : 'hrm_'.$elementType); + $subdir = '/'.$elementType; } elseif ($elementType == 'productlot') { $module = 'productbatch'; $classpath = 'product/stock/class'; @@ -15508,6 +15575,7 @@ function getElementProperties($elementType) $classfile = 'conferenceorbooth'; $classname = 'ConferenceOrBooth'; $module = 'eventorganization'; + $subdir = '/conferenceorbooth'; } elseif ($elementType == 'ccountry') { $module = ''; $classpath = 'core/class'; @@ -15597,9 +15665,24 @@ function getElementProperties($elementType) } elseif ($element == 'invoice_supplier' && isModEnabled('fournisseur')) { $dir_output = $conf->fournisseur->facture->dir_output; $dir_temp = $conf->fournisseur->facture->dir_temp; + } elseif ($elementType == 'payment' && isModEnabled('invoice') && isset($conf->compta->payment)) { + // A customer payment is stored into a sub object of $conf, not handled by the generic case. + // Note: we must test $elementType and not $element, because the 'myobject_mysubobject' rule above + // rewrites $element to 'payment' for the element 'payment_salary' too, which is stored elsewhere. + $dir_output = $conf->compta->payment->dir_output; + $dir_temp = $conf->compta->payment->dir_temp; + } elseif ($elementType == 'payment_supplier' && isModEnabled('fournisseur') && isset($conf->fournisseur->payment)) { + $dir_output = $conf->fournisseur->payment->dir_output; + $dir_temp = $conf->fournisseur->payment->dir_temp; + } + // The sub directory must not be appended when the module is disabled, because $dir_output is then empty + // and we would return a path at the root of the file system instead of an empty string. + if (!empty($dir_output)) { + $dir_output .= $subdir; + } + if (!empty($dir_temp)) { + $dir_temp .= $subdir; } - $dir_output .= $subdir; - $dir_temp .= $subdir; $elementProperties = array( 'module' => $module, diff --git a/htdocs/core/lib/security.lib.php b/htdocs/core/lib/security.lib.php index 803beaba888..4de3acb9e5f 100644 --- a/htdocs/core/lib/security.lib.php +++ b/htdocs/core/lib/security.lib.php @@ -227,6 +227,24 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '', $parentfortableentity = 'fk_website@website'; } elseif ($features == 'project') { $features = 'projet'; + } elseif ($features == 'eventorganization' && is_object($object) && $object->element == 'conferenceorbooth') { + // The module of an event organization declares no permission of its own, on purpose, so a check on + // 'eventorganization' is refused to everyone, an administrator included. Check the parent project + // instead, which is what the card of the object does itself. + // The card refuses an external user before that check, and fk_project is nullable, so we must refuse + // both cases here too: with no parent project there is nothing left to check the access on, and + // granting it would be an access with no check at all. + if (!empty($user->socid) || empty($object->fk_project)) { + if ($mode) { + return 0; + } else { + accessforbidden(); + } + } + $features = 'projet'; + $tableandshare = 'projet&project'; + $objectid = (int) $object->fk_project; + $object = $objectid; } elseif ($features == 'product') { $features = 'produit'; } elseif ($features == 'productbatch') { @@ -235,6 +253,10 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '', $feature2 = 'charges'; } elseif ($features == 'workstation') { $feature2 = 'workstation'; + } elseif ($features == 'hrm' && is_object($object) && in_array($object->element, array('job', 'position', 'skill'))) { + $feature2 = 'all'; // These 3 objects have no permission of their own, they share the level "all" + } elseif ($features == 'stocktransfer' && is_object($object) && $object->element == 'stocktransfer') { + $feature2 = 'stocktransfer'; // This module declares no permission at its first level, only this one } elseif ($features == 'fournisseur') { // When vendor invoice and purchase order are into module 'fournisseur' if (is_object($object) && $object->element == 'invoice_supplier') { $feature2 = 'facture'; @@ -783,6 +805,30 @@ function checkUserAccessToObject($user, array $featuresarray, $object = 0, $tabl $sharedelement = (!empty($params[1]) ? $params[1] : $dbtablename); // We change dbtablename, so we set sharedelement too. } + // The default rule reads the columns entity and $dbt_keyfield of the table, but some tables own neither of + // them. The sql was then built on columns that do not exist, so it always failed and the access was refused + // to the users that this rule applies to. + // The rule is selected on the table and not on the element of the object, because $object is an id and not + // an object for most of the callers, the card of an asset and the card of a workstation included. + if (!empty($objectid) && in_array($dbtablename, array('asset', 'paiement', 'paiementfourn', 'workstation_workstation', 'hrm_job', 'hrm_job_user', 'hrm_skill'))) { + // None of these objects is linked to a third party, so an external user can own none of them. The + // default rule refused him through a link that does not exist, we must refuse him explicitly instead, + // otherwise the rules below, which do not look at the third party of the user at all, would grant it. + if (!empty($user->socid)) { + return false; + } + if (in_array($dbtablename, array('hrm_job', 'hrm_job_user', 'hrm_skill'))) { + // These 3 tables have no entity column either, so no rule that reads the table can be run on them. + // The permission is still checked by restrictedArea(), and the $checkhierarchy rule below still runs. + // Note that these 3 objects are therefore not partitioned between entities at all, in the database + // itself: their cards already answer to a user of another entity, and their lists already show the + // records of all of them. This rule does not widen that, it aligns with it. + $nocheck[] = $feature; + } else { + $check[] = $feature; // Test on the entity only, there is no third party to restrict on + } + } + // $objectid was already sanitized at begin of this method (can be an int or a list of int separated by comma). // To avoid an access forbidden with a numeric ref if ($dbt_select != 'rowid' && $dbt_select != 'id') { diff --git a/htdocs/don/card.php b/htdocs/don/card.php index 61c80893a9c..9ad1ed259d8 100644 --- a/htdocs/don/card.php +++ b/htdocs/don/card.php @@ -701,7 +701,7 @@ if (!empty($id) && $action != 'edit') { $hselected = 'card'; $head = donation_prepare_head($object); - print dol_get_fiche_head($head, $hselected, $langs->trans("Donation"), -1, 'donation'); + print dol_get_fiche_head($head, $hselected, $langs->trans("Donation"), -1, 'donation', 0, '', '', 0, '', 1); // Print form confirm print $formconfirm; diff --git a/htdocs/eventorganization/conferenceorbooth_card.php b/htdocs/eventorganization/conferenceorbooth_card.php index a1a6adbe8bf..e80d786139b 100644 --- a/htdocs/eventorganization/conferenceorbooth_card.php +++ b/htdocs/eventorganization/conferenceorbooth_card.php @@ -534,7 +534,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $head = conferenceorboothPrepareHead($object, $withproject); - print dol_get_fiche_head($head, 'card', $langs->trans("ConferenceOrBooth"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("ConferenceOrBooth"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/expedition/card.php b/htdocs/expedition/card.php index 56bbe562258..c4931b3a7b3 100644 --- a/htdocs/expedition/card.php +++ b/htdocs/expedition/card.php @@ -2687,7 +2687,7 @@ if ($action == 'create' && $usercancreate) { $res = $object->fetch_optionals(); $head = shipping_prepare_head($object); - print dol_get_fiche_head($head, 'shipping', $langs->trans("Shipment"), -1, $object->picto); + print dol_get_fiche_head($head, 'shipping', $langs->trans("Shipment"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/expensereport/card.php b/htdocs/expensereport/card.php index 91916cadd9a..1d18ec5d37a 100644 --- a/htdocs/expensereport/card.php +++ b/htdocs/expensereport/card.php @@ -1621,7 +1621,7 @@ if ($action == 'create') { print ''; print ''; - print dol_get_fiche_head($head, 'card', $langs->trans("ExpenseReport"), 0, 'trip', 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("ExpenseReport"), 0, 'trip', 0, '', '', 0, '', 0); // No drag and drop on the edit form, dropping a file reloads the page and discards it if ($object->status == ExpenseReport::STATUS_REFUSED) { print ''; diff --git a/htdocs/fourn/paiement/card.php b/htdocs/fourn/paiement/card.php index e21d842c69f..fb19a1d9d91 100644 --- a/htdocs/fourn/paiement/card.php +++ b/htdocs/fourn/paiement/card.php @@ -174,7 +174,9 @@ $formfile = new FormFile($db); $head = payment_supplier_prepare_head($object); -print dol_get_fiche_head($head, 'payment', $langs->trans('SupplierPayment'), -1, 'payment'); +// The tabs are printed before the result of the fetch is checked below, so we must not offer a drop area +// when the object was not loaded: the upload could only fail. +print dol_get_fiche_head($head, 'payment', $langs->trans('SupplierPayment'), -1, 'payment', 0, '', '', 0, '', ($result > 0 ? 1 : 0)); if ($result > 0) { /* diff --git a/htdocs/holiday/card.php b/htdocs/holiday/card.php index 7bbdeaa4307..d3feb4c19c2 100644 --- a/htdocs/holiday/card.php +++ b/htdocs/holiday/card.php @@ -1334,7 +1334,9 @@ if ((empty($id) && empty($ref)) || $action == 'create' || $action == 'add') { if ($canread) { $head = holiday_prepare_head($object); - if (($action == 'edit' && $object->status == Holiday::STATUS_DRAFT) || ($action == 'editvalidator')) { + $editmode = (($action == 'edit' && $object->status == Holiday::STATUS_DRAFT) || ($action == 'editvalidator')); + + if ($editmode) { if ($action == 'edit' && $object->status == Holiday::STATUS_DRAFT) { $edit = true; } @@ -1345,7 +1347,9 @@ if ((empty($id) && empty($ref)) || $action == 'create' || $action == 'add') { print ''."\n"; } - print dol_get_fiche_head($head, 'card', $langs->trans("CPTitreMenu"), -1, 'holiday'); + // No drop area in edit mode: these tabs are printed inside the edit form, and dropping a file + // reloads the page, which would discard what the user is typing. + print dol_get_fiche_head($head, 'card', $langs->trans("CPTitreMenu"), -1, 'holiday', 0, '', '', 0, '', ($editmode ? 0 : 1)); $linkback = ''.$langs->trans("BackToList").''; diff --git a/htdocs/hrm/evaluation_card.php b/htdocs/hrm/evaluation_card.php index 52c7193c17f..7450064fa74 100644 --- a/htdocs/hrm/evaluation_card.php +++ b/htdocs/hrm/evaluation_card.php @@ -358,7 +358,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $res = $object->fetch_optionals(); $head = evaluationPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Workstation"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("Workstation"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/hrm/job_card.php b/htdocs/hrm/job_card.php index 59fc1aa5c20..785a06744b4 100644 --- a/htdocs/hrm/job_card.php +++ b/htdocs/hrm/job_card.php @@ -314,7 +314,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $head = jobPrepareHead($object); $picto = 'company.png'; - print dol_get_fiche_head($head, 'job_card', $langs->trans("Workstation"), -1, $object->picto); + print dol_get_fiche_head($head, 'job_card', $langs->trans("Workstation"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/hrm/position_card.php b/htdocs/hrm/position_card.php index a171fa9a8a9..1285a99db31 100644 --- a/htdocs/hrm/position_card.php +++ b/htdocs/hrm/position_card.php @@ -271,7 +271,7 @@ function displayPositionCard(&$object) $head = positionCardPrepareHead($object); - print dol_get_fiche_head($head, 'position', $langs->trans("Workstation"), -1, $object->picto); + print dol_get_fiche_head($head, 'position', $langs->trans("Workstation"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/hrm/skill_card.php b/htdocs/hrm/skill_card.php index 106b1e6072f..3b5c6dad78a 100644 --- a/htdocs/hrm/skill_card.php +++ b/htdocs/hrm/skill_card.php @@ -430,7 +430,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $res = $object->fetch_optionals(); $head = skillPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Workstation"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("Workstation"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/intracommreport/card.php b/htdocs/intracommreport/card.php index c6288c30ab1..748325c23a2 100644 --- a/htdocs/intracommreport/card.php +++ b/htdocs/intracommreport/card.php @@ -284,7 +284,7 @@ if (($id || $ref) && $action == 'edit') { if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) { $head = intracommreportPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("IntraCommReport"), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("IntraCommReport"), -1, $object->picto); $formconfirm = ''; diff --git a/htdocs/knowledgemanagement/knowledgerecord_card.php b/htdocs/knowledgemanagement/knowledgerecord_card.php index 6fbb511b928..3f10add8e04 100644 --- a/htdocs/knowledgemanagement/knowledgerecord_card.php +++ b/htdocs/knowledgemanagement/knowledgerecord_card.php @@ -300,7 +300,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $res = $object->fetch_optionals(); $head = knowledgerecordPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("KnowledgeRecord"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("KnowledgeRecord"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/langs/en_US/errors.lang b/htdocs/langs/en_US/errors.lang index b5ac29a3140..412d290fc3a 100644 --- a/htdocs/langs/en_US/errors.lang +++ b/htdocs/langs/en_US/errors.lang @@ -334,6 +334,7 @@ ErrorFieldValue=Value for %s is incorrect ErrorCoherenceMenu=%s is required when %s is 'left' ErrorUploadFileDragDrop=There was an error while the file(s) upload ErrorUploadFileDragDropPermissionDenied=There was an error while the file(s) upload : Permission denied +ErrorOnAtLeastOneFileUpload=There was an error while uploading at least one file ErrorFixThisHere=Fix this here ErrorTheUrlOfYourDolInstanceDoesNotMatchURLIntoOAuthSetup=Error: The URL of you current instance (%s) does not match the URL defined into your OAuth2 login setup (%s). Doing OAuth2 login in such a configuration is not allowed. ErrorMenuExistValue=A Menu already exist with this Title or URL diff --git a/htdocs/loan/card.php b/htdocs/loan/card.php index 70e81f7bb42..6a5c407e574 100644 --- a/htdocs/loan/card.php +++ b/htdocs/loan/card.php @@ -442,7 +442,7 @@ if ($id > 0) { print ''; } - print dol_get_fiche_head($head, 'card', $langs->trans("Loan"), -1, 'money-bill-alt', 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("Loan"), -1, 'money-bill-alt', 0, '', '', 0, '', ($action == 'edit' ? 0 : 1)); // Loan card $linkback = ''.$langs->trans("BackToList").''; diff --git a/htdocs/mrp/mo_card.php b/htdocs/mrp/mo_card.php index 76b012b242e..04791cb8a68 100644 --- a/htdocs/mrp/mo_card.php +++ b/htdocs/mrp/mo_card.php @@ -584,7 +584,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $head = moPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("ManufacturingOrder"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("ManufacturingOrder"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/partnership/partnership_card.php b/htdocs/partnership/partnership_card.php index 9bb895dba7a..926ff24fff4 100644 --- a/htdocs/partnership/partnership_card.php +++ b/htdocs/partnership/partnership_card.php @@ -357,7 +357,7 @@ if (($id || $ref) && $action == 'edit') { if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) { $head = partnershipPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Partnership"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("Partnership"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/product/card.php b/htdocs/product/card.php index 407d70528bc..393c9278d17 100644 --- a/htdocs/product/card.php +++ b/htdocs/product/card.php @@ -2075,7 +2075,7 @@ if (is_object($objcanvas) && $objcanvas->displayCanvasExists($canvasdisplayactio $head = product_prepare_head($object); $titre = $langs->trans("CardProduct".$object->type); $picto = ($object->type == Product::TYPE_SERVICE ? 'service' : 'product'); - print dol_get_fiche_head($head, 'card', $titre, 0, $picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $titre, 0, $picto, 0, '', '', 0, '', 0); // No drag and drop on the edit form, dropping a file reloads the page and discards it // Call Hook tabContentEditProduct $parameters = array(); diff --git a/htdocs/product/stock/productlot_card.php b/htdocs/product/stock/productlot_card.php index 29b3238dadc..14426d4fe91 100644 --- a/htdocs/product/stock/productlot_card.php +++ b/htdocs/product/stock/productlot_card.php @@ -344,7 +344,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $res = $object->fetch_optionals(); $head = productlot_prepare_head($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Batch"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("Batch"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/product/stock/stocktransfer/stocktransfer_card.php b/htdocs/product/stock/stocktransfer/stocktransfer_card.php index 744d2a75bc1..a68cdf027ff 100644 --- a/htdocs/product/stock/stocktransfer/stocktransfer_card.php +++ b/htdocs/product/stock/stocktransfer/stocktransfer_card.php @@ -548,7 +548,7 @@ if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'crea $head = stocktransferPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("StockTransfer"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("StockTransfer"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/projet/tasks/task.php b/htdocs/projet/tasks/task.php index 3be6271041c..2bc963842bf 100644 --- a/htdocs/projet/tasks/task.php +++ b/htdocs/projet/tasks/task.php @@ -598,7 +598,7 @@ if ($id > 0 || !empty($ref)) { $param = ($withproject ? '&withproject=1' : ''); $linkback = $withproject ? ''.$langs->trans("BackToList").'' : ''; - print dol_get_fiche_head($head, 'task_task', $langs->trans("Task"), -1, 'projecttask', 0, '', 'reposition'); + print dol_get_fiche_head($head, 'task_task', $langs->trans("Task"), -1, 'projecttask', 0, '', 'reposition', 0, '', 1); if ($action == 'clone') { $formquestion = array( diff --git a/htdocs/reception/card.php b/htdocs/reception/card.php index a989e9ec06c..c3b4ccc3c24 100644 --- a/htdocs/reception/card.php +++ b/htdocs/reception/card.php @@ -2011,7 +2011,7 @@ if ($action == 'create' && $permissiontoadd) { $res = $object->fetch_optionals(); $head = reception_prepare_head($object); - print dol_get_fiche_head($head, 'reception', $langs->trans("Reception"), -1, 'dollyrevert'); + print dol_get_fiche_head($head, 'reception', $langs->trans("Reception"), -1, 'dollyrevert', 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/htdocs/resource/card.php b/htdocs/resource/card.php index 949ecc18a24..e202b36a827 100644 --- a/htdocs/resource/card.php +++ b/htdocs/resource/card.php @@ -250,7 +250,7 @@ if ($action == 'create' || $object->fetch($id, $ref) > 0) { print dol_get_fiche_head(); } else { $head = resource_prepare_head($object); - print dol_get_fiche_head($head, 'resource', $title, -1, 'resource'); + print dol_get_fiche_head($head, 'resource', $title, -1, 'resource', 0, '', '', 0, '', ($action == 'edit' ? 0 : 1)); } if ($action == 'create' || $action == 'edit') { diff --git a/htdocs/salaries/card.php b/htdocs/salaries/card.php index 6f09059997b..0a3a8443e5e 100644 --- a/htdocs/salaries/card.php +++ b/htdocs/salaries/card.php @@ -896,7 +896,7 @@ if ($id > 0) { print $formconfirm; - print dol_get_fiche_head($head, 'card', $langs->trans("SalaryPayment"), -1, 'salary', 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans("SalaryPayment"), -1, 'salary', 0, '', '', 0, '', ($action == 'edit' ? 0 : 1)); $linkback = ''.$langs->trans("BackToList").''; diff --git a/htdocs/webhook/triggerhistory_card.php b/htdocs/webhook/triggerhistory_card.php index 4cc30361323..74de6d17ebd 100644 --- a/htdocs/webhook/triggerhistory_card.php +++ b/htdocs/webhook/triggerhistory_card.php @@ -263,7 +263,7 @@ if (($id || $ref) && $action == 'edit') { if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) { $head = triggerhistoryPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans(""), -1, $object->picto, 0, '', '', 0, '', 1); + print dol_get_fiche_head($head, 'card', $langs->trans(""), -1, $object->picto); $formconfirm = ''; diff --git a/htdocs/workstation/workstation_card.php b/htdocs/workstation/workstation_card.php index f57a7dafe1d..c4e31df147e 100644 --- a/htdocs/workstation/workstation_card.php +++ b/htdocs/workstation/workstation_card.php @@ -312,7 +312,7 @@ if (($id || $ref) && $action == 'edit') { if ($object->id > 0 && (empty($action) || ($action != 'edit' && $action != 'create'))) { $head = workstationPrepareHead($object); - print dol_get_fiche_head($head, 'card', $langs->trans("Workstation"), -1, $object->picto); + print dol_get_fiche_head($head, 'card', $langs->trans("Workstation"), -1, $object->picto, 0, '', '', 0, '', 1); $formconfirm = ''; diff --git a/test/phpunit/FileUploadTest.php b/test/phpunit/FileUploadTest.php new file mode 100644 index 00000000000..c52f4116f60 --- /dev/null +++ b/test/phpunit/FileUploadTest.php @@ -0,0 +1,516 @@ +. + * or see https://www.gnu.org/ + */ + +/** + * \file test/phpunit/FileUploadTest.php + * \ingroup test + * \brief PHPUnit test of the class FileUpload, used by the drag and drop of a file on a card. + * \remarks To run this script as CLI: phpunit FileUploadTest.php < /dev/null + * The redirection of stdin is only needed on an interactive terminal, because the tested + * code reads php://input when the file is not a real http upload. + */ + +global $conf,$user,$langs,$db; +require_once dirname(__FILE__).'/../../htdocs/master.inc.php'; +require_once dirname(__FILE__).'/../../htdocs/core/class/fileupload.class.php'; +require_once dirname(__FILE__).'/../../htdocs/core/lib/files.lib.php'; +require_once dirname(__FILE__).'/../../htdocs/product/class/product.class.php'; +require_once dirname(__FILE__).'/../../htdocs/societe/class/societe.class.php'; +require_once dirname(__FILE__).'/../../htdocs/contact/class/contact.class.php'; +require_once dirname(__FILE__).'/../../htdocs/projet/class/project.class.php'; +require_once dirname(__FILE__).'/../../htdocs/projet/class/task.class.php'; +require_once dirname(__FILE__).'/CommonClassTest.class.php'; + +if (!defined('NOREQUIREUSER')) { + define('NOREQUIREUSER', '1'); +} +if (!defined('NOREQUIREDB')) { + define('NOREQUIREDB', '1'); +} +if (!defined('NOREQUIRESOC')) { + define('NOREQUIRESOC', '1'); +} +if (!defined('NOREQUIRETRAN')) { + define('NOREQUIRETRAN', '1'); +} +if (!defined('NOCSRFCHECK')) { + define('NOCSRFCHECK', '1'); +} +if (!defined('NOTOKENRENEWAL')) { + define('NOTOKENRENEWAL', '1'); +} +if (!defined('NOREQUIREMENU')) { + define('NOREQUIREMENU', '1'); +} +if (!defined('NOREQUIREHTML')) { + define('NOREQUIREHTML', '1'); +} +if (!defined('NOREQUIREAJAX')) { + define('NOREQUIREAJAX', '1'); +} +if (!defined("NOLOGIN")) { + define("NOLOGIN", '1'); +} + +/** + * Class for PHPUnit tests of FileUpload + * + * @backupGlobals disabled + * @backupStaticAttributes enabled + * @remarks backupGlobals must be disabled to have db,conf,user and lang not erased. + */ +class FileUploadTest extends CommonClassTest +{ + /** + * Directories created by the tests, removed at the end + * @var string[] + */ + protected static $dirstoclean = array(); + + /** + * Temporary files created by the tests, removed at the end + * @var string[] + */ + protected static $filestoclean = array(); + + /** + * Objects of the fixture, removed by the rollback of the transaction + * @var array + */ + protected static $objects = array(); + + + /** + * setUpBeforeClass + * + * @return void + */ + public static function setUpBeforeClass(): void + { + global $conf, $db, $user; + + parent::setUpBeforeClass(); + + $conf->global->MAIN_DISABLE_SUGGEST_REF_AS_PREFIX = 0; + + // A thirdparty + $thirdparty = new Societe($db); + $thirdparty->initAsSpecimen(); + $thirdparty->name = 'Test FileUpload'; + $thirdparty->country_id = 1; + $id = $thirdparty->create($user); + if ($id <= 0) { + die("Failed to create the thirdparty: ".$thirdparty->errorsToString()."\n"); + } + $thirdparty->fetch($id); + self::$objects['societe'] = $thirdparty; + + // A contact of this thirdparty + $contact = new Contact($db); + $contact->lastname = 'Doe'; + $contact->firstname = 'John'; + $contact->socid = $thirdparty->id; + $contact->country_id = 1; + $id = $contact->create($user); + if ($id <= 0) { + die("Failed to create the contact: ".$contact->errorsToString()."\n"); + } + $contact->fetch($id); + self::$objects['contact'] = $contact; + + // A product + $product = new Product($db); + $product->ref = 'PRODUCT-FILEUPLOAD-TEST'; + $product->label = 'Test FileUpload'; + $product->type = Product::TYPE_PRODUCT; + $id = $product->create($user); + if ($id <= 0) { + die("Failed to create the product: ".$product->errorsToString()."\n"); + } + $product->fetch($id); + self::$objects['product'] = $product; + + // A project whose ref holds the chars a path must not keep, and a task of this project + $project = new Project($db); + $project->ref = 'PJ/UP:é 2026'; + $project->title = 'Test FileUpload'; + $project->socid = $thirdparty->id; + $id = $project->create($user); + if ($id <= 0) { + die("Failed to create the project: ".$project->errorsToString()."\n"); + } + $project->fetch($id); + self::$objects['project'] = $project; + + $task = new Task($db); + $task->ref = 'TASK-FILEUPLOAD-TEST'; + $task->label = 'Test FileUpload'; + $task->fk_project = $project->id; + $id = $task->create($user); + if ($id <= 0) { + die("Failed to create the task: ".$task->errorsToString()."\n"); + } + $task->fetch($id); + self::$objects['project_task'] = $task; + } + + /** + * tearDownAfterClass + * + * @return void + */ + public static function tearDownAfterClass(): void + { + foreach (self::$dirstoclean as $dir) { + if (is_dir($dir)) { + dol_delete_dir_recursive($dir); + } + } + foreach (self::$filestoclean as $file) { + @unlink($file); + } + + // The objects created by setUpBeforeClass() are removed by the rollback of the parent, which + // closes the transaction opened by its own setUpBeforeClass(). + parent::tearDownAfterClass(); + } + + /** + * Build a FileUpload for an object and return its upload directory. + * + * @param string $element Element code + * @param int $id Id of the object + * @return string Upload directory + */ + protected function uploadDirOf($element, $id) + { + $upload = new FileUpload(null, $id, $element); + $this->assertIsArray($upload->options); + $this->assertArrayHasKey('upload_dir', $upload->options); + return $upload->options['upload_dir']; + } + + + // + // Resolution of the directory where the file is stored + // + + /** + * The directory of a product must be the one read by product/document.php, otherwise the file is + * stored but never shown into the "Attached files" tab. + * + * @return void + */ + public function testUploadDirOfAProduct() + { + global $conf; + + $object = self::$objects['product']; + + // The formula of product/document.php, written here on purpose so the test does not use the + // code it checks to build its expectation. + $expected = $conf->product->multidir_output[$object->entity].'/'.dol_sanitizeFileName($object->ref).'/'; + + $this->assertSame($expected, $this->uploadDirOf('product', $object->id)); + } + + /** + * A thirdparty stores its documents into a directory named after its id, not its ref. + * + * @return void + */ + public function testUploadDirOfAThirdpartyUsesTheId() + { + global $conf; + + $object = self::$objects['societe']; + + // The formula of societe/document.php + $expected = $conf->societe->multidir_output[$object->entity].'/'.$object->id.'/'; + + $this->assertSame($expected, $this->uploadDirOf('societe', $object->id)); + $this->assertStringNotContainsString(dol_sanitizeFileName($object->name), $this->uploadDirOf('societe', $object->id), 'The name of a thirdparty must never appear into its directory'); + } + + /** + * A contact is stored into a sub directory of the thirdparty module. getMultidirOutput() does not know + * this element and answers its sentinel, so this asserts the fallback on getElementProperties(). + * + * @return void + */ + public function testUploadDirOfAContactFallsBackOnGetElementProperties() + { + global $conf; + + $object = self::$objects['contact']; + + $this->assertStringStartsWith('error-diroutput-not-defined-for-this-object=', (string) getMultidirOutput($object, 'contact'), 'The fixture requires getMultidirOutput to fail on a contact'); + + // The formula of contact/document.php + $expected = $conf->societe->multidir_output[$object->entity].'/contact/'.dol_sanitizeFileName($object->ref).'/'; + + $this->assertSame($expected, $this->uploadDirOf('contact', $object->id)); + } + + /** + * A task is stored into a sub directory named after the ref of its project. That ref is a user input, + * so it must be sanitized the same way projet/tasks/document.php does it. + * + * @return void + */ + public function testUploadDirOfATaskSanitizesTheProjectRef() + { + global $conf; + + $task = self::$objects['project_task']; + $project = self::$objects['project']; + + $this->assertStringContainsString('/', $project->ref, 'The fixture requires a project ref holding a slash'); + + // The formula of projet/tasks/document.php + $expected = $conf->project->multidir_output[$project->entity].'/'.dol_sanitizeFileName($project->ref).'/'.dol_sanitizeFileName($task->ref).'/'; + + $dir = $this->uploadDirOf('project_task', $task->id); + + $this->assertSame($expected, $dir); + $this->assertStringNotContainsString('PJ/UP', $dir, 'The slash of the project ref must not create a sub directory'); + $this->assertStringNotContainsString(':', $dir, 'A colon of the project ref must not be kept'); + } + + /** + * Whatever the element, the directory is absolute, inside DOL_DATA_ROOT, and never holds the sentinel + * string returned by getMultidirOutput() when it fails. + * + * @return void + */ + public function testUploadDirIsNeverTheSentinelOfGetMultidirOutput() + { + foreach (array('product', 'societe', 'contact', 'project_task') as $element) { + $dir = $this->uploadDirOf($element, self::$objects[$element]->id); + + $this->assertStringNotContainsString('error-diroutput-not-defined-for-this-object', $dir, 'The sentinel leaked into the directory of the element '.$element); + $this->assertStringStartsWith(DOL_DATA_ROOT, $dir, 'The directory of the element '.$element.' must be inside DOL_DATA_ROOT'); + $this->assertStringNotContainsString('//', $dir, 'The directory of the element '.$element.' must not hold a double slash'); + $this->assertStringEndsWith('/', $dir, 'The directory must end with a slash, the file name is concatenated to it'); + } + } + + /** + * The file name is prefixed by the ref of the object, and that ref is sanitized because it is a user + * input that may hold a slash. + * + * @return void + */ + public function testSavingDocMaskIsSanitized() + { + $project = self::$objects['project']; + + $upload = new FileUpload(null, $project->id, 'project'); + + $this->assertSame(dol_sanitizeFileName($project->ref).'-__file__', $upload->options['saving_doc_mask']); + $this->assertStringNotContainsString('/', $upload->options['saving_doc_mask'], 'A file name mask must never hold a slash'); + } + + + // + // Refusals + // + + /** + * An object that does not exist must be refused, otherwise the file is stored at the root of the + * directory of the module, out of any object and out of any permission check. + * + * @return void + */ + public function testConstructRefusesAnObjectThatDoesNotExist() + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('objectnotfound'); + + new FileUpload(null, 99999999, 'product'); + } + + /** + * An id of 0 must be refused too, it would answer the directory of every unsaved object. + * + * @return void + */ + public function testConstructRefusesAnEmptyId() + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('objectnotfound'); + + new FileUpload(null, 0, 'product'); + } + + /** + * An element with no directory at all must be refused instead of writing anywhere. + * + * @return void + */ + public function testConstructRefusesAnUnknownElement() + { + $this->expectException(Exception::class); + + new FileUpload(null, 1, 'anelementthatdoesnotexist'); + } + + + // + // Deduplication of the name of the uploaded file + // + + + /** + * Build a FileUpload for an object of the fixture and empty its upload directory, so the tests below + * start from a known set of already used names. + * + * @param string $element Element code of the fixture + * @return FileUpload Instance ready to use + */ + protected function prepareUploadDir($element) + { + $object = self::$objects[$element]; + $upload = new FileUpload(null, $object->id, $element); + $dir = $upload->options['upload_dir']; + + if (is_dir($dir)) { + dol_delete_dir_recursive($dir); + } + dol_mkdir($dir); + self::$dirstoclean[$dir] = $dir; + + return $upload; + } + + /** + * Create the temporary file playing the role of the file uploaded by the browser. + * + * @return string Path of the file + */ + protected function makeTmpFile() + { + $tmpfile = DOL_DATA_ROOT.'/admin/temp/fileuploadtest-'.getmypid().'.txt'; + dol_mkdir(dirname($tmpfile)); + file_put_contents($tmpfile, 'content'); + self::$filestoclean[$tmpfile] = $tmpfile; + + return $tmpfile; + } + + /** + * Call the protected handleFileUpload() of FileUpload. + * + * @param FileUpload $upload Instance + * @param string $tmpfile Path of the file to upload + * @param string $name Name sent by the browser + * @return stdClass The file object answered by handleFileUpload() + */ + protected function callHandleFileUpload($upload, $tmpfile, $name) + { + $method = new ReflectionMethod('FileUpload', 'handleFileUpload'); + $method->setAccessible(true); + + // validate() reads CONTENT_LENGTH when the file is not a real http upload + $_SERVER['CONTENT_LENGTH'] = 10; + + return $method->invoke($upload, $tmpfile, $name, 0, 'text/plain', 0, 0); + } + + /** + * A name that is free must be kept as it is, only prefixed by the ref of the object. + * + * @return void + */ + public function testUploadOfAFreeNameIsKept() + { + $upload = $this->prepareUploadDir('product'); + $prefix = dol_sanitizeFileName(self::$objects['product']->ref).'-'; + + $file = $this->callHandleFileUpload($upload, $this->makeTmpFile(), 'afreename.txt'); + + $this->assertSame($prefix.'afreename.txt', $file->name, 'A free name must be kept, only the prefix of the ref is added'); + } + + /** + * Uploading twice the same file must not overwrite the first one: dol_move_uploaded_file() is called + * with $allowoverwrite = 1, so the name must be made unique before. + * trimFileName() already does that check, but on the name before the prefix of the ref is added, so + * it compares a name that is not the one stored. + * + * @return void + */ + public function testUploadOfAnAlreadyExistingNameIsRenamed() + { + $upload = $this->prepareUploadDir('product'); + $dir = $upload->options['upload_dir']; + $prefix = dol_sanitizeFileName(self::$objects['product']->ref).'-'; + + // The name is already used, with the prefix that trimFileName() does not know about + file_put_contents($dir.$prefix.'mydoc.txt', 'first'); + + $file = $this->callHandleFileUpload($upload, $this->makeTmpFile(), 'mydoc.txt'); + + $this->assertSame($prefix.'mydoc (1).txt', $file->name, 'The name must be made unique, the first file must not be overwritten'); + $this->assertSame('first', file_get_contents($dir.$prefix.'mydoc.txt'), 'The first file must be untouched'); + $this->assertFileExists($dir.$file->name, 'The new file must be stored under its new name'); + } + + /** + * The renaming must be repeated as long as the name is used, so a third upload gets a third name. + * + * @return void + */ + public function testUploadOfAnAlreadyExistingNameIsRenamedAgain() + { + $upload = $this->prepareUploadDir('product'); + $dir = $upload->options['upload_dir']; + $prefix = dol_sanitizeFileName(self::$objects['product']->ref).'-'; + + file_put_contents($dir.$prefix.'mydoc.txt', 'first'); + file_put_contents($dir.$prefix.'mydoc (1).txt', 'second'); + + $file = $this->callHandleFileUpload($upload, $this->makeTmpFile(), 'mydoc.txt'); + + $this->assertSame($prefix.'mydoc (2).txt', $file->name); + $this->assertSame('first', file_get_contents($dir.$prefix.'mydoc.txt')); + $this->assertSame('second', file_get_contents($dir.$prefix.'mydoc (1).txt')); + } + + /** + * A file whose name is executable is stored by dol_move_uploaded_file() with a '.noexe' suffix added. + * The check on an already used name must look for that suffixed name too, otherwise such a file is + * silently overwritten at each upload. + * + * @return void + */ + public function testUploadOfAnAlreadyExistingNoexeNameIsRenamed() + { + $upload = $this->prepareUploadDir('product'); + $dir = $upload->options['upload_dir']; + $prefix = dol_sanitizeFileName(self::$objects['product']->ref).'-'; + + // A previous upload of the same file was renamed with the .noexe suffix + file_put_contents($dir.$prefix.'myscript.php.noexe', 'first'); + + $file = $this->callHandleFileUpload($upload, $this->makeTmpFile(), 'myscript.php'); + + $this->assertStringStartsWith($prefix.'myscript (1).php', $file->name, 'A name already used with the .noexe suffix must be made unique too'); + $this->assertSame('first', file_get_contents($dir.$prefix.'myscript.php.noexe'), 'The first file must be untouched'); + } +} diff --git a/test/phpunit/FunctionsLibDragDropTest.php b/test/phpunit/FunctionsLibDragDropTest.php new file mode 100644 index 00000000000..33dae26d9f2 --- /dev/null +++ b/test/phpunit/FunctionsLibDragDropTest.php @@ -0,0 +1,482 @@ +. + * or see https://www.gnu.org/ + */ + +/** + * \file test/phpunit/FunctionsLibDragDropTest.php + * \ingroup test + * \brief PHPUnit test of the directory resolution used by the drag and drop of a file on a card. + * \remarks To run this script as CLI: phpunit FunctionsLibDragDropTest.php + */ + +global $conf,$user,$langs,$db; +require_once dirname(__FILE__).'/../../htdocs/master.inc.php'; +require_once dirname(__FILE__).'/../../htdocs/product/class/product.class.php'; +require_once dirname(__FILE__).'/../../htdocs/societe/class/societe.class.php'; +require_once dirname(__FILE__).'/../../htdocs/contact/class/contact.class.php'; +require_once dirname(__FILE__).'/CommonClassTest.class.php'; + +if (!defined('NOREQUIREUSER')) { + define('NOREQUIREUSER', '1'); +} +if (!defined('NOREQUIREDB')) { + define('NOREQUIREDB', '1'); +} +if (!defined('NOREQUIRESOC')) { + define('NOREQUIRESOC', '1'); +} +if (!defined('NOREQUIRETRAN')) { + define('NOREQUIRETRAN', '1'); +} +if (!defined('NOCSRFCHECK')) { + define('NOCSRFCHECK', '1'); +} +if (!defined('NOTOKENRENEWAL')) { + define('NOTOKENRENEWAL', '1'); +} +if (!defined('NOREQUIREMENU')) { + define('NOREQUIREMENU', '1'); +} +if (!defined('NOREQUIREHTML')) { + define('NOREQUIREHTML', '1'); +} +if (!defined('NOREQUIREAJAX')) { + define('NOREQUIREAJAX', '1'); +} +if (!defined("NOLOGIN")) { + define("NOLOGIN", '1'); +} + +/** + * Class for PHPUnit tests of the directory resolution used by the drag and drop of a file on a card + * + * @backupGlobals disabled + * @backupStaticAttributes enabled + * @remarks backupGlobals must be disabled to have db,conf,user and lang not erased. + */ +class FunctionsLibDragDropTest extends CommonClassTest +{ + /** + * The string returned by getMultidirOutput() when it does not know the module of the object. + * It is a relative path, so writing into it creates files under the web root. + */ + const SENTINEL = 'error-diroutput-not-defined-for-this-object='; + + /** + * The elements equipped with the drag and drop of a file on their card by this work. + * Their 'dir_output' must always be usable to forge an absolute path. + * + * @return array> + */ + public static function providerEquippedElements() + { + $elements = array( + 'contact', 'product', 'societe', 'action', 'expedition', 'reception', 'don', 'expensereport', + 'holiday', 'mo', 'partnership', 'stocktransfer', 'productlot', 'resource', 'workstation', + 'job', 'position', 'skill', 'evaluation', 'knowledgerecord', 'conferenceorbooth', 'asset', + 'payment', 'payment_supplier', 'payment_various', 'salary', 'chargesociales', 'project_task', + ); + $out = array(); + foreach ($elements as $element) { + $out[$element] = array($element); + } + return $out; + } + + + // + // getMultidirOutput() + // + + /** + * getMultidirOutput() answers a sentinel string, not an empty string, when it does not know the module. + * Every caller must reject it, so the contract is asserted here once. + * + * @return void + */ + public function testGetMultidirOutputReturnsARelativeSentinelOnFailure() + { + $object = new stdClass(); + $object->element = 'anelementthatdoesnotexist'; + $object->id = 1; + $object->entity = 1; + + $dir = getMultidirOutput($object, 'anelementthatdoesnotexist'); + + $this->assertSame(self::SENTINEL.'anelementthatdoesnotexist', $dir, 'The failure of getMultidirOutput must be reported by the sentinel string'); + $this->assertStringStartsNotWith('/', $dir, 'The sentinel is a relative path, so it must never be used to forge a path'); + + // The 'temp' mode has its own sentinel + $this->assertSame('error-dirtemp-not-defined-for-this-object=anelementthatdoesnotexist', getMultidirTemp($object, 'anelementthatdoesnotexist')); + + // And a bad mode has a third one + $this->assertSame('error-bad-value-for-mode', getMultidirOutput($object, 'anelementthatdoesnotexist', 0, 'notamode')); + } + + /** + * The sentinel is the answer for a majority of the elements equipped with the drag and drop, so any + * caller that forges a path from getMultidirOutput() alone is broken. This test documents the list. + * + * @return void + */ + public function testGetMultidirOutputIsUnableToResolveMostEquippedElements() + { + $nbresolved = 0; + $nbsentinel = 0; + foreach (self::providerEquippedElements() as $row) { + $element = $row[0]; + if ($element == 'project_task') { + continue; // Needs a real object, it calls fetchProject() + } + $object = new stdClass(); + $object->element = $element; + $object->id = 1; + $object->entity = 1; + $dir = getMultidirOutput($object, $element); + if (strpos((string) $dir, self::SENTINEL) === 0) { + $nbsentinel++; + } else { + $nbresolved++; + $this->assertStringStartsWith(DOL_DATA_ROOT, (string) $dir, 'A resolved directory must be inside DOL_DATA_ROOT for the element '.$element); + } + } + $this->assertGreaterThan(0, $nbsentinel, 'getMultidirOutput is expected to fail on some equipped elements, the fallback on getElementProperties is required'); + $this->assertGreaterThan(0, $nbresolved, 'getMultidirOutput is expected to resolve some equipped elements'); + } + + /** + * A module with a directory for the current entity only must not answer an undefined index (so a path + * relative to the web root) when the object belongs to another entity. + * + * @return void + */ + public function testGetMultidirOutputFallsBackOnTheCurrentEntity() + { + global $conf; + + $object = new Product($conf->db); + $object->id = 1; + $object->ref = 'AREF'; + $object->entity = 99; // No directory is declared for this entity + + $this->assertArrayNotHasKey(99, $conf->product->multidir_output, 'The fixture requires no directory declared for the entity 99'); + + $dir = getMultidirOutput($object, 'product'); + + $this->assertSame($conf->product->multidir_output[$conf->entity], $dir, 'The directory of the current entity must be used as a fallback'); + $this->assertStringStartsWith(DOL_DATA_ROOT, $dir, 'The fallback must not answer a path relative to the web root'); + + // Same fallback for the temporary directory + $dirtemp = getMultidirTemp($object, 'product'); + $this->assertStringStartsWith(DOL_DATA_ROOT, $dirtemp, 'The temporary directory must not be relative either'); + } + + /** + * The subdirectory of a partnership and of a stock transfer must be appended, because it is the one + * read by their "Attached files" tab (see partnership_document.php and stocktransfer_document.php). + * + * @return void + */ + public function testGetMultidirOutputAppendsTheSubDirectoryOfTheTab() + { + global $conf; + + $expected = array( + 'partnership' => '/partnership/partnership', + 'stocktransfer' => '/stocktransfer/stocktransfer', + 'knowledgerecord' => '/knowledgemanagement/knowledgerecord', + 'expedition' => '/expedition/sending', + ); + $asserted = 0; + foreach ($expected as $element => $suffix) { + if (!isset($conf->{explode('/', ltrim($suffix, '/'))[0]})) { + continue; + } + $object = new stdClass(); + $object->element = $element; + $object->id = 1; + $object->entity = $conf->entity; + $dir = getMultidirOutput($object, $element); + if (strpos((string) $dir, self::SENTINEL) === 0) { + continue; // Module not enabled on this instance + } + $this->assertSame(DOL_DATA_ROOT.$suffix, $dir, 'Wrong directory for the element '.$element); + $asserted++; + } + + if (!$asserted) { + // None of the modules of the elements above is enabled, so the test asserted nothing. Say it + // instead of reporting a green test that checked nothing. + $this->markTestSkipped('None of the modules partnership, stocktransfer, knowledgemanagement and expedition is enabled'); + } + } + + + // + // get_exdir() + // + + /** + * get_exdir() is the reference implementation used both by the "Attached files" tabs and by FileUpload + * to forge the directory of an object. Its fallbacks must be asserted, they are load bearing. + * + * @return void + */ + public function testGetExdirFallbacks() + { + global $db; + + $object = new Product($db); + $object->id = 42; + $object->ref = 'MYREF'; + + // Nominal case: the ref is used + $this->assertSame('MYREF', get_exdir(0, 0, 0, 1, $object, 'product')); + + // The trailing slash is added when $withoutslash is 0. FileUpload appends its own '/', so it must + // call get_exdir() with $withoutslash = 1 to avoid a double slash into the path. + $this->assertSame('MYREF/', get_exdir(0, 0, 0, 0, $object, 'product')); + + // The id is used as a fallback when the ref is empty (a draft object with no numbering yet) + $object->ref = ''; + $this->assertSame('42', get_exdir(0, 0, 0, 1, $object, 'product'), 'The id must be used when the ref is empty'); + $object->ref = null; + $this->assertSame('42', get_exdir(0, 0, 0, 1, $object, 'product'), 'The id must be used when the ref is null'); + $object->ref = '0'; + $this->assertSame('42', get_exdir(0, 0, 0, 1, $object, 'product'), 'A ref "0" is empty for php, the id is used'); + + // An object with neither a ref nor an id gives the directory '0', because the id is cast to an int + // and then to a string. This is a shared directory for every unsaved object, so a caller must never + // forge a path from an object it did not load: FileUpload throws 'objectnotfound' before this point. + $empty = new Product($db); + $empty->id = 0; + $empty->ref = ''; + $this->assertSame('0', get_exdir(0, 0, 0, 1, $empty, 'product'), 'An object with no id and no ref falls back on the directory "0"'); + + // The ref is a user input, it must be sanitized: no directory traversal, no separator + $object->ref = '../../etc'; + $this->assertStringNotContainsString('..', get_exdir(0, 0, 0, 1, $object, 'product'), 'A ref must never allow a directory traversal'); + $object->ref = 'A/B'; + $this->assertStringNotContainsString('/', get_exdir(0, 0, 0, 1, $object, 'product'), 'A ref must never introduce a sub directory'); + + // The modulepart is read from the object when it is not given + $object->ref = 'MYREF'; + $this->assertSame('MYREF', get_exdir(0, 0, 0, 1, $object), 'The modulepart must be deduced from the object'); + } + + /** + * A thirdparty stores its documents into a directory named after its id, because its ref is a company + * name and two thirdparties may share the same name. + * + * @return void + */ + public function testGetExdirUsesTheIdForAThirdparty() + { + global $db; + + $thirdparty = new Societe($db); + $thirdparty->id = 7; + $thirdparty->ref = 'My company'; + + $this->assertSame('7', get_exdir(0, 0, 0, 1, $thirdparty, 'societe'), 'The id must be used for a thirdparty'); + $this->assertSame('7', get_exdir(0, 0, 0, 1, $thirdparty, 'thirdparty'), 'The id must be used for a thirdparty'); + + // The rule is on the class, not only on the modulepart: a contact of the module 'societe' keeps its ref + $contact = new Contact($db); + $contact->id = 8; + $contact->ref = 'DOE'; + $this->assertSame('DOE', get_exdir(0, 0, 0, 1, $contact, 'contact'), 'A contact is not a thirdparty, its ref is used'); + } + + /** + * A module storing its documents on 2 levels answers the level directories only, so the caller must + * append the directory of the object itself. + * + * @return void + */ + public function testGetExdirForATwoLevelsModule() + { + global $db; + + $object = new Product($db); // Any object, only the id and the modulepart matter here + $object->id = 42; + $object->ref = 'MYREF'; + + $this->assertSame('2/4', get_exdir(0, 0, 0, 1, $object, 'invoice_supplier'), 'Two levels of directories are expected'); + $this->assertSame('2/4', get_exdir(0, 0, 0, 1, $object, 'supplier_invoice'), 'The two aliases must answer the same directory'); + $this->assertSame('2/4/', get_exdir(0, 0, 0, 0, $object, 'invoice_supplier')); + + // The levels are built from the id, not from the ref + $object->id = 1234; + $this->assertSame('4/3', get_exdir(0, 0, 0, 1, $object, 'invoice_supplier')); + } + + + // + // getElementProperties() + // + + /** + * The elements added or fixed by this work must answer the class that is really able to load them. + * + * @return void + */ + public function testGetElementPropertiesOfTheNewElements() + { + $expected = array( + // element => array(module, classname, classpath, classfile, table_element) + 'payment' => array('facture', 'Paiement', 'compta/paiement/class', 'paiement', 'paiement'), + 'payment_supplier' => array('fournisseur', 'PaiementFourn', 'fourn/class', 'paiementfourn', 'paiementfourn'), + 'payment_various' => array('bank', 'PaymentVarious', 'compta/bank/class', 'paymentvarious', 'payment_various'), + 'stocktransfer' => array('stocktransfer', 'StockTransfer', 'product/stock/stocktransfer/class', 'stocktransfer', 'stocktransfer_stocktransfer'), + 'job' => array('hrm', 'Job', 'hrm/class', 'job', 'hrm_job'), + 'position' => array('hrm', 'Position', 'hrm/class', 'position', 'hrm_job_user'), + 'skill' => array('hrm', 'Skill', 'hrm/class', 'skill', 'hrm_skill'), + 'evaluation' => array('hrm', 'Evaluation', 'hrm/class', 'evaluation', 'hrm_evaluation'), + ); + + foreach ($expected as $element => $values) { + $prop = getElementProperties($element); + + $this->assertSame($element, $prop['element'], 'The element '.$element.' must not be rewritten by the myobject_mysubobject rule'); + $this->assertSame($values[0], $prop['module'], 'Wrong module for the element '.$element); + $this->assertSame($values[1], $prop['classname'], 'Wrong classname for the element '.$element); + $this->assertSame($values[2], $prop['classpath'], 'Wrong classpath for the element '.$element); + $this->assertSame($values[3], $prop['classfile'], 'Wrong classfile for the element '.$element); + $this->assertSame($values[4], $prop['table_element'], 'Wrong table for the element '.$element); + + // The class must really exist, otherwise fetchObjectByElement() ends on a fatal error + $file = DOL_DOCUMENT_ROOT.'/'.$prop['classpath'].'/'.$prop['classfile'].'.class.php'; + $this->assertFileExists($file, 'The class file of the element '.$element.' does not exist'); + require_once $file; + $this->assertTrue(class_exists($prop['classname']), 'The class '.$prop['classname'].' of the element '.$element.' does not exist'); + + // And the table must exist too, restrictedArea() builds its sql on it. + // The table of an optional module is only created when the module is enabled, so the check is + // skipped otherwise: the mapping asserted above does not depend on the module being enabled. + if (isModEnabled($values[0])) { + $this->assertGreaterThan(0, $this->countTable($prop['table_element']), 'The table '.$prop['table_element'].' of the element '.$element.' is not readable'); + } + } + } + + /** + * Count the rows of a table, only to assert the table exists. + * + * @param string $table Table name without the prefix + * @return int -1 if the table does not exist, 1 otherwise + */ + protected function countTable($table) + { + global $db; + + $sql = "SELECT COUNT(*) as nb FROM ".$db->prefix().$db->escape($table); + $resql = $db->query($sql); + if (!$resql) { + return -1; + } + $db->free($resql); + return 1; + } + + /** + * The 'dir_output' of an element must be the directory read by its "Attached files" tab, otherwise a + * file uploaded by drag and drop is stored but never shown to the user. + * + * @return void + */ + public function testGetElementPropertiesDirOutputSubDirectory() + { + global $conf; + + $expected = array( + 'contact' => array('societe', '/societe/contact', '/societe/temp/contact'), + 'job' => array('hrm', '/hrm/job', '/hrm/temp/job'), + 'position' => array('hrm', '/hrm/position', '/hrm/temp/position'), + 'skill' => array('hrm', '/hrm/skill', '/hrm/temp/skill'), + 'evaluation' => array('hrm', '/hrm/evaluation', '/hrm/temp/evaluation'), + 'conferenceorbooth' => array('eventorganization', '/eventorganization/conferenceorbooth', '/eventorganization/temp/conferenceorbooth'), + ); + foreach ($expected as $element => $values) { + if (!isset($conf->{$values[0]})) { + $this->markTestSkipped('The module '.$values[0].' is not enabled, the fixture is not usable'); + } + $prop = getElementProperties($element); + $this->assertSame(DOL_DATA_ROOT.$values[1], $prop['dir_output'], 'Wrong dir_output for the element '.$element); + $this->assertSame(DOL_DATA_ROOT.$values[2], $prop['dir_temp'], 'Wrong dir_temp for the element '.$element.', the sub directory applies to it too'); + } + } + + /** + * When the module of an element is disabled, its directory must be an empty string and NOT the sub + * directory alone: a caller would then read or write into '/contact', at the root of the file system. + * This is the regression the guard on $dir_output prevents, and it can only be seen with a module off. + * + * @return void + */ + public function testGetElementPropertiesDirOutputWhenTheModuleIsDisabled() + { + global $conf; + + $elements = array('contact' => 'societe', 'job' => 'hrm', 'position' => 'hrm', 'skill' => 'hrm', + 'evaluation' => 'hrm', 'conferenceorbooth' => 'eventorganization'); + + foreach ($elements as $element => $module) { + $savconfmodule = isset($conf->$module) ? $conf->$module : null; + $savmodules = $conf->modules; + + // Simulate the module being disabled: no entry into $conf and no entry into $conf->modules + unset($conf->$module); + unset($conf->modules[$module]); + + try { + $prop = getElementProperties($element); + + $this->assertSame('', $prop['dir_output'], 'The dir_output of the element '.$element.' must be empty when the module '.$module.' is disabled, not the sub directory alone'); + $this->assertSame('', $prop['dir_temp'], 'The dir_temp of the element '.$element.' must be empty when the module '.$module.' is disabled'); + } finally { + if ($savconfmodule !== null) { + $conf->$module = $savconfmodule; + } + $conf->modules = $savmodules; + } + } + } + + /** + * The directory of any equipped element is either empty or inside DOL_DATA_ROOT. It is never a path + * at the root of the file system, and never the sentinel of getMultidirOutput(). + * + * @dataProvider providerEquippedElements + * + * @param string $element Element to check + * @return void + */ + public function testGetElementPropertiesDirOutputIsAlwaysSafe($element) + { + $prop = getElementProperties($element); + + foreach (array('dir_output', 'dir_temp') as $key) { + $dir = (string) $prop[$key]; + $this->assertStringNotContainsString(self::SENTINEL, $dir, 'The '.$key.' of the element '.$element.' must never be the sentinel'); + $this->assertTrue( + $dir === '' || strpos($dir, DOL_DATA_ROOT) === 0, + 'The '.$key.' of the element '.$element.' must be empty or inside DOL_DATA_ROOT, got "'.$dir.'"' + ); + } + } +} diff --git a/test/phpunit/FunctionsLibTest.php b/test/phpunit/FunctionsLibTest.php index c586d238936..c76ce2e67f5 100644 --- a/test/phpunit/FunctionsLibTest.php +++ b/test/phpunit/FunctionsLibTest.php @@ -33,6 +33,8 @@ global $conf,$user,$langs,$db,$mysoc; require_once dirname(__FILE__).'/../../htdocs/master.inc.php'; require_once dirname(__FILE__).'/../../htdocs/core/lib/date.lib.php'; require_once dirname(__FILE__).'/../../htdocs/product/class/product.class.php'; +require_once dirname(__FILE__).'/../../htdocs/societe/class/societe.class.php'; +require_once dirname(__FILE__).'/../../htdocs/contact/class/contact.class.php'; require_once dirname(__FILE__).'/CommonClassTest.class.php'; if (! defined('NOREQUIREUSER')) { @@ -109,6 +111,212 @@ class FunctionsLibTest extends CommonClassTest } + /** + * testGetExdirForObject + * + * get_exdir() with $level = 0 and $withoutslash = 1 is the reference implementation used to forge the + * directory where the documents of an object are stored. FileUpload (the drag and drop of a file on a + * card) relies on it to store the file into the directory read by the "Attached files" tab. + * + * @return void + */ + public function testGetExdirForObject() + { + global $conf, $db; + + // PRODUCT_USE_OLD_PATH_FOR_PHOTO makes get_exdir() answer a two level path built on the id for a + // product, instead of the ref. The assertions below are on the default path, so the option is forced + // off here: it is a global, another test of the suite may have left it on, and the whole suite runs in + // a single process. The option gets its own coverage at the end of this test. + $savoldpath = getDolGlobalInt('PRODUCT_USE_OLD_PATH_FOR_PHOTO'); + $conf->global->PRODUCT_USE_OLD_PATH_FOR_PHOTO = 0; + + // The ref is used when it is defined + $object = new Product($db); + $object->id = 42; + $object->ref = 'MYREF'; + $this->assertSame('MYREF', get_exdir(0, 0, 0, 1, $object, 'product'), 'The ref must be used when it is defined'); + + // The trailing slash is added when $withoutslash is 0. FileUpload appends its own '/' to the + // result, so it must call get_exdir() with $withoutslash = 1 to avoid a double slash in the path. + $this->assertSame('MYREF/', get_exdir(0, 0, 0, 0, $object, 'product'), 'A trailing slash is expected when $withoutslash is 0'); + + // The modulepart is deduced from the object when it is not given + $this->assertSame('MYREF', get_exdir(0, 0, 0, 1, $object), 'The modulepart must be deduced from the object'); + + // The ref is a user input: it must never introduce a directory traversal nor a sub directory + $object->ref = '../../etc'; + $this->assertStringNotContainsString('..', get_exdir(0, 0, 0, 1, $object, 'product'), 'A ref must never allow a directory traversal'); + $object->ref = 'A/B'; + $this->assertStringNotContainsString('/', get_exdir(0, 0, 0, 1, $object, 'product'), 'A ref must never introduce a sub directory'); + + // The id is used as a fallback when the ref is empty + $object->ref = ''; + $this->assertSame('42', get_exdir(0, 0, 0, 1, $object, 'product'), 'The id must be used when the ref is empty'); + $object->ref = null; + $this->assertSame('42', get_exdir(0, 0, 0, 1, $object, 'product'), 'The id must be used when the ref is null'); + $object->ref = '0'; + $this->assertSame('42', get_exdir(0, 0, 0, 1, $object, 'product'), 'A ref "0" is empty for php, so the id is used'); + + // An object with neither a ref nor an id falls back on the directory '0', shared by every unsaved + // object. A caller must never forge a path from an object it did not load. + $empty = new Product($db); + $empty->id = 0; + $empty->ref = ''; + $this->assertSame('0', get_exdir(0, 0, 0, 1, $empty, 'product'), 'An object with no id and no ref falls back on the directory "0"'); + + // The id is always used for a thirdparty, because its ref is a company name, so it is not unique + $thirdparty = new Societe($db); + $thirdparty->id = 7; + $thirdparty->ref = 'My company'; + $this->assertSame('7', get_exdir(0, 0, 0, 1, $thirdparty, 'societe'), 'The id must be used for a thirdparty'); + $this->assertSame('7', get_exdir(0, 0, 0, 1, $thirdparty, 'thirdparty'), 'The id must be used for a thirdparty'); + + // The rule is on the class, not only on the modulepart: a contact belongs to the module 'societe' + // but it is not a Societe, so it keeps its ref + $contact = new Contact($db); + $contact->id = 8; + $contact->ref = 'DOE'; + $this->assertSame('DOE', get_exdir(0, 0, 0, 1, $contact, 'contact'), 'A contact is not a thirdparty, its ref is used'); + + // A module storing its documents on several levels returns the level directories only, not the object + $object->ref = 'MYREF'; + $this->assertSame('2/4', get_exdir(0, 0, 0, 1, $object, 'invoice_supplier'), 'Two levels of directories are expected'); + $this->assertSame('2/4', get_exdir(0, 0, 0, 1, $object, 'supplier_invoice'), 'The two aliases must answer the same directory'); + $this->assertSame('2/4/', get_exdir(0, 0, 0, 0, $object, 'invoice_supplier'), 'A trailing slash is expected when $withoutslash is 0'); + + // The levels are built from the id, not from the ref + $object->id = 1234; + $this->assertSame('4/3', get_exdir(0, 0, 0, 1, $object, 'invoice_supplier'), 'The levels must be built from the id'); + + // With PRODUCT_USE_OLD_PATH_FOR_PHOTO a product joins the modules storing on two levels, so its + // directory is built on the id and the ref is ignored. This is the path the "Attached files" tab of a + // product reads on such an instance, so FileUpload must forge the same one. + $conf->global->PRODUCT_USE_OLD_PATH_FOR_PHOTO = 1; + $object->id = 42; + $object->ref = 'MYREF'; + $this->assertSame('2/4', get_exdir(0, 0, 0, 1, $object, 'product'), 'With the old path option a product uses two levels built on its id'); + + $conf->global->PRODUCT_USE_OLD_PATH_FOR_PHOTO = $savoldpath; + } + + /** + * testGetElementPropertiesDirOutput + * + * The 'dir_output' returned for an element must be the directory read by the "Attached files" tab of + * this element, otherwise a file uploaded by drag and drop is stored but never shown to the user. + * + * @return void + */ + public function testGetElementPropertiesDirOutput() + { + global $conf, $db; + + // A contact is stored into a sub directory of the thirdparty module, see contact/document.php. + // The exact value is asserted and not only the suffix: an assertion on the suffix alone would also + // pass on the value '/contact' returned when the module is disabled, which is the bug guarded here. + if (!isModEnabled('societe')) { + $this->markTestSkipped('The module societe must be enabled to check the directory of a contact'); + } + $prop = getElementProperties('contact'); + $this->assertSame('societe', $prop['module']); + $this->assertSame($conf->societe->multidir_output[$conf->entity].'/contact', $prop['dir_output'], 'A contact is stored into a /contact sub directory'); + $this->assertSame($conf->societe->multidir_temp[$conf->entity].'/contact', $prop['dir_temp'], 'The sub directory applies to the temporary directory too'); + + // The elements added or fixed here must answer the class that is really able to load them, and the + // table restrictedArea() builds its sql on. + $expected = array( + // element => array(module, classname, classpath, classfile, table_element) + 'payment' => array('facture', 'Paiement', 'compta/paiement/class', 'paiement', 'paiement'), + 'payment_supplier' => array('fournisseur', 'PaiementFourn', 'fourn/class', 'paiementfourn', 'paiementfourn'), + 'payment_various' => array('bank', 'PaymentVarious', 'compta/bank/class', 'paymentvarious', 'payment_various'), + 'stocktransfer' => array('stocktransfer', 'StockTransfer', 'product/stock/stocktransfer/class', 'stocktransfer', 'stocktransfer_stocktransfer'), + 'job' => array('hrm', 'Job', 'hrm/class', 'job', 'hrm_job'), + 'position' => array('hrm', 'Position', 'hrm/class', 'position', 'hrm_job_user'), + 'skill' => array('hrm', 'Skill', 'hrm/class', 'skill', 'hrm_skill'), + 'evaluation' => array('hrm', 'Evaluation', 'hrm/class', 'evaluation', 'hrm_evaluation'), + ); + foreach ($expected as $element => $values) { + $prop = getElementProperties($element); + + $this->assertSame($element, $prop['element'], 'The element '.$element.' must not be truncated by the myobject_mysubobject rule'); + $this->assertSame($values[0], $prop['module'], 'Wrong module for the element '.$element); + $this->assertSame($values[1], $prop['classname'], 'Wrong classname for the element '.$element); + $this->assertSame($values[2], $prop['classpath'], 'Wrong classpath for the element '.$element); + $this->assertSame($values[3], $prop['classfile'], 'Wrong classfile for the element '.$element); + $this->assertSame($values[4], $prop['table_element'], 'Wrong table for the element '.$element); + + // The class must really exist, otherwise fetchObjectByElement() ends on a fatal error + $file = DOL_DOCUMENT_ROOT.'/'.$prop['classpath'].'/'.$prop['classfile'].'.class.php'; + $this->assertFileExists($file, 'The class file of the element '.$element.' does not exist'); + require_once $file; + $this->assertTrue(class_exists($prop['classname']), 'The class '.$prop['classname'].' of the element '.$element.' does not exist'); + + // And the table must exist too, restrictedArea() builds its sql on it. + // The table of an optional module is only created when the module is enabled, so the check is + // skipped otherwise: the mapping asserted above does not depend on the module being enabled. + if (isModEnabled($values[0])) { + $sql = "SELECT COUNT(*) as nb FROM ".$db->prefix().$db->escape($values[4]); + $resql = $db->query($sql); + $this->assertNotFalse($resql, 'The table '.$values[4].' of the element '.$element.' is not readable'); + if ($resql) { + $db->free($resql); + } + } + } + + // The elements of the hrm module are all stored into a sub directory named after the element + if (isModEnabled('hrm')) { + foreach (array('job', 'position', 'skill', 'evaluation') as $element) { + $prop = getElementProperties($element); + $this->assertSame($conf->hrm->dir_output.'/'.$element, $prop['dir_output'], 'The element '.$element.' is stored into a /'.$element.' sub directory'); + $this->assertSame($conf->hrm->dir_temp.'/'.$element, $prop['dir_temp'], 'The sub directory applies to the temporary directory of '.$element.' too'); + } + } + + // The sub directory must not be appended when the module is disabled, otherwise we would return a + // path at the root of the file system (for example '/contact') instead of an empty string. This is + // only observable with the module really off, so it is simulated here. + $elements = array('contact' => 'societe', 'job' => 'hrm', 'position' => 'hrm', 'skill' => 'hrm', + 'evaluation' => 'hrm', 'conferenceorbooth' => 'eventorganization'); + foreach ($elements as $element => $module) { + $savconfmodule = isset($conf->$module) ? $conf->$module : null; + $savmodules = $conf->modules; + + unset($conf->$module); + unset($conf->modules[$module]); + + try { + $prop = getElementProperties($element); + $this->assertSame('', $prop['dir_output'], 'The dir_output of the element '.$element.' must be empty when the module '.$module.' is disabled, not the sub directory alone'); + $this->assertSame('', $prop['dir_temp'], 'The dir_temp of the element '.$element.' must be empty when the module '.$module.' is disabled'); + } finally { + if ($savconfmodule !== null) { + $conf->$module = $savconfmodule; + } + $conf->modules = $savmodules; + } + } + + // So the directory of an element is either empty, or a directory of the data directory of Dolibarr, + // and never the sentinel string returned by getMultidirOutput() when it fails. + $allelements = array('contact', 'job', 'position', 'skill', 'evaluation', 'conferenceorbooth', + 'partnership', 'stocktransfer', 'payment', 'payment_supplier', 'payment_various', 'product', + 'societe', 'action', 'expedition', 'reception', 'don', 'expensereport', 'holiday', 'mo', + 'productlot', 'resource', 'workstation', 'knowledgerecord', 'asset', 'salary', 'chargesociales'); + foreach ($allelements as $element) { + $prop = getElementProperties($element); + foreach (array('dir_output', 'dir_temp') as $key) { + $dir = (string) $prop[$key]; + $this->assertStringNotContainsString('error-diroutput-not-defined-for-this-object', $dir, 'The '.$key.' of the element '.$element.' must never be the sentinel of getMultidirOutput'); + $this->assertTrue( + $dir === '' || strpos($dir, DOL_DATA_ROOT) === 0, + 'The '.$key.' of the element '.$element.' must be empty or inside DOL_DATA_ROOT, got "'.$dir.'"' + ); + } + } + } + /** * testDolCheckFilters *