From 12cd58d4fe6ee040b891e3aa8e5757a9fd4fcdf6 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Sat, 29 Aug 2026 15:49:45 -0500 Subject: [PATCH] Constrain capone and subnetgroup, and gate the classification The companion to fogproject #1481, which declares the three relationships. This applies them, and adds the gate that should have demanded them. WHY THEY WERE MISSED. fogproject's tests/foreign-key-map.test.php requires every id column to be classified, but it walks commons/schema-expected.php -- 70 core tables, no plugin table. So nothing has ever required a decision about a plugin's id columns, and capone.cImageID, capone.cOSID and subnetgroup.sgGroupID had never had one. That gate cannot see this repository, so the missing half belongs here: plugin-id-columns-are-classified.test.php walks every manager's createTableSql() field list and fails any id column with no recorded decision. 52 columns across 24 tables, with one bounded exception -- OIDCProviders.opClientID ends in ID and is a client credential, not a reference. CAPONE needs two preconditions before its constraints can exist, and both are measured rather than assumed. cOSID is int(11) against a mediumint(9) parent, which InnoDB refuses at errno 150. And both columns carry a `0` sentinel, because the model declares no $databaseFieldsRequired and save()'s optional-`*id` branch writes 0 for an empty value -- the same shape that made tasks.taskStateID a runtime 1452 and was fixed in core's step 389. A foreign key spells "no reference" NULL and nothing else, and the sweep deliberately skips `<> 0`, so it cannot clean a sentinel: step 2 does, and createSql() builds both columns that way for a fresh install. Once the column is nullable save() writes NULL on its own, because FOGBase::columnType() falls back to _loadPluginColumnTypes() for tables the core manifest does not describe. Making them nullable also makes the sweep non-destructive: planSweep() nulls a dangling reference where the column allows it and only deletes where it does not. No capone row is removed. SUBNETGROUP needs nothing. sgGroupID is already int(11) NOT NULL against an int(11) parent, and `groupID` IS in its model's required list, so save() refuses an empty value rather than writing 0. Its CASCADE pins what already happens -- removesubnetgroupgroup.hook.php deletes those rows when a group is destroyed -- so the constraint states in the schema what was relying on a hook being registered. The hook stays; it is what fires the plugin's own events. Both steps are appended, not folded in, because installdb() skips the pSchema steps an install has already passed. Gate mutation-verified four ways: unclassifying a known column, introducing a new unclassified one, removing a pinned column, and breaking the extraction regex. The second is the one that matters -- it is the future case, and the whole reason the gate exists. Co-Authored-By: Claude --- capone/class/caponemanager.class.php | 107 +++++++++- .../class/subnetgroupmanager.class.php | 44 ++++ .../foreign-keys-applied-per-plugin.test.php | 2 + .../plugin-id-columns-are-classified.test.php | 193 ++++++++++++++++++ 4 files changed, 343 insertions(+), 3 deletions(-) create mode 100644 tests/plugin-id-columns-are-classified.test.php diff --git a/capone/class/caponemanager.class.php b/capone/class/caponemanager.class.php index 923964f..836aaea 100644 --- a/capone/class/caponemanager.class.php +++ b/capone/class/caponemanager.class.php @@ -47,14 +47,23 @@ public function createSql() ], [ 'INTEGER', + // MEDIUMINT(9) matches os.osID. InnoDB requires an EXACT type + // match on both sides of a foreign key and answers errno 150 + // otherwise -- int(11) against mediumint(9) is refused, which + // is what this column was until fogproject ADR 0031. 'INTEGER', - 'INTEGER', + 'MEDIUMINT(9)', 'VARCHAR(255)' ], [ false, - false, - false, + // Nullable, because a foreign key spells "no reference" NULL + // and nothing else. These carried 0, which is a value: a + // constraint over them would demand an image with imageID 0 + // and an os with osID 0 and refuse every row without one. + // Step 2 converts the rows an existing install already has. + true, + true, false ], [ @@ -139,6 +148,98 @@ public function schema() function () { return $this->seedSettings(); }, + // 2 - cImageID and cOSID stop spelling "no reference" as 0. + // + // fogproject ADR 0031. A foreign key accepts NULL for "no + // reference" and nothing else, so a constraint over these columns + // would demand an image with imageID 0 and an OS with osID 0 and + // refuse every row that had neither. cOSID also has to become + // MEDIUMINT(9): os.osID is mediumint(9) and InnoDB refuses a + // foreign key whose sides are not the same type, at errno 150. + // + // Capone declares no $databaseFieldsRequired, so save()'s + // optional-`*id` branch is what wrote the 0 -- the same shape + // that made tasks.taskStateID a runtime 1452 and was fixed in + // core's schema step 389. Once the column is nullable the branch + // writes NULL on its own: FOGBase::columnType() falls back to + // _loadPluginColumnTypes(), which reads the server's own catalog + // for tables the core manifest does not describe. + // + // Appended rather than folded into step 0 because installdb() + // SKIPS the pSchema steps an install has already passed instead + // of replaying them. createSql() above now builds both columns + // this way for a fresh install; this is what an existing one + // gets. + function () { + $sql = sprintf( + 'ALTER TABLE `%s` MODIFY COLUMN `cImageID` INTEGER NULL' + . ' DEFAULT NULL, MODIFY COLUMN `cOSID` MEDIUMINT(9) NULL' + . ' DEFAULT NULL', + $this->tablename + ); + if (false !== self::$DB->query($sql)->error) { + return self::$DB->error; + } + $converted = 0; + foreach (['cImageID', 'cOSID'] as $column) { + $sql = sprintf( + 'UPDATE `%s` SET `%s` = NULL WHERE `%s` = 0', + $this->tablename, + $column, + $column + ); + if (false !== self::$DB->query($sql)->error) { + return self::$DB->error; + } + $converted += (int)self::$DB->affectedRows(); + } + // Logged rather than silent: this rewrites rows, and the + // count is the only evidence it did. + error_log( + sprintf( + '%s: %d %s', + _('Capone schema'), + $converted, + _('row(s) converted from 0 to NULL') + ) + ); + return true; + }, + // 3 - the plugin's foreign keys. + // + // fogproject ADR 0031 decision 8: sweep, then add. ADD CONSTRAINT + // validates the rows already in the table and answers 1452 if any + // of them point at a parent that is gone -- and applyConstraints() + // REPORTS a refusal rather than returning it, so an install that + // skipped the sweep would succeed while silently not creating the + // constraint. Both columns are nullable by step 2, so the sweep + // nulls the dangling references rather than deleting the rows. + // + // Both calls are filtered to this plugin's own group, so neither + // can reach another plugin's tables or core's. The relationships + // are declared in fogproject's commons/schema-constraints.php: + // capone.cImageID RESTRICT to `images`, capone.cOSID RESTRICT to + // `os`. + // + // RESTRICT, not CASCADE, for both. Nothing in FOG deletes a + // capone row today when its image or OS goes -- there is no hook + // and deletemass() has no case -- so the reference simply + // dangles, and the map's rule is RESTRICT exactly where PHP does + // nothing and the reference dangles. CASCADE would silently + // delete an administrator's deployment rule as a side effect of + // deleting an image, which is the argument core used to reject + // CASCADE for storage nodes. + // + // Idempotent, and re-run by the unfiltered reconcile after every + // core schema update: planConstraints() skips a constraint whose + // declaration already matches the map. + function () { + $res = \FOG\Db\SchemaReconciler::sweepOrphans('capone'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints('capone'); + }, ]; } /** diff --git a/subnetgroup/class/subnetgroupmanager.class.php b/subnetgroup/class/subnetgroupmanager.class.php index d633df4..ef2c50a 100644 --- a/subnetgroup/class/subnetgroupmanager.class.php +++ b/subnetgroup/class/subnetgroupmanager.class.php @@ -89,6 +89,50 @@ public function schema() return [ // 0 $this->createSql(), + // 1 - the plugin's foreign key. + // + // fogproject ADR 0031 decision 8: sweep, then add. ADD CONSTRAINT + // validates the rows already in the table and answers 1452 if any + // of them point at a parent that is gone -- and applyConstraints() + // REPORTS a refusal rather than returning it, so an install that + // skipped the sweep would succeed while silently not creating the + // constraint. The sweep is the precondition for the statement, + // not a policy choice. + // + // Both calls are filtered to this plugin's own group, so neither + // can reach another plugin's tables or core's. The relationship + // is declared in fogproject's commons/schema-constraints.php: + // subnetgroup.sgGroupID CASCADE to `groups`. + // + // CASCADE PINS WHAT ALREADY HAPPENS, so nothing observable + // changes. removesubnetgroupgroup.hook.php already calls + // Route::deletemass('subnetgroup', ['groupID' => ...]) when a + // group is destroyed; the constraint states that in the schema + // instead of relying on a hook being registered. The hook stays + // -- it is what fires the plugin's own events -- and the two + // agree rather than compete. + // + // No column change was needed: sgGroupID is already int(11) NOT + // NULL against an int(11) parent, and it carries no sentinel + // because `groupID` IS in the model's $databaseFieldsRequired, + // so save() refuses an empty value rather than writing 0. + // + // Appended rather than folded into step 0 because installdb() + // SKIPS the pSchema steps an install has already passed instead + // of replaying them. + // + // Idempotent, and re-run by the unfiltered reconcile after every + // core schema update: planConstraints() skips a constraint whose + // declaration already matches the map. + function () { + $res = \FOG\Db\SchemaReconciler::sweepOrphans('subnetgroup'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints( + 'subnetgroup' + ); + }, ]; } /** diff --git a/tests/foreign-keys-applied-per-plugin.test.php b/tests/foreign-keys-applied-per-plugin.test.php index 71390e8..b046628 100644 --- a/tests/foreign-keys-applied-per-plugin.test.php +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -57,6 +57,8 @@ 'windowskey' => 'windowskey/class/windowskeymanager.class.php', 'ldap' => 'ldap/class/ldapmanager.class.php', 'oidc' => 'oidc/class/oidcmanager.class.php', + 'capone' => 'capone/class/caponemanager.class.php', + 'subnetgroup' => 'subnetgroup/class/subnetgroupmanager.class.php', ]; /** diff --git a/tests/plugin-id-columns-are-classified.test.php b/tests/plugin-id-columns-are-classified.test.php new file mode 100644 index 0000000..522198f --- /dev/null +++ b/tests/plugin-id-columns-are-classified.test.php @@ -0,0 +1,193 @@ + + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +$root = dirname(__DIR__); +$failures = []; +$checks = 0; + +/* + * Each table's OWN primary key. Not a reference, so nothing to classify. + */ +$primary = [ + 'LDAPGroups.lgID', 'LDAPServers.lsID', 'OIDCGroups.ogID', + 'OIDCProviders.opID', 'capone.cID', 'helloWorld.hwID', + 'ldapGroupRoleAssoc.lgraID', 'ldapGroupUserGroupAssoc.lgugID', + 'ldapUserGrant.lugID', 'location.lID', 'locationAssoc.laID', 'ntfy.nID', + 'oidcGroupRoleAssoc.ograID', 'oidcGroupUserGroupAssoc.ogugID', + 'oidcIdentity.oiID', 'oidcUserGrant.ougID', 'ou.ouID', 'ouAssoc.oaID', + 'pushbullet.pID', 'slack.sID', 'subnetgroup.sgID', 'windowsKeys.wkID', + 'windowsKeysAssoc.wkaID', 'wolbroadcast.wbID', +]; + +/* + * Classified in fogproject's commons/schema-constraints.php. A constraint is + * declared for each of these; the group name is the plugin's directory. + */ +$classified = [ + 'locationAssoc.laLocationID', 'locationAssoc.laHostID', + 'location.lStorageGroupID', 'location.lStorageNodeID', + 'ouAssoc.oaOUID', 'ouAssoc.oaHostID', + 'windowsKeysAssoc.wkaImageID', 'windowsKeysAssoc.wkaKeyID', + 'LDAPGroups.lgServerID', 'ldapGroupRoleAssoc.lgraGroupID', + 'ldapGroupRoleAssoc.lgraRoleID', 'ldapGroupUserGroupAssoc.lgugGroupID', + 'ldapGroupUserGroupAssoc.lgugUserGroupID', 'ldapUserGrant.lugUserID', + 'OIDCGroups.ogProviderID', 'oidcIdentity.oiProviderID', + 'oidcIdentity.oiUserID', 'oidcGroupRoleAssoc.ograGroupID', + 'oidcGroupRoleAssoc.ograRoleID', 'oidcGroupUserGroupAssoc.ogugGroupID', + 'oidcGroupUserGroupAssoc.ogugUserGroupID', 'oidcUserGrant.ougUserID', + 'capone.cImageID', 'capone.cOSID', 'subnetgroup.sgGroupID', +]; + +/* + * Classified `poly` in that map: the target table is named by a sibling + * column, so no foreign key is expressible. A decision, not an omission. + */ +$poly = ['ldapUserGrant.lugTargetID', 'oidcUserGrant.ougTargetID']; + +/* + * Ends in ID and is not an id. The single bounded exception; see the header. + */ +$notAReference = ['OIDCProviders.opClientID']; + +$decided = array_flip( + array_merge($primary, $classified, $poly, $notAReference) +); + +/** + * Source with comments removed, so a commented-out column cannot satisfy a + * check or a commented-out table escape one. + * + * @param string $file the file to read + * + * @return string + */ +function idStrip($file) +{ + $clean = ''; + foreach (token_get_all((string)file_get_contents($file)) as $token) { + if (is_array($token) + && ($token[0] === T_COMMENT || $token[0] === T_DOC_COMMENT) + ) { + continue; + } + $clean .= is_array($token) ? $token[1] : $token; + } + + return $clean; +} + +$seen = []; +$iterator = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($root) +); +foreach ($iterator as $file) { + if (!preg_match('/manager\.class\.php$/', $file->getFilename())) { + continue; + } + $path = $file->getPathname(); + if (false !== strpos($path, '/tests/')) { + continue; + } + $src = idStrip($path); + if (false === strpos($src, 'createTableSql(')) { + continue; + } + if (!preg_match('/\$tablename\s*=\s*.([A-Za-z_]+)./', $src, $tm)) { + continue; + } + $table = $tm[1]; + // Only the createTableSql() argument list. A column name mentioned in a + // query elsewhere in the manager is not this table declaring it. + $chunk = substr($src, (int)strpos($src, 'createTableSql('), 2000); + preg_match_all('/.([A-Za-z_]+(?:ID|Id)).\s*[,\]]/', $chunk, $cm); + foreach (array_unique($cm[1]) as $column) { + $key = $table . '.' . $column; + $seen[$key] = true; + $checks++; + if (!isset($decided[$key])) { + $failures[] = "$key has never been classified." + . ' Add it to fogproject commons/schema-constraints.php --' + . ' as a constraint, or as `poly` if a sibling column names' + . ' its table -- and pin it in this test. An id column with' + . ' no decision is how a dangling reference ships'; + } + } +} + +// The inverse: a pinned entry whose column is gone is a stale pin, and a +// stale pin is how this test quietly stops covering the thing it names. +foreach (array_keys($decided) as $key) { + $checks++; + if (!isset($seen[$key])) { + $failures[] = "$key is pinned in this test but no plugin table" + . ' declares it. Remove the pin, or restore the column'; + } +} + +// A regex that matches nothing passes every check above. Anchor the sweep on +// a count that a broken extraction cannot reach. +$checks++; +if (count($seen) < 50) { + $failures[] = sprintf( + 'only %d id column(s) were found across the plugin managers, so the' + . ' checks above proved almost nothing -- the extraction is broken', + count($seen) + ); +} + +if (count($failures)) { + echo 'FAIL: ' . count($failures) . " problem(s).\n\n"; + foreach ($failures as $f) { + echo " $f\n"; + } + exit(1); +} + +printf( + "plugin-id-columns-are-classified: %d checks passed, %d id column(s)\n", + $checks, + count($seen) +); +exit(0);