diff --git a/lang/en.yml b/lang/en.yml index 849dad055..7fb0e9ab6 100644 --- a/lang/en.yml +++ b/lang/en.yml @@ -217,6 +217,7 @@ en: GROUPNAME: 'Group name' GroupReminder: 'If you choose a parent group, this group will take all it''s roles' HierarchyPermsError: 'Can''t assign parent group "{group}" with privileged permissions (requires ADMIN access)' + ValidationIdentifierAlreadyExists: 'A Group ({group}) already exists with the same {identifier}' Locked: 'Locked?' MEMBERS: Members NEWGROUP: 'New Group' diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index 2a0d24622..c46a650a7 100644 --- a/src/Dev/SapphireTest.php +++ b/src/Dev/SapphireTest.php @@ -2434,9 +2434,15 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly $member = $this->cache_generatedMembers[$permCode]; } else { // Generate group with these permissions - $group = Group::create(); - $group->Title = "$permCode group"; - $group->write(); + $group = Group::get()->filterAny([ + 'Code' => "$permCode-group", + 'Title' => "$permCode group", + ])->first(); + if (!$group || !$group->exists()) { + $group = Group::create(); + $group->Title = "$permCode group"; + $group->write(); + } // Create each individual permission foreach ($permArray as $permArrayItem) { diff --git a/src/Security/Group.php b/src/Security/Group.php index 48636f452..958467b4d 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -4,6 +4,7 @@ namespace SilverStripe\Security; use SilverStripe\Admin\SecurityAdmin; use SilverStripe\Core\Convert; +use SilverStripe\Forms\CompositeValidator; use SilverStripe\Forms\DropdownField; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\Form; @@ -21,6 +22,7 @@ use SilverStripe\Forms\HiddenField; use SilverStripe\Forms\HTMLEditor\HTMLEditorConfig; use SilverStripe\Forms\ListboxField; use SilverStripe\Forms\LiteralField; +use SilverStripe\Forms\RequiredFields; use SilverStripe\Forms\Tab; use SilverStripe\Forms\TabSet; use SilverStripe\Forms\TextareaField; @@ -480,7 +482,16 @@ class Group extends DataObject */ public function setCode($val) { - $this->setField("Code", Convert::raw2url($val)); + $currentGroups = Group::get() + ->map('Code', 'Title') + ->toArray(); + $code = Convert::raw2url($val); + $count = 2; + while (isset($currentGroups[$code])) { + $code = Convert::raw2url($val . '-' . $count); + $count++; + } + $this->setField("Code", $code); } public function validate() @@ -506,9 +517,45 @@ class Group extends DataObject } } + $currentGroups = Group::get() + ->filter('ID:not', $this->ID) + ->map('Code', 'Title') + ->toArray(); + + if (isset($currentGroups[$this->Code])) { + $result->addError( + _t( + 'SilverStripe\\Security\\Group.ValidationIdentifierAlreadyExists', + 'A Group ({group}) already exists with the same {identifier}', + ['group' => $this->Code, 'identifier' => 'Code'] + ) + ); + } + + if (in_array($this->Title, $currentGroups)) { + $result->addError( + _t( + 'SilverStripe\\Security\\Group.ValidationIdentifierAlreadyExists', + 'A Group ({group}) already exists with the same {identifier}', + ['group' => $this->Title, 'identifier' => 'Title'] + ) + ); + } + return $result; } + public function getCMSCompositeValidator(): CompositeValidator + { + $validator = parent::getCMSCompositeValidator(); + + $validator->addValidator(RequiredFields::create([ + 'Title' + ])); + + return $validator; + } + public function onBeforeWrite() { parent::onBeforeWrite(); diff --git a/tests/php/Security/GroupCsvBulkLoaderTest.php b/tests/php/Security/GroupCsvBulkLoaderTest.php index 5bb41a93c..531bc33e9 100644 --- a/tests/php/Security/GroupCsvBulkLoaderTest.php +++ b/tests/php/Security/GroupCsvBulkLoaderTest.php @@ -38,7 +38,7 @@ class GroupCsvBulkLoaderTest extends SapphireTest $updated = $results->Updated()->toArray(); $this->assertEquals(count($updated), 1); - $this->assertEquals($updated[0]->Code, 'newgroup1'); + $this->assertEquals($updated[0]->Code, 'newgroup1-2'); $this->assertEquals($updated[0]->Title, 'New Group 1'); } diff --git a/tests/php/Security/GroupTest.php b/tests/php/Security/GroupTest.php index 79d446311..b36b9a9b8 100644 --- a/tests/php/Security/GroupTest.php +++ b/tests/php/Security/GroupTest.php @@ -5,6 +5,7 @@ namespace SilverStripe\Security\Tests; use InvalidArgumentException; use SilverStripe\Control\Controller; use SilverStripe\Dev\FunctionalTest; +use SilverStripe\Forms\RequiredFields; use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataObject; use SilverStripe\Security\Group; @@ -33,7 +34,7 @@ class GroupTest extends FunctionalTest $this->assertEquals('my-title', $g1->Code, 'Custom title gets converted to code if none exists already'); $g2 = new Group(); - $g2->Title = "My Title"; + $g2->Title = "My Title and Code"; $g2->Code = "my-code"; $g2->write(); $this->assertEquals('my-code', $g2->Code, 'Custom attributes are not overwritten by Title field'); @@ -101,6 +102,7 @@ class GroupTest extends FunctionalTest { $member = $this->objFromFixture(TestMember::class, 'admin'); $group = new Group(); + $group->Title = 'Title'; // Can save user to unsaved group $group->Members()->add($member); @@ -121,6 +123,7 @@ class GroupTest extends FunctionalTest /** @var Group $childGroup */ $childGroup = $this->objFromFixture(Group::class, 'childgroup'); $orphanGroup = new Group(); + $orphanGroup->Title = 'Title'; $orphanGroup->ParentID = 99999; $orphanGroup->write(); @@ -280,4 +283,59 @@ class GroupTest extends FunctionalTest 'Members with only APPLY_ROLES can\'t assign parent groups with inherited ADMIN permission' ); } + + public function testGroupTitleValidation() + { + $group1 = $this->objFromFixture(Group::class, 'group1'); + + $newGroup = new Group(); + + $validators = $newGroup->getCMSCompositeValidator()->getValidators(); + $this->assertCount(1, $validators); + $this->assertInstanceOf(RequiredFields::class, $validators[0]); + $this->assertTrue(in_array('Title', $validators[0]->getRequired())); + + $newGroup->Title = $group1->Title; + $result = $newGroup->validate(); + $this->assertFalse( + $result->isValid(), + 'Group names cannot be duplicated' + ); + + $newGroup->Title = 'Title'; + $result = $newGroup->validate(); + $this->assertTrue($result->isValid()); + } + + public function testGroupTitleDuplication() + { + $group = $this->objFromFixture(Group::class, 'group1'); + $group->Title = 'Group title modified'; + $group->write(); + $this->assertEquals('group-1', $group->Code); + + $group = new Group(); + $group->Title = 'Group 1'; + $group->write(); + $this->assertEquals('group-1-2', $group->Code); + + $group = new Group(); + $group->Title = 'Duplicate'; + $group->write(); + $group->Title = 'Duplicate renamed'; + $group->write(); + $this->assertEquals('duplicate', $group->Code); + + $group = new Group(); + $group->Title = 'Duplicate'; + $group->write(); + $group->Title = 'More renaming'; + $group->write(); + $this->assertEquals('duplicate-2', $group->Code); + + $group = new Group(); + $group->Title = 'Duplicate'; + $group->write(); + $this->assertEquals('duplicate-3', $group->Code); + } } diff --git a/tests/php/Security/GroupTest.yml b/tests/php/Security/GroupTest.yml index b5dd61595..19975ff31 100644 --- a/tests/php/Security/GroupTest.yml +++ b/tests/php/Security/GroupTest.yml @@ -1,12 +1,16 @@ 'SilverStripe\Security\Group': admingroup: + Title: Admin Group Code: admingroup parentgroup: + Title: Parent Group Code: parentgroup childgroup: + Title: Child Group Code: childgroup Parent: '=>SilverStripe\Security\Group.parentgroup' grandchildgroup: + Title: Grandchild Group Code: grandchildgroup Parent: '=>SilverStripe\Security\Group.childgroup' group1: