Filtering ASP.NET membership roles without the default attributes

I am building an application, and I needed an attribute similar to AuthorizeAttribute, supporting Roles but not using the MembershipProvider roles. (Basically, with the setup I have, I cannot rely on the User.IsInRole method - it's not always accurate, so I manually query the roles in my MasterDbContext instead.)

I have two different parts to it:

public class RequiredRoleAttribute : FilterAttribute, IAuthorizationFilter
{
    public string Roles { get; set; }
    public string Destination { get; set; } = "~/Account/Login";

    public void OnAuthorization(AuthorizationContext filterContext)
    {
        var context = new MasterDbContext();
        var user = context.Users.Find(filterContext.HttpContext.User.Identity.GetUserId());

        var requiredRoles = Roles.Split(',').Select(roleName => context.Roles.First(role => role.Name == roleName).Name).ToList();

        if (!requiredRoles.All(r => user.InRole(r, context)))
        {
            if (Destination == null)
            {
                filterContext.Result = new RedirectToRouteResult("Default", null);
            }
            else
            {
                filterContext.Result = new RedirectResult(Destination);
            }
        }
    }
}

This should be self-explanatory. It does the same thing as [AuthorizeAttribute(Roles = "SomeString")], used in conjunction with that attribute:

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderators)]
public abstract class BaseController : Controller
{
    public MasterDbContext Context { get; } = new MasterDbContext();
}

So that's easy. As you can see, I can still use [Authorize], I just can't use [Authorize(Roles = ...)].

Then, I wrote User.InRole instead of the User.IsInRole method:

```
public bool InRole(string roleName, MasterDbContext providedContext = null)
{
var context = providedContext ?? new MasterDbContext();
var roleId = context.Roles.First(r => r.Name == roleName).Id;
return Roles.ToList

public abstract class BaseController : Controller
{
    public MasterDbContext Context { get; } = new MasterDbContext();
}

I really don't like this. Every single controller class derived from BaseController has a public IDisposable dependency. That's taking encapsulation for a little "talk", outside, in a dark alley.

I can't think of a single reason Context would need to be public for. It should be protected.

But that's not the biggest issue. From the outside, say, from a derived class' point of view, inheriting from BaseController seems like a casual thing to do: the name suggests it, strongly.

Every derived controller inherits an IDisposable dependency that it didn't ask for - and since the base class creates it and doesn't take its responsibilities (he who creates an IDisposable instance, should be the one to call Dispose), if every derived type fails to override the controller's Dispose method, every single controller is leaking resources.

So you could implement IDisposable.Dispose in the BaseController.

Or, you could simply get rid of BaseController altogether. Disposable resources are important, they shouldn't be implicit. There's a very solidly (yet far from SOLID) tightly-coupled relationship between every controller and MasterDbContext - and that relationship is toxic.

There has to be a better way. This is more explicit:

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderator)]
public class AdminController : Controller
{
    private MasterDbContext Context { get; } = new MasterDbContext();

    // controller methods...

    protected override void Dispose(bool disposing)
    {
        if (disposing && Context != null)
        {
            Context.Dispose();
            Context = null;
        }

        base.Dispose(disposing);
    }
}

Now now, I hear you - "but what about DRY? I have to repeat that code in every controller?". No you don't.

What you can do, is make it someone else's job to create (and therefore to dispose) the context: you tell that "someone else" that you need a MasterDbContext, and let them deal with its lifetime.

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderator)]
public class AdminController : Controller
{
    private readonly MasterDbContext _context

    public AdminController(MasterDbContext context)
    {
        _context = context;
    }

    // controller methods...
}

Now anyone can look at that class and know that it requires a Constants.Roles.Moderator role, and that it has a MasterDbContext dependency. It's still coupled with a concrete type, but at least it's explicit about it. And we don't need to worry about disposing the context anymore - a controller that receives its DbContext through the constructor can happily assume that whoever put it there is going to dispose it when it destroys it.. and exactly when that happens is not its job either: the controller had nothing to do with creating the context.

This poses a problem though: the default controller factory requires a default constructor. And if you add one, then your context is always going to be null... which defeats the purpose of telling your caller that you have a dependency on MasterDbContext.

The solution is nothing less than implementing your own controller factory! Kidding. You can simply take an existing one - ninject, for example, will substitute the default controller factory for one that uses the Ninject IoC container to create an instance per request and inject it into the controllers' constructors.

All that's left to do is to tell Ninject that whenever a controller requests a MasterDbContext, you give it a MasterDbContext instance. The registration code might look like this:

Kernel.Bind().To().InRequestScope();

And from there, you're an inch away from being able to do this instead:

Kernel.Bind().To().InRequestScope();

Which means your controllers can look like this:

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderator)]
public class AdminController : Controller
{
    private readonly IUnitOfWork _context

    public AdminController(IUnitOfWork context)
    {
        _context = context;
    }

    // controller methods...
}

And here goes the "D" of SOLID:


Depend on abstractions, not on concrete types.

The IUnitOfWork interface can be a simple, minimal wrapper for DbContext:

public interface IUnitOfWork : IDisposable
{
    IDbSet Set();
    void Commit();
}

And now you can instantiate an AdminController in a unit test, mock the IUnitOfWork dependency, and write unit tests for every controller method!

StackExchange Code Review Q#153209, answer score: 4