Add IsSystem property to IRole interface to allow identifing the system roles easily. #11920
Description
Is your feature request related to a problem? Please describe.
In many instances in OrchardCore's code, we repetitively use the following code
var allRoles = (await _roleService.GetRoleNamesAsync()).Except(new[] { "Anonymous", "Authenticated" }, StringComparer.OrdinalIgnoreCase);
These two roles are system roles; IMO, both should be grouped somehow.
Describe the solution you'd like
First it may be a good idea to add the following class to reduce the use of these terms all over the place.
public class CommonRoleConstants
{
public const string Administrator = "Administrator";
public const string Editor = "Editor";
public const string Moderator = "Moderator";
public const string Author = "Author";
public const string Contributor = "Contributor";
public const string Authenticated = "Authenticated";
public const string Anonymous = "Anonymous";
}
I can think of two approaches to clean up the code
Option 1: Flagging the System Role
I think this may be a better option even if it require few changes. But the idea is to add bool IsSystem { get; } property to the IRole interface Then,
- Either, set the
IsSystemflag to true when adding these 2 roles. Perhaps we should also addIsSystemflag to PermissionStereotype also since that is used to create the role internally via RoleUpdater
otherwise, we would have to add a check if the new role is system based or not
role = new Role
{
RoleName = stereotype.Name,
RoleDescription = stereotype.Name + " role",
IsSystem = new[] { CommonRoleConstants.Anonymous, CommonRoleConstants.Authenticated }.Contains(stereotype.Name, StringComparer.OrdinalIgnoreCase)
};
- Or, define the
bool IsSystemproperty in Role like this
public class Role : IRole
{
// ... existing properties
public bool IsSystem => new[] { CommonRoleConstants.Anonymous, CommonRoleConstants.Authenticated }.Contains(RoleName, StringComparer.OrdinalIgnoreCase);
}
- In addition to any of the above, we'll update the IRoleService to accept
bool includeSystemRoles = trueparameter to allow us to easily remove the system roles.
Option 2: Use extensions methods
A less invasive options may be create an extension for the IRoleService. I feel approach is a patch but may be more acceptable since the change would be minimal. The idea would be to update the existing RoleServiceExtensions by changing it to something like this
public static class RoleServiceExtensions
{
public static async Task<IEnumerable<string>> GetRoleNamesAsync(this IRoleService roleService, bool includeSystemRoles = true)
{
var roles = await roleService.GetRolesAsync(includeSystemRoles);
return roles.Select(r => r.RoleName);
}
public static async Task<IEnumerable<IRole>> GetRolesAsync(this IRoleService roleService, bool includeSystemRoles)
{
var roles = await roleService.GetRolesAsync();
if (!includeSystemRoles)
{
return roles.Where(x => !RoleHelper.IsSystemRole(x.RoleName));
}
return roles;
}
}
public static class RoleHelper
{
public static bool IsSystemRole(string roleName)
{
return new[] { CommonRoleConstants.Anonymous, CommonRoleConstants.Authenticated }.Contains(x.RoleName, StringComparer.OrdinalIgnoreCase));
}
}
The RoleHelper method would come in handy when refactoring code like the one here
There could be better ways to do this. Idea?
If this is something we are willing to adopt, suggest the better approach and I'll create a PR for this.