From 900783751bd11c399840fe1e9c59b30a21eddaf3 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Sat, 29 Aug 2026 12:18:05 -0500 Subject: [PATCH 1/5] location: declare its four foreign keys, and stop spelling "no node" as 0 fogproject ADR 0031, the first of the five plugins. Two appended schema steps and one correction to createSql(). Step 4 makes lStorageNodeID nullable and converts its 0s. The column is a tri-state and always has been -- Location::getStorageNode() returns the named node when it is truthy and falls through to the storage group's optimal node when it is not -- but it spelled "no node" as 0, and a foreign key accepts NULL for "no reference" and nothing else. A constraint over the column as it stood would demand a storage node with ngmID = 0 and refuse every location that had not pinned one. Nothing reading the column changed, and that is the point: FOGController::get() reads through isset(), so NULL comes back as '' and is falsy exactly as 0 was. createSql() now builds the column nullable so a fresh install is not created already needing the migration -- appended rather than folded into step 0 for the reason step 3 already gives, that installdb() skips the pSchema steps an install has passed instead of replaying them. Step 5 sweeps, then adds. Decision 8: ADD CONSTRAINT validates the rows already in the table and answers 1452 if any point at a parent that is gone, and applyConstraints() reports a refusal rather than returning it -- so an install that skips the sweep succeeds, silently, without the constraint. Both calls are filtered to this plugin's own group, so neither can reach another plugin's tables or core's. The relationships themselves are declared in fogproject's commons/schema-constraints.php, not here. Half of them point at core tables, and that map is meant to answer "what points at hosts?" from one file. Two of the four needed a decision rather than the obvious CASCADE: - lStorageNodeID gets SET NULL. Deleting a storage node that a location names degrades that location to group-optimal selection -- a state the plugin already implements and already handles. RESTRICT would refuse the delete with an opaque foreign key error naming no location. - lStorageGroupID gets RESTRICT and no sentinel: storagegroupID is in Location::$databaseFieldsRequired, so the column has no legitimate 0. Verified against a lab clone of the live database at schema 390. Fresh install: table built nullable, all four constraints applied. Upgrade over dirty data: the sweep removed one orphaned association and cleared one location naming a node that no longer existed, then all four applied. Re-run: nothing planned, nothing issued. Behavior probed individually -- deleting a host removed its association, deleting the location removed the rest, deleting a referenced storage group was refused with 1451 naming the constraint, and deleting a named storage node left the location with its reference NULL rather than removing it. The new gate is mutation-tested: dropping either call, reversing their order, dropping the group argument, reverting the column to NOT NULL, dropping the 0-to-NULL conversion, and building the column NOT NULL in createSql() each turn a check red. It does not check that SchemaReconciler is fully qualified -- core-references-are-qualified.test.php already does that generically, and a check written here would pass while one of the two calls kept its namespace. Co-Authored-By: Claude --- location/class/locationmanager.class.php | 82 +++++- .../foreign-keys-applied-per-plugin.test.php | 243 ++++++++++++++++++ 2 files changed, 324 insertions(+), 1 deletion(-) create mode 100644 tests/foreign-keys-applied-per-plugin.test.php diff --git a/location/class/locationmanager.class.php b/location/class/locationmanager.class.php index dec6fa4..5884c38 100644 --- a/location/class/locationmanager.class.php +++ b/location/class/locationmanager.class.php @@ -62,12 +62,18 @@ public function createSql() 'TINYINT(1)', "ENUM('http', 'https')" ], + // Nullability, in field order. lStorageNodeID is the one true + // entry: the column is a tri-state, and NULL is how it spells + // "no specific node -- let the group choose". See step 4 of + // schema() for why that stopped being 0, and + // Location::getStorageNode() for the reader, which treats any + // falsy value the same way and so needed no change. [ false, false, false, false, - false, + true, false, false, false, @@ -159,6 +165,80 @@ function () { ] ); }, + // 4 - lStorageNodeID stops spelling "no node" as 0. + // + // fogproject ADR 0031. A foreign key accepts NULL for "no + // reference" and nothing else: 0 is a value, so a constraint + // over this column would demand a storage node with ngmID = 0 + // and refuse every location that had not pinned one. + // + // The column is genuinely a tri-state and always has been -- + // Location::getStorageNode() returns the named node when it is + // truthy and falls through to the group's optimal node when it + // is not -- so this changes how "none" is spelled, not what the + // column means. FOGController::get() reads through isset(), so + // NULL comes back as '' and is falsy exactly as 0 was; the + // reader needed no change, which is the point. + // + // Appended rather than folded into step 0 for the reason step 3 + // gives: installdb() skips the pSchema steps an install has + // already passed. createSql() above now builds the column + // nullable for a fresh install; this is what an existing one + // gets. + function () { + $sql = sprintf( + 'ALTER TABLE `%s` MODIFY COLUMN `lStorageNodeID`' + . ' INTEGER NULL DEFAULT NULL', + $this->tablename + ); + if (false !== self::$DB->query($sql)->error) { + return self::$DB->error; + } + $sql = sprintf( + 'UPDATE `%s` SET `lStorageNodeID` = NULL' + . ' WHERE `lStorageNodeID` = 0', + $this->tablename + ); + if (false !== self::$DB->query($sql)->error) { + return self::$DB->error; + } + // Logged rather than silent: this rewrites rows, and the + // count is the only evidence it did. + error_log( + sprintf( + '%s: %s: %d %s', + _('Location schema'), + 'lStorageNodeID', + (int)self::$DB->affectedRows(), + _('row(s) converted from 0 to NULL') + ) + ); + return true; + }, + // 5 - the plugin's foreign keys. + // + // fogproject ADR 0031 decision 8: sweep, then add. ADD + // CONSTRAINT validates the rows already there and answers 1452 + // if any of them point at a parent that is gone, so the sweep is + // the precondition for the statement rather than a policy + // choice. Both calls are filtered to this plugin's own group, so + // nothing here can reach another plugin's tables or core's. + // + // The relationships themselves are declared in fogproject's + // commons/schema-constraints.php, not here -- half of them point + // at core tables, and the map is meant to answer "what points at + // hosts?" from one file. + // + // 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('location'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints('location'); + }, ]; } /** diff --git a/tests/foreign-keys-applied-per-plugin.test.php b/tests/foreign-keys-applied-per-plugin.test.php new file mode 100644 index 0000000..1a1f7c9 --- /dev/null +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -0,0 +1,243 @@ + + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +$root = dirname(__DIR__); +$failures = []; +$checks = 0; + +/* + * Plugin directory => the manager file that owns its schema() and the group + * name it must pass. + * + * The group is the plugin's own directory name, and it is a STRING where + * core's groups are ints. That is not decoration: planConstraints() and + * planSweep() both select on ===, and PHP 7.4 -- FOG's floor -- calls + * `5 == 'ldap'` true. A plugin group written as a number would be applied by + * a core schema step, against tables core has not swept. + * + * ADD A ROW HERE IN THE SAME COMMIT THAT LANDS A PLUGIN'S STEP. + */ +$expected = [ + 'location' => 'location/class/locationmanager.class.php', +]; + +/** + * Source with comments removed, so a commented-out call cannot satisfy a + * check. + * + * @param string $file the file to read + * + * @return string + */ +function fkStrip($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; +} + +foreach ($expected as $plugin => $relative) { + $file = $root . '/' . $relative; + $checks++; + if (!is_file($file)) { + $failures[] = "$plugin: $relative does not exist"; + continue; + } + $src = fkStrip($file); + + $sweep = "SchemaReconciler::sweepOrphans('$plugin')"; + $apply = "SchemaReconciler::applyConstraints('$plugin')"; + + $checks++; + $sweepAt = strpos($src, $sweep); + if (false === $sweepAt) { + $failures[] = "$plugin: schema() never calls $sweep. Adding a" + . ' constraint to a table holding orphans answers 1452 and the' + . ' constraint is silently never created'; + } + + $checks++; + $applyAt = strpos($src, $apply); + if (false === $applyAt) { + $failures[] = "$plugin: schema() never calls $apply, so its" + . ' relationships are declared in the map and never applied'; + } + + // Order, not just presence. Sweeping after adding is the same as not + // sweeping at all. + $checks++; + if (false !== $sweepAt && false !== $applyAt && $sweepAt > $applyAt) { + $failures[] = "$plugin: sweeps AFTER it adds, which is the same as" + . ' not sweeping'; + } + + // Neither call may be made unfiltered. applyConstraints() with no group + // applies every enabled relationship in the map -- core's included -- + // from inside a plugin install. + $checks++; + if (strpos($src, 'SchemaReconciler::applyConstraints()') !== false + || strpos($src, 'SchemaReconciler::sweepOrphans()') !== false + ) { + $failures[] = "$plugin: calls applyConstraints() or sweepOrphans()" + . " with no group; that reaches core's relationships too"; + } + + // Qualification is NOT checked here. core-references-are-qualified.php + // already does it generically, for every bare class name in the tree, + // and better -- a check written here would pass as long as ONE of the + // two calls kept its namespace. +} + +/* + * location.lStorageNodeID has to be nullable, and a fresh install has to + * build it that way. + * + * The column is a tri-state -- Location::getStorageNode() returns the named + * node when it is truthy and falls through to the storage group's optimal + * node when it is not -- and it spelled "no node" as 0. A foreign key + * accepts NULL for "no reference" and nothing else, so a constraint over a + * column still holding 0 demands a storage node with ngmID = 0 and refuses + * every location that has not pinned one. + * + * schema() step 4 converts an existing install. This checks the OTHER half: + * createSql() builds the column nullable, so a fresh install does not have + * to be migrated the moment it is created. The two are easy to separate -- + * step 4 alone passes every upgrade test and fails only on a brand new + * server. + */ +$src = fkStrip($root . '/location/class/locationmanager.class.php'); +$checks++; +if (strpos($src, "MODIFY COLUMN `lStorageNodeID`") === false + || strpos($src, 'NULL DEFAULT NULL') === false +) { + $failures[] = 'location: schema() does not make lStorageNodeID nullable;' + . ' a foreign key cannot accept its 0 sentinel'; +} +$checks++; +if (strpos($src, "SET `lStorageNodeID` = NULL") === false) { + $failures[] = 'location: schema() does not convert lStorageNodeID = 0 to' + . ' NULL, so existing rows still name a storage node that does not' + . ' exist'; +} + +/* + * The fresh-install half, read out of createSql() rather than grepped for. + * + * createTableSql() takes parallel arrays -- fields, types, nulls, defaults -- + * and renders NOT NULL for a `nulls` entry that is `=== false`. So the check + * is positional: find lStorageNodeID's index in the field list and require + * the matching nulls entry to be true. A grep for "true" anywhere in the + * method would pass on the `$exists` argument. + */ +$body = ''; +$at = strpos($src, 'function createSql('); +if (false !== $at) { + $open = strpos($src, '{', $at); + $depth = 0; + for ($i = $open, $n = strlen($src); $i < $n; $i++) { + if ($src[$i] === '{') { + $depth++; + } + if ($src[$i] === '}') { + $depth--; + if ($depth === 0) { + $body = substr($src, $open, $i - $open + 1); + break; + } + } + } +} +$checks++; +if (!preg_match_all('/\[([^\[\]]*)\]/s', $body, $m)) { + $failures[] = 'location: could not read createSql()\'s argument arrays'; +} else { + $fields = null; + $nulls = null; + foreach ($m[1] as $group) { + $items = array_map('trim', explode(',', $group)); + $items = array_values(array_filter($items, 'strlen')); + if (null === $fields && in_array("'lStorageNodeID'", $items, true)) { + $fields = $items; + continue; + } + if (null !== $fields + && null === $nulls + && count($items) === count($fields) + && $items === array_map( + static function ($v) { + return in_array($v, ['true', 'false'], true) ? $v : null; + }, + $items + ) + ) { + $nulls = $items; + break; + } + } + $index = null === $fields + ? false + : array_search("'lStorageNodeID'", $fields, true); + if (false === $index || null === $nulls) { + $failures[] = 'location: could not locate lStorageNodeID in' + . ' createSql()\'s field and nulls arrays'; + } elseif (($nulls[$index] ?? '') !== 'true') { + $failures[] = 'location: createSql() builds lStorageNodeID NOT NULL,' + . ' so a fresh install cannot take its foreign key'; + } +} + +if (count($failures)) { + echo 'FAIL: ' . count($failures) . " problem(s).\n\n"; + foreach ($failures as $f) { + echo " $f\n"; + } + exit(1); +} + +printf( + "foreign-keys-applied-per-plugin: %d checks passed, %d plugin(s)\n", + $checks, + count($expected) +); From d3c68c14e06bebec604daf5c6c468ec61acec5bf Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Sat, 29 Aug 2026 12:19:55 -0500 Subject: [PATCH 2/5] ou: declare its two foreign keys fogproject ADR 0031. One appended schema step, no column changes: both ouAssoc columns are already int(11) NOT NULL against int(11) parents, and neither carries a sentinel -- an association row exists only to name both ends, so there is no "no reference" state to spell. Decision 8's order, 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 calls are filtered to this plugin's own group. The relationships are declared in fogproject's commons/schema-constraints.php: oaOUID CASCADE to `ou`, oaHostID CASCADE to `hosts`. Both arrows run plugin -> core or plugin -> plugin, so uninstalling the plugin is still just dropping its tables. Appended rather than folded into step 0 because installdb() skips the pSchema steps an install has already passed instead of replaying them. Verified against a lab clone at schema 390: both constraints applied, the sweep found nothing to remove, a second run planned no ALTER at all. Behavior probed and rolled back -- deleting a host removed its association (3 -> 2), deleting the OU removed the rest (2 -> 0). Co-Authored-By: Claude --- ou/class/oumanager.class.php | 37 +++++++++++++++++++ .../foreign-keys-applied-per-plugin.test.php | 1 + 2 files changed, 38 insertions(+) diff --git a/ou/class/oumanager.class.php b/ou/class/oumanager.class.php index 7b81647..c06deb7 100644 --- a/ou/class/oumanager.class.php +++ b/ou/class/oumanager.class.php @@ -94,6 +94,43 @@ public function schema() $this->createSql(), // 1 self::getClass('OUAssociationManager')->createSql(), + // 2 - 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. 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 relationships + // themselves are declared in fogproject's + // commons/schema-constraints.php: ouAssoc.oaOUID CASCADE to `ou`, + // ouAssoc.oaHostID CASCADE to `hosts`. Half of them point at core + // tables, and that map is meant to answer "what points at hosts?" + // from one file. + // + // Appended rather than folded into an earlier step because + // installdb() SKIPS the pSchema steps an install has already + // passed instead of replaying them, so an edit to an earlier step + // is invisible to everyone already past it. + // + // No column change was needed: both columns are already + // int(11) NOT NULL against int(11) parents, and neither carries a + // sentinel -- an association row exists only to name both ends. + // + // 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('ou'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints('ou'); + }, ]; } /** diff --git a/tests/foreign-keys-applied-per-plugin.test.php b/tests/foreign-keys-applied-per-plugin.test.php index 1a1f7c9..a155d36 100644 --- a/tests/foreign-keys-applied-per-plugin.test.php +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -53,6 +53,7 @@ */ $expected = [ 'location' => 'location/class/locationmanager.class.php', + 'ou' => 'ou/class/oumanager.class.php', ]; /** From e0f626f5eb3fd65be4bfa7925fd9359739d601e8 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Sat, 29 Aug 2026 12:21:19 -0500 Subject: [PATCH 3/5] windowskey: declare its two foreign keys fogproject ADR 0031. One appended schema step, no column changes: both windowsKeysAssoc columns are already int(11) NOT NULL against int(11) parents, and neither carries a sentinel. Decision 8's order, sweep then add, both calls filtered to this plugin's own group. The relationships are declared in fogproject's commons/schema-constraints.php: wkaImageID CASCADE to `images`, wkaKeyID CASCADE to `windowsKeys`. CASCADE on the image side is the behavior that already existed -- deleting an image was never meant to leave a key assigned to it, and the association row carries nothing of its own to preserve. The gate stopped matching on raw source and now strips whitespace as well as comments. It failed this plugin's perfectly valid applyConstraints() purely because the argument was wrapped to keep the line under 80 columns, which is a test that fails on formatting rather than on behavior. Re-proved after the change: dropping either call, reversing their order, dropping the group argument, and each of the three location-specific checks still turn red. Verified against a lab clone at schema 390: both constraints applied, the sweep found nothing, a second run planned no ALTER. Behavior probed and rolled back -- deleting an image removed its association (3 -> 2), deleting the key removed the rest (2 -> 0). Co-Authored-By: Claude --- .../foreign-keys-applied-per-plugin.test.php | 14 +++++-- windowskey/class/windowskeymanager.class.php | 41 +++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/tests/foreign-keys-applied-per-plugin.test.php b/tests/foreign-keys-applied-per-plugin.test.php index a155d36..824590f 100644 --- a/tests/foreign-keys-applied-per-plugin.test.php +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -54,6 +54,7 @@ $expected = [ 'location' => 'location/class/locationmanager.class.php', 'ou' => 'ou/class/oumanager.class.php', + 'windowskey' => 'windowskey/class/windowskeymanager.class.php', ]; /** @@ -86,7 +87,11 @@ function fkStrip($file) $failures[] = "$plugin: $relative does not exist"; continue; } - $src = fkStrip($file); + // Whitespace stripped as well as comments. A call is the same call + // whether or not its argument was wrapped to keep the line under 80, + // and the first version of this test failed a perfectly good + // applyConstraints( \n 'windowskey' \n ) on formatting alone. + $src = preg_replace('/\s+/', '', fkStrip($file)); $sweep = "SchemaReconciler::sweepOrphans('$plugin')"; $apply = "SchemaReconciler::applyConstraints('$plugin')"; @@ -149,15 +154,16 @@ function fkStrip($file) * server. */ $src = fkStrip($root . '/location/class/locationmanager.class.php'); +$tight = preg_replace('/\s+/', '', $src); $checks++; -if (strpos($src, "MODIFY COLUMN `lStorageNodeID`") === false - || strpos($src, 'NULL DEFAULT NULL') === false +if (strpos($tight, "MODIFYCOLUMN`lStorageNodeID`") === false + || strpos($tight, 'NULLDEFAULTNULL') === false ) { $failures[] = 'location: schema() does not make lStorageNodeID nullable;' . ' a foreign key cannot accept its 0 sentinel'; } $checks++; -if (strpos($src, "SET `lStorageNodeID` = NULL") === false) { +if (strpos($tight, "SET`lStorageNodeID`=NULL") === false) { $failures[] = 'location: schema() does not convert lStorageNodeID = 0 to' . ' NULL, so existing rows still name a storage node that does not' . ' exist'; diff --git a/windowskey/class/windowskeymanager.class.php b/windowskey/class/windowskeymanager.class.php index aee380c..8694eb9 100644 --- a/windowskey/class/windowskeymanager.class.php +++ b/windowskey/class/windowskeymanager.class.php @@ -115,6 +115,47 @@ public function schema() 'ALTER TABLE `%s` DROP INDEX `index0`', $this->tablename ), + // 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. 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 relationships + // themselves are declared in fogproject's + // commons/schema-constraints.php: windowsKeysAssoc.wkaImageID + // CASCADE to `images`, windowsKeysAssoc.wkaKeyID CASCADE to + // `windowsKeys`. Half of them point at core tables, and that map + // is meant to answer "what points at images?" from one file. + // + // CASCADE on wkaImageID is the behavior that already existed: + // deleting an image was never meant to leave a key assigned to it, + // and the association carries nothing of its own to preserve. + // + // Appended rather than folded into an earlier step because + // installdb() SKIPS the pSchema steps an install has already + // passed instead of replaying them. + // + // No column change was needed: both columns are already + // int(11) NOT NULL against int(11) parents, and neither carries a + // sentinel. + // + // Idempotent, and re-run by the unfiltered reconcile after every + // core schema update. + function () { + $res = \FOG\Db\SchemaReconciler::sweepOrphans('windowskey'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints( + 'windowskey' + ); + }, ]; } /** From ab9865cc5f5fbcca5482c007a2acaa713608f63e Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Sat, 29 Aug 2026 12:22:47 -0500 Subject: [PATCH 4/5] ldap: declare its six foreign keys fogproject ADR 0031. One appended schema step, no column changes: all six are int(11) NOT NULL against int(11) parents and none carries a sentinel. LDAPGroups.lgServerID -> LDAPServers.lsID ldapGroupRoleAssoc.lgraGroupID -> LDAPGroups.lgID ldapGroupRoleAssoc.lgraRoleID -> roles.rID ldapGroupUserGroupAssoc.lgugGroupID -> LDAPGroups.lgID ldapGroupUserGroupAssoc.lgugUserGroupID -> userGroups.ugID ldapUserGrant.lugUserID -> users.uId All CASCADE. LDAPGroups is a satellite of its server -- a group has no meaning without the directory it was read from -- and the rest are junctions. This plugin is where the problem was already visible. Steps 19-21 are the same sweep hand-written, added because deleting a user, role or user group did not clear this plugin's rows, so an install upgraded from before LDAPDeleteMassItems existed held mappings pointing at ids that were gone. Those steps stay as they are -- anyone past them has already run them -- but they only ever ran once, against a backlog. These constraints are what stop the backlog forming again, in the database rather than in a hook that has to be remembered at every delete site. lugTargetID is deliberately excluded and stays polymorphic: its parent table is chosen by the sibling lugTargetType column, so there is no single table to reference. Step 19 sweeps it by hand and the hook keeps it clean, the same shape as core's scheduledTasks.stGroupHostID. Verified against a lab clone at schema 390 holding 12 LDAP groups, 12 role associations and 21 user grants. All six applied, the sweep found nothing, a second run planned no ALTER. Behavior probed and rolled back: deleting one LDAP server removed its 2 groups AND the 2 role associations pointing at those groups -- a two-level cascade with no PHP involved, which is the whole point -- and deleting a user removed its 2 grants. Co-Authored-By: Claude --- ldap/class/ldapmanager.class.php | 50 +++++++++++++++++++ .../foreign-keys-applied-per-plugin.test.php | 1 + 2 files changed, 51 insertions(+) diff --git a/ldap/class/ldapmanager.class.php b/ldap/class/ldapmanager.class.php index 596effe..405f8d3 100644 --- a/ldap/class/ldapmanager.class.php +++ b/ldap/class/ldapmanager.class.php @@ -910,6 +910,56 @@ function () { ] ); }, + // 28 - the plugin's six 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 calls are filtered to this plugin's own group, + // so neither can reach another plugin's tables or core's. + // + // Steps 19-21 above are the same idea hand-written, and they are + // why this plugin needed it first: deleting a user, role or user + // group did not clear this plugin's rows, so an install upgraded + // from before LDAPDeleteMassItems existed holds mappings pointing + // at ids that are gone. They stay exactly as they are -- anyone + // past them has already run them -- but they only ever ran once, + // against a backlog. These constraints are what stops the backlog + // ever forming again, in the database rather than in a hook. + // + // The relationships are declared in fogproject's + // commons/schema-constraints.php: + // LDAPGroups.lgServerID -> LDAPServers.lsID + // ldapGroupRoleAssoc.lgraGroupID -> LDAPGroups.lgID + // ldapGroupRoleAssoc.lgraRoleID -> roles.rID + // ldapGroupUserGroupAssoc.lgugGroupID -> LDAPGroups.lgID + // ldapGroupUserGroupAssoc.lgugUserGroupID -> userGroups.ugID + // ldapUserGrant.lugUserID -> users.uId + // all CASCADE. LDAPGroups is a satellite of its server -- a group + // has no meaning without the directory it was read from -- and + // the rest are junctions. + // + // ldapUserGrant.lugTargetID is deliberately NOT among them. Its + // parent table is chosen by the sibling lugTargetType column, so + // there is no single table to reference; step 19 sweeps it by + // hand and LDAPDeleteMassItems keeps it clean. Same shape as + // core's scheduledTasks.stGroupHostID. + // + // No column change was needed: all six are int(11) NOT NULL + // against int(11) parents, and none carries a sentinel. + // + // Appended, never folded into an earlier step -- installdb() + // SKIPS the pSchema steps an install has already passed, which is + // the mistake steps 10-16 above exist to repair. + function () { + $res = \FOG\Db\SchemaReconciler::sweepOrphans('ldap'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints('ldap'); + }, ]; } /** diff --git a/tests/foreign-keys-applied-per-plugin.test.php b/tests/foreign-keys-applied-per-plugin.test.php index 824590f..856243f 100644 --- a/tests/foreign-keys-applied-per-plugin.test.php +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -55,6 +55,7 @@ 'location' => 'location/class/locationmanager.class.php', 'ou' => 'ou/class/oumanager.class.php', 'windowskey' => 'windowskey/class/windowskeymanager.class.php', + 'ldap' => 'ldap/class/ldapmanager.class.php', ]; /** From bc1706f6cb916865f3bf6c8e41ae63295dbeb132 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Sat, 29 Aug 2026 12:24:16 -0500 Subject: [PATCH 5/5] oidc: declare its eight foreign keys fogproject ADR 0031, the last of the five plugins. One appended schema step, no column changes: all eight are int(11) NOT NULL against int(11) parents and none carries a sentinel. OIDCGroups.ogProviderID -> OIDCProviders.opID oidcIdentity.oiProviderID -> OIDCProviders.opID oidcIdentity.oiUserID -> users.uId oidcGroupRoleAssoc.ograGroupID -> OIDCGroups.ogID oidcGroupRoleAssoc.ograRoleID -> roles.rID oidcGroupUserGroupAssoc.ogugGroupID -> OIDCGroups.ogID oidcGroupUserGroupAssoc.ogugUserGroupID -> userGroups.ugID oidcUserGrant.ougUserID -> users.uId All CASCADE. OIDCGroups and oidcIdentity are satellites -- a group claim mapping and a subject-to-user binding both mean nothing without the provider they came from -- and the rest are junctions. oidcIdentity is the one worth being deliberate about rather than defaulting. It is the record that this external subject IS this FOG user, so deleting either end has to take it; leaving it behind would let the next user created with a recycled id inherit someone else's identity binding. All eight land in OIDCManager even though five of the tables belong to other managers, including oidcIdentity, whose own schema() runs separately at step 1. The calls are driven by the table names in the map rather than by whose manager is executing, so the plugin needs exactly one constraint step and it belongs in the orchestrator -- by which point every one of its tables exists. ougTargetID is excluded and stays polymorphic: its parent table is chosen by the sibling ougTargetType column, the same shape as core's scheduledTasks.stGroupHostID. Verified against a lab clone at schema 390 holding 1 provider, 1 group, 2 identities and 2 user grants. All eight applied, the sweep found nothing, a second run planned no ALTER. Behavior probed and rolled back: deleting the provider removed both identities, its group, and the role association pointing at that group -- two levels, no PHP -- and deleting the user that held the grants removed both. Co-Authored-By: Claude --- oidc/class/oidcmanager.class.php | 53 +++++++++++++++++++ .../foreign-keys-applied-per-plugin.test.php | 1 + 2 files changed, 54 insertions(+) diff --git a/oidc/class/oidcmanager.class.php b/oidc/class/oidcmanager.class.php index cf32eb3..c100297 100644 --- a/oidc/class/oidcmanager.class.php +++ b/oidc/class/oidcmanager.class.php @@ -273,6 +273,59 @@ function () { ] ); }, + // 9 - the plugin's eight 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 calls are filtered to this plugin's own group, + // so neither can reach another plugin's tables or core's. + // + // All eight land here even though five of the tables belong to + // other managers, including oidcIdentity, whose own schema() runs + // separately at step 1 above. The calls are driven by the table + // names in fogproject's commons/schema-constraints.php rather than + // by whose manager is executing, so the plugin needs exactly one + // constraint step and it belongs in the orchestrator -- by which + // point every one of its tables exists. + // + // OIDCGroups.ogProviderID -> OIDCProviders.opID + // oidcIdentity.oiProviderID -> OIDCProviders.opID + // oidcIdentity.oiUserID -> users.uId + // oidcGroupRoleAssoc.ograGroupID -> OIDCGroups.ogID + // oidcGroupRoleAssoc.ograRoleID -> roles.rID + // oidcGroupUserGroupAssoc.ogugGroupID -> OIDCGroups.ogID + // oidcGroupUserGroupAssoc.ogugUserGroupID -> userGroups.ugID + // oidcUserGrant.ougUserID -> users.uId + // + // All CASCADE. OIDCGroups and oidcIdentity are satellites -- a + // group claim mapping and a subject-to-user binding both mean + // nothing without the provider they came from -- and the rest are + // junctions. oidcIdentity is the one worth being deliberate about: + // it is the record that this external subject IS this FOG user, so + // deleting either end has to take it. Leaving it would let the + // next user created with a recycled id inherit someone else's + // identity binding. + // + // ougTargetID is deliberately excluded and stays polymorphic: its + // parent table is chosen by the sibling ougTargetType column, so + // there is no single table to reference. Same shape as core's + // scheduledTasks.stGroupHostID. + // + // No column change was needed: all eight are int(11) NOT NULL + // against int(11) parents, and none carries a sentinel. + // + // Appended, never folded into an earlier step -- installdb() SKIPS + // the pSchema steps an install has already passed. + function () { + $res = \FOG\Db\SchemaReconciler::sweepOrphans('oidc'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints('oidc'); + }, ]; } /** diff --git a/tests/foreign-keys-applied-per-plugin.test.php b/tests/foreign-keys-applied-per-plugin.test.php index 856243f..71390e8 100644 --- a/tests/foreign-keys-applied-per-plugin.test.php +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -56,6 +56,7 @@ 'ou' => 'ou/class/oumanager.class.php', 'windowskey' => 'windowskey/class/windowskeymanager.class.php', 'ldap' => 'ldap/class/ldapmanager.class.php', + 'oidc' => 'oidc/class/oidcmanager.class.php', ]; /**