From f647ce61c28f55b8f756a2b7de8cf623d6bbc026 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Wed, 10 Feb 2021 14:34:21 +0100 Subject: [PATCH 01/23] =?UTF-8?q?N=C2=B03721=20-=20toolkit's=20"update=20i?= =?UTF-8?q?Top"=20with=20the=20"Create=20symbolic=20links"=20option=20chec?= =?UTF-8?q?ked=20now=20empty=20the=20compiled=20directory=20as=20expected?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/compiler.class.inc.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup/compiler.class.inc.php b/setup/compiler.class.inc.php index f541058f2..49239404f 100644 --- a/setup/compiler.class.inc.php +++ b/setup/compiler.class.inc.php @@ -129,6 +129,8 @@ class MFCompiler { // Skip the creation of a temporary dictionary, not compatible with symbolic links $sTempTargetDir = $sFinalTargetDir; + SetupUtils::rrmdir($sFinalTargetDir); + SetupUtils::builddir($sFinalTargetDir); // Here is the directory } else { From 656fa3208a763e97a8a488d7890871472ce80933 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Wed, 10 Feb 2021 15:33:01 +0100 Subject: [PATCH 02/23] =?UTF-8?q?N=C2=B03721=20-=20revert=20the=20feature?= =?UTF-8?q?=20(will=20only=20be=20available=20on=20the=203.0)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/compiler.class.inc.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/setup/compiler.class.inc.php b/setup/compiler.class.inc.php index 49239404f..f541058f2 100644 --- a/setup/compiler.class.inc.php +++ b/setup/compiler.class.inc.php @@ -129,8 +129,6 @@ class MFCompiler { // Skip the creation of a temporary dictionary, not compatible with symbolic links $sTempTargetDir = $sFinalTargetDir; - SetupUtils::rrmdir($sFinalTargetDir); - SetupUtils::builddir($sFinalTargetDir); // Here is the directory } else { From c8e8778d7b94a7d1026c5477e1be4bf6c0f57f05 Mon Sep 17 00:00:00 2001 From: Eric Date: Tue, 26 Jan 2021 16:08:30 +0100 Subject: [PATCH 03/23] =?UTF-8?q?N=C2=B03468=20-=20Fix=20extension.xml=20p?= =?UTF-8?q?reventing=20extensions=20installation=20(cherry=20picked=20from?= =?UTF-8?q?=20commit=2092c8af1b19cd9c0c780d2dc27c45b8d9c46f270f)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/extensionsmap.class.inc.php | 25 +++++++------ setup/wizardsteps.class.inc.php | 59 +++++++++++++++++++++---------- 2 files changed, 55 insertions(+), 29 deletions(-) diff --git a/setup/extensionsmap.class.inc.php b/setup/extensionsmap.class.inc.php index 4401175e6..7d8efb934 100644 --- a/setup/extensionsmap.class.inc.php +++ b/setup/extensionsmap.class.inc.php @@ -61,24 +61,29 @@ class iTopExtension * @var bool */ public $bVisible; - + /** * @var string[] */ public $aModules; - + /** * @var string[] */ public $aModuleVersion; - + + /** + * @var string[] + */ + public $aModuleInfo; + /** * @var string */ public $sSourceDir; - + /** - * + * * @var string[] */ public $aMissingDependencies; @@ -96,6 +101,7 @@ class iTopExtension $this->sInstalledVersion = ''; $this->aModules = array(); $this->aModuleVersion = array(); + $this->aModuleInfo = array(); $this->sSourceDir = ''; $this->bVisible = true; $this->aMissingDependencies = array(); @@ -310,11 +316,11 @@ class iTopExtensionsMap $sModuleVersion = '0.0.1'; } - if (($sParentExtensionId !== null) && (array_key_exists($sParentExtensionId, $this->aExtensions)) && ($this->aExtensions[$sParentExtensionId] instanceof iTopExtension)) - { + if (($sParentExtensionId !== null) && (array_key_exists($sParentExtensionId, $this->aExtensions)) && ($this->aExtensions[$sParentExtensionId] instanceof iTopExtension)) { // Already inside an extension, let's add this module the list of modules belonging to this extension $this->aExtensions[$sParentExtensionId]->aModules[] = $sModuleName; $this->aExtensions[$sParentExtensionId]->aModuleVersion[$sModuleName] = $sModuleVersion; + $this->aExtensions[$sParentExtensionId]->aModuleInfo[$sModuleName] = $aModuleInfo[2]; } else { @@ -329,8 +335,6 @@ class iTopExtensionsMap } // Let's create a "fake" extension from this module (containing just this module) for backwards compatibility - $sExtensionId = $sModuleId; - $oExtension = new iTopExtension(); $oExtension->sCode = $sModuleName; $oExtension->sLabel = $aModuleInfo[2]['label']; @@ -341,9 +345,10 @@ class iTopExtensionsMap $oExtension->sMoreInfoUrl = $aModuleInfo[2]['doc.more_information']; $oExtension->aModules = array($sModuleName); $oExtension->aModuleVersion[$sModuleName] = $sModuleVersion; + $oExtension->aModuleInfo[$sModuleName] = $aModuleInfo[2]; $oExtension->sSourceDir = $sSearchDir; $oExtension->bVisible = $bVisible; - $this->AddExtension($oExtension); + $this->AddExtension($oExtension); } closedir($hDir); diff --git a/setup/wizardsteps.class.inc.php b/setup/wizardsteps.class.inc.php index 5795c62fc..40d407055 100644 --- a/setup/wizardsteps.class.inc.php +++ b/setup/wizardsteps.class.inc.php @@ -1666,10 +1666,12 @@ EOF /** * Converts the list of selected "choices" into a list of "modules": take into account the selected and the mandatory modules - * @param hash $aInfo Info about the "choice" array('options' => array(...), 'alternatives' => array(...)) - * @param hash $aSelectedChoices List of selected choices array('name' => 'selected_value_id') - * @param hash $aModules Return parameter: List of selected modules array('module_id' => true) + * + * @param array $aInfo Info about the "choice" array('options' => array(...), 'alternatives' => array(...)) + * @param array $aSelectedChoices List of selected choices array('name' => 'selected_value_id') + * @param array $aModules Return parameter: List of selected modules array('module_id' => true) * @param string $sParentId Used for recursion + * * @return string A text representation of what will be installed */ protected function GetSelectedModules($aInfo, $aSelectedChoices, &$aModules, $sParentId = '', $sDisplayChoices = '', &$aSelectedExtensions = null) @@ -1690,35 +1692,55 @@ EOF } } $aOptions = isset($aInfo['options']) ? $aInfo['options'] : array(); - foreach($aOptions as $index => $aChoice) - { + foreach($aOptions as $index => $aChoice) { $sChoiceId = $sParentId.self::$SEP.$index; - if ( (isset($aChoice['mandatory']) && $aChoice['mandatory']) || - (isset($aSelectedChoices[$sChoiceId]) && ($aSelectedChoices[$sChoiceId] == $sChoiceId)) ) - { + $aModuleInfo = []; + // Get the extension corresponding to the choice + foreach ($this->oExtensionsMap->GetAllExtensions() as $sExtensionVersion => $oExtension) { + if (utils::StartsWith($sExtensionVersion, $aChoice['extension_code'].'/')) { + $aModuleInfo = $oExtension->aModuleInfo; + break; + } + } + if ((isset($aChoice['mandatory']) && $aChoice['mandatory']) || + (isset($aSelectedChoices[$sChoiceId]) && ($aSelectedChoices[$sChoiceId] == $sChoiceId))) { $sDisplayChoices .= '
  • '.$aChoice['title'].'
    • '; - if (isset($aChoice['modules'])) - { - foreach($aChoice['modules'] as $sModuleId) - { - $aModules[$sModuleId] = true; // store the Id of the selected module + if (isset($aChoice['modules'])) { + foreach ($aChoice['modules'] as $sModuleId) { + $bSelected = true; + if (isset($aModuleInfo[$sModuleId])) { + // Test if module has 'auto_select' + $aInfo = $aModuleInfo[$sModuleId]; + if (isset($aInfo['auto_select'])) { + // Check the module selection + try { + $bSelected = false; + SetupInfo::SetSelectedModules($aModules); + eval('$bSelected = ('.$aInfo['auto_select'].');'); + } + catch (Exception $e) { + $bSelected = false; + } + } + } + if ($bSelected) { + $aModules[$sModuleId] = true; // store the Id of the selected module + SetupInfo::SetSelectedModules($aModules); + } } } $sChoiceType = isset($aChoice['type']) ? $aChoice['type'] : 'wizard_option'; - if ($aSelectedExtensions !== null) - { + if ($aSelectedExtensions !== null) { $aSelectedExtensions[] = $aChoice['extension_code']; } // Recurse only for selected choices - if (isset($aChoice['sub_options'])) - { + if (isset($aChoice['sub_options'])) { $sDisplayChoices .= '
      '; $sDisplayChoices = $this->GetSelectedModules($aChoice['sub_options'], $aSelectedChoices, $aModules, $sChoiceId, $sDisplayChoices, $aSelectedExtensions); $sDisplayChoices .= '
    '; } $sDisplayChoices .= ''; } - $index++; } $aAlternatives = isset($aInfo['alternatives']) ? $aInfo['alternatives'] : array(); @@ -1754,7 +1776,6 @@ EOF } $sDisplayChoices .= ''; } - $index++; } if ($sParentId == '') { From 38bc2d9d584759c72cbdd55082b98998fef1781d Mon Sep 17 00:00:00 2001 From: Molkobain Date: Mon, 15 Feb 2021 13:49:39 +0100 Subject: [PATCH 04/23] :wrench: Change max line length in .editorConfig --- .editorconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.editorconfig b/.editorconfig index 0d75f75f2..46ae2703a 100644 --- a/.editorconfig +++ b/.editorconfig @@ -6,14 +6,14 @@ end_of_line = lf indent_size = 4 indent_style = space insert_final_newline = false -max_line_length = 140 +max_line_length = 300 tab_width = 4 ij_continuation_indent_size = 8 ij_formatter_off_tag = @formatter:off ij_formatter_on_tag = @formatter:on ij_formatter_tags_enabled = false ij_smart_tabs = false -ij_visual_guides = 80,120 +ij_visual_guides = 300 ij_wrap_on_typing = true [*.css] From 3058b2eb00a34c5ec8785913126961041b23fe58 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Mon, 15 Feb 2021 17:08:47 +0100 Subject: [PATCH 05/23] =?UTF-8?q?N=C2=B03142=20-=20Add=20(missing)=20trans?= =?UTF-8?q?lations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dictionaries/en.dictionary.itop.core.php | 10 ++++++++++ dictionaries/fr.dictionary.itop.core.php | 12 ++++++++++++ 2 files changed, 22 insertions(+) diff --git a/dictionaries/en.dictionary.itop.core.php b/dictionaries/en.dictionary.itop.core.php index 38214190e..d49eba723 100644 --- a/dictionaries/en.dictionary.itop.core.php +++ b/dictionaries/en.dictionary.itop.core.php @@ -1045,4 +1045,14 @@ Dict::Add('EN US', 'English', 'English', array( 'Class:AsyncTask/Attribute:event_id+' => '', 'Class:AsyncTask/Attribute:finalclass' => 'Final class', 'Class:AsyncTask/Attribute:finalclass+' => '', + 'Class:AsyncTask/Attribute:status' => 'Status', + 'Class:AsyncTask/Attribute:status+' => '', + 'Class:AsyncTask/Attribute:remaining_retries' => 'Remaining retries', + 'Class:AsyncTask/Attribute:remaining_retries+' => '', + 'Class:AsyncTask/Attribute:last_error_code' => 'Last error code', + 'Class:AsyncTask/Attribute:last_error_code+' => '', + 'Class:AsyncTask/Attribute:last_error' => 'Last error', + 'Class:AsyncTask/Attribute:last_error+' => '', + 'Class:AsyncTask/Attribute:last_attempt' => 'Last attempt', + 'Class:AsyncTask/Attribute:last_attempt+' => '', )); diff --git a/dictionaries/fr.dictionary.itop.core.php b/dictionaries/fr.dictionary.itop.core.php index e4d60f306..b673a7178 100644 --- a/dictionaries/fr.dictionary.itop.core.php +++ b/dictionaries/fr.dictionary.itop.core.php @@ -1043,6 +1043,18 @@ Dict::Add('FR FR', 'French', 'Français', array( 'Class:AsyncTask/Attribute:event_id+' => '', 'Class:AsyncTask/Attribute:finalclass' => 'Sous-classe de tâche asynchrone', 'Class:AsyncTask/Attribute:finalclass+' => '', + 'Class:AsyncTask/Attribute:status' => 'Status', + 'Class:AsyncTask/Attribute:status+' => '', + 'Class:AsyncTask/Attribute:remaining_retries' => 'Éssais restants', + 'Class:AsyncTask/Attribute:remaining_retries+' => '', + 'Class:AsyncTask/Attribute:last_error_code' => 'Dernier code d\'erreur', + 'Class:AsyncTask/Attribute:last_error_code+' => '', + 'Class:AsyncTask/Attribute:last_error' => 'Dernière erreur', + 'Class:AsyncTask/Attribute:last_error+' => '', + 'Class:AsyncTask/Attribute:last_attempt' => 'Dernière tentative', + 'Class:AsyncTask/Attribute:last_attempt+' => '', + + )); // Additional language entries not present in English dict From e1b2a767f52e45d2c421cde5068c899437967db3 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Mon, 15 Feb 2021 17:49:25 +0100 Subject: [PATCH 06/23] =?UTF-8?q?N=C2=B03142=20-=20fix=20typos?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dictionaries/fr.dictionary.itop.core.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dictionaries/fr.dictionary.itop.core.php b/dictionaries/fr.dictionary.itop.core.php index b673a7178..0703c5e28 100644 --- a/dictionaries/fr.dictionary.itop.core.php +++ b/dictionaries/fr.dictionary.itop.core.php @@ -1043,9 +1043,9 @@ Dict::Add('FR FR', 'French', 'Français', array( 'Class:AsyncTask/Attribute:event_id+' => '', 'Class:AsyncTask/Attribute:finalclass' => 'Sous-classe de tâche asynchrone', 'Class:AsyncTask/Attribute:finalclass+' => '', - 'Class:AsyncTask/Attribute:status' => 'Status', + 'Class:AsyncTask/Attribute:status' => 'Statut', 'Class:AsyncTask/Attribute:status+' => '', - 'Class:AsyncTask/Attribute:remaining_retries' => 'Éssais restants', + 'Class:AsyncTask/Attribute:remaining_retries' => 'Essais restants', 'Class:AsyncTask/Attribute:remaining_retries+' => '', 'Class:AsyncTask/Attribute:last_error_code' => 'Dernier code d\'erreur', 'Class:AsyncTask/Attribute:last_error_code+' => '', From 0b95220d1b7417ef8e986ab775fb6f42ba4d3ed1 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Tue, 16 Feb 2021 09:46:04 +0100 Subject: [PATCH 07/23] =?UTF-8?q?N=C2=B03466=20-=20Add=20(missing)=20trans?= =?UTF-8?q?lations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dictionaries/en.dictionary.itop.ui.php | 2 ++ dictionaries/fr.dictionary.itop.ui.php | 2 ++ pages/UI.php | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/dictionaries/en.dictionary.itop.ui.php b/dictionaries/en.dictionary.itop.ui.php index 431360ec0..12c8a7815 100644 --- a/dictionaries/en.dictionary.itop.ui.php +++ b/dictionaries/en.dictionary.itop.ui.php @@ -1559,6 +1559,8 @@ When associated with a trigger, each action is given an "order" number, specifyi 'UI:Search:Criteria:Raw:Filtered' => 'Filtered', 'UI:Search:Criteria:Raw:FilteredOn' => 'Filtered on %1$s', + + 'UI:StateChanged' => 'State changed', )); // diff --git a/dictionaries/fr.dictionary.itop.ui.php b/dictionaries/fr.dictionary.itop.ui.php index 3b1dba5b4..208b749ce 100644 --- a/dictionaries/fr.dictionary.itop.ui.php +++ b/dictionaries/fr.dictionary.itop.ui.php @@ -1539,6 +1539,8 @@ Lors de l\'association à un déclencheur, on attribue à chaque action un numé 'UI:Search:Criteria:Raw:Filtered' => 'Filtré', 'UI:Search:Criteria:Raw:FilteredOn' => 'Filtré sur %1$s', + + 'UI:StateChanged' => 'Etat modifié', )); // diff --git a/pages/UI.php b/pages/UI.php index c93fc194d..cb66fb0e0 100644 --- a/pages/UI.php +++ b/pages/UI.php @@ -1210,7 +1210,7 @@ HTML $sOrigState = utils::ReadPostedParam('obj_state_orig', ''); if ($sTargetState != $sOrigState) { - $aWarnings[] = 'State changed'; + $aWarnings[] = Dict::S('UI:StateChanged'); } $oObj->Set($sStateAttCode, $sTargetState); } From 905ee19519d0cc44a282f5ed0d23c49572f7106e Mon Sep 17 00:00:00 2001 From: odain Date: Mon, 15 Feb 2021 18:36:27 +0100 Subject: [PATCH 08/23] =?UTF-8?q?N=C2=B03412=20-=20Command=20Injection=20v?= =?UTF-8?q?ulnerability=20in=20the=20Setup=20Wizard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/setuputils.class.inc.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/setup/setuputils.class.inc.php b/setup/setuputils.class.inc.php index 217c4f77d..b3727f207 100644 --- a/setup/setuputils.class.inc.php +++ b/setup/setuputils.class.inc.php @@ -554,6 +554,12 @@ class SetupUtils $aResult[] = new CheckResult(CheckResult::ERROR, "The PHP exec() function has been disabled on this server"); } + $sEscapedGraphvizPath = \escapeshellarg($sGraphvizPath); + if (!is_file($sEscapedGraphvizPath) || ! is_executable($sEscapedGraphvizPath)){ + //N°3412 avoid shell injection + return new CheckResult(CheckResult::WARNING, "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); + } + // availability of dot / dot.exe if (empty($sGraphvizPath)) { From e9cff0920bdebd340f50944af4ab005a53306ea9 Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 16 Feb 2021 17:12:41 +0100 Subject: [PATCH 09/23] =?UTF-8?q?N=C2=B03412=20-=20Command=20Injection=20v?= =?UTF-8?q?ulnerability=20in=20the=20Setup=20Wizard=20-=20fix=20test=20and?= =?UTF-8?q?=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/setuputils.class.inc.php | 14 +++---- test/setup/SetupUtilsTest.php | 67 ++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 test/setup/SetupUtilsTest.php diff --git a/setup/setuputils.class.inc.php b/setup/setuputils.class.inc.php index b3727f207..a5c7643b8 100644 --- a/setup/setuputils.class.inc.php +++ b/setup/setuputils.class.inc.php @@ -551,15 +551,17 @@ class SetupUtils SetupPage::log('Info - PHP functions disabled: '.implode(', ', $aDisabled)); if (in_array('exec', $aDisabled)) { - $aResult[] = new CheckResult(CheckResult::ERROR, "The PHP exec() function has been disabled on this server"); + return new CheckResult(CheckResult::ERROR, "The PHP exec() function has been disabled on this server"); } - $sEscapedGraphvizPath = \escapeshellarg($sGraphvizPath); - if (!is_file($sEscapedGraphvizPath) || ! is_executable($sEscapedGraphvizPath)){ + clearstatcache(); + if (!is_file($sGraphvizPath) || ! is_executable($sGraphvizPath)){ //N°3412 avoid shell injection - return new CheckResult(CheckResult::WARNING, "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); + return new CheckResult(CheckResult::ERROR, "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); } + $sGraphvizPath = escapeshellcmd($sGraphvizPath); + // availability of dot / dot.exe if (empty($sGraphvizPath)) { @@ -574,10 +576,6 @@ class SetupUtils { $oResult = new CheckResult(CheckResult::INFO, "dot is present: ".$aOutput[0]); } - elseif ($iRetCode == 1) - { - $oResult = new CheckResult(CheckResult::WARNING, "dot could not be found: ".implode(' ', $aOutput)." - Please make sure it is installed and in the path."); - } else { $oResult = new CheckResult(CheckResult::WARNING, "dot could not be executed (retcode=$iRetCode): Please make sure it is installed and in the path"); diff --git a/test/setup/SetupUtilsTest.php b/test/setup/SetupUtilsTest.php new file mode 100644 index 000000000..3ce4cee77 --- /dev/null +++ b/test/setup/SetupUtilsTest.php @@ -0,0 +1,67 @@ +assertEquals($iSeverity, $oCheck->iSeverity); + $this->assertContains($sLabel, $oCheck->sLabel); + } + + public function CheckGravitzProvider(){ + if (substr(PHP_OS,0,3) === 'WIN'){ + return []; + } + + return [ + "bash injection" => [ + "touch /tmp/toto", + 0, + "could not be executed: Please make sure it is installed and in the path", + ], + "command ok" => [ + "/usr/bin/whereis", + 2, + "", + ], + "command failed" => [ + "/bin/ls", + 1, + "dot could not be executed (retcode=2): Please make sure it is installed and in the path", + ] + ]; + } + + +} From 571520815a1ef42dd8c18cc4ef425ecd376782f3 Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 16 Feb 2021 17:25:45 +0100 Subject: [PATCH 10/23] =?UTF-8?q?N=C2=B03412=20-=20Command=20Injection=20v?= =?UTF-8?q?ulnerability=20in=20the=20Setup=20Wizard=20-=20include=20test?= =?UTF-8?q?=20to=20CI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/phpunit.xml.dist | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/phpunit.xml.dist b/test/phpunit.xml.dist index 2929c7e78..844d229aa 100644 --- a/test/phpunit.xml.dist +++ b/test/phpunit.xml.dist @@ -62,6 +62,9 @@ status + + setup/SetupUtilsTest.php + integration From dcd4abe72b603954bc8a6eb66d47363bdc446508 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Tue, 16 Feb 2021 17:33:04 +0100 Subject: [PATCH 11/23] =?UTF-8?q?N=C2=B03430=20-=20security=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../portal/src/Form/ObjectFormManager.php | 224 +++++++++--------- .../portal/src/Form/PasswordFormManager.php | 13 +- .../src/Form/PreferencesFormManager.php | 13 +- sources/form/formmanager.class.inc.php | 33 ++- 4 files changed, 159 insertions(+), 124 deletions(-) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php index 11c9f3995..b3fc3b883 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php @@ -1048,6 +1048,24 @@ class ObjectFormManager extends FormManager $this->CancelAttachments(); } + public function CheckTransaction($aData) + { + if (! utils::IsTransactionValid($this->oForm->GetTransactionId())) { + if ($this->oObject->IsNew()) { + $sError = Dict::S('UI:Error:ObjectAlreadyCreated'); + } else { + $sError = Dict::S('UI:Error:ObjectAlreadyUpdated'); + } + + $aData['messages']['error'] += [ + '_main' => [$sError] + ]; + $aData['valid'] = false; + } + + return $aData; + } + /** * Validates the form and returns an array with the validation status and the messages. * If the form is valid, creates/updates the object. @@ -1070,124 +1088,116 @@ class ObjectFormManager extends FormManager */ public function OnSubmit($aArgs = null) { - $aData = array( - 'valid' => true, - 'messages' => array( - 'success' => array(), - 'warnings' => array(), // Not used as of today, just to show that the structure is ready for change like this. - 'error' => array(), - ), - ); + $aData = parent::OnSubmit($aArgs); + + if (! $aData['valid']) { + return $aData; + } // Update object and form $this->OnUpdate($aArgs); // Check if form valid - if ($this->oForm->Validate()) - { - // The try catch is essentially to start a MySQL transaction in order to ensure that all or none objects are persisted when creating an object with links - try - { - $sObjectClass = get_class($this->oObject); - - // Starting transaction - CMDBSource::Query('START TRANSACTION'); - // Forcing allowed writing on the object if necessary. This is used in some particular cases. - $bAllowWrite = ($sObjectClass === 'Person' && $this->oObject->GetKey() == UserRights::GetContactId()); - if ($bAllowWrite) - { - $this->oObject->AllowWrite(true); - } - - // Writing object to DB - $bActivateTriggers = (!$this->oObject->IsNew() && $this->oObject->IsModified()); - $bWasModified = $this->oObject->IsModified(); - try - { - $this->oObject->DBWrite(); - } - catch (CoreCannotSaveObjectException $e) - { - throw new Exception($e->getHtmlMessage()); - } - // Finalizing images link to object, otherwise it will be cleaned by the GC - InlineImage::FinalizeInlineImages($this->oObject); - // Finalizing attachments link to object - // TODO : This has to be refactored when the function from itop-attachments has been migrated into the core - if (isset($aArgs['attachmentIds'])) - { - $this->FinalizeAttachments($aArgs['attachmentIds']); - } - - // Ending transaction with a commit as everything was fine - CMDBSource::Query('COMMIT'); - - // Checking if we have to apply a stimulus - if (isset($aArgs['applyStimulus'])) - { - $this->oObject->ApplyStimulus($aArgs['applyStimulus']['code']); - } - // Activating triggers only on update - if ($bActivateTriggers) - { - $sTriggersQuery = $this->oContainer->getParameter('combodo.portal.instance.conf')['properties']['triggers_query']; - if ($sTriggersQuery !== null) - { - $aParentClasses = MetaModel::EnumParentClasses($sObjectClass, ENUM_PARENT_CLASSES_ALL); - $oTriggerSet = new DBObjectSet(DBObjectSearch::FromOQL($sTriggersQuery), array(), - array('parent_classes' => $aParentClasses)); - /** @var \Trigger $oTrigger */ - while ($oTrigger = $oTriggerSet->Fetch()) - { - try - { - $oTrigger->DoActivate($this->oObject->ToArgs('this')); - } - catch(Exception $e) - { - utils::EnrichRaisedException($oTrigger, $e); - } - } - } - } - - // Resetting caselog fields value, otherwise the value will stay in it after submit. - $this->oForm->ResetCaseLogFields(); - - if ($bWasModified) - { - //=if (isNew) because $bActivateTriggers = (!$this->oObject->IsNew() && $this->oObject->IsModified()) - if(!$bActivateTriggers) - { - $aData['messages']['success'] += array( '_main' => array(Dict::Format('UI:Title:Object_Of_Class_Created', $this->oObject->GetName(),MetaModel::GetName(get_class($this->oObject))))); - } - else - { - $aData['messages']['success'] += array('_main' => array(Dict::Format('UI:Class_Object_Updated', MetaModel::GetName(get_class($this->oObject)), $this->oObject->GetName()))); - } - } - } - catch (Exception $e) - { - // End transaction with a rollback as something failed - CMDBSource::Query('ROLLBACK'); - $aData['valid'] = false; - $aData['messages']['error'] += array('_main' => array($e->getMessage())); - IssueLog::Error(__METHOD__.' at line '.__LINE__.' : Rollback during submit ('.$e->getMessage().')'); - } - finally - { - // Removing transaction id - utils::RemoveTransaction($this->oForm->GetTransactionId()); - } - } - else + if (! $this->oForm->Validate()) { // Handle errors $aData['valid'] = false; $aData['messages']['error'] += $this->oForm->GetErrorMessages(); + return $aData; } + // The try catch is essentially to start a MySQL transaction in order to ensure that all or none objects are persisted when creating an object with links + try + { + $sObjectClass = get_class($this->oObject); + + // Starting transaction + CMDBSource::Query('START TRANSACTION'); + // Forcing allowed writing on the object if necessary. This is used in some particular cases. + $bAllowWrite = ($sObjectClass === 'Person' && $this->oObject->GetKey() == UserRights::GetContactId()); + if ($bAllowWrite) + { + $this->oObject->AllowWrite(true); + } + + // Writing object to DB + $bActivateTriggers = (!$this->oObject->IsNew() && $this->oObject->IsModified()); + $bWasModified = $this->oObject->IsModified(); + try + { + $this->oObject->DBWrite(); + } + catch (CoreCannotSaveObjectException $e) + { + throw new Exception($e->getHtmlMessage()); + } + // Finalizing images link to object, otherwise it will be cleaned by the GC + InlineImage::FinalizeInlineImages($this->oObject); + // Finalizing attachments link to object + // TODO : This has to be refactored when the function from itop-attachments has been migrated into the core + if (isset($aArgs['attachmentIds'])) + { + $this->FinalizeAttachments($aArgs['attachmentIds']); + } + + // Ending transaction with a commit as everything was fine + CMDBSource::Query('COMMIT'); + + // Checking if we have to apply a stimulus + if (isset($aArgs['applyStimulus'])) + { + $this->oObject->ApplyStimulus($aArgs['applyStimulus']['code']); + } + // Activating triggers only on update + if ($bActivateTriggers) + { + $sTriggersQuery = $this->oContainer->getParameter('combodo.portal.instance.conf')['properties']['triggers_query']; + if ($sTriggersQuery !== null) + { + $aParentClasses = MetaModel::EnumParentClasses($sObjectClass, ENUM_PARENT_CLASSES_ALL); + $oTriggerSet = new DBObjectSet(DBObjectSearch::FromOQL($sTriggersQuery), array(), + array('parent_classes' => $aParentClasses)); + /** @var \Trigger $oTrigger */ + while ($oTrigger = $oTriggerSet->Fetch()) + { + try + { + $oTrigger->DoActivate($this->oObject->ToArgs('this')); + } + catch(Exception $e) + { + utils::EnrichRaisedException($oTrigger, $e); + } + } + } + } + + // Resetting caselog fields value, otherwise the value will stay in it after submit. + $this->oForm->ResetCaseLogFields(); + + if ($bWasModified) + { + //=if (isNew) because $bActivateTriggers = (!$this->oObject->IsNew() && $this->oObject->IsModified()) + if(!$bActivateTriggers) + { + $aData['messages']['success'] += array( '_main' => array(Dict::Format('UI:Title:Object_Of_Class_Created', $this->oObject->GetName(),MetaModel::GetName(get_class($this->oObject))))); + } + else + { + $aData['messages']['success'] += array('_main' => array(Dict::Format('UI:Class_Object_Updated', MetaModel::GetName(get_class($this->oObject)), $this->oObject->GetName()))); + } + } + } + catch (Exception $e) + { + // End transaction with a rollback as something failed + CMDBSource::Query('ROLLBACK'); + $aData['valid'] = false; + $aData['messages']['error'] += array('_main' => array($e->getMessage())); + IssueLog::Error(__METHOD__.' at line '.__LINE__.' : Rollback during submit ('.$e->getMessage().')'); + } + + return $aData; } diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php index 98508b172..8daccbfe0 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php @@ -93,14 +93,11 @@ class PasswordFormManager extends FormManager */ public function OnSubmit($aArgs = null) { - $aData = array( - 'valid' => true, - 'messages' => array( - 'success' => array(), - 'warnings' => array(), // Not used as of today, just to show that the structure is ready for change like this. - 'error' => array(), - ), - ); + $aData = parent::OnSubmit($aArgs); + + if (! $aData['valid']) { + return $aData; + } // Update object and form $this->OnUpdate($aArgs); diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php index b64388bd2..ca7303837 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php @@ -97,14 +97,11 @@ class PreferencesFormManager extends FormManager */ public function OnSubmit($aArgs = null) { - $aData = array( - 'valid' => true, - 'messages' => array( - 'success' => array(), - 'warnings' => array(), // Not used as of today, just to show that the structure is ready for change like this. - 'error' => array(), - ), - ); + $aData = parent::OnSubmit($aArgs); + + if (! $aData['valid']) { + return $aData; + } // Update object and form $this->OnUpdate($aArgs); diff --git a/sources/form/formmanager.class.inc.php b/sources/form/formmanager.class.inc.php index 3b34e089a..9dc89e5c6 100644 --- a/sources/form/formmanager.class.inc.php +++ b/sources/form/formmanager.class.inc.php @@ -162,7 +162,38 @@ abstract class FormManager * * @return mixed */ - abstract public function OnSubmit($aArgs = null); + public function OnSubmit($aArgs = null) + { + $aData = array( + 'valid' => true, + 'messages' => array( + 'success' => array(), + 'warnings' => array(), // Not used as of today, just to show that the structure is ready for change like this. + 'error' => array(), + ), + ); + + $aData = $this->CheckTransaction($aData); + + return $aData; + } + + /** + * @param $aData + * + * @return array + */ + public function CheckTransaction($aData) + { + if (! \utils::IsTransactionValid($this->oForm->GetTransactionId())) { + $aData['messages']['error'] += [ + '_main' => [\Dict::S('UI:Error:InvalidToken')] //This message is generic, if you override this method you should use a more precise message. @see \Combodo\iTop\Portal\Form\ObjectFormManager::CheckTransaction + ]; + $aData['valid'] = false; + } + + return $aData; + } /** * @param array|null $aArgs From 83434b55066df49aaa3a822acf716884283a8aac Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Tue, 16 Feb 2021 17:42:11 +0100 Subject: [PATCH 12/23] =?UTF-8?q?N=C2=B03430=20-=20add=20translations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dictionaries/en.dictionary.itop.ui.php | 1 + dictionaries/fr.dictionary.itop.ui.php | 1 + 2 files changed, 2 insertions(+) diff --git a/dictionaries/en.dictionary.itop.ui.php b/dictionaries/en.dictionary.itop.ui.php index 12c8a7815..66c9c78bb 100644 --- a/dictionaries/en.dictionary.itop.ui.php +++ b/dictionaries/en.dictionary.itop.ui.php @@ -465,6 +465,7 @@ Dict::Add('EN US', 'English', 'English', array( 'UI:Error:InvalidDashboard' => 'Error: invalid dashboard', 'UI:Error:MaintenanceMode' => 'Application is currently in maintenance', 'UI:Error:MaintenanceTitle' => 'Maintenance', + 'UI:Error:InvalidToken' => 'Error: the requested operation have already been performed (CSRF token not found)', 'UI:GroupBy:Count' => 'Count', 'UI:GroupBy:Count+' => 'Number of elements', diff --git a/dictionaries/fr.dictionary.itop.ui.php b/dictionaries/fr.dictionary.itop.ui.php index 208b749ce..db43f78bf 100644 --- a/dictionaries/fr.dictionary.itop.ui.php +++ b/dictionaries/fr.dictionary.itop.ui.php @@ -448,6 +448,7 @@ Dict::Add('FR FR', 'French', 'Français', array( 'UI:Error:InvalidDashboard' => 'Erreur: Le tableau de bord est invalide', 'UI:Error:MaintenanceMode' => 'L\'application est en maintenance', 'UI:Error:MaintenanceTitle' => 'Maintenance', + 'UI:Error:InvalidToken' => 'Erreur: l\'opération a déjà été effectuée (CSRF token not found)', 'UI:GroupBy:Count' => 'Nombre', 'UI:GroupBy:Count+' => 'Nombre d\'éléments', From a12959d60e311651d18871848598d7f0cff6eaa3 Mon Sep 17 00:00:00 2001 From: odain Date: Wed, 17 Feb 2021 07:50:10 +0100 Subject: [PATCH 13/23] =?UTF-8?q?N=C2=B03412=20-=20Command=20Injection=20v?= =?UTF-8?q?ulnerability=20in=20the=20Setup=20Wizard=20-=20handle=20empty?= =?UTF-8?q?=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/setuputils.class.inc.php | 18 ++++++++++-------- test/setup/SetupUtilsTest.php | 5 +++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/setup/setuputils.class.inc.php b/setup/setuputils.class.inc.php index a5c7643b8..9413636bc 100644 --- a/setup/setuputils.class.inc.php +++ b/setup/setuputils.class.inc.php @@ -554,19 +554,21 @@ class SetupUtils return new CheckResult(CheckResult::ERROR, "The PHP exec() function has been disabled on this server"); } - clearstatcache(); - if (!is_file($sGraphvizPath) || ! is_executable($sGraphvizPath)){ - //N°3412 avoid shell injection - return new CheckResult(CheckResult::ERROR, "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); - } - - $sGraphvizPath = escapeshellcmd($sGraphvizPath); - // availability of dot / dot.exe if (empty($sGraphvizPath)) { $sGraphvizPath = 'dot'; + } else { + clearstatcache(); + if (!is_file($sGraphvizPath) || !is_executable($sGraphvizPath)) { + //N°3412 avoid shell injection + return new CheckResult(CheckResult::ERROR, + "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); + } + + $sGraphvizPath = escapeshellcmd($sGraphvizPath); } + $sCommand = "\"$sGraphvizPath\" -V 2>&1"; $aOutput = array(); diff --git a/test/setup/SetupUtilsTest.php b/test/setup/SetupUtilsTest.php index 3ce4cee77..efa110ec1 100644 --- a/test/setup/SetupUtilsTest.php +++ b/test/setup/SetupUtilsTest.php @@ -55,6 +55,11 @@ class SetupUtilsTest extends ItopTestCase 2, "", ], + "empty command => dot by default" => [ + "", + 2, + "", + ], "command failed" => [ "/bin/ls", 1, From bb877a244be035235834cbd45009867ef913cf60 Mon Sep 17 00:00:00 2001 From: odain Date: Wed, 17 Feb 2021 10:09:39 +0100 Subject: [PATCH 14/23] =?UTF-8?q?N=C2=B03412=20-=20Command=20Injection=20v?= =?UTF-8?q?ulnerability=20in=20the=20Setup=20Wizard=20-=20do=20not=20use?= =?UTF-8?q?=20escapeshellcmd=20before=20execution=20in=20Windows=20envt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/utils.inc.php | 8 ++++++++ setup/setuputils.class.inc.php | 4 +++- test/setup/SetupUtilsTest.php | 4 ++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/application/utils.inc.php b/application/utils.inc.php index ec5b43ab5..738971a60 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -2337,4 +2337,12 @@ class utils $e = new CoreException($sMessage, null, '', $oException); throw $e; } + + /** + * @return bool : indicate whether we run under a windows environnement or not + * @since 2.7.4 : N°3412 + */ + public static function IsWindows(){ + return (substr(PHP_OS,0,3) === 'WIN'); + } } diff --git a/setup/setuputils.class.inc.php b/setup/setuputils.class.inc.php index 9413636bc..23f1d8692 100644 --- a/setup/setuputils.class.inc.php +++ b/setup/setuputils.class.inc.php @@ -566,7 +566,9 @@ class SetupUtils "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); } - $sGraphvizPath = escapeshellcmd($sGraphvizPath); + if (!utils::IsWindows()){ + $sGraphvizPath = escapeshellcmd($sGraphvizPath); + } } $sCommand = "\"$sGraphvizPath\" -V 2>&1"; diff --git a/test/setup/SetupUtilsTest.php b/test/setup/SetupUtilsTest.php index efa110ec1..14e996a8e 100644 --- a/test/setup/SetupUtilsTest.php +++ b/test/setup/SetupUtilsTest.php @@ -30,7 +30,7 @@ class SetupUtilsTest extends ItopTestCase } /** - * @dataProvider CheckGravitzProvider + * @dataProvider CheckGravitProvider */ public function testCheckGravitz($sScriptPath, $iSeverity, $sLabel){ /** @var \CheckResult $oCheck */ @@ -39,7 +39,7 @@ class SetupUtilsTest extends ItopTestCase $this->assertContains($sLabel, $oCheck->sLabel); } - public function CheckGravitzProvider(){ + public function CheckGravitProvider(){ if (substr(PHP_OS,0,3) === 'WIN'){ return []; } From 82ba7f25b0ad8f9a9ab92ae443c6568543deb5ef Mon Sep 17 00:00:00 2001 From: odain Date: Wed, 17 Feb 2021 10:18:28 +0100 Subject: [PATCH 15/23] =?UTF-8?q?N=C2=B03412=20-=20Command=20Injection=20v?= =?UTF-8?q?ulnerability=20in=20the=20Setup=20Wizard=20-=20do=20not=20use?= =?UTF-8?q?=20escapeshellcmd=20before=20execution=20in=20Windows=20envt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/utils.inc.php | 2 +- setup/setuputils.class.inc.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/application/utils.inc.php b/application/utils.inc.php index 738971a60..92915bfc2 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -2342,7 +2342,7 @@ class utils * @return bool : indicate whether we run under a windows environnement or not * @since 2.7.4 : N°3412 */ - public static function IsWindows(){ + public static function IsWindowsEnvironment(){ return (substr(PHP_OS,0,3) === 'WIN'); } } diff --git a/setup/setuputils.class.inc.php b/setup/setuputils.class.inc.php index 23f1d8692..fc69f16bb 100644 --- a/setup/setuputils.class.inc.php +++ b/setup/setuputils.class.inc.php @@ -566,7 +566,7 @@ class SetupUtils "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); } - if (!utils::IsWindows()){ + if (!utils::IsWindowsEnvironment()){ $sGraphvizPath = escapeshellcmd($sGraphvizPath); } } From 913ea0cef22b39c9539346c13add89853be559f5 Mon Sep 17 00:00:00 2001 From: odain Date: Wed, 17 Feb 2021 10:22:21 +0100 Subject: [PATCH 16/23] =?UTF-8?q?N=C2=B03412=20-=20Command=20Injection=20v?= =?UTF-8?q?ulnerability=20in=20the=20Setup=20Wizard=20-=20renaming?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/setup/SetupUtilsTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/setup/SetupUtilsTest.php b/test/setup/SetupUtilsTest.php index 14e996a8e..6ccae4a86 100644 --- a/test/setup/SetupUtilsTest.php +++ b/test/setup/SetupUtilsTest.php @@ -30,16 +30,16 @@ class SetupUtilsTest extends ItopTestCase } /** - * @dataProvider CheckGravitProvider + * @dataProvider CheckGravizProvider */ - public function testCheckGravitz($sScriptPath, $iSeverity, $sLabel){ + public function testCheckGraviz($sScriptPath, $iSeverity, $sLabel){ /** @var \CheckResult $oCheck */ $oCheck = SetupUtils::CheckGraphviz($sScriptPath); $this->assertEquals($iSeverity, $oCheck->iSeverity); $this->assertContains($sLabel, $oCheck->sLabel); } - public function CheckGravitProvider(){ + public function CheckGravizProvider(){ if (substr(PHP_OS,0,3) === 'WIN'){ return []; } From 9d2fc883b84b61bf93e3fbb1f7fcf77a87a2a9d4 Mon Sep 17 00:00:00 2001 From: Molkobain Date: Wed, 17 Feb 2021 10:31:59 +0100 Subject: [PATCH 17/23] Fix test name --- test/setup/SetupUtilsTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/setup/SetupUtilsTest.php b/test/setup/SetupUtilsTest.php index 6ccae4a86..47a996bb7 100644 --- a/test/setup/SetupUtilsTest.php +++ b/test/setup/SetupUtilsTest.php @@ -30,16 +30,16 @@ class SetupUtilsTest extends ItopTestCase } /** - * @dataProvider CheckGravizProvider + * @dataProvider CheckGraphvizProvider */ - public function testCheckGraviz($sScriptPath, $iSeverity, $sLabel){ + public function testCheckGraphviz($sScriptPath, $iSeverity, $sLabel){ /** @var \CheckResult $oCheck */ $oCheck = SetupUtils::CheckGraphviz($sScriptPath); $this->assertEquals($iSeverity, $oCheck->iSeverity); $this->assertContains($sLabel, $oCheck->sLabel); } - public function CheckGravizProvider(){ + public function CheckGraphvizProvider(){ if (substr(PHP_OS,0,3) === 'WIN'){ return []; } From e9e18513be4768a85e4f9722b965c14c472707bf Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Thu, 18 Feb 2021 12:18:38 +0100 Subject: [PATCH 18/23] =?UTF-8?q?N=C2=B03430=20-=20fix=20preference=20page?= =?UTF-8?q?'s=20warning=20and=20add=20missing=20token=20generation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - fix the warning (ajax call interrupted) if preference form ajax call is way faster than the one of the 2 other by adding a new timeout_duration option before the redirect. --- .../portal/public/js/portal_form_handler.js | 20 ++++++++++++++----- .../Controller/UserProfileBrickController.php | 1 + .../portal/src/Form/ObjectFormManager.php | 3 ++- .../portal/src/Form/PasswordFormManager.php | 2 ++ .../src/Form/PreferencesFormManager.php | 2 ++ .../bricks/user-profile/layout.html.twig | 10 ++-------- sources/form/formmanager.class.inc.php | 11 +++++++--- 7 files changed, 32 insertions(+), 17 deletions(-) diff --git a/datamodels/2.x/itop-portal-base/portal/public/js/portal_form_handler.js b/datamodels/2.x/itop-portal-base/portal/public/js/portal_form_handler.js index 6fe572daa..e9c3d1eee 100644 --- a/datamodels/2.x/itop-portal-base/portal/public/js/portal_form_handler.js +++ b/datamodels/2.x/itop-portal-base/portal/public/js/portal_form_handler.js @@ -33,6 +33,7 @@ $(function() category: 'redirect', url: null, modal: false, + timeout_duration: 400, }, cancel_rule: { category: 'close', @@ -56,7 +57,7 @@ $(function() this.options.submit_rule.url = this.options.submit_url; if((this.options.cancel_url !== null) && (this.options.cancel_url !== '')) this.options.cancel_rule.url = this.options.cancel_url; - + this._super(); }, @@ -143,12 +144,14 @@ $(function() // Determine where we go in case validation is successful var sRuleType = me.options.submit_rule.category; var bRedirectInModal = me.options.submit_rule.modal; + var iRedirectTimeout = me.options.submit_rule.timeout_duration; var sRedirectUrl = me.options.submit_rule.url; // - The validation might want us to be redirect elsewhere if(oValidation.valid) { // Checking if we have to redirect to another page // Typically this happens when applying a stimulus, we redirect to the transition form + // This code let the ajax response override the initial parameters if(oValidation.redirection !== undefined) { var oRedirection = oValidation.redirection; @@ -160,6 +163,10 @@ $(function() { sRedirectUrl = oRedirection.url; } + if(oRedirection.timeout_duration !== undefined) + { + iRedirectTimeout = oRedirection.timeout_duration; + } sRuleType = 'redirect'; } } @@ -241,7 +248,7 @@ $(function() // Checking if we have to redirect to another page if(sRuleType === 'redirect') { - me._applyRedirectRule(sRedirectUrl, bRedirectInModal); + me._applyRedirectRule(sRedirectUrl, bRedirectInModal, iRedirectTimeout); } // Close rule only needs to be applied to non modal forms (modal is always closed on submit) else if(sRuleType === 'close') @@ -360,10 +367,13 @@ $(function() { $('#page_overlay').fadeOut(200); }, - _applyRedirectRule: function(sRedirectUrl, bRedirectInModal) + _applyRedirectRule: function(sRedirectUrl, bRedirectInModal, iRedirectTimeout) { var me = this; + //optional argument + iRedirectTimeout = (typeof iRedirectTimeout !== 'undefined' && iRedirectTimeout != null ) ? iRedirectTimeout : 400; + // Always close current modal if(this.options.is_modal) { @@ -391,8 +401,8 @@ $(function() // Showing loader while redirecting, otherwise user tend to click somewhere in the page. // Note: We use a timeout because .always() is called right after here and will hide the loader setTimeout(function(){ me._disableFormBeforeLoading(); }, 50); - // Redirecting after a few ms so the user can see what happend - setTimeout(function() { location.href = sRedirectUrl; }, 400); + // Redirecting after a few ms so the user can see what happened + setTimeout(function() { location.href = sRedirectUrl; }, iRedirectTimeout); } } }, diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/UserProfileBrickController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/UserProfileBrickController.php index 5118c492a..2e30da41d 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/UserProfileBrickController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/UserProfileBrickController.php @@ -211,6 +211,7 @@ class UserProfileBrickController extends BrickController { $aFormData['validation']['redirection'] = array( 'url' => $oUrlGenerator->generate('p_user_profile_brick'), + 'timeout_duration' => 1000, //since there are several ajax request, we use a longer timeout in hope that they will all be finished in time. A promise would have been more reliable, but since this change is made in a minor version, this approach is less error prone. ); } } diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php index b3fc3b883..5335b6d28 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php @@ -1050,7 +1050,8 @@ class ObjectFormManager extends FormManager public function CheckTransaction($aData) { - if (! utils::IsTransactionValid($this->oForm->GetTransactionId())) { + $isTransactionValid = \utils::IsTransactionValid($this->oForm->GetTransactionId(), false); //The transaction token is kept in order to preserve BC with ajax forms (the second call would fail if the token is deleted). (The GC will take care of cleaning the token for us later on) + if (!$isTransactionValid) { if ($this->oObject->IsNew()) { $sError = Dict::S('UI:Error:ObjectAlreadyCreated'); } else { diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php index 8daccbfe0..dd06311a1 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/PasswordFormManager.php @@ -48,6 +48,8 @@ class PasswordFormManager extends FormManager // Building the form $oForm = new Form('change_password'); + $oForm->SetTransactionId(\utils::GetNewTransactionId()); + // Adding hidden field with form type $oField = new HiddenField('form_type'); $oField->SetCurrentValue('change_password'); diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php index ca7303837..2b3c6b8a4 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/PreferencesFormManager.php @@ -49,6 +49,8 @@ class PreferencesFormManager extends FormManager // Building the form $oForm = new Form('preferences'); + $oForm->SetTransactionId(\utils::GetNewTransactionId()); + // Adding hidden field with form type $oField = new HiddenField('form_type'); $oField->SetCurrentValue('preferences'); diff --git a/datamodels/2.x/itop-portal-base/portal/templates/bricks/user-profile/layout.html.twig b/datamodels/2.x/itop-portal-base/portal/templates/bricks/user-profile/layout.html.twig index 445ed5a3b..e482db0b6 100644 --- a/datamodels/2.x/itop-portal-base/portal/templates/bricks/user-profile/layout.html.twig +++ b/datamodels/2.x/itop-portal-base/portal/templates/bricks/user-profile/layout.html.twig @@ -227,16 +227,10 @@ $('#user-profile-wrapper .form_field .help-block > p').remove(); // Submiting contact form through AJAX - //if($('#{{ oContactForm.id }} .field_set').field_set('hasTouchedFields')) - //{ - $('#{{ oContactForm.id }}').portal_form_handler('submit', oEvent); - //} + $('#{{ oContactForm.id }}').portal_form_handler('submit', oEvent); // Submiting preferences form through AJAX - //if($('#{{ oPreferencesForm.id }} .field_set').field_set('hasTouchedFields')) - //{ - $('#{{ oPreferencesForm.id }}').portal_form_handler('submit', oEvent); - //} + $('#{{ oPreferencesForm.id }}').portal_form_handler('submit', oEvent); {% if oPasswordForm is not null %} // Submiting password form through AJAX diff --git a/sources/form/formmanager.class.inc.php b/sources/form/formmanager.class.inc.php index 9dc89e5c6..cb6e15f2d 100644 --- a/sources/form/formmanager.class.inc.php +++ b/sources/form/formmanager.class.inc.php @@ -160,7 +160,9 @@ abstract class FormManager /** * @param array|null $aArgs * - * @return mixed + * @return array + * + * @since 2.7.4 3.0.0 N°3430 */ public function OnSubmit($aArgs = null) { @@ -179,13 +181,16 @@ abstract class FormManager } /** - * @param $aData + * @param array $aData * * @return array + * + * @since 2.7.4 3.0.0 N°3430 */ public function CheckTransaction($aData) { - if (! \utils::IsTransactionValid($this->oForm->GetTransactionId())) { + $isTransactionValid = \utils::IsTransactionValid($this->oForm->GetTransactionId(), false); //The transaction token is kept in order to preserve BC with ajax forms (the second call would fail if the token is deleted). (The GC will take care of cleaning the token for us later on) + if (!$isTransactionValid) { $aData['messages']['error'] += [ '_main' => [\Dict::S('UI:Error:InvalidToken')] //This message is generic, if you override this method you should use a more precise message. @see \Combodo\iTop\Portal\Form\ObjectFormManager::CheckTransaction ]; From 6e0af1a3b73b185c3bac9738f6225ae7dc623ae0 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Thu, 18 Feb 2021 13:21:06 +0100 Subject: [PATCH 19/23] :bulb: Add variable typing --- application/cmdbabstract.class.inc.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/application/cmdbabstract.class.inc.php b/application/cmdbabstract.class.inc.php index 61777fe51..6ff969b78 100644 --- a/application/cmdbabstract.class.inc.php +++ b/application/cmdbabstract.class.inc.php @@ -3256,16 +3256,15 @@ EOF */ public function DisplayDocumentInline(WebPage $oPage, $sAttCode) { + /** @var \ormDocument $oDoc */ $oDoc = $this->Get($sAttCode); $sClass = get_class($this); $Id = $this->GetKey(); - switch ($oDoc->GetMainMimeType()) - { + switch ($oDoc->GetMainMimeType()) { case 'text': case 'html': $data = $oDoc->GetData(); - switch ($oDoc->GetMimeType()) - { + switch ($oDoc->GetMimeType()) { case 'text/xml': $oPage->add("\n"); break; From c31df5fff3ad81bacbb421f5cc1ec5251139b3c5 Mon Sep 17 00:00:00 2001 From: odain Date: Thu, 18 Feb 2021 16:07:37 +0100 Subject: [PATCH 20/23] fix ci: adapt test to make sure config date_and_time is set properly before --- test/application/search/CriterionConversionTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/application/search/CriterionConversionTest.php b/test/application/search/CriterionConversionTest.php index 9fb9f1ed9..0f48c986e 100644 --- a/test/application/search/CriterionConversionTest.php +++ b/test/application/search/CriterionConversionTest.php @@ -537,10 +537,10 @@ class CriterionConversionTest extends ItopDataTestCase */ function testOqlToForSearchToOqlAltLanguageEN($sOQL, $sExpectedOQL, $aExpectedCriterion) { + \MetaModel::GetConfig()->Set('date_and_time_format', array('default' => array('date' => 'Y-m-d', 'time' => 'H:i:s', 'date_time' => '$date $time'))); $this->OqlToSearchToOqlAltLanguage($sOQL, $sExpectedOQL, $aExpectedCriterion, "EN US"); } - function OqlProviderDates() { return array( From 46f9fe743c6b37903e392daefaeeb01640893f78 Mon Sep 17 00:00:00 2001 From: odain Date: Thu, 18 Feb 2021 16:27:38 +0100 Subject: [PATCH 21/23] fix ci: adapt test to make sure config date_and_time is set properly before --- test/application/search/CriterionConversionTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/application/search/CriterionConversionTest.php b/test/application/search/CriterionConversionTest.php index 0f48c986e..a3d988d0f 100644 --- a/test/application/search/CriterionConversionTest.php +++ b/test/application/search/CriterionConversionTest.php @@ -517,7 +517,8 @@ class CriterionConversionTest extends ItopDataTestCase */ function testOqlToForSearchToOqlAltLanguageFR($sOQL, $sExpectedOQL, $aExpectedCriterion) { - $this->OqlToSearchToOqlAltLanguage($sOQL, $sExpectedOQL, $aExpectedCriterion, "FR FR"); + \MetaModel::GetConfig()->Set('date_and_time_format', array('default' => array('date' => 'Y-m-d', 'time' => 'H:i:s', 'date_time' => '$date $time'))); + $this->OqlToSearchToOqlAltLanguage($sOQL, $sExpectedOQL, $aExpectedCriterion, "FR FR"); } From 74246a8278b2f1569b21bce67ffd9992b3356dc8 Mon Sep 17 00:00:00 2001 From: odain Date: Thu, 18 Feb 2021 18:24:01 +0100 Subject: [PATCH 22/23] =?UTF-8?q?N=C2=B03065=20-=20Failed=20enum=20compari?= =?UTF-8?q?son=20when=20values=20contains=20parenthesis=20-=20enhance=20db?= =?UTF-8?q?=20model=20parsing=20used=20during=20setup=20comparison=20with?= =?UTF-8?q?=20expected=20one=20to=20generate=20SQL=20migration=20queries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/cmdbsource.class.inc.php | 41 ++++++++++++++++++++++++++++++++++- core/metamodel.class.php | 5 ----- test/core/CMDBSourceTest.php | 14 ++++++------ 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/core/cmdbsource.class.inc.php b/core/cmdbsource.class.inc.php index d7a48bf4c..4dcdbec7b 100644 --- a/core/cmdbsource.class.inc.php +++ b/core/cmdbsource.class.inc.php @@ -1181,7 +1181,8 @@ class CMDBSource } /** - * There may have some differences between DB : for example in MySQL 5.7 we have "INT", while in MariaDB >= 10.2 you get "int DEFAULT 'NULL'" + * There may have some differences between DB : for example in MySQL 5.7 we have "INT", while in MariaDB >= 10.2 you get "int DEFAULT + * 'NULL'" * * We still do a case sensitive comparison for enum values ! * @@ -1192,6 +1193,7 @@ class CMDBSource * @param string $sDbFieldType * * @return bool true if same type and options (case sensitive comparison only for type options), false otherwise + * @throws \CoreException * @since 2.7.0 N°2490 */ public static function IsSameFieldTypes($sItopGeneratedFieldType, $sDbFieldType) @@ -1245,18 +1247,55 @@ class CMDBSource * 1. data type : for example 'VARCHAR' * 2. type value : for example '255' * 3. other options : for example ' CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT 0' + * @throws \CoreException */ private static function GetFieldDataTypeAndOptions($sCompleteFieldType) { preg_match('/^([a-zA-Z]+)(\(([^\)]+)\))?( .+)?/', $sCompleteFieldType, $aMatches); $sDataType = isset($aMatches[1]) ? $aMatches[1] : ''; + + if (strcasecmp($sDataType, 'ENUM') === 0){ + return self::GetEnumOptions($sDataType, $sCompleteFieldType); + } + $sTypeOptions = isset($aMatches[2]) ? $aMatches[3] : ''; $sOtherOptions = isset($aMatches[4]) ? $aMatches[4] : ''; return array($sDataType, $sTypeOptions, $sOtherOptions); } + /** + * @since 2.7.4 N°3065 + * Handle ENUM options + * + * @param $sDataType + * @param $sCompleteFieldType + * Example: ENUM('CSP A','CSP (aaaa) M','NA','OEM(ROC)','OPEN(VL)','RETAIL (Boite)') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci + * + * @return string[] consisting of 3 items : + * 1. data type : ENUM or enum here + * 2. type value : in-between EUM parenthesis + * 3. other options : for example ' CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT 0' + * @throws \CoreException + */ + private static function GetEnumOptions($sDataType, $sCompleteFieldType) + { + $iFirstOpeningParenthesis = strpos($sCompleteFieldType, '('); + $iLastEndingParenthesis = strrpos($sCompleteFieldType, ')'); + + if ($iFirstOpeningParenthesis === false || $iLastEndingParenthesis === false ){ + //should never happen as GetFieldDataTypeAndOptions regexp matched. + //except if regexp is modiied/broken somehow one day... + throw new CoreException("GetEnumOptions issue with $sDataType parsing : " . $sCompleteFieldType); + } + + $sTypeOptions = substr($sCompleteFieldType, $iFirstOpeningParenthesis + 1, $iLastEndingParenthesis - 1); + $sOtherOptions = substr($sCompleteFieldType, $iLastEndingParenthesis + 1); + + return array($sDataType, $sTypeOptions, $sOtherOptions); + } + /** * @param string $sTable * @param string $sField diff --git a/core/metamodel.class.php b/core/metamodel.class.php index bf26e01a5..2dbeed108 100644 --- a/core/metamodel.class.php +++ b/core/metamodel.class.php @@ -5545,15 +5545,10 @@ abstract class MetaModel // The field already exists, does it have the relevant properties? // - $bToBeChanged = false; $sActualFieldSpec = CMDBSource::GetFieldSpec($sTable, $sField); if (!CMDBSource::IsSameFieldTypes($sDBFieldSpec, $sActualFieldSpec)) { - $bToBeChanged = true; $aErrors[$sClass][$sAttCode][] = "field '$sField' in table '$sTable' has a wrong type: found $sActualFieldSpec while expecting $sDBFieldSpec"; - } - if ($bToBeChanged) - { $aSugFix[$sClass][$sAttCode][] = "ALTER TABLE `$sTable` CHANGE `$sField` $sFieldDefinition"; $aAlterTableItems[$sTable][$sField] = "CHANGE `$sField` $sFieldDefinition"; } diff --git a/test/core/CMDBSourceTest.php b/test/core/CMDBSourceTest.php index db06932e1..132d71292 100644 --- a/test/core/CMDBSourceTest.php +++ b/test/core/CMDBSourceTest.php @@ -35,7 +35,7 @@ class CMDBSourceTest extends ItopTestCase */ public function testCompareFieldTypes($bResult, $sItopFieldType, $sDbFieldType) { - $this->assertEquals($bResult, CMDBSource::IsSameFieldTypes($sItopFieldType, $sDbFieldType)); + $this->assertEquals($bResult, CMDBSource::IsSameFieldTypes($sItopFieldType, $sDbFieldType), "$sItopFieldType\n VS\n $sDbFieldType"); } public function compareFieldTypesProvider() @@ -106,12 +106,12 @@ class CMDBSourceTest extends ItopTestCase "ENUM('CSP A','CSP M','NA','OEM(ROC)','OPEN(VL)','RETAIL (Boite)') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", "enum('CSP A','CSP M','NA','OEM(ROC)','OPEN(VL)','RETAIL (Boite)') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", ), -//FIXME N°3065 before the fix this returns true :( -// 'ENUM with different values, containing parenthesis' => array( -// false, -// "ENUM('value 1 (with parenthesis)','value 2') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", -// "enum('value 1 (with parenthesis)','value 3') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", -// ), + //FIXME N°3065 before the fix this returns true :( + 'ENUM with different values, containing parenthesis' => array( + false, + "ENUM('value 1 (with parenthesis)','value 2') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", + "enum('value 1 (with parenthesis)','value 3') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", + ), ); } } From 71c5f47cd80938f4434b4800752c9b574143f119 Mon Sep 17 00:00:00 2001 From: Molkobain Date: Fri, 19 Feb 2021 09:37:53 +0100 Subject: [PATCH 23/23] PHPDoc --- .../2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php index 5335b6d28..88d434496 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php @@ -1048,6 +1048,9 @@ class ObjectFormManager extends FormManager $this->CancelAttachments(); } + /** + * @inheritDoc + */ public function CheckTransaction($aData) { $isTransactionValid = \utils::IsTransactionValid($this->oForm->GetTransactionId(), false); //The transaction token is kept in order to preserve BC with ajax forms (the second call would fail if the token is deleted). (The GC will take care of cleaning the token for us later on)