Skip to content

Constrain capone and subnetgroup, and gate the classification #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mastacontrola merged 1 commit into from
Aug 29, 2026
Merged

Constrain capone and subnetgroup, and gate the classification #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 104 additions & 3 deletions capone/class/caponemanager.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
],
[
Expand Down Expand Up @@ -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');
},
];
}
/**
Expand Down
44 changes: 44 additions & 0 deletions subnetgroup/class/subnetgroupmanager.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);
},
];
}
/**
Expand Down
2 changes: 2 additions & 0 deletions tests/foreign-keys-applied-per-plugin.test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
];

/**
Expand Down
193 changes: 193 additions & 0 deletions tests/plugin-id-columns-are-classified.test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
<?php
/**
* Every id column in a plugin table has been CLASSIFIED in core's map.
*
* fogproject's tests/foreign-key-map.test.php makes the same demand of core,
* and says why: nobody forgets a constraint they have decided to add, what
* gets forgotten is DECIDING. A table lands with a `somethingID` column,
* nothing enforces the relationship, and it joins the class of orphan sources
* ADR 0031 exists to close -- silently, and for years.
*
* That test cannot see this repository. It walks
* `commons/schema-expected.php`, which describes core's 70 tables and no
* plugin table at all, so for the whole of ADR 0031 nothing required a
* decision about a plugin's id columns. Three had never had one:
* `capone.cImageID`, `capone.cOSID` and `subnetgroup.sgGroupID`, found by
* looking rather than by any gate. This is the missing half.
*
* WHAT COUNTS AS AN ID COLUMN. A name ending in `ID` or `Id` appearing in a
* manager's createTableSql() field list. Across all 24 plugin tables that
* yields exactly one false positive -- OIDCProviders.opClientID is an OIDC
* client credential, a string, not a reference to anything -- so it is
* bounded by a single named exception rather than by a list that could
* quietly absorb real ones.
*
* THE ESCAPE IS TO CLASSIFY THE COLUMN, not to add it to $primary. Put it in
* fogproject's commons/schema-constraints.php, including as `poly` (target
* table named by a sibling column, so no constraint is expressible). That is
* an answer. Absence is not.
*
* Like tests/foreign-keys-applied-per-plugin.test.php, the expected sets are
* pinned here by hand because bin/fetch-plugins.sh fetches this repository on
* its own and cannot assume a fogproject checkout is anywhere nearby. Both
* halves have to be edited together.
*
* Exit status 0 = pass, 1 = fail.
*
* PHP version 7.4+
*
* @category Tests
* @package FOGProject
* @author Tom Elliott <tommygunsster@gmail.com>
* @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);