U4-11163 - MemberService AssignRole does not consider 'case sensitivity' when creating 'new roles' during assignment

Created by Marc Goodson 28 Mar 2018, 23:00:14 Updated by Sebastiaan Janssen 25 Jul 2018, 12:35:24

Tags: PR

If you have an existing MemberGroup called

SPECIAL - Admin

and you use

_memberService.AssignRole(123,"Special - Admin")

instead of assigning the user with id 123 to the existing group SPECIAL - Admin, a new Member Group will be created 'Special - Admin' and the user will be assigned to that.

There will be two Member groups in Umbraco "Special - Admin" and "SPECIAL - Admin"

when using Public access to protect a node by these groups, an error will be thrown if you try to protect the node by both of these variations of the Member Group node - "duplicate Member Groups cannot be assigned"

It would make sense that when creating new Member Groups as part of the AssignRole method, that new groups aren't created, if one already exists and the only difference is the 'case' of the text.

The issue is in the MemberGroupRepository, in the AssignRolesInternal method where '.Except' is used to identify new roles in comparing a string array of exiting roles, with the new roles to be assigned. var existingSql = new Sql() .Select("*") .From() .Where(dto => dto.NodeObjectType == NodeObjectTypeId) .Where("umbracoNode." + SqlSyntax.GetQuotedColumnName("text") + " in (@names)", new ); var existingRoles = Database.Fetch(existingSql).Select(x => x.Text); var missingRoles = roleNames.Except(existingRoles);

suggest tweaking this to be: var existingSql = new Sql() .Select("*") .From() .Where(dto => dto.NodeObjectType == NodeObjectTypeId) .Where("umbracoNode." + SqlSyntax.GetQuotedColumnName("text") + " in (@names)", new ); var existingRoles = Database.Fetch(existingSql).Select(x => x.Text); var missingRoles = roleNames.Except(existingRoles, StringComparer.CurrentCultureIgnoreCase);

To prevent the duplicates from being created.

And then similarly further down the method, a 'Contains' is used to map members to the existing roles and an Except to find the non assigned ones... so again I think: var assignedRoles = found.Where(x => roleNames.Contains(x.RoleName)).Select(x => x.RoleName); var nonAssignedRoles = roleNames.Except(assignedRoles); would become: var assignedRoles = found.Where(x => roleNames.Contains(x.RoleName,StringComparer.CurrentCultureIgnoreCase)).Select(x => x.RoleName); var nonAssignedRoles = roleNames.Except(assignedRoles, StringComparer.CurrentCultureIgnoreCase);

Comments

Marc Goodson 29 Mar 2018, 08:23:24

PR is here; https://github.com/umbraco/Umbraco-CMS/pull/2552


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.4.3, 7.6.13, 7.9.3

Due in version: 7.12.0

Sprint:

Story Points:

Cycle: