How do I create a custom AuthorizeAttribute that is specific to the area, controller and action?

Question

In other words, is this a really stupid idea?

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
public class AuthorizeActionAttribute : AuthorizeAttribute
{
    public override void OnAuthorization(AuthorizationContext filterContext)
    {
        // get the area, controller and action
        var area = filterContext.RouteData.Values["area"];
        var controller = filterContext.RouteData.Values["controller"];
        var action = filterContext.RouteData.Values["action"];
        string verb = filterContext.HttpContext.Request.HttpMethod;

        // these values combined are our roleName
        string roleName = String.Format("{0}/{1}/{2}/{3}", area, controller, action, verb);

        // set role name to area/controller/action name
        this.Roles = roleName;

        base.OnAuthorization(filterContext);
    }
}

UPDATE I'm trying to avoid the following, in a scenario where we have extremely granular role permissions because the roles are setup on a per-client basis and attached to user groups:

public partial class HomeController : Controller
{
    [Authorize(Roles = "/supplierarea/homecontroller/indexaction/")]
    public virtual ActionResult Index()
    {
        return View();
    }

    [Authorize(Roles = "/supplierarea/homecontroller/aboutaction/")]
    public virtual ActionResult About()
    {
        return View();
    }
}

Can anyone enlighten me to a secure way to write this AuthorizeRouteAttribute to access the route information and use this as the role name? As Levi says, the RouteData.Values isn't secure.

Is the use of the executing httpContext.Request.Path any more secure or better practice?

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

    if (!filterContext.HttpContext.User.Identity.IsAuthenticated)
    {
        // auth failed, redirect to login page
        filterContext.Result = new HttpUnauthorizedResult();
        return;
    }

    var path = filterContext.HttpContext.Request.Path;
    var verb = filterContext.HttpContext.Request.HttpMethod;

    // these values combined are our roleName
    string roleName = String.Format("{0}/{1}", path, verb);

    if (!filterContext.HttpContext.User.IsInRole(roleName))
    {
        // role auth failed, redirect to login page
        filterContext.Result = new HttpUnauthorizedResult();
        // P.S. I want to tell the logged in user they don't 
        // have access, not ask them to login. They are already
        // logged in!
        return;
    }

    //
    base.OnAuthorization(filterContext);
}

This maybe illustrates the issue a little further:

enum Version
{
    PathBasedRole,
    InsecureButWorks,
    SecureButMissingAreaName
}

string GetRoleName(AuthorizationContext filterContext, Version version)
{
    //
    var path = filterContext.HttpContext.Request.Path;
    var verb = filterContext.HttpContext.Request.HttpMethod;

    // recommended way to access controller and action names
    var controller = 
        filterContext.ActionDescriptor.ControllerDescriptor.ControllerName;
    var action = 
        filterContext.ActionDescriptor.ActionName;
    var area = "oh dear...."; // mmmm, where's thearea name???

    //
    var insecureArea = filterContext.RouteData.Values["area"];
    var insecureController = filterContext.RouteData.Values["controller"];
    var insecureAction = filterContext.RouteData.Values["action"];

    string pathRoleName = 
        String.Format("{0}/{1}", path, verb);
    string insecureRoleName = 
        String.Format("{0}/{1}/{2}/{3}", 
        insecureArea, 
        insecureController, 
        insecureAction, 
        verb);
    string secureRoleName = 
        String.Format("{0}/{1}/{2}/{3}", 
        area, 
        controller, 
        action, 
        verb);

    string roleName = String.Empty;

    switch (version)
    {
        case Version.InsecureButWorks:
            roleName = insecureRoleName;
            break;
        case Version.PathBasedRole:
            roleName = pathRoleName; 
            break;
        case Version.SecureButMissingAreaName:
            // let's hope they don't choose this, because
            // I have no idea what the area name is
            roleName = secureRoleName;
            break;
        default:
            roleName = String.Empty;
            break;
    }

    return roleName;
}

Answer 1

Please do not do this.

