diff --git a/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po index b611c179e8..a5c1dc68f5 100644 --- a/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po @@ -9271,6 +9271,9 @@ msgstr "Fehler beim Erstellen eines Tasks" msgid "The current user is invalid." msgstr "Task-Typ ist nicht gültig" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po index 2c1e9f92e7..b9bbc68341 100644 --- a/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po @@ -9280,6 +9280,9 @@ msgstr "Failed to create task" msgid "The current user is invalid." msgstr "Task type is not valid" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po index 0fc0a3b51d..e827bdc418 100644 --- a/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po @@ -9443,6 +9443,9 @@ msgstr "No se pudo crear la tarea" msgid "The current user is invalid." msgstr "tipo de tarea no es válida" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po index c259762c71..d6a14afcd7 100644 --- a/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po @@ -9272,6 +9272,9 @@ msgstr "Fehler beim Erstellen eines Tasks" msgid "The current user is invalid." msgstr "Task-Typ ist nicht gültig" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po index ff229d8bc6..a2e3b668f2 100644 --- a/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po @@ -9265,6 +9265,9 @@ msgstr "Impossible de créer la tâche" msgid "The current user is invalid." msgstr "Type de tâche n'est pas valide" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po index 66db4d5575..065253cd5b 100644 --- a/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po @@ -8986,6 +8986,9 @@ msgstr "Impossibile creare un'attività" msgid "The current user is invalid." msgstr "Tipo di attività non è valido" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po index 29a029f68a..dd03a306ec 100644 --- a/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po @@ -8929,6 +8929,9 @@ msgstr "タスクの作成に失敗しました" msgid "The current user is invalid." msgstr "" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/messages.pot b/packages/web/management/languages/messages.pot index 8e4fa039a4..5397aa7b0a 100644 --- a/packages/web/management/languages/messages.pot +++ b/packages/web/management/languages/messages.pot @@ -7905,6 +7905,9 @@ msgstr "" msgid "The current user is invalid." msgstr "" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po index 49cf83b80c..e675c44999 100644 --- a/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po @@ -9267,6 +9267,9 @@ msgstr "Falha ao criar tarefa" msgid "The current user is invalid." msgstr "tipo de tarefa não é válido" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po index 084d8e4326..5fd5afffa1 100644 --- a/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po @@ -9267,6 +9267,9 @@ msgstr "无法创建任务" msgid "The current user is invalid." msgstr "任务类型无效" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/src/Pages/HostManagement.php b/packages/web/src/Pages/HostManagement.php index 588b55c5bb..65ee1eeee0 100644 --- a/packages/web/src/Pages/HostManagement.php +++ b/packages/web/src/Pages/HostManagement.php @@ -4226,9 +4226,18 @@ private function massEditSelection() private function massEditCoreFields() { return [ + // NULL, not 0. `hosts`.`hostImage` carries a foreign key to + // `images`.`imageID` (ADR 0031, schema-constraints.php:145) and + // 0 is not exempt from a constraint just because it looks like + // an absence -- there is no image with id 0, so clearing to 0 is + // rejected outright and the whole mass edit fails. The column is + // nullable and the reconciler already swept every legacy 0 to + // NULL, so NULL is what "no image" actually IS on any 1.6 + // database. Verified against a live install: 77 hosts hold NULL + // and none holds 0. 'image' => [ 'field' => 'imageID', - 'empty' => 0, + 'empty' => null, 'label' => _('Image'), 'kind' => 'image', 'tab' => 'general' @@ -5143,8 +5152,18 @@ public function massEditPost() // non-empty because an UPDATE with no assignments is either // a syntax error or, worse, a statement whose WHERE is the // only part left. - self::getClass('HostManager') - ->update(['id' => $hosts], '', $updates); + // The return value is CHECKED. perform_update() answers + // false and writes a fault rather than throwing, so ignoring + // it reported "Updated 1 field(s) on 86 host(s)" for a write + // the database had refused -- which is how a clear that + // never landed reads as a clear that did. + if (!self::getClass('HostManager') + ->update(['id' => $hosts], '', $updates) + ) { + throw new \Exception( + _('The database refused the update; nothing was changed') + ); + } $affected = count($hosts); } // Row-backed fields count toward the same affectedCount: one diff --git a/packages/web/src/Util/MassEdit.php b/packages/web/src/Util/MassEdit.php index fca97ad5e5..34fe128ffa 100644 --- a/packages/web/src/Util/MassEdit.php +++ b/packages/web/src/Util/MassEdit.php @@ -231,8 +231,11 @@ public static function resolveComposite(array $keys, $actions, $values) * @param array $resolved output of resolve() * @param array $spec key => ['field' => , * 'empty' => ]. - * A key absent from the spec is skipped: the spec - * is what says a key may touch a column at all. + * `empty` may be null, and null is written as + * NULL rather than treated as absent -- that is + * what a column with a foreign key needs. A key + * absent from the spec is skipped: the spec is + * what says a key may touch a column at all. * * @return array field name => value, ready for a manager update(). EMPTY * when nothing was set or cleared -- and an empty map must @@ -260,11 +263,23 @@ public static function columnUpdates(array $resolved, array $spec) $updates[$spec[$key]['field']] = $instruction['value']; } elseif (self::CLEAR === $action) { // Per field, because "empty" is not one value. A varchar - // column clears to '', an int foreign key to 0; writing '' - // into an int column stores 0 on a permissive server and - // errors on a strict one, so the answer belongs in the spec - // rather than in a cast here. - $updates[$spec[$key]['field']] = $spec[$key]['empty'] ?? ''; + // column clears to '', a plain int to 0, and a column + // carrying a FOREIGN KEY to null -- 0 is not exempt from a + // constraint, so clearing `hosts`.`hostImage` to 0 is + // rejected outright unless an image with id 0 exists, which + // none does. The answer belongs in the spec rather than in a + // cast here. + // + // array_key_exists, NOT `?? ''`. Null is the whole point for + // the foreign-key case and `??` treats it as absent, so the + // coalesce silently turned every intended NULL back into '' + // -- which an int column then stores as 0, i.e. exactly the + // value that fails. A spec that genuinely omits `empty` + // still gets ''. + $updates[$spec[$key]['field']] = + array_key_exists('empty', $spec[$key]) + ? $spec[$key]['empty'] + : ''; } } diff --git a/tests/fk-columns-clear-to-null.test.php b/tests/fk-columns-clear-to-null.test.php new file mode 100644 index 0000000000..f42525226a --- /dev/null +++ b/tests/fk-columns-clear-to-null.test.php @@ -0,0 +1,255 @@ + 0` entry in + * commons/schema-constraints.php saying 0 was once its "none" value. Every + * one of them rejects a literal 0 today. A hand-written list would be a + * second place that has to agree with the constraint file, and the failure + * when it did not agree would be exactly this bug again, silently. So the + * constraint file is read, the column is mapped back to its FOGController + * field name through the Items' own $databaseFields, and any mass-edit spec + * naming one of those fields is required to clear it to null. + * + * A NEW field added to the spec for an FK-bearing column fails here on the + * day it is added rather than on the day somebody clears it. + * + * The second half is executed rather than grepped: columnUpdates() used + * `$spec[$key]['empty'] ?? ''`, and `??` treats null as absent -- so even + * with the spec corrected, the coalesce turned the intended NULL straight + * back into '', which an int column stores as 0. The fix and the spec change + * are worthless without each other, so both are checked. + * + * Usage: php tests/fk-columns-clear-to-null.test.php + * Exit status 0 = pass, 1 = fail. + * + * PHP version 7.4+ + * + * @category Tests + * @package FOGProject + * @author Tom Elliott + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +require_once __DIR__ . '/lib/fog-test-harness.php'; + +// boot() owns the temp dirs, the constants, Initiator and the language +// globals -- the same preamble every other test in the suite uses, rather +// than a hand-rolled copy that drifts from it. +FogTestHarness::boot('fk-clear'); + +$root = dirname(__DIR__); +$webroot = $root . '/packages/web'; + +use FOG\Util\MassEdit; + +$checks = 0; +$failures = []; +$check = static function ($what, $ok) use (&$checks, &$failures) { + $checks++; + if (!$ok) { + $failures[] = $what; + } +}; + +// --- The FK-bearing columns, straight off the constraint file -------------- + +$constraintFile = $webroot . '/commons/schema-constraints.php'; +$constraintSrc = (string)file_get_contents($constraintFile); +$check( + 'the constraint file is readable', + '' !== $constraintSrc +); + +// Every relation that declares a 0 sentinel: the constraint file saying so +// IS the statement that 0 used to mean "none" here and no longer may. +preg_match_all( + "/'child'\s*=>\s*'([^']+)'\s*,\s*'column'\s*=>\s*'([^']+)'" + . "(?=[^\]]*'sentinel'\s*=>\s*0)/", + $constraintSrc, + $m, + PREG_SET_ORDER +); +$sentinelColumns = []; +foreach ($m as $hit) { + $sentinelColumns[strtolower($hit[2])] = $hit[1]; +} +$check( + 'the constraint file still declares sentinel columns (' + . count($sentinelColumns) . ' found)', + count($sentinelColumns) > 0 +); + +// --- Map each column back to the field name a spec would name -------------- + +// Scoped to ONE Item class, because a field name is only unique within one. +// `imageID` is `hosts`.`hostImage` on Host and `tasks`.`taskImageID` on Task, +// and a global map keyed on the field name silently picks whichever file was +// read last -- which would name the wrong column in a failure message and, +// worse, would flag a field that is a plain int here because it is a foreign +// key somewhere else. +$fieldMap = static function ($itemClass) use ($webroot) { + $src = (string)@file_get_contents( + $webroot . '/src/Items/' . $itemClass . '.php' + ); + $map = []; + if ('' === $src + || !preg_match( + '/\$databaseFields\s*=\s*\[(.*?)\n \];/s', + $src, + $block + ) + ) { + return $map; + } + preg_match_all( + "/'([A-Za-z0-9_]+)'\s*=>\s*'([A-Za-z0-9_]+)'/", + $block[1], + $pairs, + PREG_SET_ORDER + ); + foreach ($pairs as $pair) { + $map[$pair[1]] = strtolower($pair[2]); + } + return $map; +}; + +// massEditCoreFields() names Host's fields, so Host's map is the right one. +$hostFields = $fieldMap('Host'); +$check( + 'Host\'s databaseFields map was parsed (' . count($hostFields) . ' fields)', + count($hostFields) > 10 +); +$fieldsForColumn = []; +foreach ($hostFields as $field => $column) { + if (isset($sentinelColumns[$column])) { + $fieldsForColumn[$field] = $column; + } +} +$check( + 'at least one sentinel column maps to a field name (' + . implode(', ', array_keys($fieldsForColumn)) . ')', + count($fieldsForColumn) > 0 +); +$check( + 'imageID maps to `hosts`.`hostImage` -- the field the original bug was on', + ($fieldsForColumn['imageID'] ?? null) === 'hostimage' +); + +// --- No mass-edit spec may clear one of them to anything but null --------- + +$class = \FOG\Pages\HostManagement::class; +$page = (new \ReflectionClass($class))->newInstanceWithoutConstructor(); +$call = static function ($method) use ($class, $page) { + $m = new \ReflectionMethod($class, $method); + $m->setAccessible(true); + return $m->invoke($page); +}; +$core = $call('massEditCoreFields'); + +$wrong = []; +$guarded = 0; +foreach ($core as $key => $spec) { + if (!isset($spec['field']) || !isset($fieldsForColumn[$spec['field']])) { + continue; + } + $guarded++; + // array_key_exists, not isset: a spec that correctly says null must not + // read here as a spec that forgot to say anything. + if (!array_key_exists('empty', $spec) || null !== $spec['empty']) { + $wrong[] = $key . ' (' . $spec['field'] . ' -> `' + . $fieldsForColumn[$spec['field']] . '`)'; + } +} +$check( + 'the spec actually covers a foreign-key field, or this test proves nothing', + $guarded > 0 +); +$check( + 'every foreign-key field clears to NULL, not to a sentinel (' + . implode(', ', $wrong) . ')', + 0 === count($wrong) +); + +// --- columnUpdates() must PRESERVE that null ------------------------------ + +$resolved = MassEdit::resolve( + ['image'], + ['image' => MassEdit::CLEAR], + [] +); +$check( + 'a clear with no posted value still resolves to CLEAR', + MassEdit::CLEAR === $resolved['image']['action'] +); +$updates = MassEdit::columnUpdates($resolved, $core); +$check( + 'clearing the image reaches the column map at all', + array_key_exists('imageID', $updates) +); +$check( + 'and reaches it as NULL -- `?? \'\'` would have made it the empty string,' + . ' which an int column stores as the 0 the constraint refuses', + array_key_exists('imageID', $updates) && null === $updates['imageID'] +); + +// A spec that genuinely omits `empty` still gets the empty string, so the +// change above did not quietly turn every unspecified field into NULL. +$plain = MassEdit::columnUpdates( + MassEdit::resolve(['thing'], ['thing' => MassEdit::CLEAR], []), + ['thing' => ['field' => 'thing']] +); +$check( + 'a spec with no `empty` at all still clears to the empty string', + array_key_exists('thing', $plain) && '' === $plain['thing'] +); +// And an explicit 0 is still an explicit 0, for the plain int columns that +// genuinely clear that way. +$zero = MassEdit::columnUpdates( + MassEdit::resolve(['lvl'], ['lvl' => MassEdit::CLEAR], []), + ['lvl' => ['field' => 'lvl', 'empty' => 0]] +); +$check( + 'an explicit 0 empty is still written as 0', + array_key_exists('lvl', $zero) && 0 === $zero['lvl'] +); + +// --- The write must not report success when the database refused ---------- + +$source = (string)file_get_contents($webroot . '/src/Pages/HostManagement.php'); +$check( + 'massEditPost() checks the update return rather than discarding it', + 1 === preg_match( + '/if\s*\(\s*!\s*self::getClass\(\s*.HostManager.\s*\)\s*' + . '->update\(\s*\[\s*.id.\s*=>\s*\$hosts\s*\]/s', + $source + ) +); + +if (count($failures)) { + fwrite( + STDERR, + "FAIL: a mass edit would write a sentinel into a foreign-key column:\n" + ); + foreach ($failures as $f) { + fwrite(STDERR, " - $f\n"); + } + fwrite( + STDERR, + sprintf("%d of %d checks failed\n", count($failures), $checks) + ); + exit(1); +} + +printf("PASS foreign-key columns clear to NULL: %d checks\n", $checks); +exit(0); diff --git a/tests/mass-edit-endpoint-is-gated.test.php b/tests/mass-edit-endpoint-is-gated.test.php index d73df8f9fa..301570a793 100644 --- a/tests/mass-edit-endpoint-is-gated.test.php +++ b/tests/mass-edit-endpoint-is-gated.test.php @@ -111,7 +111,10 @@ $auth = $at('self::checkAuthAndCSRF();'); $scope = $at("Authorization::requirePageObjectScopeMass('host', \$hosts);"); $resolve = $at('$resolved = MassEdit::resolve('); -$write = $at("->update(['id' => \$hosts], '', \$updates);"); +// The call is wrapped in `if (!...)` now, so the statement no longer ends in +// `;` right after the arguments -- the write itself is unchanged, and this +// anchors on the arguments rather than on the punctuation around them. +$write = $at("->update(['id' => \$hosts], '', \$updates)"); $hook = $at("'HOST_MASSEDIT_APPLY',"); $check('it checks auth and CSRF', false !== $auth);