ActionFilter should not be called if HttpResponseMessage had been populated by previous ActionFilter

[akaSybe]

Let's imagine we have a controller:

namespace WebApplication.Controllers
{
    using System.Collections.Generic;
    using System.Web.Http;

    public class ValuesController : ApiController
    {
        public IEnumerable<string> Get()
        {
            return new string[] { "value1", "value2" };
        }
    }
}

Now we want to decorate it with two action filters

Filter1

public class Filter1 : IActionFilter
{
    public bool AllowMultiple { get; }

    public async Task<HttpResponseMessage> ExecuteActionFilterAsync(HttpActionContext actionContext, CancellationToken cancellationToken, Func<Task<HttpResponseMessage>> continuation)
    {
        Debug.WriteLine("Filter 1 invoked");
        return actionContext.Request.CreateErrorResponse(HttpStatusCode.Forbidden, "forbidden");
    }
}

Filter2

public class Filter2 : IActionFilter
{
    public bool AllowMultiple { get; }

    public async Task<HttpResponseMessage> ExecuteActionFilterAsync(HttpActionContext actionContext, CancellationToken cancellationToken, Func<Task<HttpResponseMessage>> continuation)
    {
        Debug.WriteLine("Filter 2 invoked");
        ... // do something
    }
}

Filters added to HttpConfiguration.Filters:

public static class WebApiConfig
{
    public static void Register(HttpConfiguration config)
    {
        // Web API configuration and services

        config.Filters.Add(new Filter1());
        config.Filters.Add(new Filter2());
    }
}

Now if we run an app and navigate to route, in Output Window will be shown only "Filter 1 invoked", Filter2 will not be fired.

################################################################################

If we make the same only using IAutofacActionFilter, both filters will be fired

Filter1

public class Filter1 : IAutofacActionFilter
{
    public async Task OnActionExecutedAsync(HttpActionExecutedContext actionExecutedContext, CancellationToken cancellationToken)
    {
    }

    public async Task OnActionExecutingAsync(HttpActionContext actionContext, CancellationToken cancellationToken)
    {
        Debug.WriteLine("Filter 1 invoked");
        actionContext.Response = actionContext.Request.CreateErrorResponse(HttpStatusCode.Forbidden, "forbidden");

    }
}

Filter2

public class Filter2 : IAutofacActionFilter
{
    public async Task OnActionExecutedAsync(HttpActionExecutedContext actionExecutedContext, CancellationToken cancellationToken)
    {
    }

    public async Task OnActionExecutingAsync(HttpActionContext actionContext, CancellationToken cancellationToken)
    {
        Debug.WriteLine("Filter 2 invoked");
        // do something
    }
}

Global.asax.cs

public class WebApiApplication : System.Web.HttpApplication
{
    protected void Application_Start()
    {
        AreaRegistration.RegisterAllAreas();
        GlobalConfiguration.Configure(WebApiConfig.Register);
        FilterConfig.RegisterGlobalFilters(GlobalFilters.Filters);
        RouteConfig.RegisterRoutes(RouteTable.Routes);
        BundleConfig.RegisterBundles(BundleTable.Bundles);

        var builder = new ContainerBuilder();

        var config = GlobalConfiguration.Configuration;

        Assembly assembly = Assembly.GetExecutingAssembly();
        builder.RegisterApiControllers(assembly).PropertiesAutowired();
        builder.RegisterWebApiFilterProvider(config);

        builder.Register(c => new Filter1()).AsWebApiActionFilterFor<ValuesController>().InstancePerRequest();
        builder.Register(c => new Filter2()).AsWebApiActionFilterFor<ValuesController>().InstancePerRequest();
        var container = builder.Build();
        config.DependencyResolver = new AutofacWebApiDependencyResolver(container);
    }
}

Run an app and navigate to route, in Output Window will be shown "Filter 1 invoked", "Filter 2 invoked". Property Response of actionContext parameter in OnActionExecutingAsync method of Filter2 equal to Forbidden response.

Libraries:

Autofac 4.6.2
Autofac.WebApi2 4.1.0

[alistairjevans]

Having looked at this issue this morning, here are my findings.

TLDR; I think that a change is needed, but the original example is not 100% applicable, and I think the PR could do with an adjustment.

---

The reason that Filter2 in the 'normal' filter example is never called is because you return the response message directly, without invoking the continuation method to continue the chain. If you change your code in Filter1 to:

  public class Filter1 : IActionFilter
  {
    public bool AllowMultiple => true;

    public Task<HttpResponseMessage> ExecuteActionFilterAsync(HttpActionContext actionContext, 
                                                                    CancellationToken cancellationToken, 
                                                                    Func<Task<HttpResponseMessage>> continuation)
    {
      Debug.WriteLine("Filter 1");
      actionContext.Response = actionContext.Request.CreateErrorResponse(HttpStatusCode.Forbidden, "forbidden");
      return continuation();
    }
  }

That will cause Filter2 to be invoked, but so will the Action method, effectively ignoring your forbidden, which is also definitely not what you want.

It feels to me that IAutofacActionFilter isn't particularly analogous to IActionFilter, because IActionFilter is the low-level implementation that has to handle the continuations itself.

The behaviour of IAutofacActionFilter is more of a match with the public methods on the actual ActionFilterAttribute class, which deals with all the continuation logic for you.

If you change your original example to derive from ActionFilterAttribute then the code becomes much more similar to the Autofac example:

  public class Filter1 : ActionFilterAttribute
  {
    public override async Task OnActionExecutingAsync(HttpActionContext actionContext, CancellationToken cancellationToken)
    {
      Debug.WriteLine("Filter 1");
      actionContext.Response = actionContext.Request.CreateErrorResponse(HttpStatusCode.Forbidden, "forbidden");
    }
  }

Now, in this updated example, the code still doesn't execute Filter2.

The reason it doesn't is because of explicit code in the ActionFilterAttribute class:

https://github.com/aspnet/AspNetWebStack/blob/ebef5b7d821b64ed5c48765d0caf6ce8a9bcfaf5/src/System.Web.Http/Filters/ActionFilterAttribute.cs#L70-L80

Since the ActionFilterWrapper is effectively just a nested collection of ActionFilterAttribute-style behaviour, I feel like it should emulate the same behaviour as the ActionFilter, so we should replicate the 'stop if response is set' behaviour.

---

With regards to the provided PR, part of the change is valid; in the OnActionExecuting handler, I believe we should not continue the filter loop if the response is set by a filter.

However, the change in the PR that applies the same behaviour to OnActionExecuted is not valid. The non-autofac ActionFilterAttribute behaviour is to execute all the filters for OnActionExecuted, and that makes sense. Only OnActionExecuting would typically be used to 'stop' a request.

In addition, the provided PR continues to loop after the response has been created, whereas I would suggest breaking out of the loop to avoid the additional 'FilterMatchesMetadata' checks on subsequent filters.

I'll get round to providing an updated fix (and tests) for this issue shortly.