diff --git a/CHANGELOG.md b/CHANGELOG.md index 780eec26..9092e2b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - 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) +- Fix validation, permissions and entity handling during imports (backport of PR #664 from GLPI 11-compatible line) +- Improve partial import error reporting (backport of PR #664 from GLPI 11-compatible line) +- Correct user password import and creation handling (policy, history, expiration and confirmation) (backport of PR #664 from GLPI 11-compatible line) ## [2.14.4] - 2025-11-25 diff --git a/inc/commoninjectionlib.class.php b/inc/commoninjectionlib.class.php index d6d0b225..3b80a4a4 100644 --- a/inc/commoninjectionlib.class.php +++ b/inc/commoninjectionlib.class.php @@ -547,24 +547,22 @@ private function manageFieldValues() /** - * Get the ID associated with a value from the CSV file - * - * @param PluginDatainjectionInjectionInterface|null $injectionClass - * @param string $itemtype itemtype of the values to inject - * @param array $searchOption option associated with the field to check - * @param string $field the field to check - * @param string $value the value coming from the CSV file - * @param boolean $add is insertion (true) or update (false) (true by default) - * - * @return void nothing - **/ + * Get the ID associated with a value from the CSV file + * + * @param PluginDatainjectionInjectionInterface|null $injectionClass + * @param string $itemtype itemtype of the values to inject + * @param array $searchOption option associated with the field to check + * @param string $field the field to check + * @param string $value the value coming from the CSV file + * + * @return void nothing + **/ private function getFieldValue( $injectionClass, $itemtype, $searchOption, $field, - $value, - $add = true + $value ) { if (isset($searchOption['storevaluein'])) { $linkfield = $searchOption['storevaluein']; @@ -583,13 +581,12 @@ private function getFieldValue( break; case 'password': - //To add a user password, it's mandatory is give a password and it's confirmation - //Here we cannot detect if it's an add or update. We'll handle updates later in the process - if ($add && $itemtype == 'User') { + //Core needs both the password and its confirmation to validate and hash it, on add as well as on update + if ($itemtype == 'User') { $this->setValueForItemtype($itemtype, $linkfield, $value); - //Add field password2 is not already present + //Add field password2 if not already present //(can be present if password was an addtional information) - if (!isset($this->values[$itemtype][$field])) { + if (!isset($this->values[$itemtype][$linkfield . "2"])) { $this->setValueForItemtype($itemtype, $linkfield . "2", $value); } } @@ -927,7 +924,8 @@ private function unsetValue($itemtype, $field) **/ private function setValueForItemtype($itemtype, $field, $value, $fromdb = false) { - if ($itemtype === User::class && $field === "pdffont" && $fromdb) { + //The stored password is a hash: taking it back from the DB would overwrite the imported one + if ($itemtype === User::class && in_array($field, ['pdffont', 'password'], true) && $fromdb) { return; } $injectionClass = self::getInjectionClassInstance($itemtype); @@ -1577,14 +1575,17 @@ public function processAddOrUpdate() $newID = $this->effectiveAddOrUpdate($this->injectionClass, $item, $values, $add); if (!$newID) { - $this->results['status'] = self::WARNING; + $this->addCheckWarning(self::WARNING, get_class($item)); } else { //Store id of the injected item $this->setValueForItemtype($this->primary_type, 'id', $newID); - //If type needs it : process more data after type import - $this->processAfterInsertOrUpdate($this->injectionClass, $add); - //$this->results['status'] = self::SUCCESS; + //If type needs it : process more data after type import + if ($this->processAfterInsertOrUpdate($this->injectionClass, $add) === false) { + $this->addCheckWarning(self::WARNING, get_class($item)); + } + + //$this->results['status'] = self::SUCCESS; $this->results[get_class($item)] = $newID; //Process other types @@ -1619,7 +1620,11 @@ public function processAddOrUpdate() $values = $this->getValuesForItemtype($itemtype); if ($this->lastCheckBeforeProcess($injectionClass, $values)) { $tmpID = $this->effectiveAddOrUpdate($injectionClass, $item, $values, $add); - $this->processAfterInsertOrUpdate($injectionClass, $add); + if (!$tmpID) { + $this->addCheckWarning(self::WARNING, $itemtype); + } elseif ($this->processAfterInsertOrUpdate($injectionClass, $add) === false) { + $this->addCheckWarning(self::WARNING, $itemtype); + } } } } @@ -1630,6 +1635,20 @@ public function processAddOrUpdate() } + /** + * Flag the current line as partially injected and log the reason + * + * @param integer $code log label describing the reason + * @param string $itemtype itemtype that could not be written + **/ + private function addCheckWarning(int $code, string $itemtype): void + { + $this->results['status'] = self::WARNING; + $this->results[self::ACTION_CHECK]['status'] = self::WARNING; + $this->results[self::ACTION_CHECK][] = [$code, $itemtype]; + } + + /** * Perform data injection into GLPI DB * @@ -1643,7 +1662,25 @@ public function processAddOrUpdate() private function effectiveAddOrUpdate($injectionClass, $item, $values, $add = true) { - //Insert data using the standard add() method + //The plugin acts as the front controller here: rights must be checked before writing. + //Skipped without a session, as the lib is also a programmatic entry point for scripts. + if (Session::getLoginUserID() !== false) { + $input = is_array($values) ? $values : []; + if ($add) { + //Passing the input to can() makes the check cover the target entity + if (!$item->can(-1, CREATE, $input)) { + $this->addCheckWarning(self::ERROR_CANNOT_IMPORT, get_class($item)); + return 0; + } + + //On the update path the target id is known, so the per-item check also covers the entity scope + } elseif (!isset($values['id']) || !$item->can($values['id'], UPDATE)) { + $this->addCheckWarning(self::ERROR_CANNOT_UPDATE, get_class($item)); + return 0; + } + } + + //Insert data using the standard add() method $toinject = []; $options = $injectionClass->getOptions(); @@ -1816,7 +1853,6 @@ private function manageRelations() $option, $option['linkfield'], $value, - true ); } } @@ -2317,15 +2353,17 @@ public static function addTemplateSearchOptions($injectionClass, &$tab) * @param PluginDatainjectionInjectionInterface $injectionClass the injection class to use * @param $add true if an item is created, false if it's an update * - * @return void nothing + * @return bool false if the injection class rejected a post-processing step **/ private function processAfterInsertOrUpdate($injectionClass, $add = true) { //If itemtype implements special process after type injection if (method_exists($injectionClass, 'processAfterInsertOrUpdate')) { - //Invoke it - $injectionClass->processAfterInsertOrUpdate($this->values, $add, $this->rights); + //Invoke it + return $injectionClass->processAfterInsertOrUpdate($this->values, $add, $this->rights) !== false; } + + return true; } } diff --git a/inc/model.class.php b/inc/model.class.php index c63d6723..03f7d259 100644 --- a/inc/model.class.php +++ b/inc/model.class.php @@ -1357,13 +1357,25 @@ public static function checkRightOnModel(int $models_id): bool } } + $check_add = (bool) ($model->fields['behavior_add'] ?? 0); + $check_update = (bool) ($model->fields['behavior_update'] ?? 0); + + //A model doing nothing still requires the creation right to be listed + if (!$check_add && !$check_update) { + $check_add = true; + } + foreach (array_unique($itemtypes) as $itemtype) { if ($itemtype == PluginDatainjectionInjectionType::NO_VALUE || !is_a($itemtype, CommonDBTM::class, true)) { continue; } $item = new $itemtype(); - if (!$item->canCreate()) { + if ($check_add && !$item->canCreate()) { + return false; + } + + if ($check_update && !$item->canUpdate()) { return false; } } diff --git a/inc/userinjection.class.php b/inc/userinjection.class.php index 3a8a67bf..3c9cd997 100644 --- a/inc/userinjection.class.php +++ b/inc/userinjection.class.php @@ -189,12 +189,11 @@ public function reformat(&$values) * @param array $values * @param boolean $add (true by default) * @param array|null $rights array + * + * @return bool false if a post-processing step was rejected */ public function processAfterInsertOrUpdate($values, $add = true, $rights = []) { - /** @var DBmysql $DB */ - global $DB; - //Manage user emails (both for add and update) if ( isset($values['User']['useremails_id']) @@ -234,13 +233,7 @@ public function processAfterInsertOrUpdate($values, $add = true, $rights = []) } } - if (isset($values['User']['password']) && ($values['User']['password'] != '')) { - $DB->update( - 'glpi_users', - ['password' => Auth::getPasswordHash(Sanitizer::unsanitize($values['User']['password']))], - ['id' => $values['User']['id']], - ); - } + return true; } diff --git a/tests/unit/InjectionWriteRightTest.php b/tests/unit/InjectionWriteRightTest.php new file mode 100644 index 00000000..51ee23dc --- /dev/null +++ b/tests/unit/InjectionWriteRightTest.php @@ -0,0 +1,159 @@ +. + * ------------------------------------------------------------------------- + * @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 InjectionWriteRightTest extends DbTestCase +{ + private function injectData( + object $injection_class, + array $injected_data, + array $mandatory_fields + ): array { + $lib = new PluginDatainjectionCommonInjectionLib( + $injection_class, + $injected_data, + [ + 'rights' => [ + 'can_add' => true, + 'can_update' => true, + 'add_dropdown' => true, + ], + 'mandatory_fields' => $mandatory_fields, + 'entities_id' => 0, + ], + ); + + $lib->processAddOrUpdate(); + + return $lib->getInjectionResults(); + } + + public function testInjectedUserPasswordIsUsableAndRaisesNoError(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $login = 'test_injected_user_' . random_int(1, PHP_INT_MAX); + $password = 'Ohbah7ohw!aeK3'; + + $results = $this->injectData( + new PluginDatainjectionUserInjection(), + ['User' => ['name' => $login, 'password' => $password]], + ['User' => ['name' => true]], + ); + + self::assertSame(PluginDatainjectionCommonInjectionLib::SUCCESS, $results['status']); + self::assertEmpty($_SESSION['MESSAGE_AFTER_REDIRECT'][ERROR] ?? []); + + $user = new User(); + self::assertTrue($user->getFromDB($results['User'])); + self::assertTrue(Auth::checkPassword($password, $user->fields['password'])); + } + + public function testInjectedUserPasswordIsUpdatedOnExistingUser(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $login = 'test_injected_user_' . random_int(1, PHP_INT_MAX); + $password = 'Ohbah7ohw!aeK3'; + $new_password = 'Eiy4ohn!ohGh1o'; + + $results = $this->injectData( + new PluginDatainjectionUserInjection(), + ['User' => ['name' => $login, 'password' => $password]], + ['User' => ['name' => true]], + ); + self::assertSame(PluginDatainjectionCommonInjectionLib::SUCCESS, $results['status']); + + $results = $this->injectData( + new PluginDatainjectionUserInjection(), + ['User' => ['name' => $login, 'password' => $new_password]], + ['User' => ['name' => true]], + ); + self::assertSame(PluginDatainjectionCommonInjectionLib::SUCCESS, $results['status']); + self::assertEmpty($_SESSION['MESSAGE_AFTER_REDIRECT'][ERROR] ?? []); + + $user = new User(); + self::assertTrue($user->getFromDB($results['User'])); + self::assertTrue(Auth::checkPassword($new_password, $user->fields['password'])); + } + + public function testInjectionIsRejectedWithoutUpdateRightOnExistingItem(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $computer = $this->createItem(Computer::class, [ + 'name' => 'Test_Computer_write_right_' . random_int(1, PHP_INT_MAX), + 'entities_id' => 0, + ]); + $comment = $computer->fields['comment']; + + $_SESSION['glpiactiveprofile'][Computer::$rightname] = READ; + + $results = $this->injectData( + new PluginDatainjectionComputerInjection(), + ['Computer' => ['name' => $computer->fields['name'], 'comment' => 'Injected comment']], + ['Computer' => ['name' => true]], + ); + + self::assertSame(PluginDatainjectionCommonInjectionLib::WARNING, $results['status']); + + self::assertTrue($computer->getFromDB($computer->getID())); + self::assertSame($comment, $computer->fields['comment']); + } + + public function testInjectionIsRejectedWithoutCreateRight(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $name = 'Test_Computer_no_create_' . random_int(1, PHP_INT_MAX); + + $_SESSION['glpiactiveprofile'][Computer::$rightname] = READ; + + $results = $this->injectData( + new PluginDatainjectionComputerInjection(), + ['Computer' => ['name' => $name]], + ['Computer' => ['name' => true]], + ); + + self::assertSame(PluginDatainjectionCommonInjectionLib::WARNING, $results['status']); + self::assertSame(0, countElementsInTable('glpi_computers', ['name' => $name])); + } +} diff --git a/tests/unit/ModelCheckRightTest.php b/tests/unit/ModelCheckRightTest.php index e039d770..cdd4f8ee 100644 --- a/tests/unit/ModelCheckRightTest.php +++ b/tests/unit/ModelCheckRightTest.php @@ -31,16 +31,19 @@ final class ModelCheckRightTest extends DbTestCase { private function createModel(string $itemtype = Computer::class): int + { + return $this->createModelWithBehaviors($itemtype, ['behavior_add' => 1, 'behavior_update' => 0]); + } + + private function createModelWithBehaviors(string $itemtype, array $behaviors): int { $model = new PluginDatainjectionModel(); - $models_id = $model->add([ + $models_id = $model->add($behaviors + [ '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); @@ -100,4 +103,42 @@ public function testMappedRelationItemtypeWithoutRightsIsDenied(): void $this->assertTrue(PluginDatainjectionModel::checkRightOnModel($control_id)); $this->assertFalse(PluginDatainjectionModel::checkRightOnModel($models_id)); } + + public function testUpdateEnabledModelIsDeniedWithCreateRightOnly(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $models_id = $this->createModelWithBehaviors( + Computer::class, + ['behavior_add' => 0, 'behavior_update' => 1], + ); + + $_SESSION['glpiactiveprofile'][Computer::$rightname] = CREATE; + $this->assertFalse(PluginDatainjectionModel::checkRightOnModel($models_id)); + + $_SESSION['glpiactiveprofile'][Computer::$rightname] = CREATE | UPDATE; + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel($models_id)); + } + + public function testAddOnlyModelIsDeniedWithUpdateRightOnly(): void + { + global $CFG_GLPI; + $CFG_GLPI["event_loglevel"] = 0; + + $this->login(); + + $models_id = $this->createModelWithBehaviors( + Computer::class, + ['behavior_add' => 1, 'behavior_update' => 0], + ); + + $_SESSION['glpiactiveprofile'][Computer::$rightname] = UPDATE; + $this->assertFalse(PluginDatainjectionModel::checkRightOnModel($models_id)); + + $_SESSION['glpiactiveprofile'][Computer::$rightname] = CREATE; + $this->assertTrue(PluginDatainjectionModel::checkRightOnModel($models_id)); + } }