From e0c4f01c11a30b6f0ea4691c41070ba9fd4047db Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Wed, 25 May 2022 11:42:12 +1200 Subject: [PATCH] FIX Resolve deduping problem with group codes. Also remove dead validation code. --- src/Security/Group.php | 45 ++++++++++--------- tests/php/Security/GroupCsvBulkLoaderTest.php | 33 +++++++------- tests/php/Security/GroupTest.php | 34 +++++++++++++- 3 files changed, 76 insertions(+), 36 deletions(-) diff --git a/src/Security/Group.php b/src/Security/Group.php index 958467b4d88..144d628b703 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -482,16 +482,7 @@ public function getTreeTitle() */ public function setCode($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); + $this->setField('Code', Convert::raw2url($val)); } public function validate() @@ -522,16 +513,6 @@ public function validate() ->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( @@ -566,6 +547,9 @@ public function onBeforeWrite() if (!$this->Code && $this->Title != _t(__CLASS__ . '.NEWGROUP', "New Group")) { $this->setCode($this->Title); } + + // Make sure the code for this group is unique. + $this->dedupeCode(); } public function onBeforeDelete() @@ -726,4 +710,25 @@ public function requireDefaultRecords() // Members are populated through Member->requireDefaultRecords() } + + /** + * Code needs to be unique as it is used to identify a specific group. Ensure no duplicate + * codes are created. + * + * @deprecated 5.0 Remove deduping in favour of throwing a validation error for duplicates. + */ + private function dedupeCode(): void + { + $currentGroups = Group::get() + ->exclude('ID', $this->ID) + ->map('Code', 'Title') + ->toArray(); + $code = $this->Code; + $count = 2; + while (isset($currentGroups[$code])) { + $code = $this->Code . '-' . $count; + $count++; + } + $this->setField('Code', $code); + } } diff --git a/tests/php/Security/GroupCsvBulkLoaderTest.php b/tests/php/Security/GroupCsvBulkLoaderTest.php index 531bc33e969..0b0f75f9c9d 100644 --- a/tests/php/Security/GroupCsvBulkLoaderTest.php +++ b/tests/php/Security/GroupCsvBulkLoaderTest.php @@ -24,6 +24,7 @@ public function testNewImport() public function testOverwriteExistingImport() { + // This group will be overwritten. $existinggroup = new Group(); $existinggroup->Title = 'Old Group Title'; $existinggroup->Code = 'newgroup1'; @@ -33,13 +34,15 @@ public function testOverwriteExistingImport() $results = $loader->load(__DIR__ . '/GroupCsvBulkLoaderTest/GroupCsvBulkLoaderTest.csv'); $created = $results->Created()->toArray(); - $this->assertEquals(count($created), 1); - $this->assertEquals($created[0]->Code, 'newchildgroup1'); + $this->assertEquals(1, count($created)); + $this->assertEquals('newchildgroup1', $created[0]->Code); + $this->assertEquals('New Child Group 1', $created[0]->Title); + // This overrides the group because the code matches, which takes precedence over the ID. $updated = $results->Updated()->toArray(); - $this->assertEquals(count($updated), 1); - $this->assertEquals($updated[0]->Code, 'newgroup1-2'); - $this->assertEquals($updated[0]->Title, 'New Group 1'); + $this->assertEquals(1, count($updated)); + $this->assertEquals('newgroup1', $updated[0]->Code); + $this->assertEquals('New Group 1', $updated[0]->Title); } public function testImportPermissions() @@ -48,17 +51,17 @@ public function testImportPermissions() $results = $loader->load(__DIR__ . '/GroupCsvBulkLoaderTest/GroupCsvBulkLoaderTest_withExisting.csv'); $created = $results->Created()->toArray(); - $this->assertEquals(count($created), 1); - $this->assertEquals($created[0]->Code, 'newgroup1'); - $this->assertEquals($created[0]->Permissions()->column('Code'), ['CODE1']); + $this->assertEquals(1, count($created)); + $this->assertEquals('newgroup1', $created[0]->Code); + $this->assertEquals(['CODE1'], $created[0]->Permissions()->column('Code')); $updated = $results->Updated()->toArray(); - $this->assertEquals(count($updated), 1); - $this->assertEquals($updated[0]->Code, 'existinggroup'); - $array1=$updated[0]->Permissions()->column('Code'); - $array2=['CODE1', 'CODE2']; - sort($array1); - sort($array2); - $this->assertEquals($array1, $array2); + $this->assertEquals(1, count($updated)); + $this->assertEquals('existinggroup', $updated[0]->Code); + $actual = $updated[0]->Permissions()->column('Code'); + $expected = ['CODE1', 'CODE2']; + sort($actual); + sort($expected); + $this->assertEquals($expected, $actual); } } diff --git a/tests/php/Security/GroupTest.php b/tests/php/Security/GroupTest.php index b36b9a9b8e9..3425caeb04f 100644 --- a/tests/php/Security/GroupTest.php +++ b/tests/php/Security/GroupTest.php @@ -334,8 +334,40 @@ public function testGroupTitleDuplication() $this->assertEquals('duplicate-2', $group->Code); $group = new Group(); - $group->Title = 'Duplicate'; + $group->Title = 'Any Title'; + $group->Code = 'duplicate'; $group->write(); $this->assertEquals('duplicate-3', $group->Code); + + $group1 = new Group(); + $group1->Title = 'Any Title1'; + $group1->Code = 'some-code'; + $group2 = new Group(); + $group2->Title = 'Any Title2'; + $group2->Code = 'some-code'; + $group1->write(); + $group2->write(); + $this->assertEquals('some-code', $group1->Code); + $this->assertEquals('some-code-2', $group2->Code); + } + + public function testSettingCodeRepeatedly() + { + // Setting the code to the code it already was doesn't modify it + $group = $this->objFromFixture(Group::class, 'group1'); + $previousCode = $group->Code; + $group->Code = $previousCode; + $group->write(); + $this->assertEquals($previousCode, $group->Code); + + // Setting the code to a new code does modify it + $group->Code = 'new-code'; + $group->write(); + $this->assertEquals('new-code', $group->Code); + + // The old code can be reused + $group->Code = $previousCode; + $group->write(); + $this->assertEquals($previousCode, $group->Code); } }