Revisiting custom authorization in ASP.NET MVC

Several months ago I blogged about custom attributes based on the AuthorizeAttribute class in ASP.NET MVC. One of the bits that I got wrong or, at least, not as right as I would like, is caching. I didn’t extend the caching to allow it to take into account the new tests for authorization that I’d developed. The end result of this was that if you derived your permission to access the method based on the new test, you always got served a fresh page. It was never delivered from the cache.

In my application, this didn’t cause too much of a performance it. In fact, most of the actions protected in such a way are explicitly not cached anyway so the drawback rarely (if ever, I haven’t checked all the methods) came into play. I got asked about it recently on a StackOverflow answer so I thought I’d take a stab at improving the cache handling to account for the new tests.

There are a couple of things you need to be aware of. First, the code in OnCacheAuthorization must be thread-safe. Instances of the attribute are reused for multiple requests. In particular, this means that you cannot store information from the request in the attribute and expect it to remain inviolate during the life of the attribute. Another request may come along later and overwrite your stored request data and cause your code to return incorrect results. Since we’re talking about authorization this is a serious problem since it could lead to users getting access to information they should not be able to see.

Second, you need to be careful not to reuse the request data associated with the request that creates the cached entry. While there is a mechanism that allows you to pass data to the CacheValidationHandler method I created in the first post, you need to be careful to only pass data that actually applies to all requests, since you wouldn’t want to depend on the data being the same in the request that is served up from the cache. It may be an entirely different user making that request.

Our attribute still derives from AuthorizeAttribute, with the same modifications due to the fact that it is specific to a particular request and so doesn’t make sense to apply it at the class level or have multiples.

 AttributeUsage( AttributeTargets.Method, Inherited = true, AllowMultiple = false )]
 public class RoleOrOwnerAuthorizationAttribute : AuthorizationAttribute
 {
        private IDataContextFactory ContextFactory { get; set; }

        private string routeParameter = "id";
        /// <summary>
        /// The name of the routing parameter to use to identify the owner of the data (user id) in question.  Default is "id".
        /// </summary>
        public string RouteParameter
        {
            get { return this.routeParameter; }
            set { this.routeParameter = value; }
        }

        ...
}

The constructors, including reliance on my data context factory and its constructor injection, remain the same as well as the methods that I’ve refactored from the original AuthorizeAttribute. Notice, though, that I've made them virtual now in case we want to derive from this attribute.

protected IDataContextFactory ContextFactory { get; set; }

public RoleOrOwnerAuthorizationAttribute()
    : this( null )
{
}

protected virtual void CacheValidateHandler( HttpContext context, object data, ref HttpValidationStatus validationStatus )
{
    validationStatus = OnCacheAuthorization( new HttpContextWrapper( context ) );
}

 protected virtual void SetCachePolicy( AuthorizationContext filterContext )
{
    // ** IMPORTANT **
    // Since we're performing authorization at the action level, the authorization code runs
    // after the output caching module. In the worst case this could allow an authorized user
    // to cause the page to be cached, then an unauthorized user would later be served the
    // cached page. We work around this by telling proxies not to cache the sensitive page,
    // then we hook our custom authorization code into the caching mechanism so that we have
     // the final say on whether a page should be served from the cache.
    HttpCachePolicyBase cachePolicy = filterContext.HttpContext.Response.Cache;
    cachePolicy.SetProxyMaxAge( new TimeSpan( 0 ) );
    cachePolicy.AddValidationCallback( CacheValidateHandler, null /* data */);
}

Now I introduce a new, inner class CurrentRequest to help in abstracting the different between the authorization request that creates the cached copy and subsequent cache requests that need to be served from the cache, or not. It stores the OwnerID of the item being requested and the current Username of the user making the request. Our IsOwner method will be refactored to use this class instead of the AuthorizationContext for this data. Note that it has three constructors; the default constructor will be used for unit testing since I won’t always want to mock up either an AuthorizationContext or an HttpContextBase object in my tests.  The other constructors are used for requests made from OnAuthorization and OnCacheAuthorization, respectively.  Note the real difference is what context we have available to the code. In the first case we can use RouteValues to extract the OwnerID, in the second, we use the Request parameters directly.

