From c1b59b195013668329aaf1c923865bb822416495 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Thu, 26 Jan 2017 18:08:00 +0100 Subject: [PATCH 1/4] Fix several security bugs on expense report --- htdocs/core/class/html.form.class.php | 28 ++--- .../core/modules/modExpenseReport.class.php | 104 ++++++++++-------- htdocs/expensereport/card.php | 14 +-- .../class/expensereport.class.php | 3 + htdocs/expensereport/list.php | 2 +- htdocs/index.php | 32 +++--- htdocs/user/card.php | 5 +- htdocs/user/class/user.class.php | 45 +++++--- 8 files changed, 128 insertions(+), 105 deletions(-) diff --git a/htdocs/core/class/html.form.class.php b/htdocs/core/class/html.form.class.php index bc761b2d2b1..6cf24b2e38f 100644 --- a/htdocs/core/class/html.form.class.php +++ b/htdocs/core/class/html.form.class.php @@ -1372,7 +1372,7 @@ class Form * @param int $show_empty 0=list with no empty value, 1=add also an empty value into list * @param array $exclude Array list of users id to exclude * @param int $disabled If select list must be disabled - * @param array $include Array list of users id to include or 'hierarchy' to have only supervised users + * @param array|string $include Array list of users id to include or 'hierarchy' to have only supervised users or 'hierarchyme' to have supervised + me * @param array $enableonly Array list of users id to be enabled. All other must be disabled * @param int $force_entity 0 or Id of environment to force * @param int $maxlength Maximum length of string into list (0=no limit) @@ -1396,24 +1396,20 @@ class Form $includeUsers=null; // Permettre l'exclusion d'utilisateurs - if (is_array($exclude)) $excludeUsers = implode("','",$exclude); + if (is_array($exclude)) $excludeUsers = implode(",",$exclude); // Permettre l'inclusion d'utilisateurs - if (is_array($include)) $includeUsers = implode("','",$include); + if (is_array($include)) $includeUsers = implode(",",$include); else if ($include == 'hierarchy') { // Build list includeUsers to have only hierarchy - $userid=$user->id; - $include=array(); - if (empty($user->users) || ! is_array($user->users)) $user->get_full_tree(); - foreach($user->users as $key => $val) - { - if (preg_match('/_'.$userid.'/',$val['fullpath'])) $include[]=$val['id']; - } - $includeUsers = implode("','",$include); - //var_dump($includeUsers);exit; - //var_dump($user->users);exit; + $includeUsers = implode(",",$user->getAllChildIds(0)); } - + else if ($include == 'hierarchyme') + { + // Build list includeUsers to have only hierarchy and current user + $includeUsers = implode(",",$user->getAllChildIds(1)); + } + $out=''; // On recherche les utilisateurs @@ -1443,8 +1439,8 @@ class Form } } if (! empty($user->societe_id)) $sql.= " AND u.fk_soc = ".$user->societe_id; - if (is_array($exclude) && $excludeUsers) $sql.= " AND u.rowid NOT IN ('".$excludeUsers."')"; - if (is_array($include) && $includeUsers) $sql.= " AND u.rowid IN ('".$includeUsers."')"; + if (is_array($exclude) && $excludeUsers) $sql.= " AND u.rowid NOT IN (".$excludeUsers.")"; + if ($includeUsers) $sql.= " AND u.rowid IN (".$includeUsers.")"; if (! empty($conf->global->USER_HIDE_INACTIVE_IN_COMBOBOX) || $noactive) $sql.= " AND u.statut <> 0"; if (! empty($morefilter)) $sql.=" ".$morefilter; $sql.= " ORDER BY u.lastname ASC"; diff --git a/htdocs/core/modules/modExpenseReport.class.php b/htdocs/core/modules/modExpenseReport.class.php index 1507d236698..96b30d92467 100644 --- a/htdocs/core/modules/modExpenseReport.class.php +++ b/htdocs/core/modules/modExpenseReport.class.php @@ -111,54 +111,62 @@ class modExpenseReport extends DolibarrModules $this->rights = array(); // Permission array used by this module $this->rights_class = 'expensereport'; - $this->rights[1][0] = 771; - $this->rights[1][1] = 'Read expense reports (yours and your subordinates)'; - $this->rights[1][2] = 'r'; - $this->rights[1][3] = 0; - $this->rights[1][4] = 'lire'; - - $this->rights[3][0] = 772; - $this->rights[3][1] = 'Create/modify expense reports'; - $this->rights[3][2] = 'w'; - $this->rights[3][3] = 0; - $this->rights[3][4] = 'creer'; - - $this->rights[4][0] = 773; - $this->rights[4][1] = 'Delete expense reports'; - $this->rights[4][2] = 'd'; - $this->rights[4][3] = 0; - $this->rights[4][4] = 'supprimer'; - - $this->rights[6][0] = 775; - $this->rights[6][1] = 'Approve expense reports'; - $this->rights[6][2] = 'w'; - $this->rights[6][3] = 0; - $this->rights[6][4] = 'approve'; - - $this->rights[7][0] = 776; - $this->rights[7][1] = 'Pay expense reports'; - $this->rights[7][2] = 'w'; - $this->rights[7][3] = 0; - $this->rights[7][4] = 'to_paid'; - - $this->rights[2][0] = 777; - $this->rights[2][1] = 'Read expense reports of everybody'; - $this->rights[2][2] = 'r'; - $this->rights[2][3] = 1; - $this->rights[2][4] = 'readall'; - - $this->rights[2][0] = 778; - $this->rights[2][1] = 'Create expense reports for everybody'; - $this->rights[2][2] = 'w'; - $this->rights[2][3] = 0; - $this->rights[2][4] = 'writeall_advance'; - - $this->rights[5][0] = 779; - $this->rights[5][1] = 'Export expense reports'; - $this->rights[5][2] = 'r'; - $this->rights[5][3] = 0; - $this->rights[5][4] = 'export'; - + $this->rights[$r][0] = 771; + $this->rights[$r][1] = 'Read expense reports (yours and your subordinates)'; + $this->rights[$r][2] = 'r'; + $this->rights[$r][3] = 0; + $this->rights[$r][4] = 'lire'; + $r++; + + $this->rights[$r][0] = 772; + $this->rights[$r][1] = 'Create/modify expense reports'; + $this->rights[$r][2] = 'w'; + $this->rights[$r][3] = 0; + $this->rights[$r][4] = 'creer'; + $r++; + + $this->rights[$r][0] = 773; + $this->rights[$r][1] = 'Delete expense reports'; + $this->rights[$r][2] = 'd'; + $this->rights[$r][3] = 0; + $this->rights[$r][4] = 'supprimer'; + $r++; + + $this->rights[$r][0] = 775; + $this->rights[$r][1] = 'Approve expense reports'; + $this->rights[$r][2] = 'w'; + $this->rights[$r][3] = 0; + $this->rights[$r][4] = 'approve'; + $r++; + + $this->rights[$r][0] = 776; + $this->rights[$r][1] = 'Pay expense reports'; + $this->rights[$r][2] = 'w'; + $this->rights[$r][3] = 0; + $this->rights[$r][4] = 'to_paid'; + $r++; + + $this->rights[$r][0] = 777; + $this->rights[$r][1] = 'Read expense reports of everybody'; + $this->rights[$r][2] = 'r'; + $this->rights[$r][3] = 1; + $this->rights[$r][4] = 'readall'; + $r++; + + $this->rights[$r][0] = 778; + $this->rights[$r][1] = 'Create expense reports for everybody'; + $this->rights[$r][2] = 'w'; + $this->rights[$r][3] = 0; + $this->rights[$r][4] = 'writeall_advance'; + $r++; + + $this->rights[$r][0] = 779; + $this->rights[$r][1] = 'Export expense reports'; + $this->rights[$r][2] = 'r'; + $this->rights[$r][3] = 0; + $this->rights[$r][4] = 'export'; + $r++; + // Menus //------- $this->menu = 1; // This module add menu entries. They are coded into menu manager. diff --git a/htdocs/expensereport/card.php b/htdocs/expensereport/card.php index 2d93c88159c..a0c5138200e 100644 --- a/htdocs/expensereport/card.php +++ b/htdocs/expensereport/card.php @@ -1,6 +1,6 @@ - * Copyright (C) 2004-2015 Laurent Destailleur + * Copyright (C) 2004-2017 Laurent Destailleur * Copyright (C) 2005-2009 Regis Houssin * Copyright (C) 2015-2016 Alexandre Spangaro * @@ -1228,8 +1228,8 @@ if ($action == 'create') print ''; $defaultselectuser=$user->id; if (GETPOST('fk_user_author') > 0) $defaultselectuser=GETPOST('fk_user_author'); - $include_users = array($user->id); - if (! empty($user->rights->expensereport->writeall)) $include_users=array(); + $include_users = 'hierarchyme'; + if (! empty($conf->global->MAIN_USE_ADVANCED_PERMS) && ! empty($user->rights->expensereport->writeall_advance)) $include_users=array(); $s=$form->select_dolusers($defaultselectuser, "fk_user_author", 0, "", 0, $include_users); print $s; print ''; @@ -1313,10 +1313,10 @@ else if ($result > 0) { - if ($object->fk_user_author != $user->id) + if (! in_array($object->fk_user_author, $user->getAllChildIds(1))) { if (empty($user->rights->expensereport->readall) && empty($user->rights->expensereport->lire_tous) - && empty($user->rights->expensereport->writeall_advance)) + && (empty($conf->global->MAIN_USE_ADVANCED_PERMS) || empty($user->rights->expensereport->writeall_advance))) { print load_fiche_titre($langs->trans('TripCard')); @@ -2090,7 +2090,7 @@ if ($action != 'create' && $action != 'edit') */ if ($user->rights->expensereport->creer && $object->fk_statut==0) { - if ($object->fk_user_author == $user->id) + if (in_array($object->fk_user_author, $user->getAllChildIds(1))) { // Modify print '
id.'">'.$langs->trans('Modify').'
'; @@ -2138,7 +2138,7 @@ if ($action != 'create' && $action != 'edit') */ if ($object->fk_statut == 2) { - if ($object->fk_user_author == $user->id) + if (in_array($object->fk_user_author, $user->getAllChildIds(1))) { // Brouillonner print '
id.'">'.$langs->trans('SetToDraft').'
'; diff --git a/htdocs/expensereport/class/expensereport.class.php b/htdocs/expensereport/class/expensereport.class.php index 574fde69c2a..209d7edb12c 100644 --- a/htdocs/expensereport/class/expensereport.class.php +++ b/htdocs/expensereport/class/expensereport.class.php @@ -1674,11 +1674,14 @@ class ExpenseReport extends CommonObject $now=dol_now(); + $userchildids = $user->getAllChildIds(1); + $sql = "SELECT ex.rowid, ex.date_valid"; $sql.= " FROM ".MAIN_DB_PREFIX."expensereport as ex"; if ($option == 'toapprove') $sql.= " WHERE ex.fk_statut = 2"; else $sql.= " WHERE ex.fk_statut = 5"; $sql.= " AND ex.entity IN (".getEntity('expensereport', 1).")"; + $sql.= " AND ex.fk_user_author IN (".join(',',$userchildids).")"; $resql=$this->db->query($sql); if ($resql) diff --git a/htdocs/expensereport/list.php b/htdocs/expensereport/list.php index 7928e0f8e3d..cb54414fe35 100644 --- a/htdocs/expensereport/list.php +++ b/htdocs/expensereport/list.php @@ -240,7 +240,7 @@ if ($search_status != '' && $search_status >= 0) } // RESTRICT RIGHTS if (empty($user->rights->expensereport->readall) && empty($user->rights->expensereport->lire_tous) - && empty($user->rights->expensereport->writeall_advance)) + && (empty($conf->global->MAIN_USE_ADVANCED_PERMS) || empty($user->rights->expensereport->writeall_advance))) { $childids = $user->getAllChildIds(); $childids[]=$user->id; diff --git a/htdocs/index.php b/htdocs/index.php index f4be0dc9c63..16126975b9f 100644 --- a/htdocs/index.php +++ b/htdocs/index.php @@ -178,8 +178,8 @@ if (empty($user->societe_id)) ! empty($conf->supplier_order->enabled) && $user->rights->fournisseur->commande->lire && empty($conf->global->SOCIETE_DISABLE_SUPPLIERS_ORDERS_STATS), ! empty($conf->supplier_invoice->enabled) && $user->rights->fournisseur->facture->lire && empty($conf->global->SOCIETE_DISABLE_SUPPLIERS_INVOICES_STATS), ! empty($conf->supplier_proposal->enabled) && $user->rights->supplier_proposal->lire && empty($conf->global->SOCIETE_DISABLE_SUPPLIERS_PROPOSAL_STATS), - ! empty($conf->expensereport->enabled) && $user->rights->expensereport->lire, - ! empty($conf->projet->enabled) && $user->rights->projet->lire + ! empty($conf->projet->enabled) && $user->rights->projet->lire, + ! empty($conf->expensereport->enabled) && $user->rights->expensereport->lire ); // Class file containing the method load_state_board for each line $includes=array( @@ -199,8 +199,8 @@ if (empty($user->societe_id)) DOL_DOCUMENT_ROOT."/fourn/class/fournisseur.commande.class.php", DOL_DOCUMENT_ROOT."/fourn/class/fournisseur.facture.class.php", DOL_DOCUMENT_ROOT."/supplier_proposal/class/supplier_proposal.class.php", - DOL_DOCUMENT_ROOT."/expensereport/class/expensereport.class.php", - DOL_DOCUMENT_ROOT."/projet/class/project.class.php" + DOL_DOCUMENT_ROOT."/projet/class/project.class.php", + DOL_DOCUMENT_ROOT."/expensereport/class/expensereport.class.php" ); // Name class containing the method load_state_board for each line $classes=array('User', @@ -219,8 +219,8 @@ if (empty($user->societe_id)) 'CommandeFournisseur', 'FactureFournisseur', 'SupplierProposal', - 'ExpenseReport', - 'Project' + 'Project', + 'ExpenseReport' ); // Cle array returned by the method load_state_board for each line $keys=array('users', @@ -239,8 +239,8 @@ if (empty($user->societe_id)) 'supplier_orders', 'supplier_invoices', 'askprice', - 'expensereports', - 'projects' + 'projects', + 'expensereports' ); // Dashboard Icon lines $icons=array('user', @@ -259,8 +259,8 @@ if (empty($user->societe_id)) 'order', 'bill', 'propal', - 'trip', - 'project' + 'project', + 'trip' ); // Translation keyword $titres=array("Users", @@ -279,8 +279,8 @@ if (empty($user->societe_id)) "SuppliersOrders", "SuppliersInvoices", "SupplierProposalShort", - "ExpenseReports", - "Projects" + "Projects", + "ExpenseReports" ); // Dashboard Link lines $links=array( @@ -300,8 +300,8 @@ if (empty($user->societe_id)) DOL_URL_ROOT.'/fourn/commande/list.php', DOL_URL_ROOT.'/fourn/facture/list.php', DOL_URL_ROOT.'/supplier_proposal/list.php', - DOL_URL_ROOT.'/expensereport/list.php?mainmenu=hrm', - DOL_URL_ROOT.'/projet/list.php?mainmenu=project' + DOL_URL_ROOT.'/projet/list.php?mainmenu=project', + DOL_URL_ROOT.'/expensereport/list.php?mainmenu=hrm' ); // Translation lang files $langfile=array("users", @@ -318,8 +318,8 @@ if (empty($user->societe_id)) "supplier_proposal", "contracts", "interventions", - "trips", - "projects" + "projects", + "trips" ); diff --git a/htdocs/user/card.php b/htdocs/user/card.php index e7614f06f9d..f6ec03abf91 100644 --- a/htdocs/user/card.php +++ b/htdocs/user/card.php @@ -752,9 +752,10 @@ if (($action == 'create') || ($action == 'adduserldap')) print ''; // Employee + $defaultemployee=1; print ''; - print ''.fieldLabel('Employee','employee',0).''; - print $form->selectyesno("employee",(GETPOST('employee')?GETPOST('employee'):0),1); + print ''.$langs->trans('Employee').''; + print $form->selectyesno("employee",(GETPOST('employee')!=''?GETPOST('employee'):$defaultemployee),1); print ''; // Position/Job diff --git a/htdocs/user/class/user.class.php b/htdocs/user/class/user.class.php index c215f5f29cc..68bfad3a988 100644 --- a/htdocs/user/class/user.class.php +++ b/htdocs/user/class/user.class.php @@ -127,6 +127,9 @@ class User extends CommonObject public $dateemployment; // Define date of employment by company + private $cache_childids; + + /** * Constructor de la classe * @@ -2600,26 +2603,38 @@ class User extends CommonObject } /** - * Return list of all child users id in herarchy (all sublevels). + * Return list of all child users id in herarchy of current user (all sublevels). * + * @return int $addcurrentuser 1=Add also current user id to the list. * @return array Array of user id lower than user. This overwrite this->users. * @see get_children */ - function getAllChildIds() + function getAllChildIds($addcurrentuser=0) { - // Init this->users - $this->get_full_tree(); - - $idtoscan=$this->id; - $childids=array(); - - dol_syslog("Build childid for id = ".$idtoscan); - foreach($this->users as $id => $val) - { - //var_dump($val['fullpath']); - if (preg_match('/_'.$idtoscan.'_/', $val['fullpath'])) $childids[$val['id']]=$val['id']; - } - + $childids=array(); + + if (isset($this->cache_childids[$this->id])) + { + $childids = $this->cache_childids[$this->id]; + } + else + { + // Init this->users + $this->get_full_tree(); + + $idtoscan=$this->id; + + dol_syslog("Build childid for id = ".$idtoscan); + foreach($this->users as $id => $val) + { + //var_dump($val['fullpath']); + if (preg_match('/_'.$idtoscan.'_/', $val['fullpath'])) $childids[$val['id']]=$val['id']; + } + } + $this->cache_childids[$this->id] = $childids; + + if ($addcurrentuser) $childids[$this->id]=$this->id; + return $childids; } From 60ac751d84aa5f74deb6d4da1b035325bcbe0044 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Thu, 26 Jan 2017 18:21:39 +0100 Subject: [PATCH 2/4] Fix translation --- htdocs/langs/en_US/admin.lang | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/langs/en_US/admin.lang b/htdocs/langs/en_US/admin.lang index 5fd1ae065ec..e8c60db1fc9 100644 --- a/htdocs/langs/en_US/admin.lang +++ b/htdocs/langs/en_US/admin.lang @@ -689,7 +689,7 @@ PermissionAdvanced253=Create/modify internal/external users and permissions Permission254=Create/modify external users only Permission255=Modify other users password Permission256=Delete or disable other users -Permission262=Extend access to all third parties (not only third parties that user is a sale representative). Not effective for external users (always limited to themselves for proposals, orders, invoices, contracts, etc). Not effective for projects (only rules on project permissions, visibility and assignement matters). +Permission262=Extend access to all third parties (not only third parties that user is a sale representative).
Not effective for external users (always limited to themselves for proposals, orders, invoices, contracts, etc).
Not effective for projects (only rules on project permissions, visibility and assignement matters). Permission271=Read CA Permission272=Read invoices Permission273=Issue invoices From 72fd21b1833266f39fed5ed0922ac9e57160be8d Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Thu, 26 Jan 2017 20:41:16 +0100 Subject: [PATCH 3/4] Fix phpcs --- htdocs/bookmarks/card.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/bookmarks/card.php b/htdocs/bookmarks/card.php index b89dbf291b7..0583c45bdab 100644 --- a/htdocs/bookmarks/card.php +++ b/htdocs/bookmarks/card.php @@ -219,7 +219,7 @@ if ($id > 0 && ! preg_match('/^add/i',$action)) $linkback = ''.$langs->trans("BackToList").''; - dol_banner_tab($object, 'id', $linkback, 1, 'rowid', 'ref', '' , '', 0, '', '', 0); + dol_banner_tab($object, 'id', $linkback, 1, 'rowid', 'ref', '', '', 0, '', '', 0); print '
'; print ''; From ca79d0c274201563d25539580623113f97ed8f80 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Thu, 26 Jan 2017 23:36:58 +0100 Subject: [PATCH 4/4] Fix PHPCS --- htdocs/user/class/user.class.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/user/class/user.class.php b/htdocs/user/class/user.class.php index 68bfad3a988..188195b04bf 100644 --- a/htdocs/user/class/user.class.php +++ b/htdocs/user/class/user.class.php @@ -2605,7 +2605,7 @@ class User extends CommonObject /** * Return list of all child users id in herarchy of current user (all sublevels). * - * @return int $addcurrentuser 1=Add also current user id to the list. + * @param int $addcurrentuser 1=Add also current user id to the list. * @return array Array of user id lower than user. This overwrite this->users. * @see get_children */