Questions about a Custom Security setup for MVC3 u

问题:

So after searching for a robust security solution for my MVC3 app, I came across this blog post by Rick Anderson. It details a WhiteList approach where a custom implementation of AuthorizeAttribute is applied as a Global Filter, and you decorate actions/controllers you wish to allow Anonymous access to using a dummy attribute AllowAnonymousAttribute (I say dummy because there is no logic inside of AllowAnonymousAttribute, it's just an empty attribute class)

bool allowAnnonymous = filterContext.ActionDescriptor.IsDefined(typeof(AllowAnonymousAttribute), true)
|| filterContext.ActionDescriptor.ControllerDescriptor.IsDefined(typeof(AllowAnonymousAttribute), true);
if (allowAnnonymous) return;

This (along with other recommendations for security mentioned on his blog like HTTPS) gives me a secure by default model whereby I don't have to apply a security check to every single action, and remember to also add it to future feature additions.

First Part of Question

Now, I'm not using the Users/Roles properties on the AuthorizeAttribute, I need to grab that stuff from a database. To me that's something that would be in AuthorizeCore, since it's sole responsability is to return a true false, does the user have access. However I have a problem, AuthorizeCore has to be thread safe based off my reading of the source for the AuthorizeAttribute class, and I am not sure the best way to go about accessing my database to determine user permissions and adhere to that. My app is using IoC and currently letting my IoC container inject my repository handling all that to the constructor of the AuthorizeAttribute, but by doing this and then accessing it within AuthorizeCore, am I not causing problems with thread safety? Or will an IoC implementation and the MVC3 DependencyResolver I'm using to provide the parameter to my custom AuthorizeAttribute constructor handle the thread safety adequately? Note, my repositories are using a UnitOfWork pattern that includes my nHibernate SessionFactory as a contructor to the repository and the Unit of Work class is provided from my IoC container, implemented by StructureMap using the line below, am I correct in thinking the scope used here would handle threading concerns?

For<IUnitOfWork>().HybridHttpOrThreadLocalScoped().Use<UnitOfWork>();

Second Part of Question

My data model (and thus security model) is setup so that my primary business objects are all defined is such a way that it's one large hierarchy model, and that when I check for permissions I look in that hierarchy model for where the user's account was defined and grant access to everything underneath it by default. The secondary permissions check is the one that uses Administrative defined business logic permissions like can users in role X access the Delete Widget functionality. For this I'm using the Route data and pulling out the Controller and Action names, and use them in conjunction with details from the current users Principal details to hit my database to resolve the permissions for this request. However this logic is being repeated for each ChildAction used on the page as well, but because I'm using the Controller and Action names from the Route data, I'm not actually getting the Child Action information. It stays as the parent action name, not the child action since the child action is not being executed via a URL request. This is causes redundant security checks on my database for the details of the Parent Action and needless resource hits. In researching this I decided to simply bypass the security check for Child actions and rely on the parent action for this.

bool bypassChildAction = filterContext.ActionDescriptor.IsDefined(typeof (ChildActionOnlyAttribute), true) || filterContext.IsChildAction;
if (bypassChildAction) return;

Does it make sense to do this, and if so/not, why? In my mind, if the Action is decorated with ChildActionOnlyAttribute, it's inaccessible publicly via a URL anyway. And if it's being executed as a Child Action but is not exclusively a child action, I can bypass the security check just for this execution since the parent action will handle the permissions. Would you ever have a situation where you would need restrict access to a child action? Knowing that child actions are typically very small partial views I don't anticipate this being a problem but I also saw reference to a line in the default implementation of OnAuthorization outlining some concerns over caching. Does anyone know if that affects my proposed solution?

Summary concerns:

• Multi-threading concerns for accessing user permission from database within AuthorizeCore
• Security concerns over bypassing Authorization check for child actions
• Caching concerns for Child Actions combines with previous point

Any opinions or help with these aspects would be greatly appreciated!

回答1:

Heya Yarx - Part 1- cache all permissions for the user upon login. Multi threaded access then is not an issue as your AuthorizeCore simply gets the roles from the cache which at that time can be considered read only.

Part 2: Again going to point 1 above : ) - if your security checks are so heavy, why not load all permissions for a user upon login and cache them. Upon hitting your child actions you can demand the permissions and at that time check the cache for them.

There is definitely a better way to handle this that isn't as heavy. If you are hitting the db multiple times in a single request solely for permissions, you need to either cache your permission set via some mechanism (custom or implementing another claims based system, etc)

I'm not 100% following your mechanism though for authorization based on the route. You mentioned that you are pulling info from the route - can you give an example here?

It absolutely makes sense to protect your child actions. What if two views call Html.Action - one specifically as an admin and the other is erroneously copy and pasted into another view? Your child actions should always be protected, don't assume they are ok since they are only called from another view.

Also if you cannot cache the entire tree for a user, you can certainly cache the security checks in the first call to AuthorizeCore. Subsequent calls would simply check for ex. cached roles - if there then it uses them, otherwise look to the database.

