IFilterProvider and separation of concerns

Question

I have a situation where I need to inject some dependencies in a action filter, namely, my custom authorization provider in my custom authorization attribute. I stumbled upon a lot of people and posts who were saying that we should be separating the 'attribute metadata' from the 'behavior'. This makes sense and there is also the fact that filter attributes are not instantiated through the 'DependencyResolver' so it is difficult to inject the dependencies.

So I did a little refactoring of my code and I wanted to know if I had it right (I'm using Castle Windsor as the DI framework).

First off I stripped my attribute to contain only the raw data I need

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
public class MyAuthorizeAttribute : Attribute
{
    public string Code { get; set; }
}

I created a custom authorization filter that would contain the logic of determining if the current user has the proper authorization

public class MyAuthorizationFilter : IAuthorizationFilter
{
    private IAuthorizationProvider _authorizationProvider;
    private string _code;

    public MyAuthorizationFilter(IAuthorizationProvider authorizationProvider, string code)
    {
        Contract.Requires(authorizationProvider != null);
        Contract.Requires(!string.IsNullOrWhiteSpace(code));

        _authorizationProvider = authorizationProvider;
        _code = code;
    }

    public void OnAuthorization(AuthorizationContext filterContext)
    {
        if (filterContext == null)
        {
            throw new ArgumentNullException("filterContext");
        }

        if (filterContext.HttpContext.Request.IsAuthenticated)
        {
            BaseController controller = filterContext.Controller as BaseController;
            if (controller != null)
            {
                if (!IsAuthorized(controller.CurrentUser, controller.GetCurrentSecurityContext()))
                {
                    // forbidden
                    filterContext.RequestContext.HttpContext.Response.StatusCode = 403;
                    if (filterContext.RequestContext.HttpContext.Request.IsAjaxRequest())
                    {
                        filterContext.Result = new RedirectToRouteResult("default", new RouteValueDictionary(new
                        {
                            action = "http403",
                            controller = "error"
                        }), false);
                    }
                    else
                    {
                        filterContext.Result = controller.InvokeHttp404(filterContext.HttpContext);
                    }
                }
            }
            else
            {

            }
        }
        else
        {
            filterContext.Result = new RedirectResult(FormsAuthentication.LoginUrl);
        }
    }

    private bool IsAuthorized(MyUser user, BaseSecurityContext securityContext)
    {
        bool has = false;
        if (_authorizationProvider != null && !string.IsNullOrWhiteSpace(_code))
        {
            if (user != null)
            {
                if (securityContext != null)
                {
                    has = _authorizationProvider.HasPermission(user, _code, securityContext);
                }
            }
        }
        else
        {
            has = true;
        }
        return has;
    }
}

The last part was to create a custom filter provider that would fetch this specific attribute and instantiate my custom filter passing its dependencies and any data it needs, extracted from the attribute.

public class MyAuthorizationFilterProvider : IFilterProvider
{
    private IWindsorContainer _container;

    public MyAuthorizationFilterProvider(IWindsorContainer container)
    {
        Contract.Requires(container != null);
        _container = container;
    }

    public IEnumerable<Filter> GetFilters(ControllerContext controllerContext, ActionDescriptor actionDescriptor)
    {
        Type controllerType = controllerContext.Controller.GetType();
        var authorizationProvider = _container.Resolve<IAuthorizationProvider>();
        foreach (MyAuthorizeAttribute attribute in controllerType.GetCustomAttributes(typeof(MyAuthorizeAttribute), false))
        {
            yield return new Filter(new MyAuthorizationFilter(authorizationProvider, attribute.Code), FilterScope.Controller, 0);
        }
        foreach (MyAuthorizeAttribute attribute in actionDescriptor.GetCustomAttributes(typeof(MyAuthorizeAttribute), false))
        {
            yield return new Filter(new MyAuthorizationFilter(authorizationProvider, attribute.Code), FilterScope.Action, 0);
        }
    }
}

The last step is the register the filter provider in the global.asax

FilterProviders.Providers.Add(new MyAuthorizationFilterProvider(_container));

So I'm wondering first, if I got the idea right and second, what could be improved.

Answer 1

Yes, I think you got the idea right. I like that you're separating concerns between the attribute and the filter implementation, and I like that you're using constructor DI rather than property DI.

Your approach works well if you only have one type of filter. I think the biggest potential area for improvement, if you had more than one type of filter, would be how the filter provider is implemented. Currently, the filter provider is tightly coupled to the attribute and filter instances it is providing.

If you're willing to combine the attribute with the filter and use property DI, there's a simple way to have a more decoupled filter provider. Here are two examples of that approach: http://www.thecodinghumanist.com/blog/archives/2011/1/27/structuremap-action-filters-and-dependency-injection-in-asp-net-mvc-3 http://lozanotek.com/blog/archive/2010/10/12/dependency_injection_for_filters_in_mvc3.aspx

There are two challenges to solve with the current approach: 1. Injecting some, but not all, of the filter constructor parameters via DI. 2. Mapping from an attribute to a (dependency-injected) filter instance.

Currently, you're doing both manually, which is certainly fine when there's only one filter/attribute. If there were more, you'd probably want a more general approach for both parts.

For challenge #1, you could use something like a _container.Resolve overload that lets you pass in arguments. That solution is rather container-specific and probably a bit tricky.

Another solution, which I'll describe here, separates out a factory class that only takes dependencies in its constructor and produces a filter instance requiring both DI and non-DI arguments.

Here's what that factory might look like:

public interface IFilterInstanceFactory
{
    object Create(Attribute attribute);
}

You'd then implement a factory for each attribute/filter pair:

public class MyAuthorizationFilterFactory : IFilterInstanceFactory
{
    private readonly IAuthorizationProvider provider;

    public MyAuthorizationFilterFactory(IAuthorizationProvider provider)
    {
        this.provider = provider;
    }

    public object Create(Attribute attribute)
    {
        MyAuthorizeAttribute authorizeAttribute = attribute as MyAuthorizeAttribute;

        if (authorizeAttribute == null)
        {
            return null;
        }

        return new MyAuthorizationFilter(provider, authorizeAttribute.Code);
   }
}

You can solve challenge #2 by just registering each implementation of IFilterInstanceFactory with CastleWindsor.

The filter provider can now be decoupled from any knowledge of specific attributes and filters:

public class MyFilterProvider : IFilterProvider
{
    private IWindsorContainer _container;

    public MyFilterProvider(IWindsorContainer container)
    {
        Contract.Requires(container != null);
        _container = container;
    }

    public IEnumerable<Filter> GetFilters(ControllerContext controllerContext, ActionDescriptor actionDescriptor)
    {
        Type controllerType = controllerContext.Controller.GetType();
        var authorizationProvider = _container.Resolve<IAuthorizationProvider>();
        foreach (FilterAttribute attribute in controllerType.GetCustomAttributes(typeof(FilterAttribute), false))
        {
            object instance = Resolve(attribute);
            yield return new Filter(instance, FilterScope.Controller, 0);
        }
        foreach (FilterAttribute attribute in actionDescriptor.GetCustomAttributes(typeof(FilterAttribute), false))
        {
            object instance = Resolve(attribute);
            yield return new Filter(instance, FilterScope.Action, 0);
        }
    }

    private object Resolve(Attribute attribute)
    {
        IFilterInstanceFactory[] factories = _container.ResolveAll<IFilterInstanceFactory>();

        foreach (IFilterInstanceFactory factory in factories)
        {
            object dependencyInjectedInstance = factory.Create(attribute);

            if (dependencyInjectedInstance != null)
            {
                return dependencyInjectedInstance;
            }
        }

        return attribute;
    }
}

David

Answer 2

This is probably a bit much, but one way of avoiding the factory as suggested by David (and making this a little more generic) is to introduce yet another attribute.

[AssociatedFilter(typeof(MyAuthorizationFilter))]

Which you could add to the original attribute as follows.

[AssociatedFilter(typeof(MyAuthorizationFilter))]
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
public class MyAuthorizeAttribute : Attribute
{
    public string Code { get; set; }
}

The AssociatedFilter Attribute looks like this.

public class AssociatedFilterAttribute : Attribute
{
    public AssociatedFilterAttribute(Type filterType)
    {
        FilterType = filterType;
    }
    public Type FilterType { get; set; }
}

Then you can retrieve the correct filter by pulling out the FilterType from this attribute.

private object Resolve(Attribute attribute)
{
    var filterAttributes = attribute.GetType().GetCustomAttributes(typeof(AssociatedFilterAttribute), false);
    var first = (AssociatedFilterAttribute)filterAttributes.FirstOrDefault();
    return new Filter(_container.Resolve(first.FilterType), FilterScope.First, null); 
}

Currently this is limited to only taking the first AssociatedFilter attribute, theoretically I guess you could add more than one (one attribute kicks off several filters) in which case you'd omit the bit where this grabs the first result.

Obviously we also need to add error handling, e.g. if there is no AssociatedFilterAttribute...