Why doesn't HandleSecurityAttribute inherit AuthorizeAttribute?

[NightOwl888]

I am a contributor on the MvcSiteMapProvider project and recently discovered that some of our users are interested in configuring security "in one place". I suggested using FluentSecurity and implementing a custom MvcSiteMapProvider.Security.IAclModule to read the security settings from FluentSecurity, but after taking a peek at your source code I think I see a much better and simpler solution that wouldn't require us to add a dependency on your library or go through the lengthy process of developing such a module. The simplest solution would be for you to inherit AuthorizeAttribute in your HandleSecurityAttribute so the settings from FluentSecurity would be picked up by MvcSiteMapProvider automatically by our existing AuthorizeAttributeAclModule.

The one constraint that we are using on AuthorizeAttribute (or any class that inherits it) is that it must set the filterContext.Result to a non-null value if the security check fails and set it to null if it succeeds, but it looks like you are already doing that. The only problem is that since you are not inheriting AuthorizeAttribute, the configuration of FluentSecurity is ignored by MvcSiteMapProvider (and possibly by other 3rd party libraries as well).

I also noticed a possible bug in HandleSecurityAttribute. The MVC AuthorizeAttribute disables the output cache when the security context succeeds to ensure that a cached copy of the page isn't served when security fails, but it doesn't look like you are doing that.

if (AuthorizeCore(filterContext.HttpContext))
{
    // ** 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 */);
}
else
{
    HandleUnauthorizedRequest(filterContext);
}

We had to jump through some hoops to generate an HttpContextBase that overrides SetProxyMaxAge() and AddValidationCallback() to ensure we don't mess up the cache on the current page. The current page is not necessarily the page we are using the AuthorizeAttribute for, which is why I am familiar with this block of code.

[kristofferahl]

Hi! Sorry for the late reply and thanks for taking the time to supply me with all the details needed and a pull-request to go with that.

Without having had the time to do any research on this myself I see no problems inheriting from AuthorizeAttribute for future versions of FluentSecurity. As for why it's not already inheriting from AuthorizeAttribute, if I remember correctly it was because of all those extra properties that I decided not to do so. But your solution might be good enough there and there are clearly some benefits to be had from doing so.

However, I don't think it would be safe to make this change without bumping the major version so it will have to wait until the 3.0 version. I think writing a custom HandleSecurityAttribute is a good solution for any 2.0 users so they won't have to wait for the next release. I will make this a part of the 3.0 milestone and apply your patch (#73) when I get a chance to start working on 3.0.

Thanks again for all your hard work!!

[kristofferahl]

If anyone can think of a reason for not inheriting from the AuthorizeAttribute, please let us know!

[NightOwl888]

@kristofferahl

I am glad to hear you are on board with making this change.

It is really not my place to tell you how to do your release schedule, but if you are following semantic versioning, clearly this is a fully backward compatible change that could go into a patch release. AuthorizeAttribute inherits from FilterAttribute which in turn inherits from Attribute, so any code that relies on the fact HandleSecurityAttribute inherits Attribute will continue to function as it did before.

The only real difference is that you are being more specific about what the HandleSecurityAttribute is intended for. It is a bit unfortunate that Microsoft didn't provide a way to be more specific without making the assumption that the security would be based on users and roles, but at least hiding those properties helps to clarify that is your intent.

[kristofferahl]

I am using SemVer to version FluentSecurity. However, I'm afraid it might break existing applications even though the code API has not changed. But maybe I'm just a big chicken. Go big or go home right!? ;)

The way that you hide those properties. Are you still able to set them if you know they are there or do the compiler tell you they aren't there?

[NightOwl888]

Copied verbatim from here.

There is no way to get rid of properties entirely if they are declared in the base class. However, this will hide them from intellisense, hide them from the VS properties designer, hide them from executing code (by re-declaring the properties with the new keyword, since they cannot be overridden), make sure they are never serialized, and VS will underline them in green indicating they are obsolete if someone tries to use them. However, none of these prevents them from being used, they just make it more obvious that they shouldn't be.

