Skip to content

Commit 1b64466

Browse files
stonebuzzstonebuzz
authored
Fix: check the right matching the model behavior (#664)
Co-authored-by: Stanislas Kita <7335054+stonebuzz@users.noreply.github.com>
1 parent e16738a commit 1b64466

6 files changed

Lines changed: 279 additions & 41 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
1212
- Move to iterator
1313
- Fix various minor bugs in the import/export workflow
1414
- Fix an issue where data are not formatted when coming from a field plugin's custom field.
15-
- Fix model selector validation and access control
15+
- Improve import/export workflow
16+
- Fix validation, permissions and entity handling during imports
17+
- Improve partial import error reporting
18+
- Correct user password import and creation handling (policy, history, expiration and confirmation)
1619

1720
## [2.15.10] - 2026-08-07
1821

‎inc/commoninjectionlib.class.php‎

Lines changed: 64 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -584,24 +584,22 @@ private function manageFieldValues()
584584

585585

586586
/**
587-
* Get the ID associated with a value from the CSV file
588-
*
589-
* @param PluginDatainjectionInjectionInterface|null $injectionClass
590-
* @param string $itemtype itemtype of the values to inject
591-
* @param array $searchOption option associated with the field to check
592-
* @param string $field the field to check
593-
* @param string $value the value coming from the CSV file
594-
* @param boolean $add is insertion (true) or update (false) (true by default)
595-
*
596-
* @return void nothing
597-
**/
587+
* Get the ID associated with a value from the CSV file
588+
*
589+
* @param PluginDatainjectionInjectionInterface|null $injectionClass
590+
* @param string $itemtype itemtype of the values to inject
591+
* @param array $searchOption option associated with the field to check
592+
* @param string $field the field to check
593+
* @param string $value the value coming from the CSV file
594+
*
595+
* @return void nothing
596+
**/
598597
private function getFieldValue(
599598
$injectionClass,
600599
$itemtype,
601600
$searchOption,
602601
$field,
603-
$value,
604-
$add = true
602+
$value
605603
) {
606604
$linkfield = $searchOption['storevaluein'] ?? $searchOption['linkfield'];
607605

@@ -614,13 +612,12 @@ private function getFieldValue(
614612
break;
615613

616614
case 'password':
617-
//To add a user password, it's mandatory is give a password and it's confirmation
618-
//Here we cannot detect if it's an add or update. We'll handle updates later in the process
619-
if ($add && $itemtype == 'User') {
615+
//Core needs both the password and its confirmation to validate and hash it, on add as well as on update
616+
if ($itemtype == 'User') {
620617
$this->setValueForItemtype($itemtype, $linkfield, $value);
621-
//Add field password2 is not already present
618+
//Add field password2 if not already present
622619
//(can be present if password was an addtional information)
623-
if (!isset($this->values[$itemtype][$field])) {
620+
if (!isset($this->values[$itemtype][$linkfield . "2"])) {
624621
$this->setValueForItemtype($itemtype, $linkfield . "2", $value);
625622
}
626623
}
@@ -976,7 +973,8 @@ private function unsetValue($itemtype, $field)
976973
**/
977974
private function setValueForItemtype($itemtype, $field, $value, $fromdb = false)
978975
{
979-
if ($itemtype === User::class && $field === "pdffont" && $fromdb) {
976+
//The stored password is a hash: taking it back from the DB would overwrite the imported one
977+
if ($itemtype === User::class && in_array($field, ['pdffont', 'password'], true) && $fromdb) {
980978
return;
981979
}
982980

@@ -1708,13 +1706,16 @@ public function processAddOrUpdate()
17081706
$newID = $this->effectiveAddOrUpdate($this->injectionClass, $item, $values, $add);
17091707

17101708
if (!$newID) {
1711-
$this->results['status'] = self::WARNING;
1709+
$this->addCheckWarning(self::WARNING, $item::class);
17121710
} else {
17131711
//Store id of the injected item
17141712
$this->setValueForItemtype($this->primary_type, 'id', $newID);
17151713

17161714
//If type needs it : process more data after type import
1717-
$this->processAfterInsertOrUpdate($this->injectionClass, $add);
1715+
if ($this->processAfterInsertOrUpdate($this->injectionClass, $add) === false) {
1716+
$this->addCheckWarning(self::WARNING, $item::class);
1717+
}
1718+
17181719
//$this->results['status'] = self::SUCCESS;
17191720
$this->results[$item::class] = $newID;
17201721

@@ -1751,8 +1752,12 @@ public function processAddOrUpdate()
17511752

17521753
$values = $this->getValuesForItemtype($itemtype);
17531754
if ($this->lastCheckBeforeProcess($injectionClass)) {
1754-
$tmpID = $this->effectiveAddOrUpdate($injectionClass, $item, $values, $add);
1755-
$this->processAfterInsertOrUpdate($injectionClass, $add);
1755+
$tmpID = $this->effectiveAddOrUpdate($injectionClass, $item, $values, $add);
1756+
if (!$tmpID) {
1757+
$this->addCheckWarning(self::WARNING, $itemtype);
1758+
} elseif ($this->processAfterInsertOrUpdate($injectionClass, $add) === false) {
1759+
$this->addCheckWarning(self::WARNING, $itemtype);
1760+
}
17561761
}
17571762
}
17581763
}
@@ -1765,6 +1770,20 @@ public function processAddOrUpdate()
17651770
}
17661771

17671772

1773+
/**
1774+
* Flag the current line as partially injected and log the reason
1775+
*
1776+
* @param integer $code log label describing the reason
1777+
* @param string $itemtype itemtype that could not be written
1778+
**/
1779+
private function addCheckWarning(int $code, string $itemtype): void
1780+
{
1781+
$this->results['status'] = self::WARNING;
1782+
$this->results[self::ACTION_CHECK]['status'] = self::WARNING;
1783+
$this->results[self::ACTION_CHECK][] = [$code, $itemtype];
1784+
}
1785+
1786+
17681787
/**
17691788
* Perform data injection into GLPI DB
17701789
*
@@ -1778,6 +1797,24 @@ public function processAddOrUpdate()
17781797
private function effectiveAddOrUpdate($injectionClass, $item, $values, $add = true)
17791798
{
17801799

1800+
//The plugin acts as the front controller here: rights must be checked before writing.
1801+
//Skipped without a session, as the lib is also a programmatic entry point for scripts.
1802+
if (Session::getLoginUserID() !== false) {
1803+
$input = is_array($values) ? $values : [];
1804+
if ($add) {
1805+
//Passing the input to can() makes the check cover the target entity
1806+
if (!$item->can(-1, CREATE, $input)) {
1807+
$this->addCheckWarning(self::ERROR_CANNOT_IMPORT, $item::class);
1808+
return 0;
1809+
}
1810+
1811+
//On the update path the target id is known, so the per-item check also covers the entity scope
1812+
} elseif (!isset($values['id']) || !$item->can($values['id'], UPDATE)) {
1813+
$this->addCheckWarning(self::ERROR_CANNOT_UPDATE, $item::class);
1814+
return 0;
1815+
}
1816+
}
1817+
17811818
//Insert data using the standard add() method
17821819
$toinject = [];
17831820
$options = $injectionClass->getOptions();
@@ -1972,7 +2009,6 @@ private function manageRelations()
19722009
$option,
19732010
$option['linkfield'],
19742011
$value,
1975-
true,
19762012
);
19772013
}
19782014
}
@@ -2476,15 +2512,17 @@ public static function addTemplateSearchOptions($injectionClass, &$tab)
24762512
* @param PluginDatainjectionInjectionInterface $injectionClass the injection class to use
24772513
* @param $add true if an item is created, false if it's an update
24782514
*
2479-
* @return void nothing
2515+
* @return bool false if the injection class rejected a post-processing step
24802516
**/
24812517
private function processAfterInsertOrUpdate($injectionClass, $add = true)
24822518
{
24832519

24842520
//If itemtype implements special process after type injection
24852521
if (method_exists($injectionClass, 'processAfterInsertOrUpdate')) {
24862522
//Invoke it
2487-
$injectionClass->processAfterInsertOrUpdate($this->values, $add, $this->rights);
2523+
return $injectionClass->processAfterInsertOrUpdate($this->values, $add, $this->rights) !== false;
24882524
}
2525+
2526+
return true;
24892527
}
24902528
}

‎inc/model.class.php‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1277,13 +1277,25 @@ public static function checkRightOnModel(int $models_id): bool
12771277
}
12781278
}
12791279

1280+
$check_add = (bool) ($model->fields['behavior_add'] ?? 0);
1281+
$check_update = (bool) ($model->fields['behavior_update'] ?? 0);
1282+
1283+
//A model doing nothing still requires the creation right to be listed
1284+
if (!$check_add && !$check_update) {
1285+
$check_add = true;
1286+
}
1287+
12801288
foreach (array_unique($itemtypes) as $itemtype) {
12811289
if ($itemtype == PluginDatainjectionInjectionType::NO_VALUE || !is_a($itemtype, CommonDBTM::class, true)) {
12821290
continue;
12831291
}
12841292

12851293
$item = new $itemtype();
1286-
if (!$item->canCreate()) {
1294+
if ($check_add && !$item->canCreate()) {
1295+
return false;
1296+
}
1297+
1298+
if ($check_update && !$item->canUpdate()) {
12871299
return false;
12881300
}
12891301
}

‎inc/userinjection.class.php‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -179,12 +179,11 @@ public function reformat(&$values)
179179
* @param array $values
180180
* @param boolean $add (true by default)
181181
* @param array|null $rights array
182+
*
183+
* @return bool false if a post-processing step was rejected
182184
*/
183185
public function processAfterInsertOrUpdate($values, $add = true, $rights = [])
184186
{
185-
/** @var DBmysql $DB */
186-
global $DB;
187-
188187
//Manage user emails
189188
if (isset($values['User']['useremails_id']) && $rights['add_dropdown'] && Session::haveRight('user', UPDATE)) {
190189
$emails = preg_split('/[\s,;]+/', $values['User']['useremails_id'], -1, PREG_SPLIT_NO_EMPTY);
@@ -213,13 +212,7 @@ public function processAfterInsertOrUpdate($values, $add = true, $rights = [])
213212
}
214213
}
215214

216-
if (isset($values['User']['password']) && ($values['User']['password'] != '')) {
217-
$DB->update(
218-
'glpi_users',
219-
['password' => Auth::getPasswordHash($values['User']['password'])],
220-
['id' => $values['User']['id']],
221-
);
222-
}
215+
return true;
223216
}
224217

225218

‎tests/unit/InjectionWriteRightTest.php‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
<?php
2+
3+
/**
4+
* -------------------------------------------------------------------------
5+
* DataInjection plugin for GLPI
6+
* -------------------------------------------------------------------------
7+
*
8+
* LICENSE
9+
*
10+
* This file is part of DataInjection.
11+
*
12+
* DataInjection is free software; you can redistribute it and/or modify
13+
* it under the terms of the GNU General Public License as published by
14+
* the Free Software Foundation; either version 2 of the License, or
15+
* (at your option) any later version.
16+
*
17+
* DataInjection is distributed in the hope that it will be useful,
18+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
19+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20+
* GNU General Public License for more details.
21+
*
22+
* You should have received a copy of the GNU General Public License
23+
* along with DataInjection. If not, see <http://www.gnu.org/licenses/>.
24+
* -------------------------------------------------------------------------
25+
* @copyright Copyright (C) 2007-2023 by DataInjection plugin team.
26+
* @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html
27+
* @link https://github.com/pluginsGLPI/datainjection
28+
* -------------------------------------------------------------------------
29+
*/
30+
31+
namespace GlpiPlugin\Datainjection\Tests\Unit;
32+
33+
use Auth;
34+
use Computer;
35+
use Glpi\Tests\DbTestCase;
36+
use PluginDatainjectionCommonInjectionLib;
37+
use PluginDatainjectionComputerInjection;
38+
use PluginDatainjectionUserInjection;
39+
use User;
40+
41+
final class InjectionWriteRightTest extends DbTestCase
42+
{
43+
private function injectData(
44+
object $injection_class,
45+
array $injected_data,
46+
array $mandatory_fields
47+
): array {
48+
$lib = new PluginDatainjectionCommonInjectionLib(
49+
$injection_class,
50+
$injected_data,
51+
[
52+
'rights' => [
53+
'can_add' => true,
54+
'can_update' => true,
55+
'add_dropdown' => true,
56+
],
57+
'mandatory_fields' => $mandatory_fields,
58+
'entities_id' => 0,
59+
],
60+
);
61+
62+
$lib->processAddOrUpdate();
63+
64+
return $lib->getInjectionResults();
65+
}
66+
67+
public function testInjectedUserPasswordIsUsableAndRaisesNoError(): void
68+
{
69+
$this->login();
70+
71+
$login = 'test_injected_user_' . random_int(1, PHP_INT_MAX);
72+
$password = 'Ohbah7ohw!aeK3';
73+
74+
$results = $this->injectData(
75+
new PluginDatainjectionUserInjection(),
76+
['User' => ['name' => $login, 'password' => $password]],
77+
['User' => ['name' => true]],
78+
);
79+
80+
self::assertSame(PluginDatainjectionCommonInjectionLib::SUCCESS, $results['status']);
81+
self::assertEmpty($_SESSION['MESSAGE_AFTER_REDIRECT'][ERROR] ?? []);
82+
83+
$user = new User();
84+
self::assertTrue($user->getFromDB($results['User']));
85+
self::assertTrue(Auth::checkPassword($password, $user->fields['password']));
86+
}
87+
88+
public function testInjectedUserPasswordIsUpdatedOnExistingUser(): void
89+
{
90+
$this->login();
91+
92+
$login = 'test_injected_user_' . random_int(1, PHP_INT_MAX);
93+
$password = 'Ohbah7ohw!aeK3';
94+
$new_password = 'Eiy4ohn!ohGh1o';
95+
96+
$results = $this->injectData(
97+
new PluginDatainjectionUserInjection(),
98+
['User' => ['name' => $login, 'password' => $password]],
99+
['User' => ['name' => true]],
100+
);
101+
self::assertSame(PluginDatainjectionCommonInjectionLib::SUCCESS, $results['status']);
102+
103+
$results = $this->injectData(
104+
new PluginDatainjectionUserInjection(),
105+
['User' => ['name' => $login, 'password' => $new_password]],
106+
['User' => ['name' => true]],
107+
);
108+
self::assertSame(PluginDatainjectionCommonInjectionLib::SUCCESS, $results['status']);
109+
self::assertEmpty($_SESSION['MESSAGE_AFTER_REDIRECT'][ERROR] ?? []);
110+
111+
$user = new User();
112+
self::assertTrue($user->getFromDB($results['User']));
113+
self::assertTrue(Auth::checkPassword($new_password, $user->fields['password']));
114+
}
115+
116+
public function testInjectionIsRejectedWithoutUpdateRightOnExistingItem(): void
117+
{
118+
$this->login();
119+
120+
$computer = $this->createItem(Computer::class, [
121+
'name' => 'Test_Computer_write_right_' . random_int(1, PHP_INT_MAX),
122+
'entities_id' => 0,
123+
]);
124+
$comment = $computer->fields['comment'];
125+
126+
$_SESSION['glpiactiveprofile'][Computer::$rightname] = READ;
127+
128+
$results = $this->injectData(
129+
new PluginDatainjectionComputerInjection(),
130+
['Computer' => ['name' => $computer->fields['name'], 'comment' => 'Injected comment']],
131+
['Computer' => ['name' => true]],
132+
);
133+
134+
self::assertSame(PluginDatainjectionCommonInjectionLib::WARNING, $results['status']);
135+
136+
self::assertTrue($computer->getFromDB($computer->getID()));
137+
self::assertSame($comment, $computer->fields['comment']);
138+
}
139+
140+
public function testInjectionIsRejectedWithoutCreateRight(): void
141+
{
142+
$this->login();
143+
144+
$name = 'Test_Computer_no_create_' . random_int(1, PHP_INT_MAX);
145+
146+
$_SESSION['glpiactiveprofile'][Computer::$rightname] = READ;
147+
148+
$results = $this->injectData(
149+
new PluginDatainjectionComputerInjection(),
150+
['Computer' => ['name' => $name]],
151+
['Computer' => ['name' => true]],
152+
);
153+
154+
self::assertSame(PluginDatainjectionCommonInjectionLib::WARNING, $results['status']);
155+
self::assertSame(0, countElementsInTable('glpi_computers', ['name' => $name]));
156+
}
157+
}

0 commit comments

Comments
 (0)