Unique Group.Title and Group.Code enforced in change in v4.10, can't save/update existing duplicates

[josephlewisnz]

Note - follow on API depredations work from #10525 (comment)

Affected Version

4.10.0 +

Description

After reading this pull request #10113
Looks as though Titles and Codes are now being validated to make sure they are unique.
We have an existing project that does not have unique group Titles and Codes.
Assuming this was just allowed prior to the 4.10 release

The issue is that admins are no longer able to update the group information even if changing the Title to a unique name (which the don't want to do right now) as the code only updates on initial creation

Any workaround or ideas would be appreciated

Steps to Reproduce

• Have a website with groups that have duplicate groups prior to 4.10
• Upgrade Silverstripe 4.10
• Try to update the group and save

eg.

[kinglozzer]

I think stopping two groups from being created with the same title was a mistake, especially so given that groups can have parents.

If you comment out this bit of validation, are you able to save the group information then?

silverstripe-framework/src/Security/Group.php

Lines 511 to 524 in 8a314a9

	$currentGroups = Group::get()
	->filter('ID:not', $this->ID)
	->map('Code', 'Title')
	->toArray();
	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']
	)
	);
	}

I think the group code should then automatically update to ensure it’s unique.

If that works, I’d propose we just remove that validation.

[michalkleiner]

Or we add ParentID to the filter so that the same groups can exist on different levels.

[josephlewisnz]

On version ~4.10.0 I would need to comment out both the validation pieces in order to save.
This would leave duplicate code's still and just ignore the validation.

        $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']
                )
            );
        }

I've just upgraded to version 4.11 as I see there have actually been some changes to this area.
And yes if I just comment out the title validation check then the group successfully saves and de-dups the code.

silverstripe-framework/src/Security/Group.php

Lines 511 to 524 in 8a314a9

	$currentGroups = Group::get()
	->filter('ID:not', $this->ID)
	->map('Code', 'Title')
	->toArray();
	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']
	)
	);
	}

It makes sense to me to not need a unique "Title". Only having one unique value to worry (Code) about/reference by.

[emteknetnz]

@kinglozzer to confirm are you proposing we keep the Group.Code unique validation though remove the Group.Title validation? If so that seems fine to me, I think a PR targeting 4.11 for a patch release is fine since we're reverting a feature that created a regression. If you're happy to spin up a PR I can get it merged and released for you @michalkleiner do you have an objections to this?

[michalkleiner]

I think the main concern was the group code duplication. The title can possibly be the same on different levels and still make sense, so I would be ok with removing the title validation. Also apologies for the delay in responding.

[josephlewisnz]

Thanks for this.

Looking into workarounds in the meantime as this has a direct impact on one of our sites.

It didn't look like I could change easily identify and change the $result = parent::validate();
I've currently chosen to just bypass and replicate the group logic in a separate injector Class, excluding the Group title and code duplication checks

If anyone could think of a better way, then that would be great to see.

<?php

use SilverStripe\ORM\ValidationResult;
use SilverStripe\Security\Group as CoreGroup;
use SilverStripe\Security\Permission;

class CustomGroup extends CoreGroup
{

    public function validate()
    {
        // $result = parent::validate();
        $result = ValidationResult::create();

        // Check if the new group hierarchy would add certain "privileged permissions",
        // and require an admin to perform this change in case it does.
        // This prevents "sub-admin" users with group editing permissions to increase their privileges.
        if ($this->Parent()->exists() && !Permission::check('ADMIN')) {
            $inheritedCodes = Permission::get()
                ->filter('GroupID', $this->Parent()->collateAncestorIDs())
                ->column('Code');
            $privilegedCodes = Permission::config()->get('privileged_permissions');
            if (array_intersect($inheritedCodes, $privilegedCodes)) {
                $result->addError(
                    _t(
                        'SilverStripe\\Security\\Group.HierarchyPermsError',
                        'Can\'t assign parent group "{group}" with privileged permissions (requires ADMIN access)',
                        ['group' => $this->Parent()->Title]
                    )
                );
            }
        }

        return $result;
    }
}

[emteknetnz]

Note: there was a @deprecation on a private method which I'll shortly remove as part of a wider CMS 5 initiative to remove deprecated cost (private methods cannot be deprecated since they are not API)

silverstripe-framework/src/Security/Group.php

Line 724 in 0b80643

* @deprecated 5.0 Remove deduping in favour of throwing a validation error for duplicates.

The deprecation text there said "Remove deduping in favour of throwing a validation error for duplicates." - I'll put this issue into our internal CMS backlog queue. I can't give a time estimate though we'll review what to do with this issue