public class CurrentRequest
{

    public CurrentRequest()
    {
        this.OwnerID = -1;
    }

    public CurrentRequest( AuthorizationContext filterContext, string routeParameter )
        : this()
    {
        if (filterContext.RouteData.Values.ContainsKey( routeParameter ))
        {
            this.OwnerID = Convert.ToInt32( filterContext.RouteData.Values[routeParameter] );
        }

        this.Username = filterContext.HttpContext.User.Identity.Name;
    }

    public CurrentRequest( HttpContextBase httpContext, string routeParameter )
    {
        if (!string.IsNullOrEmpty( routeParameter ))
        {
            if (httpContext.Request.Params[routeParameter] != null)
            {
                this.OwnerID = GetOwnerID( httpContext.Request.Params[routeParameter] );
            }
            else if (string.Equals( "id", routeParameter, StringComparison.OrdinalIgnoreCase ))
            {
                // id may be last element in path if not included as a parameter
                this.OwnerID = GetOwnerID( httpContext.Request.Path.Split( '/' ).Last() );
            }
        }

        this.Username = httpContext.User.Identity.Name;
    }

    private int GetOwnerID( string id )
    {
        int ownerID;
        if (!int.TryParse( id, out ownerID ))
        {
            ownerID = -1;
        }
        return ownerID;
    }

    public int OwnerID { get; set; }
    public string Username { get; set; }
}

NOTE: An earlier version of this post did not handle getting the route parameter from the HttpContext properly, if the route parameter was the "id" property.  This version relies on the default route set up to place the id parameter at the end of the path but, with that caveat, now handles an "id" parameter properly.

The OnAuthorization method changes slightly to use this new class. Instead of simply passing the AuthorizationContext, we pass a CurrentRequest object constructed from it using the RouteParameter on the attribute.

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

    if (AuthorizeCore( filterContext.HttpContext ))
    {
        SetCachePolicy( filterContext );
    }
    else if (!filterContext.HttpContext.User.Identity.IsAuthenticated)
    {
        // auth failed, redirect to login page
        filterContext.Result = new HttpUnauthorizedResult();
    }
    else if (filterContext.HttpContext.User.IsInRole( "SuperUser" ) || IsOwner( new CurrentRequest( filterContext, this.RouteParameter ) ))
    {
        SetCachePolicy( filterContext );
    }
    else
    {
        ViewDataDictionary viewData = new ViewDataDictionary();
        viewData.Add( "Message", "You do not have sufficient privileges for this operation." );
        filterContext.Result = new ViewResult { MasterName = this.MasterName, ViewName = this.ViewName, ViewData = viewData };
    }
}

This allows us to simplify IsOwner significantly as the CurrentRequest class now takes care of unpacking the data from the request. Note how we are able to use the ContextFactory (it’s thread-safe) to create a new data context within the method, ensuring that we don’t conflict with other threads that may be using the attribute. The data context itself needn’t be thread-safe since we create a new one each time and it’s local to the method.

private bool IsOwner( CurrentRequest requestData )
{
    using (var dc = this.ContextFactory.GetDataContextWrapper())
    {
        return dc.Table<User>().Where( p => p.UserName == requestData.Username && p.UserID == requestData.OwnerID ).Any();
    }
}

Lastly, we need to hook our IsOwner check into the authorization system.  We do this by overriding OnCacheAuthorization and doing our check when the base class method would normally invalidate the request because the user is not in the correct role. When they are allowed by the role, we needn’t perform our check.

protected override HttpValidationStatus OnCacheAuthorization( HttpContextBase httpContext )
{
    var status = base.OnCacheAuthorization( httpContext );
    if (status == HttpValidationStatus.IgnoreThisRequest && IsOwner( new CurrentRequest( httpContext, this.RouteParameter ) ))
    {
        status = HttpValidationStatus.Valid;
    }
    return status;
}

Note here that we use the constructor on CurrentRequest that takes an HttpContextBase (wrapper around HttpContext) instead because that's all we have available to use when being evaluated by the caching system. Because we've abstracted IsOwner to use this object, it doesn't need to know how we got the values that it will be checking.

Now the owners of the data will have the same cache-based performance as the administrative users who access the data via their roles.