How can I improve this design so that I don't need to dynamically scan classes at runtime?

Question

I'm in the middle of a redesign on the part of my current project that deals with user permissions and authorization. I have an interface named IUserPermissions that encapsulates this information.

One part of a user's permissions are the Roles which they are a member of. I'm trying not to violate the Open/Closed Principle by adding every possible role to IUserPermissions as a property like this:

interface IUserPermissions {
    bool IsAdmin { get; set; }
    bool IsDeveloper { get; set; }
    // Many more...Yuck.
}

I feel that having an interface like this would suit the OCP better:

interface IUserPermissions {
    ICollection<IRole> Roles { get; }
    bool IsInRole<T>() where T : IRole;  /* (For convenience; Would probably be
                                             implemented as an extension method) */
}
interface IRole {
    string Name { get; }
}
public class AdminRole : IRole {
    public string Name { get { return "Admin"; } }
}

Where I'm falling down is how to implement something like this. I'm using the ASP.NET Roles API as the backing store, meaning I'll be given a string[] of role names for the user. My IUserPermissions implementation would have to start with this collection of strings and somehow work backwards, then instantiate a bunch of appropriate IRoles to populate the Roles property.

This could be done by scanning the assembly for IRoles with a parameterless constructor, instantiating them, and reading their Name property, but...ugh. How can I fix this design so that I don't have to do this?

Answer 1

Have you considered creating an enum with all possible roles? Sure, if in the future you require a new role to be added, you'll have to modify the enum, which would break OCP, but I honestly do not believe it is worth it to go to such lengths just to avoid something that simple.

P.S.: Plus, you can use the Enum.Parse method to populate your collection from the array of strings.

Answer 2

I don't know .NET buy I will explain myself with Java. You can see it as pseudocode.

You don't have to instantiate concrete roles inside the IUserPermissions implementors, nor you need have a list of all roles or role names.

Look:

package permissions;

import java.util.Collection;

public interface IUserPermissions {
    public Collection<IRole> getRoles(); 
    public void addRole(IRole role);     // add a role to the collection
    public void removeRole(IRole role);  // remove a role from the collection
    public boolean isInRole(IRole role); 
    public boolean isInRole(String roleName);
}

Implementor of public boolean isInRole(IRole role); just checks if role is present in the collection using the constains method or similar.

The difference is here is this:

Implementor of public boolean isInRole(String roleName); traverses the collection comparing the roleName string with each role's getName() method.

If all you have is an array of role names, you use isInRole(String roleName), otherwise you use isInRole(IRole roleName)

For this to work you must ensure that no two roles have the same name. You can do that with Factory class.

It's ok for factory classes to be coupled to concrete classes. Their work is to be the only place where "new" is executed, so the rest of the system is decoupled from concrete classes.

Answer 3

Why not code the mapping (Role as String -> Role as concrete IRole type) in a Factory class?

public class RoleFactory
{
    public IEnumerable<IRole> CreateRoles(IEnumerable<String> roleStrings)
    {
        return roleStrings.Select(item => this.CreateRole(item)).ToList();
    }

    public IRole CreateRole(String roleString)
    {
        IRole result = null;

        switch(roleString)
        {
            case "Viewer":
                result = new ViewerRole();
                break;

            case "Admin":
                result = new AdminRole();
                break;

            case "Monkey":
                result = new MonkeyRole();
                break;

            default:
                throw new ArgumentException("Unexpected Role String");
        }

        // extra Defensive Programming
        if (result == null)
        {
            throw new InvalidOperationException("Role mapping is broken.");
        }

        return result;
    }
}