tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

FIX Resolve deduping problem with group codes.

Browse Source 
Also remove dead validation code.
This commit is contained in:
Guy Sartorelli 2022-05-25 11:42:12 +12:00
parent 67d5c15c5e
commit e0c4f01c11
3 changed files with 76 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
src/Security/Group.php
View File

@ -482,16 +482,7 @@ class Group extends DataObject
*/
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 @@ class Group extends DataObject
->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 @@ class Group extends DataObject
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 @@ class Group extends DataObject
// 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);
}
}

33
tests/php/Security/GroupCsvBulkLoaderTest.php
View File

@ -24,6 +24,7 @@ class GroupCsvBulkLoaderTest extends SapphireTest
public function testOverwriteExistingImport()
{
// This group will be overwritten.
$existinggroup = new Group();
$existinggroup->Title = 'Old Group Title';
$existinggroup->Code = 'newgroup1';
@ -33,13 +34,15 @@ class GroupCsvBulkLoaderTest extends SapphireTest
$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 @@ class GroupCsvBulkLoaderTest extends SapphireTest
$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);
}
}

34
tests/php/Security/GroupTest.php
View File

@ -334,8 +334,40 @@ class GroupTest extends FunctionalTest
$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);
}
}