I suppose just to ensure they are not set, you could also throw a NotImplementedException. But in that case, I would say you should hide all of the protected members as well because they may be using these properties. If someone were to call one of those members from a subclass of HandleSecurityAttribute, they might get this exception instead of having the code execute normally.

[Obsolete("Not applicable in this class.")]
[DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
[Browsable(false), EditorBrowsable(EditorBrowsableState.Never)]
new public string Roles 
{ 
    get; 
    set { throw new NotImplementedException(); }
}

[NightOwl888]

Since you are planning on doing a major version release, it might make sense if you try to use the MVC code as much as possible to also work out that (potential) issue with output caching. I haven't confirmed whether you have an issue, but reusing the code in AuthorizeAttribute.OnAuthorization will ensure it cannot happen (and you don't have to worry about maintaining it yourself).

I tried to do this for you before, but it turned out not to be very easy because it requires you to get the controller type name from HttpContextBase. This turns out to be non-trivial as you can see in the ControllerTypeResolver class. Here is what the code looked like.

using System;
using System.ComponentModel;
using System.Web;
using System.Web.Mvc;
using System.Web.Routing;
using FluentSecurity.ServiceLocation;

namespace FluentSecurity
{
    [AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
    public class HandleSecurityAttribute : AuthorizeAttribute, IAuthorizationFilter
    {
        internal ActionResult OverrideResult { get; private set; }
        internal ISecurityHandler Handler { get; private set; }

        public HandleSecurityAttribute() : this(ServiceLocator.Current.Resolve<ISecurityHandler>()) {}

        public HandleSecurityAttribute(ISecurityHandler securityHandler)
        {
            Handler = securityHandler;
        }

        protected override bool AuthorizeCore(HttpContextBase httpContext)
        {
            if (httpContext == null)
            {
                throw new ArgumentNullException("httpContext");
            }

            var routeData = RouteTable.Routes.GetRouteData(httpContext);
            var actionName = routeData.GetRequiredString("action");
            var controllerName = // TODO: Get controller type name here

            var securityContext = SecurityContext.Current;
            securityContext.Data.RouteValues = routeData.Values;

            OverrideResult = Handler.HandleSecurityFor(controllerName, actionName, securityContext);

            return (OverrideResult == null);
        }

        protected override void HandleUnauthorizedRequest(AuthorizationContext filterContext)
        {
            filterContext.Result = OverrideResult;
        }

        [Obsolete("Not applicable in this class.")]
        [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
        [Browsable(false), EditorBrowsable(EditorBrowsableState.Never)]
        new public string Roles 
        {
            get { throw new NotImplementedException(); }
            set { throw new NotImplementedException(); }
        }

        [Obsolete("Not applicable in this class.")]
        [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
        [Browsable(false), EditorBrowsable(EditorBrowsableState.Never)]
        new public string Users
        {
            get { throw new NotImplementedException(); }
            set { throw new NotImplementedException(); }
        }

        [Obsolete("Not applicable in this class.")]
        [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
        [Browsable(false), EditorBrowsable(EditorBrowsableState.Never)]
        new public int Order
        {
            get { throw new NotImplementedException(); }
            set { throw new NotImplementedException(); }
        }
    }
}

The idea is to return a boolean in the AuthorizeCore method and utilize the existing OnAuthorize method (which calls AuthorizeCore and if it returns false, calls HandleUnauthorizedRequest) so it handles the output caching for you. Note that AuthorizeCore is also called by the caching module. Since the bit that deals with users and roles is in the AuthorizeCore method, this should work out nicely. And if you do it this way, you don't have to worry about inheritors seeing the methods because you are overriding the implementation of AuthorizeAttribute.

Perhaps you could find a way to refactor the design a bit to allow resolving the controller name from HttpContextBase (maybe you could create an overload of HandleSecurityFor that accepts an area and controller name instead of controller type name), and then you could fix the output cache issue in addition to making FluentSecurity get picked up automatically by MvcSiteMapProvider.

Important: AuthorizeCore is also called by the OnCacheAuthorization method, so getting the controller type name in the OnAuthorization method, setting it to a class level variable, and then using it in the AuthorizeCore method won't work 100% of the time.