If you really need to, you can use the Type of the controller or the MethodInfo of the action to make security decisions. But basing everything off of strings is asking for trouble. Remember, there's no guaranteed 1:1 mapping of Routing values to actual controller. If you're using the Routing tuple (a, b, c) to validate access to SomeController::SomeAction but somebody discovers that (a, b', c) also hits that same action, that person can bypass your security mechanisms.

Edit to respond to comments:

You have access to the controller's Type and the action's MethodInfo via the filterContext parameter's ActionDescriptor property. This is the only sure-fire way to determine what action will really execute when the MVC pipeline is processing, because it's possible that your lookup doesn't exactly match what's going on behind the scenes with MVC. Once you have the Type / MethodInfo / whatever, you can use whatever information you wish (such as their fully-qualified names) to make security decisions.

As a practical example, consider an area MyArea with a controller FooController and an action TheAction. Normally the way that you would hit this FooController::TheAction is via this URL:

/MyArea/Foo/TheAction

And Routing gives the tuple (Area = "MyArea", Controller = "Foo", Action = "TheAction").

However, you can also hit FooController::TheAction via this URL:

/Foo/TheAction

And Routing will give the tuple (Area = "", Controller = "Foo", Action = "TheAction"). Remember, areas are associated with routes, not controllers. And since a controller can be hit by multiple routes (if the definitions match), then a controller can also be logically associated with multiple areas. This is why we tell developers never to use routes (or areas or the <location> tag, by extension) to make security decisions.

Additionally, there's a bug in your class in that it's mutable (it mutates its own Roles property in OnAuthorization). Action filter attributes must be immutable, since they may be cached by parts of the pipeline and reused. Depending on where this attribute is declared in your application, this opens a timing attack, which a malicious site visitor could then exploit to grant himself access to any action he wishes.

For more info, see also my responses at:

• Area level security for asp.net mvc
• How to get currently executing area?

Answer 2

If you want to do this, taking Levi's recommendation into account, the answer is as follows:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Web.Mvc;
using System.Web.Security;

namespace MvcApplication1.Extension.Attribute
{
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
    public class AuthorizeActionAttribute : AuthorizeAttribute
    {
        /// <summary>
        /// Called when a process requests authorization.
        /// </summary>
        /// <param name="filterContext">The filter context, which encapsulates information for using <see cref="T:System.Web.Mvc.AuthorizeAttribute"/>.</param>
        /// <exception cref="T:System.ArgumentNullException">The <paramref name="filterContext"/> parameter is null.</exception>
        public override void OnAuthorization(AuthorizationContext filterContext)
        {
            if (filterContext == null)
            {
                throw new ArgumentNullException("filterContext");
            }

            if (!filterContext.HttpContext.User.Identity.IsAuthenticated)
            {
                // auth failed, redirect to login page
                filterContext.Result = new HttpUnauthorizedResult();

                return;
            }

            // these values combined are our roleName
            string roleName = GetRoleName(filterContext);

            if (!filterContext.HttpContext.User.IsInRole(roleName))
            {
                filterContext.Controller.TempData.Add("RedirectReason", "You are not authorized to access this page.");
                filterContext.Result = new RedirectResult("~/Error/Unauthorized");

                return;
            }

            //
            base.OnAuthorization(filterContext);
        }

        /// <summary>
        /// Gets the name of the role. Theorectical construct that illustrates a problem with the
        /// area name. RouteData is apparently insecure, but the area name is available there.
        /// </summary>
        /// <param name="filterContext">The filter context.</param>
        /// <param name="version">The version.</param>
        /// <returns></returns>
        string GetRoleName(AuthorizationContext filterContext)
        {
            //
            var verb = filterContext.HttpContext.Request.HttpMethod;

            // recommended way to access controller and action names
            var controllerFullName = filterContext.ActionDescriptor.ControllerDescriptor.ControllerType.FullName;
            var actionName = filterContext.ActionDescriptor.ActionName;

            return String.Format("{0}.{1}-{2}", controllerFullName, actionName, verb);
        }
    }
}

I did not want to provide a HttpUnauthorizedResult in the case of a user not being in role, because the result is to send the user to the login page. Considering that they are already logged in, this is extremely confusing to the user.