Add IsSystem prorperty to IRole interface to allow identifing the system roles easily.

MikeAlhayek commented 2 years ago

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 IsSystem flag to true when adding these 2 roles. Perhaps we should also add IsSystem flag 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 IsSystem property 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 = true parameter 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.

hishamco commented 2 years ago

I was thinking before to add the roles in a class as you mentioned, but with extra thing. Localize them will be extra plus

hishamco commented 2 years ago

I might work on roles list with minimal solution to support localize them. Then you can do the refactoring

MikeAlhayek commented 2 years ago

@hishamco localizing the roles may be a good idea too. I like to know which approach (of the 2 above or perhaps other) would be acceptable by the OC team on cleaning the code?

hishamco commented 2 years ago

I don't think there's an issue with the extension method, but let me dig into your changes, and how many places that we have redundant code that we can simplify and refactor

MikeAlhayek commented 2 years ago

@sebastienros @hishamco @Skrypt here is maybe even a better way to do this which will give us much more flexibility and would allow uses to not use unwanted default roles using a startup recipe. Currently, in the roles recipe, we can add role, but you can't exclude recipe roles from being created or even remove permission from the default roles "at the initial setup". For example, you can't say create a tenant without Author, Contributor, Moderator...

The idea is to add a type for each role, then get roles by type. For the sake of the original enhancement, the "Anonymous" and "Authenticated" roles will have system type. Now we can easily exclude the roles with the system type instead of having to explicitly list them everywhere.

To do that, we'll add Type property in IRole like this public string Type { get; set; }

public interface IRole
{
     string RoleName { get; }
     string RoleDescription { get; }
     string Type { get; }
}

In the IRoleService we would add a new GetRolesAsync(string type) method like this

public interface IRoleService
{
    Task<IEnumerable<IRole>> GetRolesAsync();
    Task<IEnumerable<Claim>> GetRoleClaimsAsync(string role, CancellationToken cancellationToken = default);
    Task<IEnumerable<string>> GetNormalizedRoleNamesAsync();
    Task<IEnumerable<IRole>> GetRolesAsync(string type);
}

This approach will go a long way because now we don't have to force the user to even use the default roles. You would define which roles you want to use by default in the recipe. Using such approach allows us to change IPermissionProvider from something like this

public class UserPermissionProvider : IPermissionProvider
{
    public IEnumerable<PermissionStereotype> GetDefaultStereotypes() =>
        new[]
        {
                new PermissionStereotype
                {
                    Name = "Administrator",
                    Permissions = new[]
                    {
                        UserPermissions.ImportUsingExcel,
                    },
                },
        };

    public Task<IEnumerable<Permission>> GetPermissionsAsync() =>
        Task.FromResult(new[]
        {
                UserPermissions.ImportUsingExcel,
        }
        .AsEnumerable());
}

to something like this

public class UserPermissionProvider : IPermissionProvider
{
    public IRoleService _roleService;

    public UserPermissionProvider(IRoleService roleService)
    {
        _roleService = roleService
    }

    public IEnumerable<PermissionStereotype> GetDefaultStereotypes()
    {
        return _roleService.GetRolesAsync("administrator") // even better this would be _roleService.GetAdministratorRoles()
                          .GetAwaiter()
                         .GetResult()
                     .Select(role => new PermissionStereotype
                     {
                          Name = x.RoleName,
                          Permissions = new[]
                          {
                               UserPermissions.ImportUsingExcel,
                           },
                       }).ToArray();
    }

    public Task<IEnumerable<Permission>> GetPermissionsAsync() =>
        Task.FromResult(new[]
        {
                UserPermissions.ImportUsingExcel,
        }
        .AsEnumerable());
}

It would be even nicer if we convert IEnumerable<PermissionStereotype> GetDefaultStereotypes() to Task<IEnumerable<PermissionStereotype>> GetDefaultStereotypesAsync() were we can using await instead or .GetAwaiter().GetResult()

Now the RoleServiceExtensions will look 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(role => !roleService.IsSystemRole(role));
        }

        return roles;
    }

    public static async Task<IEnumerable<IRole>> GetAdministratorRoles(this IRoleService roleService)
    {
        return await roleService.GetRolesAsync("administrator");
    }

    public static async Task<IEnumerable<IRole>> GetModeratorRoles(this IRoleService roleService)
    {
        return await roleService.GetRolesAsync("moderator");
    }

    public static async Task<IEnumerable<IRole>> GetEditorRoles(this IRoleService roleService)
    {
        return await roleService.GetRolesAsync("editor");
    }

    public static async Task<IEnumerable<IRole>> GetAuthorRoles(this IRoleService roleService)
    {
        return await roleService.GetRolesAsync("author");
    }

    public static async Task<IEnumerable<IRole>> GetContributorRoles(this IRoleService roleService)
    {
        return await roleService.GetRolesAsync("contributor");
    }

    public static async Task<IEnumerable<IRole>> GetSystemRoles(this IRoleService roleService)
    {
        return await roleService.GetRolesAsync("system");
    }

    public static async Task<bool> IsSystemRole(this IRoleService roleService, string roleName)
    {
        var role = (await roleService.GetRolesAsync()).FirstOrDefault(x => x.Equals(roleName, StringComparison.OrdinalIgnoreCase));

        return role != null && roleService.IsSystemRole(role);
    }

    public static async Task<bool> IsSystemRole(this IRoleService roleService, IRole role)
    {
        return "system".Equals(role.System, StringComparison.OrdinalIgnoreCase);
    }
}

I think this would be considered a breaking change since all setup recipes will need to also include the default roles. But it should be acceptable change since we can just add the roles to the existing recipes. and we can place instruction for users to upgrade their setup recipes to include default roles.

OrchardCMS / OrchardCore