Skip to content

FIX / Mandatory field validation not enforced on update #1269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Rom1-B merged 3 commits into from
Sep 23, 2026
Merged

FIX / Mandatory field validation not enforced on update #1269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7af810
fix skipped mandatory field validation on update
Mary-Clb Sep 21, 2026
e34d7e6
update CHANGELOG.md
Mary-Clb Sep 21, 2026
f8f9f10
fix mandatory check missing sibling containers
Mary-Clb Sep 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

### Fixed

- Fix mandatory fields on a Tab block not being enforced when updating an item.
- Fix administrators losing access to a block's configuration after setting a profile to "no access" on that block.
- Fix dependency conflict with GLPI core by no longer vendoring symfony/deprecation-contracts and symfony/polyfill-ctype.
- Fix default field values not being applied when fields are empty on creation
Expand Down
148 changes: 98 additions & 50 deletions inc/container.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1746,6 +1746,7 @@ public static function validateValues($data, $itemtype, $massiveaction)
empty($value)
|| (($field['type'] === 'dropdown' || preg_match('/^dropdown-.+/i', (string) $field['type'])) && $value == 0)
|| (in_array($field['type'], ['date', 'datetime']) && $value == 'NULL')
|| ($field['multiple'] && is_string($value) && json_decode($value, true) === [])
)
) {
$empty_errors[] = $field['label'];
Expand Down Expand Up @@ -1920,87 +1921,134 @@ public static function preItemUpdate(CommonDBTM $item)
*/
public static function preItem(CommonDBTM $item)
{
//find container (if not exist, do nothing)
//find container(s) (if none exist, do nothing)
if (isset($item->input['c_id'])) {
$c_id = $item->input['c_id'];
$c_ids = [$item->input['c_id']];
} elseif (isset($_REQUEST['c_id'])) {
$c_id = $_REQUEST['c_id'];
$c_ids = [$_REQUEST['c_id']];
} elseif (isset($_REQUEST['_plugin_fields_type'])) {
// an explicit context is targeted (e.g. a domtab's own subtab form)
$type = $_REQUEST['_plugin_fields_type'];
$subtype = $type === 'domtab' ? $_REQUEST['_plugin_fields_subtype'] : '';
$c_id = self::findContainer($item::class, $type, $subtype);
$c_ids = $c_id === false ? [] : [$c_id];
} else {
$type = 'dom';
if (isset($_REQUEST['_plugin_fields_type'])) {
$type = $_REQUEST['_plugin_fields_type'];
// generic add/update: both the "dom" and "tab" containers can carry
// mandatory fields that must be enforced, even though only "dom"
// fields are actually submitted inline with the main form
$c_ids = array_filter(
[self::findContainer($item::class, 'dom'), self::findContainer($item::class, 'tab')],
static fn($id) => $id !== false,
);
}

if ($c_ids === []) {
return false;
}

if (count($item->fields) === 0) {
$item->fields = $item->input;
}

$submitted_data = null;
foreach ($c_ids as $c_id) {
$loc_c = new PluginFieldsContainer();
$loc_c->getFromDB($c_id);

// check rights on $c_id
// The profile check is only enforced when an active user profile is present in session.
// Automated contexts (cron jobs, API token sessions without profile) bypass the check
// so that plugin fields can still be persisted — authentication is already enforced
// at a higher level by the GLPI API/cron layer.
if (isset($_SESSION['glpiactiveprofile']['id']) && $_SESSION['glpiactiveprofile']['id'] != null && $c_id > 0) {
$right = PluginFieldsProfile::getRightOnContainer($_SESSION['glpiactiveprofile']['id'], $c_id);
if (($right > READ) === false) {
continue;
}
}

$subtype = '';
if ($type == 'domtab') {
$subtype = $_REQUEST['_plugin_fields_subtype'];
// need to check if container is usable on this object entity
$entities = [$loc_c->fields['entities_id']];
if ($loc_c->fields['is_recursive']) {
$entities = getSonsOf(getTableForItemType('Entity'), $loc_c->fields['entities_id']);
}

// tries for 'tab'
if (false === ($c_id = self::findContainer($item::class, $type, $subtype)) && false === $c_id = self::findContainer($item::class)) {
return false;
if ($item->isEntityAssign() && !in_array($item->getEntityID(), $entities)) {
continue;
}
}

$loc_c = new PluginFieldsContainer();
$loc_c->getFromDB($c_id);
$result = self::checkContainerMandatory($item, $loc_c);
if ($result === false) {
$item->input = [];

// check rights on $c_id
// The profile check is only enforced when an active user profile is present in session.
// Automated contexts (cron jobs, API token sessions without profile) bypass the check
// so that plugin fields can still be persisted — authentication is already enforced
// at a higher level by the GLPI API/cron layer.
if (isset($_SESSION['glpiactiveprofile']['id']) && $_SESSION['glpiactiveprofile']['id'] != null && $c_id > 0) {
$right = PluginFieldsProfile::getRightOnContainer($_SESSION['glpiactiveprofile']['id'], $c_id);
if (($right > READ) === false) {
return false;
}

if ($result !== []) {
$submitted_data = $result;
}
}

if ($submitted_data !== null) {
$item->input['_plugin_fields_data'] = $submitted_data;

// need to check if container is usable on this object entity
$entities = [$loc_c->fields['entities_id']];
if ($loc_c->fields['is_recursive']) {
$entities = getSonsOf(getTableForItemType('Entity'), $loc_c->fields['entities_id']);
return true;
}

if (count($item->fields) === 0) {
$item->fields = $item->input;
}
return false;
}

if ($item->isEntityAssign() && !in_array($item->getEntityID(), $entities)) {
return false;
}
/**
* Validate a single container's mandatory fields for the given item, using
* either the values submitted in this request or, if none were submitted
* for this container, the item's already persisted values.
*
* @return array|false The data to persist, an empty array if nothing was
* submitted but validation passed, or false if a
* mandatory field is missing (an error message has
* then been queued by validateValues()).
*/
private static function checkContainerMandatory(CommonDBTM $item, PluginFieldsContainer $loc_c): array|false
{
$c_id = $loc_c->getID();

if (false !== ($data = self::populateData($c_id, $item))) {
if (self::validateValues($data, $item::getType(), isset($_REQUEST['massiveaction'])) === false) {
$item->input = [];

return false;
}

$item->input['_plugin_fields_data'] = $data;
return $data;
}

return true;
//nothing submitted for this container in this request (e.g. untouched Tab)
//tab containers can't be filled before the item exists, so skip on creation
if ($item->isNewItem() && $loc_c->fields['type'] !== 'dom') {
return [];
}

//call validateValues() with a minimal data array to check for missing mandatory fields
//in case populateData() fails
if ($item->isNewItem() && $loc_c->fields['type'] === 'dom') {
$status_field_name = PluginFieldsStatusOverride::getStatusFieldName($item::getType());
$data = ['plugin_fields_containers_id' => $c_id];
if (array_key_exists($status_field_name, $item->input) && $item->input[$status_field_name] !== '') {
$data[$status_field_name] = (int) $item->input[$status_field_name];
} elseif (array_key_exists($status_field_name, $item->fields) && $item->fields[$status_field_name] !== '') {
$data[$status_field_name] = (int) $item->fields[$status_field_name];
}
$status_field_name = PluginFieldsStatusOverride::getStatusFieldName($item::getType());
$data = ['plugin_fields_containers_id' => $c_id];
if (array_key_exists($status_field_name, $item->input) && $item->input[$status_field_name] !== '') {
$data[$status_field_name] = (int) $item->input[$status_field_name];
} elseif (array_key_exists($status_field_name, $item->fields) && $item->fields[$status_field_name] !== '') {
$data[$status_field_name] = (int) $item->fields[$status_field_name];
}

if (self::validateValues($data, $item::getType(), isset($_REQUEST['massiveaction'])) === false) {
$item->input = [];
if (!$item->isNewItem()) {
// merge already persisted values to avoid false positives
$classname = self::getClassname($item::getType(), $loc_c->fields['name']);
$dbu = new DbUtils();
$obj = $dbu->getItemForItemtype($classname);
if ($obj !== false && $obj->getFromDBByCrit(['items_id' => $item->getID()])) {
$data += $obj->fields;
}
}

return false;
if (self::validateValues($data, $item::getType(), isset($_REQUEST['massiveaction'])) === false) {
return false;
}

return [];
}

/**
Expand Down
103 changes: 103 additions & 0 deletions tests/Units/ContainerItemUpdateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
use Ticket;
use Entity;
use Notification;
use Problem;

require_once __DIR__ . '/../FieldTestCase.php';

Expand Down Expand Up @@ -431,6 +432,108 @@ public function testCreateIsNotBlockedWhenMandatoryTabOrDomtabFieldIsMissing():
$this->assertGreaterThan(0, $ticket_id, 'A mandatory domtab field must not block creation.');
}

/**
* A mandatory TAB field left empty must block a later update, even when
* that update never touches the Tab (e.g. a plain status change on the
* main form). Regression test for GH issue #1268.
*/
public function testUpdateIsBlockedWhenMandatoryTabFieldWasNeverFilled(): void
{
$this->login();

// Problem is used here (not Ticket) since it has no pre-existing "dom"
// container in this environment, which would otherwise take priority
// over the "tab" container when no explicit c_id is given.
$tab_container = $this->createFieldContainer([
'label' => 'Mandatory Tab Update Container',
'type' => 'tab',
'itemtypes' => [Problem::class],
'is_active' => 1,
'entities_id' => 0,
'is_recursive' => 1,
]);
$this->createField([
'label' => 'Mandatory Tab Field',
'type' => 'text',
PluginFieldsContainer::getForeignKeyField() => $tab_container->getID(),
'ranking' => 1,
'is_active' => 1,
'is_readonly' => 0,
'mandatory' => 1,
]);

$problem = $this->createItem(Problem::class, [
'name' => 'Problem with mandatory tab field missing',
'content' => 'Test',
'entities_id' => 0,
]);

// Update the main form only, without ever opening the Tab.
$updated = $problem->update([
'id' => $problem->getID(),
'name' => 'Renamed while tab field still empty',
]);
$this->assertFalse($updated, 'Update must be blocked while a mandatory tab field is empty.');
$this->hasSessionMessageThatContains(
__('Some mandatory fields are empty', 'fields'),
ERROR,
);

$problem->getFromDB($problem->getID());
$this->assertNotSame('Renamed while tab field still empty', $problem->fields['name']);
}

/**
* Once a mandatory TAB field has been filled, later updates that don't
* touch the Tab must not be wrongly blocked.
*/
public function testUpdateIsNotBlockedWhenMandatoryTabFieldIsAlreadyFilled(): void
{
$this->login();

$tab_container = $this->createFieldContainer([
'label' => 'Mandatory Tab Filled Container',
'type' => 'tab',
'itemtypes' => [Problem::class],
'is_active' => 1,
'entities_id' => 0,
'is_recursive' => 1,
]);
$field = $this->createField([
'label' => 'Mandatory Tab Field',
'type' => 'text',
PluginFieldsContainer::getForeignKeyField() => $tab_container->getID(),
'ranking' => 1,
'is_active' => 1,
'is_readonly' => 0,
'mandatory' => 1,
]);
$field_name = $field->fields['name'];

$problem = $this->createItem(Problem::class, [
'name' => 'Problem with mandatory tab field',
'content' => 'Test',
'entities_id' => 0,
]);

// Fill the mandatory tab field through its own container update.
$this->updateItem(Problem::class, $problem->getID(), [
'id' => $problem->getID(),
'c_id' => $tab_container->getID(),
$field_name => 'filled value',
], [$field_name, 'c_id']);

// A later update that doesn't touch the tab must succeed.
$updated = $problem->update([
'id' => $problem->getID(),
'name' => 'Renamed after tab field was filled',
]);
$this->assertTrue($updated, 'Update must not be blocked once the mandatory tab field is filled.');

$problem->getFromDB($problem->getID());
$this->assertSame('Renamed after tab field was filled', $problem->fields['name']);
}

/**
* Update a ticket with explicit c_id through an API-like context.
*/
Expand Down
Loading