Duplicate MemberGroup names cause MemberGroup mixup

[dhymik]

Which exact Umbraco version are you using? For example: 8.13.1 - don't just write v8

8.15.1

Bug summary

PublicAccessService does not store MemberGroup ids or Guids but MemberGroup names. If a MemberGroup name changes, PublicAccessServiceExtensions.RenameMemberGroupRoleRules is called to rename changed Membergroup names in all matching MemberGroup rules stored in PublicAccessService.

Now, when a MemberGroup is renamed to a name which is already in use by another MemberGroup, all PublicAccessService rules are changed to the duplicate name which is already in use.

If one of the two MemberGroups with same name are renamed to a unique new name, all PublicAccessService rules previously associated with the 2 MemberGroups subsequently are changed to the new unique name, now pointing to one MemberGroup.

See "steps to reproduce".

The same problem arises, as far as I can see, with UserName rules in PublicAccessService. PublicAccessServiceExtensions.HasAccess (PublicAccessServiceExtensions.cs line 45) checks for UserName rather than for UserId.

Specifics

The described behavior could be circumvented by checking for duplicate MemberGroup names in the backend when a group is added or edited. So basically it is easy to deal with it (once one is aware of the problem).

But the question is why PublicAccessService does not store MemberGroup ids and UserIds. If there is a reason for it, I would like to learn about it.

MemberGroup picker, for instance, stores the MemberGroup Ids in the IPublishedContent item. This is the behavior I would expect.

I came across this problem when I implemented a MemberGroup picker in page components for restricting display of components to MemberGroups. The MemberGroup picker gives me the ids of the selected MemberGroups. I then get the roles of the current user (which are the MemberGroup names the user belongs to). Then I need to fetch fetch all Umbraco MemberGroups

IMemberGroup[] umbracoMemberGroups = Services.MemberGroupService.GetAll()

to translate the group ids to group names or the other way around, before I compare to determine access.

Storing MemberGroup names and User names in PublicAccessService, rather than ids, seems inconsistent , and could be a potential cause for many problems. I guess since this would be a breaking change, it is not feasible to change this, but I simply wanted to pint out the issue.

Steps to reproduce

• Create a MemberGroup with name "test1", lets say this MemberGroup gets id 1001.
• Create a MemberGroup with name "test2", lets say this MemberGroup gets id 1002.
• Restrict public access of a test page to MemberGroup "test2".
• Rename MemberGroup "test2" (id 1002) to "test1".
• Rename original MemberGroup "test1"(id 1001) to "test1old".

Expected result / actual result

Expected:

• Public access of test page is restricted to MemberGroup with id 1001 ("test1old").

Actual:

• Public access of test page is restricted to MemberGroup with id 1002 ("test1").

[nul800sebastiaan]

Interesting.. I never considered this scenario, obviously the advise would be to not rename them in this way at this time and I know that is not very useful advise 😅

We'd love some help with this one!

[umbrabot]

Hi @dhymik,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly Umbraco GitHub bot :-)

[JamieTownsend84]

Not sure why public access would use the group name, feels wrong but potentially a big breaking change?

As another option I have created a branch which prevents Member Group name's being duplicated which would mitigate the above happening.

I've added a new entry into language files for English, not sure what needs to be done for other languages?

[nul800sebastiaan]

Not sure why public access would use the group name, feels wrong

Legacy 🙈

As another option I have created a branch which prevents Member Group name's being duplicated which would mitigate the above happening.

@JamieTownsend84 I guess only partially. I rename "test" to "old_test", create a new "test" and ... boom 😬 this could happen months later when you already forgot you used to have a "test" before..

[JamieTownsend84]

I believe from when I was looking at the code, when you update a member group name, it updates the public access, so no that shouldn’t matter because the public access would get updated to ‘old_test’ too.

Will test to confirm.

[JamieTownsend84]

@nul800sebastiaan I can confirm when you update the member group name, it updates the public access. So your scenario wouldn't happen in this case.

[umbrabot]

Hiya @dhymik,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant
This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