From e93fb7906f6a71a1e27ccbf6310e630bfbc79c8b Mon Sep 17 00:00:00 2001 From: Benn Oshrin Date: Fri, 10 Oct 2025 17:00:13 -0400 Subject: [PATCH] Initial commit for Group Hierarchy (CO-1223) --- app/config/schema/schema.json | 14 +- app/resources/locales/en_US/error.po | 28 +- app/resources/locales/en_US/field.po | 6 + app/resources/locales/en_US/information.po | 6 + app/src/Command/UpgradeCommand.php | 60 +++- app/src/Controller/CousController.php | 34 -- app/src/Controller/ErrorController.php | 1 + app/src/Controller/StandardController.php | 2 +- .../Lib/Events/RuleBuilderEventListener.php | 2 +- app/src/Lib/Traits/AutoViewVarsTrait.php | 17 +- app/src/Lib/Util/SchemaManager.php | 21 ++ app/src/Model/Table/CousTable.php | 90 +----- app/src/Model/Table/GroupsTable.php | 295 +++++++++++++++--- app/templates/Groups/columns.inc | 4 + app/templates/Groups/fields.inc | 10 + 15 files changed, 410 insertions(+), 180 deletions(-) diff --git a/app/config/schema/schema.json b/app/config/schema/schema.json index 39b933ebe..fa2eee611 100644 --- a/app/config/schema/schema.json +++ b/app/config/schema/schema.json @@ -203,17 +203,14 @@ "id": {}, "co_id": {}, "name": {}, - "description": {}, - "parent_id": { "type": "integer", "foreignkey": { "table": "cous", "column": "id" } }, - "lft": { "type": "integer" }, - "rght": { "type": "integer" } + "description": {} }, "indexes": { "cous_i1": { "columns": [ "co_id" ] }, "cous_i2": { "columns": [ "name" ] }, - "cous_i3": { "columns": [ "co_id", "name" ] }, - "cous_i4": { "needed": false, "columns": [ "parent_id" ] } - } + "cous_i3": { "columns": [ "co_id", "name" ] } + }, + "tree": true }, "dashboards": { @@ -325,7 +322,8 @@ "groups_i4": { "columns": [ "cou_id", "group_type" ] }, "groups_i5": { "needed": false, "columns": [ "cou_id" ]}, "groups_i6": { "needed": false, "columns": [ "owners_group_id" ]} - } + }, + "tree": true }, "group_nestings": { diff --git a/app/resources/locales/en_US/error.po b/app/resources/locales/en_US/error.po index 56700ae3d..e1aca5131 100644 --- a/app/resources/locales/en_US/error.po +++ b/app/resources/locales/en_US/error.po @@ -163,8 +163,20 @@ msgstr "Target group is already nested into this Group" msgid "GroupNestings.same" msgstr "Group cannot be nested into itself" -msgid "Groups.name.prefix" -msgstr "Standard Groups may not have names starting with \"CO:\"" +msgid "Groups.child.standard" +msgstr "Only Standard Groups may have parents" + +msgid "Groups.children" +msgstr "Group has {0} child(ren) and cannot be deleted" + +msgid "Groups.name.co" +msgstr "Standard Groups may not be named \"CO\"" + +msgid "Groups.name.colon" +msgstr "Standard Groups may not have names with a colon (\":\")" + +msgid "Groups.name.inuse" +msgstr "A Group ({0}) with the same parent ({1}) is already using the requested name" msgid "Groups.nested" msgstr "Group is nested or has nestings, and cannot be suspended or deleted" @@ -172,6 +184,9 @@ msgstr "Group is nested or has nestings, and cannot be suspended or deleted" msgid "Groups.owners.update" msgstr "Update called on Owners Group" +msgid "Groups.parent.standard" +msgstr "Only Standard Groups may be parents" + msgid "IdentifierAssignments.exists" msgstr "The identifier \"{0}\" is already in use" @@ -403,6 +418,12 @@ msgstr "Ooops... Something went wrong." msgid "setup.co.comanage" msgstr "Failed to setup COmanage CO" +msgid "tree.parent.invalid" +msgstr "The selected {0} is not a valid parent for the current record" + +msgid "tree.parent.same" +msgstr "The current {0} cannot be its own parent" + msgid "Types.inuse" msgstr "Type {0} is in use and cannot be deleted" @@ -415,6 +436,9 @@ msgstr "Unknown value \"{0}\"" msgid "unknown.identifier" msgstr "Unknown Identifier \"{0}\"" +msgid "ug.task.checkGroupNames.invalid" +msgstr "Found {0} Group(s) with colons in their names or named 'CO', these Groups must be renamed before continuing" + msgid "ug.task.unknown" msgstr "Task {0} is not defined" diff --git a/app/resources/locales/en_US/field.po b/app/resources/locales/en_US/field.po index 4385417e4..3ac1739bd 100644 --- a/app/resources/locales/en_US/field.po +++ b/app/resources/locales/en_US/field.po @@ -65,6 +65,9 @@ msgstr "Parent Record ID" msgid "changelog.revision" msgstr "Revision" +msgid "children" +msgstr "Children" + msgid "code" msgstr "Code" @@ -612,6 +615,9 @@ msgstr "Open groups may be self-joined by any Person in the CO" msgid "Groups.owners.desc.affix" msgstr "{0} Owners" +msgid "Groups.owners.for" +msgstr "Owners for Group" + msgid "Groups.owners_group_id" msgstr "Owners Group" diff --git a/app/resources/locales/en_US/information.po b/app/resources/locales/en_US/information.po index 503bda478..ababf8c7a 100644 --- a/app/resources/locales/en_US/information.po +++ b/app/resources/locales/en_US/information.po @@ -186,6 +186,12 @@ msgstr "Current version: {0}" msgid "ug.version.target" msgstr "Target version: {0}" +msgid "ug.tasks.buildGroupTree" +msgstr "Recovering Group tree" + +msgid "ug.tasks.checkGroupNames" +msgstr "Verifying Group names" + msgid "ug.tasks.createDefaultGroups.co" msgstr "Creating new Default Groups for CO {0}" diff --git a/app/src/Command/UpgradeCommand.php b/app/src/Command/UpgradeCommand.php index 134fe8aa0..c3dfe8022 100644 --- a/app/src/Command/UpgradeCommand.php +++ b/app/src/Command/UpgradeCommand.php @@ -34,6 +34,7 @@ use Cake\Console\ConsoleIo; use Cake\Console\ConsoleOptionParser; use Cake\Datasource\ConnectionManager; +use \App\Lib\Enum\GroupTypeEnum; class UpgradeCommand extends BaseCommand { @@ -72,7 +73,14 @@ class UpgradeCommand extends BaseCommand ], "5.2.0" => [ 'block' => false, - 'post' => ['createDefaultGroups', 'installMostlyStaticPages'] + 'pre' => [ + 'checkGroupNames' + ], + 'post' => [ + 'buildGroupTree', + 'createDefaultGroups', + 'installMostlyStaticPages' + ] ] ]; @@ -80,6 +88,8 @@ class UpgradeCommand extends BaseCommand // to make them easier to use regardless of context (pre/post/manual). protected $taskParams = [ + 'buildGroupTree' => ['global' => true], + 'checkGroupNames' => ['global' => true], 'createDefaultGroups' => ['perCO' => true, 'perCOU' => true], 'installMostlyStaticPages' => ['perCO' => true] ]; @@ -326,6 +336,54 @@ protected function dispatch(string $task) { } } + /** + * Establish tree metadata for Groups. + * + * @since COmanage Registry v5.2.0 + */ + + protected function buildGroupTree() { + // Although we partition Groups by CO, tree metadata applies to the table + // as a whole, so we only need to run this once. + + $GroupsTable = $this->getTableLocator()->get('Groups'); + $GroupsTable->recover(); + } + + /** + * Check that no Standard Groups have colons in their names. + * + * @since COmanage Registry v5.2.0 + */ + + protected function checkGroupNames() { + // Because it's not clear how to resolve conflicting Group names, we simply throw + // an error and require the administrator to figure out what to do. + + $GroupsTable = $this->getTableLocator()->get('Groups'); + + // AR-Group-9 Standard Group names may not use colons (:) and Standard Groups may not be named CO. + $problemGroups = $GroupsTable->find() + ->where([ + 'OR' => [ + 'name LIKE' => '%:%', + 'name' => 'CO' + ], + 'group_type' => GroupTypeEnum::Standard + ]) + ->all(); + + if($problemGroups->count() > 0) { + $this->io->err(__d('error', 'ug.task.checkGroupNames.invalid', [$problemGroups->count()])); + + foreach($problemGroups as $pg) { + $this->io->err($pg->id . " = " . $pg->name); + } + + throw new \RuntimeException(__d('error', 'ug.task.checkGroupNames.invalid', [$problemGroups->count()])); + } + } + /** * Create Approver and MFA Exemption Groups. * diff --git a/app/src/Controller/CousController.php b/app/src/Controller/CousController.php index e1b6578f1..229c2738d 100644 --- a/app/src/Controller/CousController.php +++ b/app/src/Controller/CousController.php @@ -31,45 +31,11 @@ // XXX not doing anything with Log yet use Cake\Log\Log; -//use \App\Lib\Enum\PermissionEnum; class CousController extends StandardController { - - protected array $paginate = [ 'order' => [ 'Cous.name' => 'asc' ] ]; - - /** - * Callback run prior to the request render. - * - * @since COmanage Registry v5.0.0 - * @param EventInterface $event Cake Event - */ - - public function beforeRender(\Cake\Event\EventInterface $event) { - if(!$this->request->is('restful')) { - // Pull the set of potential Parent COUs - - switch($this->request->getParam('action')) { - case 'add': - $this->set('parents', $this->Cous->potentialParents($this->getCOID(), null, true)); - break; - case 'edit': - $p = $this->request->getParam('pass'); - $couId = (int)$p[0]; - $this->set('parents', $this->Cous->potentialParents($this->getCOID(), $couId, true)); - break; - case 'index': - $this->set('parents', $this->Cous->potentialParents($this->getCOID())); - break; - default: - break; - } - } - - return parent::beforeRender($event); - } } \ No newline at end of file diff --git a/app/src/Controller/ErrorController.php b/app/src/Controller/ErrorController.php index 34ee6b316..a5b81752e 100644 --- a/app/src/Controller/ErrorController.php +++ b/app/src/Controller/ErrorController.php @@ -35,6 +35,7 @@ public function initialize(): void // Tell cake to automatically negotiate JSON, which will have the // most visible side effect of causing stack traces to render as // JSON instead of HTML + // https://discourse.cakephp.org/t/rest-api-exceptions-in-html-instead-of-json/11668 $this->addViewClasses([\Cake\View\JsonView::class]); } diff --git a/app/src/Controller/StandardController.php b/app/src/Controller/StandardController.php index b80d637d4..c90cb86e9 100644 --- a/app/src/Controller/StandardController.php +++ b/app/src/Controller/StandardController.php @@ -691,7 +691,7 @@ protected function populateAutoViewVars(object $obj=null) { // AutoViewVarsTrait if(method_exists($table, 'getAutoViewVars') && $table->getAutoViewVars()) { - foreach ($table->calculateAutoViewVars($this->getCOID(), $obj) as $vvar => $value) { + foreach ($table->calculateAutoViewVars($this->getCOID(), $obj, $this->request->getParam('action')) as $vvar => $value) { $this->set($vvar, $value); } } diff --git a/app/src/Lib/Events/RuleBuilderEventListener.php b/app/src/Lib/Events/RuleBuilderEventListener.php index 23eed218d..7dd427fb0 100644 --- a/app/src/Lib/Events/RuleBuilderEventListener.php +++ b/app/src/Lib/Events/RuleBuilderEventListener.php @@ -188,7 +188,7 @@ public function ruleFreezePrimaryLink(EntityInterface $entity, array $options) { $haveCO = $table->calculateCoForRecord($entity, true); if($wantCO != $haveCO) { - $this->llog('error', "GMR-1 Attempt to move " . $table->getAlias() . " record " . $entity->id . " from CO " . $have . " to CO " . $want . " is not allowed"); + $this->llog('error', "GMR-1 Attempt to move " . $table->getAlias() . " record " . $entity->id . " from CO " . $haveCO . " to CO " . $wantCO . " is not allowed"); return __d('error', 'coid.frozen'); } diff --git a/app/src/Lib/Traits/AutoViewVarsTrait.php b/app/src/Lib/Traits/AutoViewVarsTrait.php index a128b1e6e..b9d94948d 100644 --- a/app/src/Lib/Traits/AutoViewVarsTrait.php +++ b/app/src/Lib/Traits/AutoViewVarsTrait.php @@ -65,11 +65,16 @@ public function setAutoViewVars($vars): void { * * @since COmanage Registry v5.1.0 * @param int|null $coId - * @param Object|null $obj Current object (eg: from edit), if set + * @param Object|null $obj Current object (eg: from edit), if set + * @param string $action Controller action * @return \Generator */ - public function calculateAutoViewVars(int|null $coId, Object $obj = null): \Generator { + public function calculateAutoViewVars( + int|null $coId, + Object $obj = null, + string $action = null + ): \Generator { /** var Cake\ORM\Table $table */ $table = $this; @@ -198,7 +203,13 @@ public function calculateAutoViewVars(int|null $coId, Object $obj = null): \Gene $generatedValue = $query->toArray(); break; case 'parent': - $generatedValue = $table->getParents($coId); + // Supported models must use TreeTrait + $generatedValue = $table->potentialParents( + coId: $coId, + // We only want the hierarchical prefixes ("-") for add and edit + id: $obj ? $obj->id : null, + hierarchy: in_array($action, ['add', 'edit']) + ); break; case 'plugin': if(!empty($avv['pluginType'])) { diff --git a/app/src/Lib/Util/SchemaManager.php b/app/src/Lib/Util/SchemaManager.php index 528ef2bd0..ceaddbf4e 100644 --- a/app/src/Lib/Util/SchemaManager.php +++ b/app/src/Lib/Util/SchemaManager.php @@ -305,6 +305,27 @@ protected function processSchema( $table->addIndex([$sColumn], $tablePrefix.$tName . "_im" . $i++); } + // If this table uses TreeBehavior, emit the appropriate columnsand indexes. + if(isset($tCfg->tree) && $tCfg->tree) { + // Cake's TreeBehavior uses three columns: parent_id (which fks back to the + // same table, similar to source_foo), lft, and rght. The recommendation is + // to index parent_id (which we would do anyway since DBAL wants to put indexes + // on all fks) and lft. + + $foreignTableName = $this->conn->qualifyTableName($tablePrefix.$tName); + + // Insert a foreign key to this model and index it + $table->addColumn("parent_id", "integer", ['notnull' => false]); + $table->addForeignKeyConstraint($foreignTableName, ["parent_id"], ['id'], [], $tablePrefix.$tName . "_parent_id_fkey"); + $table->addIndex(["parent_id"], $tablePrefix.$tName."_it1"); + + // Add the other columns + $table->addColumn("lft", "integer", ['notnull' => false]); + $table->addIndex(["lft"], $tablePrefix.$tName."_it2"); + + $table->addColumn("rght", "integer", ['notnull' => false]); + } + // Default is to insert timestamp and changelog fields, unless disabled if(!isset($tCfg->timestamps) || $tCfg->timestamps) { diff --git a/app/src/Model/Table/CousTable.php b/app/src/Model/Table/CousTable.php index 32b8ec114..a694bec0f 100644 --- a/app/src/Model/Table/CousTable.php +++ b/app/src/Model/Table/CousTable.php @@ -48,6 +48,7 @@ class CousTable extends Table { use \App\Lib\Traits\ProvisionableTrait; use \App\Lib\Traits\SearchFilterTrait; use \App\Lib\Traits\TableMetaTrait; + use \App\Lib\Traits\TreeTrait; use \App\Lib\Traits\ValidationTrait; /** @@ -95,9 +96,8 @@ public function initialize(array $config): void { $this->setRequiresCO(true); $this->setAutoViewVars([ - 'parentIds' => [ - 'type' => 'parent' // Even though the type is parent we refer to the parent_id - // which is an integer + 'parents' => [ + 'type' => 'parent' ] ]); @@ -153,30 +153,6 @@ public function buildRules(RulesChecker $rules): RulesChecker { return $rules; } - - /** - * Get the Parent COU list(Suitable for dropdown) - * - * @param int $coId CO ID - * - * @return array List of [id, name] Parent COUs - * @since COmanage Registry v5.0.0 - */ - public function getParents(int $coId): array - { - $subquery = $this->find(); - $subquery = $subquery->where(['co_id' => $coId]) - ->where(fn(QueryExpression $exp, Query $subquery) => $exp->isNotNull('parent_id')) - ->select(['parent_id']) - ->distinct(); - - $query = $this->find('list') - ->where(fn(QueryExpression $exp, Query $query) => $exp->in('id', $subquery)) - ->distinct() - ->select(['id', 'name']); - $results = $query->toArray(); - return $results; - } /** * Callback after model save. @@ -235,66 +211,6 @@ public function marshalProvisioningData(int $id): array { return $ret; } - - /** - * Assemble the set of potential parent COUs. - * - * @since COmanage Registry v5.0.0 - * @param int $coId CO ID - * @param int $id COU ID to determine potential parents of, or null for any (or a new) COU - * @param bool $hierarchy Render the hierarchy in the name - * @return Array Array of COU IDs and COU Names - * @todo Make a TreeTrait and move the function there - */ - - public function potentialParents(int $coId, int $id=null, bool $hierarchy=false) { - // Note prior to v5 we filtered child COUs, meaning a COU couldn't be reassigned - // to be a child of a current child. It's not clear why we imposed that restriction. - - $query = null; - - if($hierarchy) { - $query = $this->find('treeList', spacer: '-'); - } else { - $query = $this->find('list'); - } - - $query = $query->where(['co_id' => $coId]) - // true overrides the default Cake order for treeList so we get - // our items sorted alphabetically instead of by tree ID - ->orderBy(['name' => 'ASC'], true); - - if($id) { - $query = $query->where(['id <>' => $id]); - } - - return $query->toArray(); - } - - /** - * Application Rule to determine if the parent ID is a potential parent. - * - * @since COmanage Registyr v5.0.0 - * @param Entity $entity Entity to be validated - * @param array $options Application rule options - * @return boolean true if the Rule check passes, false otherwise - */ - - public function rulePotentialParent($entity, $options) { - // We want negative logic since we want to fail if we're editing the COmanage CO - if(!empty($entity->parent_id)) { - $potentialParents = $this->potentialParents( - coId: $entity->co_id, - id: (!empty($entity->id) ? $entity->id : null) - ); - - if(!isset($potentialParents[$entity->parent_id])) { - return __d('error', 'cou.parent'); - } - } - - return true; - } /** * Perform initial setup for a COU. diff --git a/app/src/Model/Table/GroupsTable.php b/app/src/Model/Table/GroupsTable.php index 96fdfa6b5..9d61ce712 100644 --- a/app/src/Model/Table/GroupsTable.php +++ b/app/src/Model/Table/GroupsTable.php @@ -29,6 +29,7 @@ namespace App\Model\Table; +use Cake\Datasource\EntityInterface; use Cake\Event\EventInterface; use Cake\ORM\Query; use Cake\ORM\RulesChecker; @@ -55,6 +56,9 @@ class GroupsTable extends Table { use \App\Lib\Traits\SearchFilterTrait; use \App\Lib\Traits\TabTrait; use \App\Lib\Traits\TableMetaTrait; + use \App\Lib\Traits\TreeTrait { + potentialParents as traitPotentialParents; + } use \App\Lib\Traits\ValidationTrait; /** @@ -69,13 +73,19 @@ public function initialize(array $config): void { $this->addBehavior('Changelog'); $this->addBehavior('Log'); $this->addBehavior('Timestamp'); + $this->addBehavior('Tree'); $this->setTableType(\App\Lib\Enum\TableTypeEnum::Primary); // Define associations $this->belongsTo('Cos'); $this->belongsTo('Cous'); - + $this->belongsTo('Groups') + ->setForeignKey('parent_id') + // Property is set so ruleValidateCO can find it. We don't use the + // _id suffix to match Cake's default pattern. + ->setProperty('parent'); + // Most Groups (except other Owners groups) have an Owner Group, // which we should define with a hasOne relation (and a foreign key // like owners_for_group_id). However, this doesn't intuitively define @@ -95,6 +105,9 @@ public function initialize(array $config): void { ->setClassName('Groups') ->setForeignKey('owners_group_id'); + $this->hasMany('ChildGroups') + ->setClassName('Groups') + ->setForeignKey('parent_id'); $this->hasMany('EnrollmentFlowSteps') ->setForeignKey('notification_group_id'); $this->hasMany('GroupMembers') @@ -122,6 +135,7 @@ public function initialize(array $config): void { $this->setRequiresCO(true); $this->setEditContains([ + 'ChildGroups', 'Identifiers', // For an Owners Group, the group it manages owners for 'OwnersForGroup', @@ -131,12 +145,13 @@ public function initialize(array $config): void { ]); $this->setViewContains([ - 'Identifiers', - // For an Owners Group, the group it manages owners for - 'OwnersForGroup', - // For a regular group, the Owners Group - 'OwnersGroup', - 'GroupMembers' + 'ChildGroups', + 'Identifiers', + // For an Owners Group, the group it manages owners for + 'OwnersForGroup', + // For a regular group, the Owners Group + 'OwnersGroup', + 'GroupMembers' ]); // XXX Also used by SearchBlocks @@ -158,6 +173,9 @@ public function initialize(array $config): void { 'types' => [ 'type' => 'auxiliary', 'model' => 'Types' + ], + 'parents' => [ + 'type' => 'parent' ] ]); @@ -335,8 +353,10 @@ public function addDefaults(int $coId, int $couId=null, bool $rename=false): boo // Construct the full group name $gname = "CO" . ($couName ? ":COU:".$couName : "") . $suffix; - // See if there is already a group with this type for this CO - + // See if there is already a group with this type for this CO. Note this implies + // two COUs can't have the same name either (since we'll construct automatic Groups + // for each COU based on its name), but that's covered by AR-COU-3. + $grp = $this->find() ->where([ 'Groups.co_id' => $coId, @@ -368,7 +388,33 @@ public function addDefaults(int $coId, int $couId=null, bool $rename=false): boo return true; } - + + /** + * Callback after data is marshaled into an entity. + * + * @since COmanage Registry v5.0.0 + * @param EventInterface $event afterMarshal event + * @param EntityInterface $entity Entity + * @param ArrayObject $data Original request data + * @param ArrayObject $options Callback options + */ + + public function afterMarshal( + EventInterface $event, + EntityInterface $entity, + \ArrayObject $data, + \ArrayObject $options + ) { + // The inbound $data will in general not include the Group Type because it's not + // provided in the form data, and isn't supposed to change anyway. + + if(empty($data['group_type']) && empty($entity->group_type)) { + // If no group_type was set, this is a Standard Group, so fill in the field. + $data['group_type'] = GroupTypeEnum::Standard; + $entity->group_type = GroupTypeEnum::Standard; + } + } + /** * Callback before model delete. * @@ -392,22 +438,6 @@ public function beforeDelete(EventInterface $event, $entity, \ArrayObject $optio $event->setResult(true); } - /** - * Callback before data is marshaled into an entity. - * - * @since COmanage Registry v5.0.0 - * @param EventInterface $event beforeMarshal event - * @param ArrayObject $data Entity data - * @param ArrayObject $options Callback options - */ - - public function beforeMarshal(EventInterface $event, \ArrayObject $data, \ArrayObject $options) { - // If no group_type was set, this is a Standard Group, so fill in the field. - if(empty($data['group_type'])) { - $data['group_type'] = GroupTypeEnum::Standard; - } - } - /** * Define business rules. * @@ -417,8 +447,11 @@ public function beforeMarshal(EventInterface $event, \ArrayObject $data, \ArrayO */ public function buildRules(RulesChecker $rules): RulesChecker { - // AR-Group-1 Two Groups within the same CO cannot share the same name - $rules->add($rules->isUnique(['name', 'co_id'], __d('error', 'exists', [__d('controller', 'Groups', [1])]))); + // AR-Group-1 Two Groups within the same CO and with the same Parent (if set) + // cannot share the same name. + $rules->addUpdate([$this, 'ruleFQNameUnique'], + 'checkFQNameUnique', + ['errorField' => 'name']); // AR-Group-2 A Group cannot be set to Suspended if it is nested into a // Target Group or is a Target Group for a nesting. This and AR-Group-3 @@ -451,11 +484,28 @@ public function buildRules(RulesChecker $rules): RulesChecker { 'typeModified', ['errorField' => 'group_type']); - // AR-Group-9 Standard Groups may not be named starting with the prefix CO:, - // which is reserved for System Groups. - $rules->add([$this, 'ruleCheckNamePrefix'], - 'checkNamePrefix', - ['errorField' => 'name']); + // AR-Group-9 Standard Group names may not use colons (:). + // AR-Group-10 Standard Groups may not be named CO. + $rules->addUpdate([$this, 'ruleNameSyntax'], + 'checkNameSyntax', + ['errorField' => 'name']); + + // AR-Group-11 Only Standard Groups may have parents. + // AR-Group-12 Only Standard Groups may be parents. + $rules->addUpdate([$this, 'ruleAreStandard'], + 'checkParentsStandard', + ['errorField' => 'parent_id']); + + // AR-Group-13 A Group may not be deleted if it has any children. + $rules->addDelete([$this, 'ruleHasChildren'], + 'hasChildrenDelete', + ['errorField' => 'parent_id']); + + // This is not an Application Rule per se, but the parent_id must be a valid + // potential parent + $rules->add([$this, 'rulePotentialParent'], + 'potentialParent', + ['errorField' => 'parent_id']); return $rules; } @@ -547,6 +597,27 @@ public function getAdminGroupId(int $coId): int { return $g->id; } + /** + * Obtain the fully qualified name for the Group, which will include all + * parent names, separated by colons. + * + * @since COmanage Registry v5.2.0 + * @param Group $group Owners Group entity + * @return string Fully qualified Group name + */ + + public function getFullyQualifiedName($group): string { + // We solve this with recursion, yay! + + if(!empty($group->parent_id)) { + $parent = $this->get($group->parent_id); + + return $this->getFullyQualifiedName($parent) . ":" . $group->name; + } else { + return $group->name; + } + } + /** * Get the MFA Exemption Group for a CO. * @@ -713,7 +784,7 @@ public function implementedEvents(): array { * @return bool True on success */ - public function localAfterSave(\Cake\Event\EventInterface $event, \Cake\Datasource\EntityInterface $entity, \ArrayObject $options): bool { + public function localAfterSave(EventInterface $event, EntityInterface $entity, \ArrayObject $options): bool { if($entity->isNew()) { $action = ActionEnum::GroupAdded; $comment = __d('result', 'Groups.added', [$entity->name]); @@ -827,6 +898,41 @@ public function marshalProvisioningData(int $id): array { return $ret; } + /** + * Assemble the set of potential parent Groups. + * + * @since COmanage Registry v5.2.0 + * @param int $coId CO ID + * @param int $id Group ID to determine potential parents of, or null for any (or a new) Group + * @param bool $hierarchy Render the hierarchy in the name + * @param array $where Additional conditions for filtering potential parents + * @return array Array of Group IDs and Group Names + * @todo Make a TreeTrait and move the function there + */ + + public function potentialParents( + int $coId, + int $id=null, + bool $hierarchy=false, + array $where=[] + ): array { + // We generally want the same functionality as TreeTrait::potentialParents, but + // only Standard Groups may have parents, so if $id is provided check it first. + + if($id) { + $entity = $this->get($id); + + if($entity->group_type != GroupTypeEnum::Standard) { + return []; + } + } + + // Additionally, we filter out non-Standard Groups to reduce noise in the UI + // (even though normally we'd want to include ineligible entities to avoid UX issues). + + return $this->traitPotentialParents($coId, $id, $hierarchy, ['group_type' => GroupTypeEnum::Standard]); + } + /** * Reconcile the members of an automatic or nested Group. * @@ -851,7 +957,7 @@ public function reconcile(int $id) { * @param EntityInterface $entity Group */ - protected function reconcileAutomaticGroup(\Cake\Datasource\EntityInterface $entity) { + protected function reconcileAutomaticGroup(EntityInterface $entity) { // In order to handle very large groups, we can't pull the full set of // members into memory. Instead, we use the paginated iterator. This // involves two passes. @@ -951,7 +1057,7 @@ protected function reconcileAutomaticGroup(\Cake\Datasource\EntityInterface $ent * @param EntityInterface $entity Group */ - protected function reconcileNestedMemberships(\Cake\Datasource\EntityInterface $entity) { + protected function reconcileNestedMemberships(EntityInterface $entity) { // When a new GroupNesting is saved, we're called on the _target_. // Start by pulling the Group Nestings for this Group. We'll only go one level deep. @@ -993,7 +1099,77 @@ protected function reconcileNestedMemberships(\Cake\Datasource\EntityInterface $ } /** - * Application Rule to determine if the Group name has an invalid prefix. + * Application Rule to determine if both the Parent and Child are Standard Groups. + * + * @since COmanage Registry v5.2.0 + * @param Entity $entity Entity to be validated + * @param array $options Application rule options + * @return boolean true if the Rule check passes, false otherwise + */ + + public function ruleAreStandard($entity, $options) { + // We check that both child and parent are Standard Groups. + + if(!empty($entity->parent_id)) { + if($entity->group_type != GroupTypeEnum::Standard) { + return __d('error', 'Groups.child.standard'); + } + + $parentEntity = $this->get($entity->parent_id); + + if($parentEntity->group_type != GroupTypeEnum::Standard) { + return __d('error', 'Groups.parent.standard'); + } + } + + return true; + } + + /** + * Application Rule to determine if the Fully Qualified Group name is unique. + * + * @since COmanage Registry v5.2.0 + * @param Entity $entity Entity to be validated + * @param array $options Application rule options + * @return boolean true if the Rule check passes, false otherwise + */ + + public function ruleFQNameUnique($entity, $options) { + // Check that the proposed fully qualified name is not already in use. + // This basically just means checking other Groups with the same parent_id, + // or if there is no parent_id the same co_id. + + // name is indexed with co_id, so we'll always include co_id in the query + // even though it's redundant when parent_id is also specified. Annoying, nulls + // are specified differently. + + // Only Standard Groups are namespaced + if($entity->group_type == GroupTypeEnum::Standard) { + $whereClause = [ + 'co_id' => $entity->co_id, + 'name' => $entity->name + ]; + + if($entity->parent_id) { + $whereClause['parent_id'] = $entity->parent_id; + } else { + $whereClause['parent_id IS'] = null; + } + + $group = $this->find() + ->where($whereClause) + ->first(); + + if(!empty($group)) { + return __d('error', 'Groups.name.inuse', [$group->id, $group->parent_id]); + } + } + + return true; + } + + /** + * Application Rule to determine if the Group name has an invalid syntax. * * @since COmanage Registry v5.0.0 * @param Entity $entity Entity to be validated @@ -1001,10 +1177,41 @@ protected function reconcileNestedMemberships(\Cake\Datasource\EntityInterface $ * @return boolean true if the Rule check passes, false otherwise */ - public function ruleCheckNamePrefix($entity, $options) { - if($entity->group_type == GroupTypeEnum::Standard - && strncmp($entity->name, "CO:", 3)==0) { - return __d('error', 'Groups.name.prefix'); + public function ruleNameSyntax($entity, $options) { + // We don't allow (1) colons anywhere in the name, because this can create conflicts + // with fully qualified names (as used in Owners Group name construction) or (2) a + // name consisting of exactly "CO" (since that prefix is reserved for special Groups, + // and it would be possible to create a CO:foo fully qualified name.). + + if($entity->group_type == GroupTypeEnum::Standard) { + if(str_contains($entity->name, ":")) { + return __d('error', 'Groups.name.colon'); + } + + if($entity->name == "CO") { + return __d('error', 'Groups.name.co'); + } + } + + return true; + } + + /** + * Application Rule to determine if the group has children. + * + * @since COmanage Registry v5.2.0 + * @param Entity $entity Entity to be validated + * @param array $options Application rule options + * @return boolean true if the Rule check passes, false otherwise + */ + + public function ruleHasChildren($entity, $options) { + $count = $this->find('all') + ->where(['parent_id' => $entity->id]) + ->count(); + + if($count > 0) { + return __d('error', 'Groups.children', [$count]); } return true; @@ -1137,8 +1344,10 @@ public function updateOwnersGroup($group): int { $ownerGroup = $this->get($group->owners_group_id); - // We synchronize name, description, and status - $ownerGroup->name = 'CO:owners:' . $group->name; + // We synchronize name, description, and status. Because special Groups + // (including Owners Groups) exist in a flat structure, the name must be + // fully qualified. + $ownerGroup->name = 'CO:owners:' . $this->getFullyQualifiedName($group); $ownerGroup->description = $group->name . " Owners"; $ownerGroup->status = $group->status; diff --git a/app/templates/Groups/columns.inc b/app/templates/Groups/columns.inc index 75abf054e..aa12d515d 100644 --- a/app/templates/Groups/columns.inc +++ b/app/templates/Groups/columns.inc @@ -36,6 +36,10 @@ $indexColumns = [ 'class' => 'SuspendableStatusEnum', 'sortable' => true ], + 'parent_id' => [ + 'type' => 'fk', + 'label' => __d('field', 'parent_id') + ], 'description' => [ 'type' => 'echo' ] diff --git a/app/templates/Groups/fields.inc b/app/templates/Groups/fields.inc index c55299873..9281be407 100644 --- a/app/templates/Groups/fields.inc +++ b/app/templates/Groups/fields.inc @@ -45,6 +45,15 @@ foreach(['name', ]); } +if(!empty($parents)) { + print $this->element('form/listItem', [ + 'arguments' => [ + 'fieldName' => 'parent_id', + 'fieldLabel' => __d('field', 'parent_id'), + ] + ]); +} + if($vv_action != 'add') { print $this->element('form/listItem', [ 'arguments' => [ @@ -61,6 +70,7 @@ if($vv_action != 'add') { print $this->element('form/listItem', [ 'arguments' => [ 'fieldName' => 'owners_group_id', + 'fieldLabel' => __d('field', 'Groups.owners.for'), 'status' => $vv_obj->owners_for_group->name, 'link' => ['url' => ['controller' => 'groups', 'action' => 'edit', $vv_obj->owners_for_group->id]], 'labelIsTextOnly' => true