回答2:

There is where I'm at so far. I feel like I've bastardized it but I'm not sure how else to do it with the requirements I'm looking to satisfy. Am I going about this all wrong or is it just in my head?

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

        if (OutputCacheAttribute.IsChildActionCacheActive(filterContext))
        {
            // If a child action cache block is active, we need to fail immediately, even if authorization
            // would have succeeded. The reason is that there's no way to hook a callback to rerun
            // authorization before the fragment is served from the cache, so we can't guarantee that this
            // filter will be re-run on subsequent requests.
            throw new InvalidOperationException("AuthorizeAttribute cannot be used within a child action caching block."); //Text pulled from System.Web.Mvc.Resources
        }
        // Bypass authorization on any action decorated with AllowAnonymousAttribute, indicationg the page allows anonymous access and
        // does not restrict access anyone (Similar to a WhiteList security model).
        bool allowAnnonymous = filterContext.ActionDescriptor.IsDefined(typeof (AllowAnonymousAttribute), true) || filterContext.ActionDescriptor.ControllerDescriptor.IsDefined(typeof (AllowAnonymousAttribute), true);
        if (allowAnnonymous) return;

        if (CustomAuthorizeCore(filterContext))
        {
            HttpCachePolicyBase cachePolicy = filterContext.HttpContext.Response.Cache;
            cachePolicy.SetProxyMaxAge(new TimeSpan(0));
            cachePolicy.AddValidationCallback(CacheValidateHandler, filterContext); //CacheValidateHandler doesn't have access to our AuthorizationContext, so we pass it in using the data object.
        }

        HandleUnauthorizedRequest(filterContext);
    }

    private void CacheValidateHandler(HttpContext context, object data, ref HttpValidationStatus validationStatus)
    {
        var filterContext = (AuthorizationContext)data;
        validationStatus = CustomOnCacheAuthorization(filterContext);
    }

    protected HttpValidationStatus CustomOnCacheAuthorization(AuthorizationContext filterContext)
    {
        if (filterContext.HttpContext == null)
        {
            throw new ArgumentNullException("filterContext.HttpContext");
        }

        bool isAuthorized = CustomAuthorizeCore(filterContext);
        return (isAuthorized) ? HttpValidationStatus.Valid : HttpValidationStatus.IgnoreThisRequest;
    }

    protected bool CustomAuthorizeCore(AuthorizationContext filterContext)
    {
        HttpContextBase httpContext = filterContext.HttpContext;

        if (httpContext == null)
            throw new ArgumentNullException("filterContext.HttpContext");

        Trace.WriteLine("Current User: " + (httpContext.User.Identity.IsAuthenticated ? httpContext.User.Identity.Name : "Anonymous"));
        if (!httpContext.User.Identity.IsAuthenticated)
            return false;

        string objectId = (httpContext.Request.RequestContext.RouteData.Values["id"] ?? Guid.Empty).ToString();
        Trace.WriteLine("Hierarchy Permissions check for Object: " + objectId);

        string controllerName = httpContext.Request.RequestContext.RouteData.GetRequiredString("controller");
        string actionName = httpContext.Request.RequestContext.RouteData.GetRequiredString("action");
        Trace.WriteLine("Policy Permissions check for Controller: " + controllerName + ", and Action: " + actionName);
        //if(!CheckHierarchyPermissions  || (!CheckHierarchyPermissions && !CheckBusinessLogicPermissions))
        //{
        //    //Check database permissions by getting DB reference from DependancyResolver
        //    DependencyResolver.Current.GetService(typeof (SecurityService)); //change this to an interface later
        //    return false;
        //}

        return true;
    }

    #region Old methods decorated with Obsolete() attributes to track down unintended uses
    [Obsolete("This overridden implementation of AuthorizeAttribute does not use the Users collection.", true)]
    public new string Users { get; set; }

    [Obsolete("This overridden implementation of AuthorizeAttribute does not use the Roles collection.", true)]
    public new string Roles { get; set; }

    [Obsolete("This overridden implementation of AuthorizeAttribute does not use the AuthorizeCore method.", true)]
    protected new bool AuthorizeCore(HttpContextBase httpContext)
    {
        return false;
    }

    [Obsolete("This overridden implementation of AuthorizeAttribute does not use the OnCacheAuthorization method.", true)]
    protected new virtual HttpValidationStatus OnCacheAuthorization(HttpContextBase httpContext)
    {
        return HttpValidationStatus.Invalid;
    }
    #endregion
}

UPDATE: Just a quick update on this one, I never did find a way to dynamically build the name of the Role I was checking for from the combination of Action name and Controller name, and still work within the limitations of the way the requests are made and caching, etc. However the pattern of the WhiteList approach to authorization as detailed on the Blog I linked above is included in MVC4. MVC4 is beta only at the moment, but I don't expect they'll remove it between now and the final version.