4

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?

1
  • I was going to suggest having all Roles auto-register themselves on startup, but apparently that's not feasible in C#. Feel free to take a look at my question trying to figure out how to do it, and you can see my sample code I was going to include in the answer and the answer I got for how to actually make it work.
    – Bobson
    Commented Jul 12, 2013 at 20:15

3 Answers 3

3

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.

2
  • 1
    Please avoid Enum.Parse. What happens when a dev renames the enum values not knowing they're tied to DB values? I've seen the happen more than once (surprisingly). Using Enum.Parse is usually a sign the your system is more coupled than it should be.
    – MetaFight
    Commented Jul 12, 2013 at 9:35
  • 1
    @MetaFight: I'd probably use something like a [RoleNameAttribute("Admin")] annotation on each enum value to avoid that problem, but I do like this idea. Commented Jul 12, 2013 at 14:04
3

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.

1
  • 1
    The factory class part is important here. It takes the sole responsibility if converting strings to roles, which sounds like the problem the OP is trying to solve. Commented Jul 12, 2013 at 16:27
2

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;
    }
}
4
  • I'd suggest making it static, but otherwise a good idea.
    – Bobson
    Commented Jul 12, 2013 at 13:36
  • The snippet above was only really meant to prove the concept. Ideally, it would implement IRoleFactory and be called AspNetRoleFactory or something similar so that calling code isn't coupled to the technology.
    – MetaFight
    Commented Jul 12, 2013 at 14:53
  • scratch that... there's still the issue of the technology-specific parameters.
    – MetaFight
    Commented Jul 12, 2013 at 14:56
  • @MetaFight You still have the right idea Commented Jul 12, 2013 at 16:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.