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/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/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/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 new file mode 100644 index 0000000..71390e8 --- /dev/null +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -0,0 +1,252 @@ + + * @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', + 'ou' => 'ou/class/oumanager.class.php', + 'windowskey' => 'windowskey/class/windowskeymanager.class.php', + 'ldap' => 'ldap/class/ldapmanager.class.php', + 'oidc' => 'oidc/class/oidcmanager.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; + } + // 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')"; + + $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'); +$tight = preg_replace('/\s+/', '', $src); +$checks++; +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($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'; +} + +/* + * 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) +); 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' + ); + }, ]; } /**