From d9dba34c85d9d9ec60b6d7d63322b45ec2e0e384 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Tue, 1 Sep 2026 18:42:01 -0500 Subject: [PATCH] feat: location and ou stop pushing a value across a group (ADR 0038 decision 13) Both plugins shipped a second hook file -- AddLocationGroup, AddOUGroup -- whose only job was to set one value on every member of a group. Neither was salvageable, for two separate reasons. It was a COPY, not a grant. The hook read the membership at the instant the button was pressed and wrote a row per member, so a host added to the group afterward got nothing and a host removed kept what it had. That is exactly the model ADR 0038 removes from the core group page. And it ALWAYS CLOBBERED. Every save ran a deletemass over every member before inserting, so "leave this host alone" was not a state the form could express. Saving the tab to set one host's location wiped it from every other member of the group. These do not become grants. A location and an OU are single-valued per host -- the write path has always been delete-then-insert-one. A grant is a SET that several groups union into, and unioning two locations means nothing. Single-valued settings go to the host list's mass edit, which is where ADR 0038 sent the core group page's imperative fields, and decision 13 added HOST_MASSEDIT_* precisely so a plugin could follow. So each plugin now contributes one field through HOST_MASSEDIT_FIELDS and applies it through HOST_MASSEDIT_APPLY, and the group hook, its JS, and the 'group' entry in its PAGE_JS_FILES mapping are deleted. Core draws the three-state action control, which is what makes "leave alone" expressible at all. Two details worth naming. The value control renders EMPTY: there is no honest value to pre-fill from a selection whose hosts disagree, so what they hold is stated in the hint instead. And the hint goes through SharedHostValues::forHostRows() rather than a hand-rolled query, because a host with no location has no row at all -- counting rows would call three hosts out of five "in agreement". forHostRows() compares the row count to the selection size for that reason. Its answer is in the column's own terms, so a uniform selection comes back as an id; the name is looked up before rendering, and only when there is one name to show. A 'set' naming a record that does not exist THROWS rather than falling through to the delete. Treating it as a clear would strip the value off every selected host -- the old clobber arriving by a new route. tests/group-push-became-mass-edit.test.php drives the apply path for real. That needed FOG\Base\Hook in the stubs, which had never been there: every hook extends it, so no hook could be loaded at all and everything a hook decided had to be pinned by reading its source. That is the right default for a hook that echoes a form and the wrong one for a hook that writes, where the branch taken decides whether rows survive. Proven by mutation: dropping the 'leave' guard, turning an invalid id into a clear, registering only the field event, reporting the raw id in the hint, dropping the empty-selection guard, and restoring the old group hook each turn it red. The last of those trips a generic check as well as the filename one, so a newly written group-push hook fails it too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM --- location/js/fog.location.group.edit.js | 11 - location/src/Hooks/AddLocationGroup.php | 282 ----------------- location/src/Hooks/AddLocationHost.php | 149 +++++++++ location/src/Hooks/AddLocationJS.php | 1 - ou/js/fog.ou.group.edit.js | 11 - ou/src/Hooks/AddOUGroup.php | 277 ----------------- ou/src/Hooks/AddOUHost.php | 149 +++++++++ ou/src/Hooks/AddOUJS.php | 1 - tests/group-push-became-mass-edit.test.php | 346 +++++++++++++++++++++ tests/stubs/fog-stubs.php | 218 +++++++++++++ 10 files changed, 862 insertions(+), 583 deletions(-) delete mode 100644 location/js/fog.location.group.edit.js delete mode 100644 location/src/Hooks/AddLocationGroup.php delete mode 100644 ou/js/fog.ou.group.edit.js delete mode 100644 ou/src/Hooks/AddOUGroup.php create mode 100644 tests/group-push-became-mass-edit.test.php diff --git a/location/js/fog.location.group.edit.js b/location/js/fog.location.group.edit.js deleted file mode 100644 index 24efaf02..00000000 --- a/location/js/fog.location.group.edit.js +++ /dev/null @@ -1,11 +0,0 @@ -(function($) { - // The whole tab -- Update wiring plus the create-and-associate button and - // modal -- is one shared helper (fog.common.js). These nine plugin tabs are - // the same card with a different noun, and each used to carry its own copy - // of the submit wiring. - $.registerSelectTab({ - slug: 'group-location', - send: 'location-send', - node: 'location' - }); -})(jQuery); diff --git a/location/src/Hooks/AddLocationGroup.php b/location/src/Hooks/AddLocationGroup.php deleted file mode 100644 index 62c9e822..00000000 --- a/location/src/Hooks/AddLocationGroup.php +++ /dev/null @@ -1,282 +0,0 @@ - - * @author Lee Rowlett - * @license http://opensource.org/licenses/gpl-3.0 GPLv3 - * @link https://fogproject.org - */ - -namespace FOG\Plugins\Location\Hooks; - -/** - * Adds the location choice to groups. - * - * @category AddLocationGroup - * @package FOGProject - * @author Tom Elliott - * @author Lee Rowlett - * @license http://opensource.org/licenses/gpl-3.0 GPLv3 - * @link https://fogproject.org - */ -class AddLocationGroup extends \FOG\Base\Hook -{ - /** - * The name of this hook. - * - * @var string - */ - public $name = 'AddLocationGroup'; - /** - * The description of this hook. - * - * @var string - */ - public $description = 'Add Location to Groups'; - /** - * The active flag (always true but for posterity) - * - * @var bool - */ - public $active = true; - /** - * The node this hook enacts with. - * - * @var string - */ - public $node = 'location'; - /** - * Initialize object. - * - * @return void - */ - public function __construct() - { - parent::__construct(); - $this->registerInstalled([ - ['PLUGINS_INJECT_TABDATA', 'groupTabData'], - ['GROUP_EDIT_SUCCESS', 'groupAddLocationEdit'], - ['GROUP_ADD_FIELDS', 'groupAddLocationField'], - ]); - } - /** - * The group tab data. - * - * @param mixed $arguments The arguments to change. - * - * @return void - */ - public function groupTabData($arguments) - { - global $node; - if ($node != 'group') { - return; - } - $obj = $arguments['obj']; - - $arguments['pluginsTabData'][] = [ - 'name' => _('Location Association'), - 'id' => 'group-location', - 'generator' => function () use ($obj) { - $this->groupLocation($obj); - } - ]; - } - /** - * The group location display - * - * @param object $obj The group object we're working with. - * - * @return void - */ - public function groupLocation($obj) - { - $locationID = (int)filter_input(INPUT_POST, 'location'); - // Group Locations - $locationSelector = self::getClass('LocationManager') - ->buildSelectBox($locationID, 'location'); - - $fields = [ - \FOG\Base\FOGPage::makeLabel( - 'col-sm-3 col-form-label', - 'location', - _('Group Location') - ) => $locationSelector - ]; - - $buttons = \FOG\Base\FOGPage::makeButton( - 'location-send', - _('Update'), - 'btn btn-primary float-end' - ); - // Create-and-associate, the same button and modal the core association - // tabs get. Added before the *_FIELDS event so a listener can still see - // it, and immediately after Update so Update stays the row's rightmost - // (primary) button with this one to its left. The modal it returns is - // echoed after the form -- see below for why it cannot go inside. - $createModal = \FOG\Base\FOGPage::renderAssocCreate( - 'group-location', - 'location', - $buttons, - $obj->get('id') - ); - - self::$HookManager->processEvent( - 'GROUP_LOCATION_FIELDS', - [ - 'fields' => &$fields, - 'buttons' => &$buttons, - 'Group' => &$obj - ] - ); - $rendered = \FOG\Base\FOGPage::formFields($fields); - unset($fields); - - echo \FOG\Base\FOGPage::makeFormTag( - '', - 'group-location-form', - \FOG\Base\FOGPage::makeTabUpdateURL( - 'group-location', - $obj->get('id') - ), - 'post', - 'application/x-www-form-urlencoded', - true - ); - echo '
'; - echo '
'; - echo '

'; - echo _('Location'); - echo '

'; - echo '
'; - echo '
'; - echo $rendered; - echo '
'; - echo '
'; - echo $buttons; - echo '
'; - echo '
'; - echo ''; - // Outside the form, deliberately. The modal holds the fetched create - // form, and a
inside another is invalid markup: the - // browser drops the inner one and the create would post nothing. - echo $createModal; - } - /** - * The location updater element. - * - * @param object $obj The object we're working with. - * - * @return void - */ - public function groupLocationPost($obj) - { - self::checkAuthAndCSRF(); - $locationID = trim( - (int)filter_input(INPUT_POST, 'location') - ); - $insert_fields = ['hostID', 'locationID']; - $insert_values = []; - $hosts = \FOG\Router\Route::getIds( - 'groupassociation', - ['groupID' => $obj->get('id')], - 'hostID' - ); - if (count($hosts ?: [])) { - \FOG\Router\Route::deletemass( - 'locationassociation', - ['hostID' => $hosts] - ); - if (self::getClass('Location', $locationID)->isValid()) { - foreach ((array)$hosts as $ind => &$hostID) { - $insert_values[] = [$hostID, $locationID]; - unset($hostID); - } - } - } - if (count($insert_values) > 0) { - self::getClass('LocationAssociationManager') - ->insertBatch( - $insert_fields, - $insert_values - ); - } - } - /** - * The group location selector. - * - * @param mixed $arguments The arguments to change. - * - * @return void - */ - public function groupAddLocationEdit($arguments) - { - self::checkAuthAndCSRF(); - global $tab; - global $node; - if ($node != 'group') { - return; - } - $obj = $arguments['Group']; - try { - switch ($tab) { - case 'group-location': - $this->groupLocationPost($obj); - break; - default: - return; - } - $arguments['code'] = \FOG\Router\HTTPResponseCodes::HTTP_ACCEPTED; - $arguments['hook'] = 'GROUP_EDIT_LOCATION_SUCCESS'; - $arguments['msg'] = json_encode( - [ - 'msg' => _('Group Location Updated!'), - 'title' => _('Group Location Update Success') - ] - ); - } catch (\Exception $e) { - $arguments['code'] = ( - $arguments['serverFault'] ? - \FOG\Router\HTTPResponseCodes::HTTP_INTERNAL_SERVER_ERROR : - \FOG\Router\HTTPResponseCodes::HTTP_BAD_REQUEST - ); - $arguments['hook'] = 'GROUP_EDIT_LOCATION_FAIL'; - $arguments['msg'] = json_encode( - [ - 'error' => $e->getMessage(), - 'title' => _('Group Update Location Fail') - ] - ); - } - } - /** - * The group location field for function add. - * - * @param mixed $arguments The arguments to change. - * - * @return void - */ - public function groupAddLocationField($arguments) - { - global $node; - if ($node != 'group') { - return; - } - $locationID = (int)filter_input(INPUT_POST, 'location'); - $locationSelector = self::getClass('LocationManager') - ->buildSelectBox($locationID, 'location'); - - $arguments['fields'][ - \FOG\Base\FOGPage::makeLabel( - 'col-sm-3 col-form-label', - 'location', - _('Group Location') - ) - ] = $locationSelector; - } -} diff --git a/location/src/Hooks/AddLocationHost.php b/location/src/Hooks/AddLocationHost.php index bda2fa8a..d29a2632 100644 --- a/location/src/Hooks/AddLocationHost.php +++ b/location/src/Hooks/AddLocationHost.php @@ -61,6 +61,8 @@ public function __construct() $this->registerInstalled([ ['PLUGINS_INJECT_TABDATA', 'hostTabData'], ['HOST_EDIT_SUCCESS', 'hostAddLocationEdit'], + ['HOST_MASSEDIT_FIELDS', 'massEditFields'], + ['HOST_MASSEDIT_APPLY', 'massEditApply'], ['HOST_ADD_FIELDS', 'hostAddLocationField'], ['HOST_REGISTER', 'hostAddLocationRegister'], ]); @@ -314,4 +316,151 @@ public function hostAddLocationField($arguments) ) ] = $locationSelector; } + /** + * Contributes the location to the host list's Mass Edit form. + * + * ADR 0038 decision 13. This replaces AddLocationGroup, whose only job was + * to set one location across a group's members. That hook was a copy, not a + * grant: it read the membership at the instant the button was pressed and + * wrote a row per member, so a host added to the group afterward got + * nothing and a host removed kept what it had. Worse, it had no way to + * express "leave this host alone" -- every save deleted the association + * for every member first, so setting one host's location through the group + * wiped it from all of them. + * + * A location is single-valued per host: the write path has always been + * delete-then-insert-one. That is why this is a mass edit rather than one + * of ADR 0038's grant tables -- a grant is a SET that several groups can + * union into, and unioning two locations means nothing. + * + * Core draws the three-state action control around this field, which is + * what makes "leave alone" expressible at all. The value control is + * rendered EMPTY on purpose: there is no honest value to pre-fill it with + * when the selection disagrees. What the hosts hold is stated in the hint + * instead, where "(varies)" is sayable. + * + * @param mixed $arguments The arguments to change. + * + * @return void + */ + public function massEditFields($arguments) + { + $hostIDs = (array)$arguments['hostIDs']; + + $arguments['fields']['location'] = [ + 'label' => _('Host Location'), + 'input' => self::getClass('LocationManager')->buildSelectBox( + '', + 'value[location]', + 'name', + '', + false, + 'id', + 'massedit-location' + ), + 'hint' => \FOG\Util\SharedHostValues::hint( + $this->_sharedLocation($hostIDs) + ), + ]; + } + /** + * What the selected hosts hold, as a name rather than an id. + * + * SharedHostValues::forHostRows() answers in the column's own terms, so a + * uniform selection comes back as the location id. Rendering that would put + * a bare number in front of the admin where every other hint shows the + * value they set. The lookup only happens when the answer is uniform and + * non-empty, which is the only case where there is one name to show. + * + * forHostRows() is also the reason this is not a hand-rolled query: a + * host with no location has no row at all, so counting rows would call + * three hosts out of five "in agreement". It compares the row count to + * the selection size for exactly that reason. + * + * @param array $hostIDs the selection + * + * @return array a SharedHostValues info array + */ + private function _sharedLocation(array $hostIDs) + { + $info = \FOG\Util\SharedHostValues::forHostRows( + $hostIDs, + 'locationAssoc', + 'laHostID', + ['location' => 'laLocationID'] + )['location']; + + if (!empty($info['uniform']) && '' !== (string)$info['value']) { + $item = self::getClass('Location', (int)$info['value']); + if ($item->isValid()) { + $info['value'] = $item->get('name'); + } + } + + return $info; + } + /** + * Applies a resolved location action across the selection. + * + * The action arrives already reduced to leave/set/clear, so there is no + * sentinel to parse and no way for an empty control to be mistaken for + * "clear it" -- which is the failure the three-state model exists to stop + * and the one AddLocationGroup could not avoid. + * + * @param mixed $arguments The arguments to change. + * + * @return void + */ + public function massEditApply($arguments) + { + if (!isset($arguments['actions']['location'])) { + return; + } + $instruction = $arguments['actions']['location']; + $action = $instruction['action']; + if ('leave' === $action) { + return; + } + $hostIDs = array_values( + array_filter( + array_map('intval', (array)$arguments['hostIDs']), + function ($id) { + return $id > 0; + } + ) + ); + if (count($hostIDs) < 1) { + return; + } + + $itemID = 0; + if ('set' === $action) { + $itemID = (int)$instruction['value']; + // A set naming a location that does not exist is a bad request, not + // an instruction to clear: silently turning it into one would + // strip the location off every selected host. + if ($itemID < 1 + || !self::getClass('Location', $itemID)->isValid() + ) { + throw new \Exception(_('Invalid Location selected')); + } + } + + // Both branches clear first. The association is single-valued per + // host, so a set is a replace -- and doing it in one deletemass + // rather than per host keeps it to one statement. + \FOG\Router\Route::deletemass( + 'locationassociation', + ['hostID' => $hostIDs] + ); + if ($itemID < 1) { + return; + } + $insert_values = []; + foreach ($hostIDs as $hostID) { + $insert_values[] = [$hostID, $itemID]; + } + self::getClass('LocationAssociationManager') + ->insertBatch(['hostID', 'locationID'], $insert_values); + } } diff --git a/location/src/Hooks/AddLocationJS.php b/location/src/Hooks/AddLocationJS.php index 5be38002..b5022ca3 100644 --- a/location/src/Hooks/AddLocationJS.php +++ b/location/src/Hooks/AddLocationJS.php @@ -75,7 +75,6 @@ public function injectJSFiles($arguments) 'location' => [], 'report' => ['secondary' => true, 'fallback' => true], 'host' => ['secondary' => true, 'fallback' => true], - 'group' => ['secondary' => true, 'fallback' => true], ]); } } diff --git a/ou/js/fog.ou.group.edit.js b/ou/js/fog.ou.group.edit.js deleted file mode 100644 index a03190b0..00000000 --- a/ou/js/fog.ou.group.edit.js +++ /dev/null @@ -1,11 +0,0 @@ -(function($) { - // The whole tab -- Update wiring plus the create-and-associate button and - // modal -- is one shared helper (fog.common.js). These nine plugin tabs are - // the same card with a different noun, and each used to carry its own copy - // of the submit wiring. - $.registerSelectTab({ - slug: 'group-ou', - send: 'ou-send', - node: 'ou' - }); -})(jQuery); diff --git a/ou/src/Hooks/AddOUGroup.php b/ou/src/Hooks/AddOUGroup.php deleted file mode 100644 index 2cf97dfb..00000000 --- a/ou/src/Hooks/AddOUGroup.php +++ /dev/null @@ -1,277 +0,0 @@ - - * @license http://opensource.org/licenses/gpl-3.0 GPLv3 - * @link https://fogproject.org - */ - -namespace FOG\Plugins\OU\Hooks; - -/** - * Adds the OU choice to groups. - * - * @category AddOUGroup - * @package FOGProject - * @author Tom Elliott - * @license http://opensource.org/licenses/gpl-3.0 GPLv3 - * @link https://fogproject.org - */ -class AddOUGroup extends \FOG\Base\Hook -{ - /** - * The name of this hook. - * - * @var string - */ - public $name = 'AddOUGroup'; - /** - * The description of this hook. - * - * @var string - */ - public $description = 'Add OU to Groups'; - /** - * The active flag (always true but for posterity) - * - * @var bool - */ - public $active = true; - /** - * The node this hook enacts with. - * - * @var string - */ - public $node = 'ou'; - /** - * Initialize object. - * - * @return void - */ - public function __construct() - { - parent::__construct(); - $this->registerInstalled([ - ['PLUGINS_INJECT_TABDATA', 'groupTabData'], - ['GROUP_EDIT_SUCCESS', 'groupAddOUEdit'], - ['GROUP_ADD_FIELDS', 'groupAddOUField'], - ]); - } - /** - * The group tab data. - * - * @param mixed $arguments The arguments to change. - * - * @return void - */ - public function groupTabData($arguments) - { - global $node; - if ($node != 'group') { - return; - } - $obj = $arguments['obj']; - - $arguments['pluginsTabData'][] = [ - 'name' => _('OU Association'), - 'id' => 'group-ou', - 'generator' => function () use ($obj) { - $this->groupOU($obj); - } - ]; - } - /** - * The group ou display - * - * @param object $obj The group object we're working with. - * - * @return void - */ - public function groupOU($obj) - { - $ouID = (int)filter_input(INPUT_POST, 'ou'); - // Group OUs - $ouSelector = self::getClass('OUManager') - ->buildSelectBox($ouID, 'ou'); - - $fields = [ - \FOG\Base\FOGPage::makeLabel( - 'col-sm-3 col-form-label', - 'ou', - _('Group OU') - ) => $ouSelector - ]; - - $buttons = \FOG\Base\FOGPage::makeButton( - 'ou-send', - _('Update'), - 'btn btn-primary float-end' - ); - // Create-and-associate, the same button and modal the core association - // tabs get. Added before the *_FIELDS event so a listener can still see - // it, and immediately after Update so Update stays the row's rightmost - // (primary) button with this one to its left. The modal it returns is - // echoed after the form -- see below for why it cannot go inside. - $createModal = \FOG\Base\FOGPage::renderAssocCreate( - 'group-ou', - 'ou', - $buttons, - $obj->get('id'), - _('OU') - ); - - self::$HookManager->processEvent( - 'GROUP_OU_FIELDS', - [ - 'fields' => &$fields, - 'buttons' => &$buttons, - 'Group' => &$obj - ] - ); - $rendered = \FOG\Base\FOGPage::formFields($fields); - unset($fields); - - echo \FOG\Base\FOGPage::makeFormTag( - '', - 'group-ou-form', - \FOG\Base\FOGPage::makeTabUpdateURL( - 'group-ou', - $obj->get('id') - ), - 'post', - 'application/x-www-form-urlencoded', - true - ); - echo '
'; - echo '
'; - echo '

'; - echo _('OU'); - echo '

'; - echo '
'; - echo '
'; - echo $rendered; - echo '
'; - echo '
'; - echo $buttons; - echo '
'; - echo '
'; - echo ''; - // Outside the form, deliberately. The modal holds the fetched create - // form, and a
inside another is invalid markup: the - // browser drops the inner one and the create would post nothing. - echo $createModal; - } - /** - * The OU updater element. - * - * @param object $obj The object we're working with. - * - * @return void - */ - public function groupOUPost($obj) - { - self::checkAuthAndCSRF(); - $ouID = trim( - (int)filter_input(INPUT_POST, 'ou') - ); - $insert_fields = ['hostID', 'ouID']; - $insert_values = []; - $hosts = $obj->get('hosts'); - if (count($hosts ?: [])) { - \FOG\Router\Route::deletemass( - 'ouassociation', - ['hostID' => $hosts] - ); - if ($ouID > 0) { - foreach ((array)$hosts as $ind => &$hostID) { - $insert_values[] = [$hostID, $ouID]; - unset($hostID); - } - } - } - if (count($insert_values) > 0) { - self::getClass('OUAssociationManager') - ->insertBatch( - $insert_fields, - $insert_values - ); - } - } - /** - * The group ou selector. - * - * @param mixed $arguments The arguments to change. - * - * @return void - */ - public function groupAddOUEdit($arguments) - { - self::checkAuthAndCSRF(); - global $tab; - global $node; - if ($node != 'group') { - return; - } - $obj = $arguments['Group']; - try { - switch ($tab) { - case 'group-ou': - $this->groupOUPost($obj); - break; - default: - return; - } - $arguments['code'] = \FOG\Router\HTTPResponseCodes::HTTP_ACCEPTED; - $arguments['hook'] = 'GROUP_EDIT_OU_SUCCESS'; - $arguments['msg'] = json_encode( - [ - 'msg' => _('Group OU Updated!'), - 'title' => _('Group OU Update Success') - ] - ); - } catch (\Exception $e) { - $arguments['code'] = ( - $arguments['serverFault'] ? - \FOG\Router\HTTPResponseCodes::HTTP_INTERNAL_SERVER_ERROR : - \FOG\Router\HTTPResponseCodes::HTTP_BAD_REQUEST - ); - $arguments['hook'] = 'GROUP_EDIT_OU_FAIL'; - $arguments['msg'] = json_encode( - [ - 'error' => $e->getMessage(), - 'title' => _('Group Update OU Fail') - ] - ); - } - } - /** - * The group OU field for function add. - * - * @param mixed $arguments The arguments to change. - * - * @return void - */ - public function groupAddOUField($arguments) - { - global $node; - if ($node != 'group') { - return; - } - $ouID = (int)filter_input(INPUT_POST, 'ou'); - $ouSelector = self::getClass('OUManager') - ->buildSelectBox($ouID, 'ou'); - - $arguments['fields'][ - \FOG\Base\FOGPage::makeLabel( - 'col-sm-3 col-form-label', - 'ou', - _('Group OU') - ) - ] = $ouSelector; - } -} diff --git a/ou/src/Hooks/AddOUHost.php b/ou/src/Hooks/AddOUHost.php index 5d8c9d80..f6e450d2 100644 --- a/ou/src/Hooks/AddOUHost.php +++ b/ou/src/Hooks/AddOUHost.php @@ -61,6 +61,8 @@ public function __construct() $this->registerInstalled([ ['PLUGINS_INJECT_TABDATA', 'hostTabData'], ['HOST_EDIT_SUCCESS', 'hostAddOUEdit'], + ['HOST_MASSEDIT_FIELDS', 'massEditFields'], + ['HOST_MASSEDIT_APPLY', 'massEditApply'], ['HOST_ADD_FIELDS', 'hostAddOUField'], ['HOST_REGISTER', 'hostAddOURegister'], ]); @@ -315,4 +317,151 @@ public function hostAddOUField($arguments) ) ] = $ouSelector; } + /** + * Contributes the ou to the host list's Mass Edit form. + * + * ADR 0038 decision 13. This replaces AddOUGroup, whose only job was + * to set one ou across a group's members. That hook was a copy, not a + * grant: it read the membership at the instant the button was pressed and + * wrote a row per member, so a host added to the group afterward got + * nothing and a host removed kept what it had. Worse, it had no way to + * express "leave this host alone" -- every save deleted the association + * for every member first, so setting one host's ou through the group + * wiped it from all of them. + * + * A ou is single-valued per host: the write path has always been + * delete-then-insert-one. That is why this is a mass edit rather than one + * of ADR 0038's grant tables -- a grant is a SET that several groups can + * union into, and unioning two ous means nothing. + * + * Core draws the three-state action control around this field, which is + * what makes "leave alone" expressible at all. The value control is + * rendered EMPTY on purpose: there is no honest value to pre-fill it with + * when the selection disagrees. What the hosts hold is stated in the hint + * instead, where "(varies)" is sayable. + * + * @param mixed $arguments The arguments to change. + * + * @return void + */ + public function massEditFields($arguments) + { + $hostIDs = (array)$arguments['hostIDs']; + + $arguments['fields']['ou'] = [ + 'label' => _('Host OU'), + 'input' => self::getClass('OUManager')->buildSelectBox( + '', + 'value[ou]', + 'name', + '', + false, + 'id', + 'massedit-ou' + ), + 'hint' => \FOG\Util\SharedHostValues::hint( + $this->_sharedOU($hostIDs) + ), + ]; + } + /** + * What the selected hosts hold, as a name rather than an id. + * + * SharedHostValues::forHostRows() answers in the column's own terms, so a + * uniform selection comes back as the ou id. Rendering that would put + * a bare number in front of the admin where every other hint shows the + * value they set. The lookup only happens when the answer is uniform and + * non-empty, which is the only case where there is one name to show. + * + * forHostRows() is also the reason this is not a hand-rolled query: a + * host with no ou has no row at all, so counting rows would call + * three hosts out of five "in agreement". It compares the row count to + * the selection size for exactly that reason. + * + * @param array $hostIDs the selection + * + * @return array a SharedHostValues info array + */ + private function _sharedOU(array $hostIDs) + { + $info = \FOG\Util\SharedHostValues::forHostRows( + $hostIDs, + 'ouAssoc', + 'oaHostID', + ['ou' => 'oaOUID'] + )['ou']; + + if (!empty($info['uniform']) && '' !== (string)$info['value']) { + $item = self::getClass('OU', (int)$info['value']); + if ($item->isValid()) { + $info['value'] = $item->get('name'); + } + } + + return $info; + } + /** + * Applies a resolved ou action across the selection. + * + * The action arrives already reduced to leave/set/clear, so there is no + * sentinel to parse and no way for an empty control to be mistaken for + * "clear it" -- which is the failure the three-state model exists to stop + * and the one AddOUGroup could not avoid. + * + * @param mixed $arguments The arguments to change. + * + * @return void + */ + public function massEditApply($arguments) + { + if (!isset($arguments['actions']['ou'])) { + return; + } + $instruction = $arguments['actions']['ou']; + $action = $instruction['action']; + if ('leave' === $action) { + return; + } + $hostIDs = array_values( + array_filter( + array_map('intval', (array)$arguments['hostIDs']), + function ($id) { + return $id > 0; + } + ) + ); + if (count($hostIDs) < 1) { + return; + } + + $itemID = 0; + if ('set' === $action) { + $itemID = (int)$instruction['value']; + // A set naming a ou that does not exist is a bad request, not + // an instruction to clear: silently turning it into one would + // strip the ou off every selected host. + if ($itemID < 1 + || !self::getClass('OU', $itemID)->isValid() + ) { + throw new \Exception(_('Invalid OU selected')); + } + } + + // Both branches clear first. The association is single-valued per + // host, so a set is a replace -- and doing it in one deletemass + // rather than per host keeps it to one statement. + \FOG\Router\Route::deletemass( + 'ouassociation', + ['hostID' => $hostIDs] + ); + if ($itemID < 1) { + return; + } + $insert_values = []; + foreach ($hostIDs as $hostID) { + $insert_values[] = [$hostID, $itemID]; + } + self::getClass('OUAssociationManager') + ->insertBatch(['hostID', 'ouID'], $insert_values); + } } diff --git a/ou/src/Hooks/AddOUJS.php b/ou/src/Hooks/AddOUJS.php index b86e7012..0a8a95a3 100644 --- a/ou/src/Hooks/AddOUJS.php +++ b/ou/src/Hooks/AddOUJS.php @@ -75,7 +75,6 @@ public function injectJSFiles($arguments) 'ou' => ['fallback' => true], 'report' => ['secondary' => true, 'fallback' => true], 'host' => ['secondary' => true], - 'group' => ['secondary' => true], ]); } } diff --git a/tests/group-push-became-mass-edit.test.php b/tests/group-push-became-mass-edit.test.php new file mode 100644 index 00000000..1c42f811 --- /dev/null +++ b/tests/group-push-became-mass-edit.test.php @@ -0,0 +1,346 @@ +]`, and no + * plugin draws its own action control -- a two-state field in a mass + * edit is the defect the design exists to prevent. + * + * Usage: php tests/group-push-became-mass-edit.test.php + * Exit status 0 = pass, 1 = fail. + */ + +$root = dirname(__DIR__); +$fails = []; + +if (!function_exists('_')) { + /** + * Stand-in for gettext. + * + * @param string $s the string + * + * @return string + */ + function _($s) + { + return $s; + } +} + +/** + * Records a failure. + * + * @param string $why what went wrong + * + * @return void + */ +function fail($why) +{ + global $fails; + $fails[] = $why; +} + +/** + * Asserts a condition. + * + * @param string $label what is being checked + * @param bool $cond the condition + * + * @return bool + */ +function check($label, $cond) +{ + if (!$cond) { + fail($label); + } + + return (bool)$cond; +} + +require_once __DIR__ . '/stubs/fog-stubs.php'; + +// ------------------------------------------------------- 1. the hooks are gone + +$gone = [ + 'location/src/Hooks/AddLocationGroup.php', + 'ou/src/Hooks/AddOUGroup.php', + 'location/js/fog.location.group.edit.js', + 'ou/js/fog.ou.group.edit.js', +]; +foreach ($gone as $rel) { + check( + "$rel is not shipped any more", + !file_exists($root . '/' . $rel) + ); +} + +// No hook anywhere may write an association over a group's membership again. +// Both old hooks did it the same way: resolve the members, then deletemass. +$hookFiles = []; +foreach (glob($root . '/*/src/Hooks/*.php') as $file) { + $hookFiles[$file] = (string)file_get_contents($file); +} +foreach ($hookFiles as $file => $src) { + $code = preg_replace('#/\*.*?\*/#s', '', $src); + $code = (string)preg_replace('#//[^\n]*#', '', (string)$code); + $readsMembers = ( + false !== strpos($code, "get('hosts')") + || false !== strpos($code, "'groupassociation'") + ); + if (!$readsMembers) { + continue; + } + check( + basename($file) . ' does not deletemass over a group membership', + false === strpos($code, 'deletemass') + ); +} + +// ------------------------------------------- 2. both events, on both plugins + +$cases = [ + [ + 'file' => 'location/src/Hooks/AddLocationHost.php', + 'class' => 'FOG\Plugins\Location\Hooks\AddLocationHost', + 'key' => 'location', + 'assoc' => 'locationassociation', + 'col' => 'locationID', + ], + [ + 'file' => 'ou/src/Hooks/AddOUHost.php', + 'class' => 'FOG\Plugins\OU\Hooks\AddOUHost', + 'key' => 'ou', + 'assoc' => 'ouassociation', + 'col' => 'ouID', + ], +]; + +foreach ($cases as $case) { + require_once $root . '/' . $case['file']; + $hook = new $case['class'](); + $key = $case['key']; + + $events = []; + foreach ($hook->registered as $pair) { + $events[] = $pair[0]; + } + check( + "$key registers HOST_MASSEDIT_FIELDS", + in_array('HOST_MASSEDIT_FIELDS', $events, true) + ); + check( + "$key registers HOST_MASSEDIT_APPLY", + in_array('HOST_MASSEDIT_APPLY', $events, true) + ); + + // ------------------------------------------------- 3. the field it offers + + \FOG\Util\SharedHostValues::$rows = [ + $key => ['uniform' => false, 'value' => ''], + ]; + $fields = []; + $hostIDs = [1, 2, 3]; + $args = ['fields' => &$fields, 'hostIDs' => &$hostIDs]; + $hook->massEditFields($args); + + if (check("$key contributes a field", isset($fields[$key]))) { + $field = $fields[$key]; + check( + "$key names its control value[$key]", + false !== strpos((string)$field['input'], 'name="value[' . $key . ']"') + ); + check( + "$key reports a mixed selection as (varies)", + '(varies)' === $field['hint'] + ); + // Core draws the action control. A plugin drawing its own could ship + // a two-state field, which is the defect this design prevents. + check( + "$key does not draw its own action control", + false === strpos((string)$field['input'], 'action[') + ); + // Rendered empty: there is no honest value when the selection differs. + check( + "$key renders its control with no value preselected", + false === strpos((string)$field['input'], 'selected') + ); + } + + // A uniform selection reports the NAME, not the raw id. + \FOG\Util\SharedHostValues::$rows = [ + $key => ['uniform' => true, 'value' => '7'], + ]; + $fields = []; + $args = ['fields' => &$fields, 'hostIDs' => &$hostIDs]; + $hook->massEditFields($args); + check( + "$key reports a uniform selection by name rather than by id", + isset($fields[$key]) && 'StubName' === $fields[$key]['hint'] + ); + + // ------------------------------------------------------- 4. applying it + + /** + * Runs one apply and returns what it wrote. + * + * @param object $hook the hook + * @param string $key the field key + * @param array $action the resolved instruction + * + * @return array [deletes, batches, threw] + */ + $apply = function ($hook, $key, $action) { + \FOG\Router\Route::$deleted = []; + $manager = new \FOG\Base\StubItem(); + \FOG\Base\Hook::$classes = [ + 'LocationAssociationManager' => $manager, + 'OUAssociationManager' => $manager, + ]; + if (isset($action['invalid'])) { + \FOG\Base\Hook::$classes['Location'] = function ($id) { + $item = new \FOG\Base\StubItem($id); + $item->valid = false; + + return $item; + }; + \FOG\Base\Hook::$classes['OU'] = + \FOG\Base\Hook::$classes['Location']; + } + $hostIDs = [1, 2, 3]; + $actions = [$key => $action]; + $args = ['hostIDs' => &$hostIDs, 'actions' => &$actions]; + $threw = false; + try { + $hook->massEditApply($args); + } catch (\Exception $e) { + $threw = true; + } + \FOG\Base\Hook::$classes = []; + + return [\FOG\Router\Route::$deleted, $manager->batches, $threw]; + }; + + // 'leave' writes NOTHING. The state the old hook could not express. + list($deleted, $batches, $threw) = $apply( + $hook, + $key, + ['action' => 'leave', 'value' => ''] + ); + check("$key leave deletes nothing", 0 === count($deleted)); + check("$key leave inserts nothing", 0 === count($batches)); + check("$key leave does not throw", !$threw); + + // 'set' replaces: one delete over the selection, one row per host. + list($deleted, $batches, $threw) = $apply( + $hook, + $key, + ['action' => 'set', 'value' => '5'] + ); + check("$key set does not throw", !$threw); + if (check("$key set deletes once", 1 === count($deleted))) { + check( + "$key set deletes from " . $case['assoc'], + $case['assoc'] === $deleted[0][0] + ); + check( + "$key set scopes the delete to the selection", + isset($deleted[0][1]['hostID']) + && [1, 2, 3] === $deleted[0][1]['hostID'] + ); + } + if (check("$key set inserts once", 1 === count($batches))) { + check( + "$key set names hostID and " . $case['col'], + ['hostID', $case['col']] === $batches[0][0] + ); + check( + "$key set writes one row per selected host", + 3 === count($batches[0][1]) + && [1, 5] === $batches[0][1][0] + ); + } + + // 'set' naming something that does not exist THROWS. Falling through to + // the delete would clear the value on every selected host. + list($deleted, $batches, $threw) = $apply( + $hook, + $key, + ['action' => 'set', 'value' => '99', 'invalid' => true] + ); + check("$key set of a missing record throws", $threw); + check( + "$key set of a missing record deletes nothing", + 0 === count($deleted) + ); + check( + "$key set of a missing record inserts nothing", + 0 === count($batches) + ); + + // 'clear' deletes and inserts nothing. + list($deleted, $batches, $threw) = $apply( + $hook, + $key, + ['action' => 'clear', 'value' => ''] + ); + check("$key clear does not throw", !$threw); + check("$key clear deletes once", 1 === count($deleted)); + check("$key clear inserts nothing", 0 === count($batches)); + + // An empty selection is not an instruction to delete everything. + \FOG\Router\Route::$deleted = []; + $none = []; + $actions = [$key => ['action' => 'set', 'value' => '5']]; + $args = ['hostIDs' => &$none, 'actions' => &$actions]; + $hook->massEditApply($args); + check( + "$key writes nothing when the selection is empty", + 0 === count(\FOG\Router\Route::$deleted) + ); +} + +if (count($fails)) { + echo "FAIL (" . count($fails) . "):\n"; + foreach ($fails as $why) { + echo " - $why\n"; + } + exit(1); +} + +echo "ok group push became mass edit\n"; +exit(0); diff --git a/tests/stubs/fog-stubs.php b/tests/stubs/fog-stubs.php index eb1abaf0..afa19049 100644 --- a/tests/stubs/fog-stubs.php +++ b/tests/stubs/fog-stubs.php @@ -155,3 +155,221 @@ public static function createTable(...$args) } } } + +namespace FOG\Base { + /** + * Enough of Hook for a hook's class body to exist and be driven. + * + * Hooks were unreachable from these tests until now: every one of them + * extends this class, so none could be loaded at all, and everything a + * hook decided had to be pinned by reading its source. That is the right + * default for a hook whose job is to echo a form -- and the wrong one for + * a hook that WRITES, where the branch taken decides whether rows survive. + * + * registerInstalled() records rather than registers, so a test can assert + * which events a hook actually asked for. + */ + class Hook + { + /** + * Events this hook registered, as [event, method] pairs. + * + * @var array + */ + public $registered = []; + /** + * What getClass() hands back, keyed by class name. + * + * @var array + */ + public static $classes = []; + /** + * Initialize. + */ + public function __construct() + { + } + /** + * Records the registration. + * + * @param array $events the [event, method] pairs + * + * @return void + */ + public function registerInstalled(array $events) + { + $this->registered = $events; + } + /** + * Hands back a fixture, or a permissive default. + * + * @param string $class the class wanted + * @param mixed $id optional id + * + * @return mixed + */ + public static function getClass($class, $id = null) + { + if (isset(self::$classes[$class])) { + $fixture = self::$classes[$class]; + + return is_callable($fixture) ? $fixture($id) : $fixture; + } + + return new StubItem($id); + } + /** + * A no-op in tests; the real one throws on a bad token. + * + * @return void + */ + public static function checkAuthAndCSRF() + { + } + } + /** + * A stand-in model that exists, has a name, and records writes. + */ + class StubItem + { + /** + * The id it was constructed with. + * + * @var mixed + */ + public $id; + /** + * Whether isValid() answers true. + * + * @var bool + */ + public $valid = true; + /** + * Every insertBatch() call made on it. + * + * @var array + */ + public $batches = []; + /** + * Initialize. + * + * @param mixed $id the id + */ + public function __construct($id = null) + { + $this->id = $id; + } + /** + * Whether the record exists. + * + * @return bool + */ + public function isValid() + { + return $this->valid; + } + /** + * Read a value. + * + * @param string $key the key + * + * @return mixed + */ + public function get($key) + { + return 'name' === $key ? 'StubName' : $this->id; + } + /** + * Records the batch. + * + * @param array $fields the columns + * @param array $values the rows + * + * @return void + */ + public function insertBatch($fields, $values) + { + $this->batches[] = [$fields, $values]; + } + /** + * Returns a select box a form can render. + * + * @return string + */ + public function buildSelectBox(...$args) + { + $name = $args[1] ?? ''; + + return ''; + } + } +} + +namespace FOG\Router { + /** + * Enough of Route to record a deletemass rather than perform one. + */ + class Route + { + /** + * Every deletemass() call, as [class, where]. + * + * @var array + */ + public static $deleted = []; + /** + * Records the delete. + * + * @param string $class the class name + * @param array $where the conditions + * + * @return void + */ + public static function deletemass($class, $where) + { + self::$deleted[] = [$class, $where]; + } + } +} + +namespace FOG\Util { + /** + * Enough of SharedHostValues to feed a hint a fixed answer. + */ + class SharedHostValues + { + /** + * What forHostRows() returns, keyed by friendly key. + * + * @var array + */ + public static $rows = []; + /** + * Returns the fixture. + * + * @return array + */ + public static function forHostRows(...$args) + { + return self::$rows; + } + /** + * Renders the info as text a test can assert on. + * + * @param array $info the info + * + * @return string + */ + public static function hint($info) + { + if (empty($info['uniform'])) { + return '(varies)'; + } + + return '' === (string)$info['value'] + ? '(empty on all)' + : (string)$info['value']; + } + } +}