From d7af810503fb5adc0dba0a78e4cf40b724ddec23 Mon Sep 17 00:00:00 2001 From: mbressy Date: Mon, 21 Sep 2026 08:45:36 +0000 Subject: [PATCH 1/3] fix skipped mandatory field validation on update --- inc/container.class.php | 37 ++++++--- tests/Units/ContainerItemUpdateTest.php | 103 ++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 12 deletions(-) diff --git a/inc/container.class.php b/inc/container.class.php index 3aa78b09..5f0ca11d 100644 --- a/inc/container.class.php +++ b/inc/container.class.php @@ -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']; @@ -1984,22 +1985,34 @@ public static function preItem(CommonDBTM $item) return true; } - //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]; - } + //fallback check when populateData() found nothing submitted (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 false; + } - if (self::validateValues($data, $item::getType(), isset($_REQUEST['massiveaction'])) === false) { - $item->input = []; + $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 (!$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; } } + if (self::validateValues($data, $item::getType(), isset($_REQUEST['massiveaction'])) === false) { + $item->input = []; + } + return false; } diff --git a/tests/Units/ContainerItemUpdateTest.php b/tests/Units/ContainerItemUpdateTest.php index 587a13be..e381a97e 100644 --- a/tests/Units/ContainerItemUpdateTest.php +++ b/tests/Units/ContainerItemUpdateTest.php @@ -37,6 +37,7 @@ use Ticket; use Entity; use Notification; +use Problem; require_once __DIR__ . '/../FieldTestCase.php'; @@ -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. */ From e34d7e6233ee589ed02e1ea5d88f4ab83acf6b1a Mon Sep 17 00:00:00 2001 From: mbressy Date: Mon, 21 Sep 2026 08:51:36 +0000 Subject: [PATCH 2/3] update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93da59e3..01ca2a42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 From f8f9f10abb9ee02bd2ecc05a6aaecba3e0ef87ac Mon Sep 17 00:00:00 2001 From: mbressy Date: Tue, 22 Sep 2026 14:09:23 +0000 Subject: [PATCH 3/3] fix mandatory check missing sibling containers --- inc/container.class.php | 119 ++++++++++++++++++++++++++-------------- 1 file changed, 77 insertions(+), 42 deletions(-) diff --git a/inc/container.class.php b/inc/container.class.php index 5f0ca11d..5dd45dee 100644 --- a/inc/container.class.php +++ b/inc/container.class.php @@ -1921,74 +1921,109 @@ 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 true; + return $data; } - //fallback check when populateData() found nothing submitted (e.g. untouched Tab) + //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 false; + return []; } $status_field_name = PluginFieldsStatusOverride::getStatusFieldName($item::getType()); @@ -2010,10 +2045,10 @@ public static function preItem(CommonDBTM $item) } if (self::validateValues($data, $item::getType(), isset($_REQUEST['massiveaction'])) === false) { - $item->input = []; + return false; } - return false; + return []; } /**