From 73916a4f50f8be7bff60e87aa993b5ac2a124610 Mon Sep 17 00:00:00 2001 From: JJ Fullmer <7743340+darksidemilk@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:57:08 -0500 Subject: [PATCH] fix(orm): keep a host with no primary MAC loadable, and self-heal it Port of working-1.6's 1cd7446f6, plus the tests that commit did not ship. `$databaseFieldClassRelationships` entries may carry an optional 4th element -- a filter on the joined table. One exists in the whole codebase, and it is load-bearing: 'MACAddressAssociation' => ['hostID', 'id', 'primac', ['primary' => 1]] so `$host->get('primac')` is the PRIMARY MAC rather than whichever row came back first. buildQuery() emitted that filter into $whereArrayAnd, and a WHERE predicate on the right-hand table of a LEFT OUTER JOIN is not a filter -- it is an INNER JOIN written the long way. Rows with nothing to join to are dropped, so a host with no hmPrimary='1' row stopped existing: new Host($id)->isValid() -> false HostManager->find(['id' => $id]) -> 0 objects GET /fog/host/$id -> 404 on a row sitting in the table -- un-loadable, un-editable, un-deletable. Verified against a 2079-host database where 1953 hosts have no primary MAC. buildQuery() recurses, so the same predicate reached every class whose relationship chain passes through Host: Task, SnapinJob, SnapinTask, ImagingLog, NodeFailure, UserTracking, LocationAssociation and SiteHostAssociation. A task belonging to such a host was invisible to TaskManager, which is the half that turns a display bug into an operational one -- and the reason Site::hostIDsForSites() had to read siteHostAssoc directly in #1233. That direct read is now belt and braces rather than the only thing holding. Moving the filter into the JOIN ON clause fixes every caller at once; the $whereInfo closure it was the sole user of goes with it. Second half: Host::save() now promotes the first remaining approved (non-pending) MAC when nothing is flagged primary, so an update that replaces the MAC set cannot strand the host again. It leaves a pending MAC pending -- approving an unapproved MAC as a side effect of an unrelated save would be a worse bug than the one being fixed -- and does nothing when a primary already exists. Two tests, because the two arms catch different regressions: - relationship-filter-in-join.test.php reads the emitted SQL, needs no database, and so is the arm that gates CI. Also added to working-1.6, which has carried the fix untested since June. - macless-host-reachable.test.php drives real rows through a real schema -- five host shapes, load, find, a transitive TaskManager lookup and all four self-heal cases -- and SKIPs where there is no database. Both were written failing and each gate mutation-verified. Co-Authored-By: Claude --- packages/web/lib/fog/fogcontroller.class.php | 71 ++--- packages/web/lib/fog/host.class.php | 38 +++ tests/macless-host-reachable.test.php | 308 +++++++++++++++++++ tests/relationship-filter-in-join.test.php | 185 +++++++++++ 4 files changed, 560 insertions(+), 42 deletions(-) create mode 100644 tests/macless-host-reachable.test.php create mode 100644 tests/relationship-filter-in-join.test.php diff --git a/packages/web/lib/fog/fogcontroller.class.php b/packages/web/lib/fog/fogcontroller.class.php index f55e04bc3b..b6f9ed995d 100644 --- a/packages/web/lib/fog/fogcontroller.class.php +++ b/packages/web/lib/fog/fogcontroller.class.php @@ -1035,42 +1035,6 @@ public function buildQuery( $not = false, $compare = '=' ) { - /** - * Lambda function to build the where array additionals. - * - * @param string $field the field to work from - * @param mixed $value the value of the field - */ - $whereInfo = function ( - &$value, - $field - ) use ( - &$whereArrayAnd, - &$c, - $not, - $compare - ) { - if (is_array($value)) { - $whereArrayAnd[] = sprintf( - "`%s`.`%s` IN ('%s')", - $c->databaseTable, - $field, - implode("','", $value) - ); - } else { - if (strpos($value, '%')) { - $compare = 'LIKE'; - } - $whereArrayAnd[] = sprintf( - "`%s`.`%s` %s '%s'", - $c->databaseTable, - $c->databaseFields[$field], - $compare, - $value - ); - } - unset($value, $field); - }; /** * Lambda function to build the join of a query. * @@ -1084,25 +1048,48 @@ public function buildQuery( &$join, &$whereArrayAnd, &$c, - $whereInfo, $not, $compare ) { $className = strtolower($class); $c = self::getClass($class); if (!array_key_exists($className, $join)) { + // The relationship's optional 4th element is a filter on the + // joined (optional) table. It must live in the JOIN ON clause, + // not in WHERE: a WHERE condition on the right-hand table of a + // LEFT JOIN silently degrades it to an INNER JOIN, dropping the + // base row entirely when there is no matching joined row (e.g. + // a host with no primary MAC would fail to load at all). + $onExtra = ''; + if (isset($fields[3]) && $fields[3]) { + foreach ((array) $fields[3] as $filterField => $filterValue) { + if (is_array($filterValue)) { + $onExtra .= sprintf( + " AND `%s`.`%s` IN ('%s')", + $c->databaseTable, + $c->databaseFields[$filterField], + implode("','", $filterValue) + ); + } else { + $onExtra .= sprintf( + " AND `%s`.`%s` = '%s'", + $c->databaseTable, + $c->databaseFields[$filterField], + $filterValue + ); + } + } + } $join[$className] = sprintf( - ' LEFT OUTER JOIN `%s` ON `%s`.`%s`=`%s`.`%s` ', + ' LEFT OUTER JOIN `%s` ON `%s`.`%s`=`%s`.`%s`%s ', $c->databaseTable, $c->databaseTable, $c->databaseFields[$fields[0]], $this->databaseTable, - $this->databaseFields[$fields[1]] + $this->databaseFields[$fields[1]], + $onExtra ); } - if (isset($fields[3])) { - array_walk($fields[3], $whereInfo); - } $c->buildQuery($join, $whereArrayAnd, $c, $not, $compare); unset($class, $fields, $c); }; diff --git a/packages/web/lib/fog/host.class.php b/packages/web/lib/fog/host.class.php index 72c3d2394b..aded46d11b 100644 --- a/packages/web/lib/fog/host.class.php +++ b/packages/web/lib/fog/host.class.php @@ -622,6 +622,44 @@ public function save() $objNeeded = false; unset($DBPowerManagementIDs, $RemovePowerManagementIDs); } + // Safety net: never leave the host with MAC rows but no primary MAC. + // The primac join requires hmPrimary='1', so a host with no primary + // MAC cannot be loaded and becomes un-editable via the API/GUI. If an + // update (e.g. replacing the MAC set) removed the former primary, + // promote the first remaining approved (non-pending) MAC so the host + // stays reachable. + $hostID = $this->get('id'); + if ($hostID) { + $primaryMacs = self::getSubObjectIDs( + 'MACAddressAssociation', + array( + 'hostID' => $hostID, + 'primary' => '1' + ), + 'mac' + ); + if (count((array)$primaryMacs) < 1) { + $approvedMacs = self::getSubObjectIDs( + 'MACAddressAssociation', + array( + 'hostID' => $hostID, + 'pending' => '0' + ), + 'mac' + ); + if (count((array)$approvedMacs) > 0) { + self::getClass('MACAddressAssociationManager') + ->update( + array( + 'hostID' => $hostID, + 'mac' => array_shift($approvedMacs) + ), + '', + array('primary' => '1') + ); + } + } + } return $this ->assocSetter('Module') ->assocSetter('Printer') diff --git a/tests/macless-host-reachable.test.php b/tests/macless-host-reachable.test.php new file mode 100644 index 0000000000..b124e5b8c5 --- /dev/null +++ b/tests/macless-host-reachable.test.php @@ -0,0 +1,308 @@ + 1), so that $host->get('primac') is the primary MAC. + * buildQuery() emitted that filter into the WHERE clause -- and a WHERE + * predicate on the optional side of a LEFT OUTER JOIN silently degrades it to + * an INNER JOIN. A host with no hmPrimary='1' row therefore matched nothing: + * + * new Host($id)->isValid() -> false + * HostManager->find(array('id' => $id)) -> 0 objects + * + * on a row that is sitting in the table. The host is un-loadable, + * un-editable and un-deletable through the ORM, and the API answers 404. + * Fixed on working-1.6 in 1cd7446f6 (June 2026); this is the same fix here, + * plus the tests that commit did not ship. + * + * IT IS NOT ONLY HOSTS. buildQuery() recurses, so the filter reaches every + * class whose relationship chain passes through Host -- on this branch that + * is Task, SnapinJob, SnapinTask, ImagingLog, NodeFailure, UserTracking, + * LocationAssociation, SiteHostAssociation and the example plugin. A task + * belonging to a host with no primary MAC was invisible to TaskManager. Those + * are asserted too, because fixing only the Host case would leave the + * interesting half of the bug in place. + * + * How a host loses its primary MAC in the first place: an update that + * replaces the MAC set, a primary MAC deleted, a MAC left pending, or a host + * created through the API or a CSV import with no MAC at all. Host::save() + * now promotes the first remaining approved MAC rather than leaving the host + * stranded, which is the second half of the same fix. + * + * Needs a real 1.5 schema -- the failure IS the SQL, so a fake database + * cannot show it. SKIPs when there is none. + * + * Usage: php tests/macless-host-reachable.test.php + * Exit status 0 = pass (or skip), 1 = fail. + */ + +$web = dirname(__DIR__) . '/packages/web'; + +require __DIR__ . '/lib/scope-harness.php'; + +$reason = scopeHarnessDbReason(); +if (null !== $reason) { + echo "SKIP: $reason\n"; + exit(0); +} + +$tmp = sys_get_temp_dir() . '/fog-macless-' . getmypid(); +@mkdir($tmp . '/cache', 0700, true); +@mkdir($tmp . '/log', 0700, true); +@mkdir($tmp . '/plugins', 0700, true); +register_shutdown_function( + function () use ($tmp) { + if (!is_dir($tmp)) { + return; + } + $it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($tmp, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($it as $f) { + $f->isDir() ? @rmdir($f->getPathname()) : @unlink($f->getPathname()); + } + @rmdir($tmp); + } +); +define('FOG_CACHE_DIR', $tmp . '/cache'); +define('FOG_LOG_DIR', $tmp . '/log'); +define('FOG_PLUGIN_DIR', $tmp . '/plugins'); +error_reporting(E_ALL & ~E_WARNING & ~E_NOTICE & ~E_DEPRECATED); + +require_once $web . '/commons/init.php'; +new Initiator(); +$dbProp = new \ReflectionProperty('FOGBase', 'DB'); +$dbProp->setAccessible(true); +$db = new PDODB(); +$dbProp->setValue(null, $db); +// FOGController::save() stamps createdBy from the acting user, and reads it +// unconditionally. Nothing is logged in as a test process, so seat an empty +// (invalid) User -- exactly what a logged-out request already has. +$userProp = new \ReflectionProperty('FOGBase', 'FOGUser'); +$userProp->setAccessible(true); +$userProp->setValue(null, new User(0)); + +$failures = []; +$checks = 0; + +function check($label, $cond, array &$failures, &$checks) +{ + $checks++; + if (!$cond) { + $failures[] = $label; + } +} + +/* + * Fixture: three hosts, differing only in their MAC rows. + * + * primary one MAC, hmPrimary='1' -- the control + * nonprim one MAC, hmPrimary='0' -- had a MAC, lost its primary flag + * nomac no MAC rows at all -- created by API or import + * pending one MAC, hmPending='1' -- MAC seen but not yet approved + * multi primary + an additional -- the ordinary multi-NIC machine + * multi2 the same, rows reversed -- primary is not the lowest hmID + * + * The control matters: without it "returns nothing" would pass for a fixture + * that never inserted anything. + */ +// `hostName` is varchar(16) -- the NetBIOS limit -- and carries a unique +// index, so a long fixture prefix truncates and the second host silently +// fails to insert. Keep the whole name inside sixteen characters. +$mark = 'zzml' . (getmypid() % 100000); +register_shutdown_function( + function () use ($db, $mark) { + $db->query( + "DELETE `hostMAC` FROM `hostMAC` JOIN `hosts`" + . " ON `hosts`.`hostID` = `hostMAC`.`hmHostID`" + . " WHERE `hosts`.`hostName` LIKE '" . $mark . "%'" + ); + $db->query( + "DELETE `tasks` FROM `tasks` JOIN `hosts`" + . " ON `hosts`.`hostID` = `tasks`.`taskHostID`" + . " WHERE `hosts`.`hostName` LIKE '" . $mark . "%'" + ); + $db->query("DELETE FROM `hosts` WHERE `hostName` LIKE '" . $mark . "%'"); + } +); + +$mkHost = function ($suffix) use ($db, $mark) { + $db->query( + "INSERT INTO `hosts` (`hostName`,`hostIP`,`hostUseAD`)" + . " VALUES ('" . $mark . $suffix . "','','0')" + ); + return (int)$db->insertId(); +}; +$mkMac = function ($hostID, $primary, $pending = '0', $nth = 0) use ($db) { + // A locally-administered address, so it can never collide with real + // hardware if this fixture ever outlives a crashed run. $nth keeps a + // host's second MAC distinct from its first; hmMAC is unique. + $mac = sprintf('02:%02x:%02x:%02x:%02x:%02x', $nth & 255, ($hostID >> 24) & 255, ($hostID >> 16) & 255, ($hostID >> 8) & 255, $hostID & 255); + $db->query( + "INSERT INTO `hostMAC` (`hmHostID`,`hmMAC`,`hmDesc`,`hmPrimary`,`hmPending`)" + . " VALUES (" . (int)$hostID . ",'" . $mac . "','fixture','" . $primary + . "','" . $pending . "')" + ); +}; + +$hosts = [ + 'primary' => $mkHost('p'), + 'nonprim' => $mkHost('n'), + 'nomac' => $mkHost('x'), + 'pending' => $mkHost('q'), + 'multi' => $mkHost('m'), + 'multi2' => $mkHost('m2'), +]; +$mkMac($hosts['primary'], '1'); +$mkMac($hosts['nonprim'], '0'); +$mkMac($hosts['pending'], '0', '1'); +$mkMac($hosts['multi'], '1'); +$mkMac($hosts['multi'], '0', '0', 1); +// Same two MACs, inserted the other way round: the additional NIC was +// recorded first and the primary set afterwards. Row order is what +// array_shift() picks from, so this is the arrangement in which a +// self-heal that forgot to check for an existing primary would promote +// the WRONG MAC and leave two rows flagged primary. +$mkMac($hosts['multi2'], '0', '0', 1); +$mkMac($hosts['multi2'], '1'); + +/* + * 1. load() -- the single-object path, which is what the API 404 came from. + */ +foreach ($hosts as $label => $id) { + $h = new Host($id); + check( + "new Host(<$label>) loads", + $h->isValid(), + $failures, + $checks + ); + check( + "new Host(<$label>) has its name", + 0 === strpos((string)$h->get('name'), $mark), + $failures, + $checks + ); +} + +/* + * 2. The manager path. Same filter, reached through find(), which is what + * every list, every report and every association lookup runs. + */ +foreach ($hosts as $label => $id) { + $found = FOGBase::getClass('HostManager')->find(['id' => [$id]]); + check( + "HostManager->find(<$label>) returns the host", + 1 === count((array)$found), + $failures, + $checks + ); +} + +/* + * 3. And the classes that only reach the filter transitively. Task is the one + * that matters -- a task nobody can see is a task nobody can cancel -- so + * it is driven for real rather than reasoned about. + */ +foreach ($hosts as $label => $id) { + $db->query( + "INSERT INTO `tasks` (`taskName`,`taskCreateTime`,`taskCheckIn`," + . "`taskHostID`,`taskStateID`,`taskTypeID`,`taskCreateBy`,`taskNFSGroupID`," + . "`taskNFSMemberID`,`taskImageID`,`taskPCT`,`taskBPM`,`taskTimeElapsed`," + . "`taskTimeRemaining`,`taskDataCopied`,`taskDataTotal`,`taskPercentText`)" + . " VALUES ('fixture',NOW(),NOW()," . (int)$id . ",1,1,'fixture',0,0,0," + . "'','','','','','','')" + ); +} +foreach ($hosts as $label => $id) { + $tasks = FOGBase::getClass('TaskManager')->find(['hostID' => [$id]]); + check( + "TaskManager->find(hostID=<$label>) sees the task", + 1 === count((array)$tasks), + $failures, + $checks + ); +} + +/* + * 4. The self-heal. A host holding approved MACs but none flagged primary is + * one save away from being stranded again, so save() promotes the first + * remaining one. The no-MAC host must NOT be given a MAC it does not have. + */ +$h = new Host($hosts['nonprim']); +$h->set('description', 'touched by the test')->save(); +$promoted = FOGBase::getSubObjectIDs( + 'MACAddressAssociation', + ['hostID' => $hosts['nonprim'], 'primary' => '1'], + 'mac' +); +check( + 'save() promotes a remaining approved MAC when none is primary', + 1 === count((array)$promoted), + $failures, + $checks +); +/* + * A MAC awaiting approval is not a MAC the host may boot from, so the + * self-heal must leave it pending rather than reach for the nearest row. + * Promoting it would approve an unapproved MAC as a side effect of an + * unrelated save, which is a worse bug than the one being fixed. + */ +$h3 = new Host($hosts['pending']); +$h3->set('description', 'touched by the test')->save(); +$stillPending = FOGBase::getSubObjectIDs( + 'MACAddressAssociation', + ['hostID' => $hosts['pending'], 'primary' => '1'], + 'mac' +); +check( + 'save() does not promote a MAC that is still pending approval', + 0 === count((array)$stillPending), + $failures, + $checks +); +/* + * And the self-heal must not fire when there is nothing to heal. A machine + * with a primary MAC and a second NIC is the common case, and promoting its + * additional MAC as well would leave two rows flagged primary -- which the + * primac join then answers arbitrarily. + */ +foreach (['multi', 'multi2'] as $label) { + $h4 = new Host($hosts[$label]); + $h4->set('description', 'touched by the test')->save(); + $multiPrimary = FOGBase::getSubObjectIDs( + 'MACAddressAssociation', + ['hostID' => $hosts[$label], 'primary' => '1'], + 'mac' + ); + check( + "save() leaves <$label>, which already has a primary MAC, with exactly one", + 1 === count((array)$multiPrimary), + $failures, + $checks + ); +} +$h2 = new Host($hosts['nomac']); +$h2->set('description', 'touched by the test')->save(); +$invented = FOGBase::getSubObjectIDs( + 'MACAddressAssociation', + ['hostID' => $hosts['nomac']], + 'mac' +); +check( + 'save() does not invent a MAC for a host that has none', + 0 === count((array)$invented), + $failures, + $checks +); + +if (count($failures)) { + fwrite(STDERR, 'FAIL (' . count($failures) . " of $checks):\n"); + foreach ($failures as $f) { + fwrite(STDERR, " - $f\n"); + } + exit(1); +} +echo "ok $checks checks passed\n"; diff --git a/tests/relationship-filter-in-join.test.php b/tests/relationship-filter-in-join.test.php new file mode 100644 index 0000000000..55dbc5e76f --- /dev/null +++ b/tests/relationship-filter-in-join.test.php @@ -0,0 +1,185 @@ + ['hostID', 'id', 'primac', ['primary' => 1]] + * + * so that `$host->get('primac')` is the host's PRIMARY MAC rather than + * whichever row came back first. `buildQuery()` used to emit that filter into + * `$whereArrayAnd`, and a WHERE predicate on the right-hand table of a LEFT + * OUTER JOIN is not a filter -- it is an INNER JOIN written the long way. + * Rows with nothing to join to are dropped by the WHERE, so a host with no + * `hmPrimary='1'` row stopped existing: + * + * new Host($id)->isValid() -> false + * HostManager->find(...) -> 0 objects + * GET /fog/host/$id -> 404 + * + * on a row sitting in the table. Un-loadable, un-editable, un-deletable. + * And `buildQuery()` recurses, so the same predicate reached every class + * whose relationship chain passes through Host -- a task belonging to such a + * host was invisible to TaskManager, which is the half that turns a display + * bug into an operational one. + * + * This is a STRUCTURAL test and needs no database: the defect is in the SQL + * text, so the SQL text is what it reads. The behavioural proof, which drives + * real rows through a real schema, is a separate file and skips where there + * is no database to drive -- meaning this is the arm that actually gates CI. + * + * Usage: php tests/relationship-filter-in-join.test.php + * Exit status 0 = pass, 1 = fail. + */ + +require __DIR__ . '/lib/scope-harness.php'; + +// This branch has no shared FogTestHarness; scopeHarnessBoot() is the +// equivalent, and it already installs a fake database, so nothing here +// touches a real one. +scopeHarnessBoot(dirname(__DIR__) . '/packages/web'); + +$t = new RelFilterChecks(); + +/** + * The assertion helper every test on this branch hand-rolls. + */ +class RelFilterChecks +{ + /** @var array */ + public $failures = array(); + + /** @var int */ + public $count = 0; + + /** + * @param string $label what is being asserted + * @param bool $cond the assertion + * + * @return bool the assertion, so a caller can branch on it + */ + public function check($label, $cond) + { + $this->count++; + if (!$cond) { + $this->failures[] = $label; + } + return (bool)$cond; + } + + /** + * Print the verdict and exit with the suite's convention. + * + * @return void + */ + public function finish() + { + if (count($this->failures)) { + fwrite( + STDERR, + 'FAIL (' . count($this->failures) . ' of ' . $this->count . "):\n" + ); + foreach ($this->failures as $f) { + fwrite(STDERR, " - $f\n"); + } + exit(1); + } + echo 'ok ' . $this->count . " checks passed\n"; + exit(0); + } +} + +/** + * Builds one class's join text and its WHERE additions. + * + * @param string $class the class to build for + * + * @return array [joins, whereArrayAnd] + */ +function relFilterBuild($class) +{ + $obj = FOGCore::getClass($class); + $join = array(); + $where = array(); + $c = null; + return $obj->buildQuery($join, $where, $c); +} + +/* + * 1. The filter is really there. Without this every assertion below would + * pass just as well against a relationship map that had quietly lost it, + * and the test would be measuring nothing. + */ +$relProp = new \ReflectionProperty( + get_class(FOGCore::getClass('Host')), + 'databaseFieldClassRelationships' +); +$relProp->setAccessible(true); +$rels = $relProp->getValue(FOGCore::getClass('Host')); +$macRel = isset($rels['MACAddressAssociation']) + ? $rels['MACAddressAssociation'] + : null; +$t->check( + 'Host still declares a filtered relationship to MACAddressAssociation', + is_array($macRel) && isset($macRel[3]) && is_array($macRel[3]) + && array_key_exists('primary', $macRel[3]) +); + +/* + * 2. It is emitted inside the ON clause of the hostMAC join, and the join is + * still an outer one. Both halves matter: moving the predicate to ON while + * turning the join inner would drop exactly the same rows. + */ +list($joins, $where) = relFilterBuild('Host'); +$t->check( + 'the hostMAC join is still a LEFT OUTER JOIN', + false !== strpos($joins, 'LEFT OUTER JOIN `hostMAC` ON ') +); +$t->check( + "hmPrimary is part of the hostMAC ON clause", + false !== strpos( + $joins, + "ON `hostMAC`.`hmHostID`=`hosts`.`hostID` AND `hostMAC`.`hmPrimary` = '1'" + ) +); + +/* + * 3. And nothing about the optional table reached WHERE -- for Host, and for + * every class that inherits the join transitively. The list is spelled out + * rather than derived so that a class LOSING its path to Host shows up as + * a skipped name here, not as silence. + */ +$classes = array( + 'Host', + 'Task', + 'SnapinJob', + 'SnapinTask', + 'ImagingLog', + 'NodeFailure', + 'UserTracking', + 'LocationAssociation', + 'SiteHostAssociation', +); +foreach ($classes as $class) { + if (!class_exists($class)) { + $t->check("$class exists, so its join is actually being checked", false); + continue; + } + list($j, $w) = relFilterBuild($class); + $t->check( + "$class puts no hostMAC predicate in WHERE", + !preg_grep('/hostMAC|hmPrimary/', (array)$w) + ); + // A class that reaches Host must actually carry the join, or "no + // predicate in WHERE" is true for the boring reason. + if ('Host' !== $class) { + $t->check( + "$class reaches the hostMAC join at all", + false !== strpos($j, 'JOIN `hostMAC` ON ') + ); + } +} + +$t->finish();