Admin cannot add a new role to himself

[mvarblow]

Describe the bug

Admin cannot add a new role to himself (or remove a role from himself) unless he is a site owner.

Orchard Core version

2.0

To Reproduce

Steps to reproduce the behavior:

1. Start with a fresh Orchard solution (empty app_data). Set up the site using the blog recipe and sqlite to keep it simple.
2. Go to '/admin' panel and sign in using the admin account (e.g. the original admin from site setup).
3. Edit the Administrator role, remove the "Site Owners Permission" permission, and click Save.
4. Create a new user named 'admin2', grant it the Administrator role, and copy the password.
5. Sign out, go back to the '/admin' panel, and sign in using the new admin2 username and password.
6. Go to the Users page, edit the admin2 user account, grant another role (e.g. Contributor), and click Save.
7. See the success notification.
8. Edit your admin user account and see that the notification was a lie. Your user still has the same roles as before.
9. Scratch your head and wonder what just happened.
10. Curse and repeat step 6.

Expected behavior

Your user account should be updated to include the roles you selected (or to remove the ones you deselected) when you edited it. If the current behavior is as documented, I couldn't find the documentation. And it is incredibly confusing for the admin.

Editing your own roles is allowed for a site admin. I don't understand (and our clients don't understand) why this should not be allowed for other admins who are granted the permission to edit user role assignments. A safeguard against getting locked out of the site? I could understand that. But then why is it allowed for site owners? And isn't that why OrchardCore includes this check?

It seems that at a minimum, there should be a notification that the change of roles was blocked. The success message in this case is very confusing and I think makes this a bug. However, I think this check should be removed. It's confusing. It doesn't seem justified given that we already have user permissions to control who can change user roles. And there is already a sanity check to ensure that the only site admin hasn't removed their admin rights. If we wanted to do something like this, it should be done with a distinct permission (e.g. "Edit my own roles") not with the Site Owner permission.

(Possibly related to #8977. Also, it looks like there used to be a notification that you couldn't change your own roles but it was removed because it was appearing in a front-end user profile edit endpoint? I suspect it was unintentionally removed for this use case where the user is edited from the admin panel?)

[MikeAlhayek]

I recall fixing something like that at one point. From the code you shared, we are only guarding against locking yourself out. The SiteOwner is only check to ensure only site owners can change roles.

I do agree this condition is wrong.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Users/Drivers/UserRoleDisplayDriver.cs

Lines 94 to 99 in 5caef1c

	// The current user cannot alter their own roles. This prevents them removing access to the site for themselves.
	if (_httpContextAccessor.HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier) == user.UserId
	&& !await _authorizationService.AuthorizeAsync(_httpContextAccessor.HttpContext.User, StandardPermissions.SiteOwner))
	{
	return null;
	}

because we later preventing an admin from removing their admin role of they are the only active user.

Instead of StandardPermissions.SiteOwner it should be CommonPermissions.AssignRoleToUsers

and the following comment should be removed

// The current user cannot alter their own roles. This prevents them removing access to the site for themselves.

Would you like to submit a PR for this? Be sure to test it thoroughly.

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[mvarblow]

@MikeAlhayek I ran several scenarios through the debugger. I think it is actually safe, and preferable, to just remove that check entirely. And also remove the check from the Edit method. Those restrictions are not necessary. The Edit and UpdateAsync method both already have logic in place to limit the user to assigning (or even viewing assignments) to roles that the user has permission to assign to.

        var roles = await GetRoleAsync();
        // Authorize each role in the model to prevent html injection.
        var accessibleRoleNames = await GetAccessibleRoleNamesAsync(roles);
        var currentUserRoleNames = await _userRoleStore.GetRolesAsync(user, default);

        var selectedRoleNames = model.Roles.Where(x => x.IsSelected).Select(x => x.Role);
        var selectedRoles = roles.Where(x => selectedRoleNames.Contains(x.RoleName, StringComparer.OrdinalIgnoreCase));
        var accessibleAndSelectedRoleNames = await GetAccessibleRoleNamesAsync(selectedRoles);

I'm preparing a pull request.

[MikeAlhayek]

Its better to check for AssignRoleToUsers early on so we are not doing the rest of the logic if you can't assign roles.

[mvarblow]

@MikeAlhayek, wouldn't that break the page for users who are allowed to assign some, but not all roles? I think the existing logic does this correctly. Anything we put in front of it is more likely to break it.

[MikeAlhayek]

@mvarblow okay. I did more looking at the code. I agree with you. I also notices a descrepency that should be addressed "could be in this PR or another PR".

There are two permission related to assigning role to users

OrchardCore/src/OrchardCore/OrchardCore.Roles.Core/CommonPermissions.cs

Line 9 in dd03cf8

public static readonly Permission AssignRoles = new("AssignRoles", "Assign Roles", [ManageRoles], isSecurityCritical: true);

and

OrchardCore/src/OrchardCore/OrchardCore.Users.Core/CommonPermissions.cs

Line 24 in dd03cf8

public static readonly Permission AssignRoleToUsers = new("AssignRoleToUsers", "Assign any role to users", [EditUsers], true);

One of these two should be removed

The one that SHOULD be kept in this one

OrchardCore/src/OrchardCore/OrchardCore.Users.Core/CommonPermissions.cs

Line 24 in dd03cf8

public static readonly Permission AssignRoleToUsers = new("AssignRoleToUsers", "Assign any role to users", [EditUsers], true);

We can add a migration step that would update all existing roles that contains "ManageRoles" or "AssignRoles" claim, and auto assign "AssignRoleToUsers" just to prevent a breaking change.

[MikeAlhayek]

@mvarblow actually, don't bother with removing the AssignRoles. I think it is more complicated and should be in a separate PR. But please proceed with your PR. ping me when it's done and I'll review it.