diff --git a/.gitignore b/.gitignore index f0331232..4caec924 100644 --- a/.gitignore +++ b/.gitignore @@ -2,4 +2,4 @@ dist/ vendor/ .gh_token *.min.* - +.phpunit.result.cache diff --git a/CHANGELOG.md b/CHANGELOG.md index a51d093f..780eec26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fix repeated User updates failing with `Data too long for column cookie_token` by excluding token fields from update payloads (backport of PR #566 from GLPI 11-compatible line) - Fix various minor bugs in the import/export workflow (backport of PR #656 from GLPI 11-compatible line) - Move network port lookup query to the GLPI DBAL iterator (backport of PR #659 from GLPI 11-compatible line) +- Fix model selector validation and access control (backport of PR #660 from GLPI 11-compatible line) ## [2.14.4] - 2025-11-25 diff --git a/front/mapping.form.php b/front/mapping.form.php index 2d77a058..73fbd75d 100644 --- a/front/mapping.form.php +++ b/front/mapping.form.php @@ -36,9 +36,21 @@ if (isset($_POST["update"])) { $at_least_one_mandatory = false; $mapping = new PluginDatainjectionMapping(); + $existing = new PluginDatainjectionMapping(); + + $models_id = (int) ($_POST['models_id'] ?? 0); + $model = new PluginDatainjectionModel(); + $model->check($models_id, UPDATE); foreach ($_POST['data'] as $id => $mapping_infos) { - $mapping_infos['id'] = $id; + if ( + !$existing->getFromDB((int) $id) + || (int) $existing->fields['models_id'] !== $models_id + ) { + continue; + } + + $mapping_infos['id'] = (int) $id; //If no field selected, reset other values if ($mapping_infos['value'] == PluginDatainjectionInjectionType::NO_VALUE) { @@ -66,12 +78,9 @@ true ); } else { - $model = new PluginDatainjectionModel(); - $model->getFromDB($_POST['models_id']); - if ($model->fields['step'] != PluginDatainjectionModel::READY_TO_USE_STEP) { PluginDatainjectionModel::changeStep( - $_POST['models_id'], + $models_id, PluginDatainjectionModel::OTHERS_STEP ); Session::setActiveTab('PluginDatainjectionModel', 'PluginDatainjectionModel$5'); diff --git a/front/popup.php b/front/popup.php index 631a75e3..8fa0ee77 100644 --- a/front/popup.php +++ b/front/popup.php @@ -34,16 +34,20 @@ switch ($_GET["popup"]) { case "preview": + $models_id = (int) ($_GET['models_id'] ?? 0); $model = new PluginDatainjectionModel(); - $model->check($_GET['models_id'], READ); + $model->check($models_id, READ); Html::popHeader(__('See the file', 'datainjection'), $_SERVER['PHP_SELF']); - PluginDatainjectionModel::showPreviewMappings($_GET['models_id']); + PluginDatainjectionModel::showPreviewMappings($models_id); Html::popFooter(); break; case "log": + $models_id = (int) ($_GET['models_id'] ?? 0); + $model = new PluginDatainjectionModel(); + $model->check($models_id, READ); Html::popHeader(__('Data injection report', 'datainjection'), $_SERVER['PHP_SELF']); - PluginDatainjectionModel::showLogResults($_GET['models_id']); + PluginDatainjectionModel::showLogResults($models_id); Html::popFooter(); break; } diff --git a/inc/model.class.php b/inc/model.class.php index cb2e375e..c63d6723 100644 --- a/inc/model.class.php +++ b/inc/model.class.php @@ -99,7 +99,7 @@ public function canViewItem() return false; } - return self::checkRightOnModel($this->fields['id']); + return self::checkRightOnModel((int) ($this->fields['id'] ?? 0)); } @@ -120,7 +120,7 @@ public function canCreateItem() return false; } - return self::checkRightOnModel($this->fields['id']); + return self::checkRightOnModel((int) ($this->fields['id'] ?? 0)); } @@ -316,10 +316,10 @@ public static function dropdown($options = []) if ($model['entities_id'] == -1) { echo "\n"; } else { - echo "\n"; + ), ENT_QUOTES, 'UTF-8') . "\">"; } $prev = $model['entities_id']; } @@ -330,12 +330,11 @@ public static function dropdown($options = []) $selected = ""; } - if ($model['comment']) { - $comment = "title='" . htmlentities($model['comment'], ENT_QUOTES, 'UTF-8') . "'"; - } else { - $comment = ""; - } - echo "\n"; + $comment = $model['comment'] + ? "title='" . htmlentities((string) $model['comment'], ENT_QUOTES, 'UTF-8') . "'" + : ""; + echo "\n"; } if ($prev >= -1) { @@ -386,7 +385,7 @@ public static function getModels($user_id, $order = "name", $entity = -1, $all = foreach ($DB->request($query) as $data) { if ( - self::checkRightOnModel($data['id']) + self::checkRightOnModel((int) $data['id']) && class_exists($data['itemtype']) ) { $models[] = $data; @@ -955,7 +954,7 @@ public function prepareInputForAdd($input) return false; } - if (!$input['behavior_add'] && !$input['behavior_update']) { + if (!($input['behavior_add'] ?? 0) && !($input['behavior_update'] ?? 0)) { Session::addMessageAfterRedirect( __( 'Your model should allow import and/or update of data', @@ -1332,42 +1331,44 @@ public function populateSeveraltimesMappedFields() } - /** - * @param int $models_id - **/ - public static function checkRightOnModel($models_id) + public static function checkRightOnModel(int $models_id): bool { /** @var DBmysql $DB */ global $DB; - $continue = true; - $model = new self(); - if ($model->getFromDB($models_id)) { - $query = "(SELECT `itemtype` - FROM `glpi_plugin_datainjection_models` - WHERE `id` = '" . $models_id . "') - UNION (SELECT DISTINCT `itemtype` - FROM `glpi_plugin_datainjection_mappings` - WHERE `models_id` = '" . $models_id . "') - UNION (SELECT DISTINCT `itemtype` - FROM `glpi_plugin_datainjection_infos` - WHERE `models_id` = '" . $models_id . "')"; - foreach ($DB->request($query) as $data) { - if ($data['itemtype'] != PluginDatainjectionInjectionType::NO_VALUE) { - if (class_exists($data['itemtype'])) { - $item = new $data['itemtype'](); - $item->fields['itemtype'] = $model->fields['itemtype']; - - if (!($item instanceof CommonDBRelation) && !$item->canCreate()) { - $continue = false; - break; - } - } - } + if (!$model->getFromDB($models_id)) { + //New model being created: no injected itemtype to check yet + return true; + } + + $itemtypes = [$model->fields['itemtype']]; + + foreach (['glpi_plugin_datainjection_mappings', 'glpi_plugin_datainjection_infos'] as $table) { + $iterator = $DB->request([ + 'SELECT' => 'itemtype', + 'DISTINCT' => true, + 'FROM' => $table, + 'WHERE' => ['models_id' => $models_id], + ]); + + foreach ($iterator as $data) { + $itemtypes[] = $data['itemtype']; } } - return $continue; + + foreach (array_unique($itemtypes) as $itemtype) { + if ($itemtype == PluginDatainjectionInjectionType::NO_VALUE || !is_a($itemtype, CommonDBTM::class, true)) { + continue; + } + + $item = new $itemtype(); + if (!$item->canCreate()) { + return false; + } + } + + return true; } diff --git a/inc/modelcsv.class.php b/inc/modelcsv.class.php index 2fbb0ec4..975e4249 100644 --- a/inc/modelcsv.class.php +++ b/inc/modelcsv.class.php @@ -129,26 +129,26 @@ public function checkFileName($filename) * * @return int the ID of the row in glpi_plugin_datainjection_modelcsv **/ - public function getFromDBByModelID($models_id) + public function getFromDBByModelID(int $models_id): int { /** @var DBmysql $DB */ global $DB; - $query = "SELECT `id` - FROM `" . $this->getTable() . "` - WHERE `models_id` = '" . $models_id . "'"; + $iterator = $DB->request([ + 'SELECT' => 'id', + 'FROM' => $this->getTable(), + 'WHERE' => ['models_id' => $models_id], + 'LIMIT' => 1, + ]); - $results = $DB->doQuery($query); - $id = 0; - - if ($DB->numrows($results) > 0) { - $id = $DB->result($results, 0, 'id'); + if (count($iterator) > 0) { + $id = (int) $iterator->current()['id']; $this->getFromDB($id); } else { $this->getEmpty(); $tmp = $this->fields; $tmp['models_id'] = $models_id; - $id = $this->add($tmp); + $id = (int) $this->add($tmp); $this->getFromDB($id); } diff --git a/tests/unit/ModelCheckRightTest.php b/tests/unit/ModelCheckRightTest.php new file mode 100644 index 00000000..e039d770 --- /dev/null +++ b/tests/unit/ModelCheckRightTest.php @@ -0,0 +1,103 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by DataInjection plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/datainjection + * ------------------------------------------------------------------------- + */ + +final class ModelCheckRightTest extends DbTestCase +{ + private function createModel(string $itemtype = Computer::class): int + { + $model = new PluginDatainjectionModel(); + $models_id = $model->add([ + 'name' => 'Test_Model_CheckRight_' . $itemtype . '_' . random_int(1, PHP_INT_MAX), + 'itemtype' => $itemtype, + 'filetype' => 'csv', + 'entities_id' => 0, + 'is_private' => 0, + 'behavior_add' => 1, + 'behavior_update' => 0, + 'users_id' => Session::getLoginUserID(), + ]); + $this->assertGreaterThan(0, $models_id); + + return (int) $models_id; + } + + public function testUnknownModelIsAllowed(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel(999999)); + } + + public function testCreationPathReturnsBooleanOnEmptyModel(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $model = new PluginDatainjectionModel(); + $model->getEmpty(); + + $this->assertIsBool($model->canCreateItem()); + } + + public function testMappedRelationItemtypeWithoutRightsIsDenied(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $models_id = $this->createModel(); + $control_id = $this->createModel(); + + $mapping = new PluginDatainjectionMapping(); + $this->assertGreaterThan(0, $mapping->add([ + 'models_id' => $models_id, + 'itemtype' => Group_User::class, + 'rank' => 0, + 'name' => 'groups_id', + 'value' => 'groups_id', + 'is_mandatory' => 0, + ])); + + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel($models_id)); + + $_SESSION['glpiactiveprofile'][User::$rightname] = 0; + $_SESSION['glpiactiveprofile'][Group::$rightname] = 0; + + //Control model keeps its own granted itemtype: only the mapped relation may deny + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel($control_id)); + $this->assertFalse(PluginDatainjectionModel::checkRightOnModel($models_id)); + } +} diff --git a/tests/unit/ModelCsvLookupTest.php b/tests/unit/ModelCsvLookupTest.php new file mode 100644 index 00000000..e674435e --- /dev/null +++ b/tests/unit/ModelCsvLookupTest.php @@ -0,0 +1,72 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by DataInjection plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/datainjection + * ------------------------------------------------------------------------- + */ + +final class ModelCsvLookupTest extends DbTestCase +{ + private function createModel(): int + { + $model = new PluginDatainjectionModel(); + $models_id = $model->add([ + 'name' => 'Test_ModelCsv_Lookup', + 'itemtype' => 'Computer', + 'filetype' => 'csv', + 'entities_id' => 0, + 'is_private' => 0, + 'behavior_add' => 1, + 'behavior_update' => 0, + 'users_id' => Session::getLoginUserID(), + ]); + $this->assertGreaterThan(0, $models_id); + + return (int) $models_id; + } + + public function testRowIsCreatedThenReusedForSameModel(): void + { + $models_id = $this->createModel(); + + $csv = new PluginDatainjectionModelCsv(); + $first = $csv->getFromDBByModelID($models_id); + + $this->assertGreaterThan(0, $first); + $this->assertSame($models_id, (int) $csv->fields['models_id']); + + $second = (new PluginDatainjectionModelCsv())->getFromDBByModelID($models_id); + $this->assertSame($first, $second); + } + + public function testNonNumericModelIdIsRejected(): void + { + $csv = new PluginDatainjectionModelCsv(); + + $this->expectException(TypeError::class); + $csv->getFromDBByModelID("id' AND SLEEP(5)-- "); + } +}