Sunday, September 06, 2009 4:06 PM
Kazi Manzur Rashid
ASP.NET MVC and Authorization and Monkey Patching
[Update: Maarten Balliauw confirmed that he has applied the suggested fix in MVC Sitemap provider]
As you know that we will be including Menu in our final release, when defining the menu, it will allow to specify the Route Name, Controller/Action name and associated route values for an menu item so that we can generate the corresponding url. One of the basic feature that we want to include is, when rendering the Menu it will scan through the controller’s actions and only render the menu items that the currently visiting user has permission. As you can guess, it is related with the AuthorizeAttribute of ASP.NET MVC framework. The actual method that is responsible for checking the permission of this attribute is AuthorizeCore which is marked as protected, so there is no way we can call this method from our code. So I decided to check, how other peoples are handling this issue, so far I have found two solutions:
- ASP.NET MVC Sitemap provider of Maarten Balliauw and
- MVCContrib.org.
But none of them are actually handling it correctly!!!.
Before moving to what is wrong with these solutions, let me paste the main portion of the AuthorizeAttribute code that is responsible for checking the permission:
public class AuthorizeAttribute : FilterAttribute, IAuthorizationFilter
{
public virtual void OnAuthorization(AuthorizationContext filterContext)
{
if (filterContext == null)
{
throw new ArgumentNullException("filterContext");
}
if (AuthorizeCore(filterContext.HttpContext))
{
HttpCachePolicyBase cachePolicy = filterContext.HttpContext.Response.Cache;
cachePolicy.SetProxyMaxAge(new TimeSpan(0));
cachePolicy.AddValidationCallback(CacheValidateHandler, null /* data */);
}
else
{
filterContext.Result = new HttpUnauthorizedResult();
}
}
protected virtual bool AuthorizeCore(HttpContextBase httpContext)
{
if (httpContext == null)
{
throw new ArgumentNullException("httpContext");
}
IPrincipal user = httpContext.User;
if (!user.Identity.IsAuthenticated)
{
return false;
}
if (_usersSplit.Length > 0 && !_usersSplit.Contains(user.Identity.Name, StringComparer.OrdinalIgnoreCase))
{
return false;
}
if (_rolesSplit.Length > 0 && !_rolesSplit.Any(user.IsInRole))
{
return false;
}
return true;
}
}As you can see the only public method OnAuthorization (which is also an implementation of IAuthorizationFilter interface, we will discuss this interface after a little while) is calling the protected AuthorizeCore to check the permission, if it is permitted then it is adding some callback to sync with the ASP.NET Caching otherwise it returns Unauthorized Result.
Now lets see, what is wrong with the above two solutions, First the ASP.NET MVC SiteMap provider, the code that is responsible for checking the controller/action permission is the following:
IController controller = provider.GetController(requestContext, mvcNode.Controller); // get controller
if (controller is ControllerBase)
{
var controllerContext = new ControllerContext(requestContext, (ControllerBase)controller);
var authorizationContext = new AuthorizationContext(controllerContext);
foreach (IAuthorizationFilter att in controller.GetType().GetCustomAttributes(typeof(IAuthorizationFilter), true).Union(// get controller authorization filters
controller.GetType().GetMethods(BindingFlags.Public | BindingFlags.Instance).Where(m =>
{
var nameAtt = m.GetCustomAttributes(typeof(ActionNameAttribute), true).FirstOrDefault() as ActionNameAttribute;
return string.Equals(mvcNode.Action, nameAtt != null ? nameAtt.Name : m.Name, StringComparison.OrdinalIgnoreCase);
})
.SelectMany(m => m.GetCustomAttributes(typeof(IAuthorizationFilter), true))))// get authorization filters from all related methods
{
if (att is ValidateAntiForgeryTokenAttribute)
{
return true;
}
att.OnAuthorization(authorizationContext); // run authorization
if (authorizationContext.Result != null) // authorization failed
{
return false;
}
}
}There are two issues with the above code:
- It depends upon the
IAuthorizationFilter rather than AuthorizeAttribute, the problem with this approach is there more filters other than AuthorizeAttribute which implements this interface, as you can see there is a check for ValidateAntiForgeryTokenAttribute in the above code, there are few more already in the ASP.NET MVC Framework e.g. ValidateInputAttribute, RequireSslAttribute and anyone can write a custom filter that implements this interface and decorate the controller/action, so this code will return incorrect results, in those cases.
- The next issue, when it is executed for
AuthorizeAttribute which in turns adds caching callback and there is no way to remove that callback once the OnAuthorization calls gets completed, so it will screw the ASP.NET Caching and might hurt the application performance.
The correct way to address this issue:
- Don’t depend on the
IAuthorizationFilter interface, instead check the concrete AuthorizeAttribute which is meant to be dealing with Roles/Users, also it is not marked as sealed, so it is expected that you will be inheriting this class and override the AuthorizeCore method if you want to put your custom logic.
- Find a way to call the
protected AuthorizeCore method which is actually responsible for checking the permission.
Now, lets see how the MVCContrib team is handling this issue, basically they have created a new class inherited from the AuthorizeAttribute and it also contains a public method Authorized which in turn calls the protected AuthorizeCore method:
public class OpenAuthorizeAttribute : AuthorizeAttribute
{
public OpenAuthorizeAttribute(AuthorizeAttribute attribute)
{
Order = attribute.Order;
Roles = attribute.Roles;
Users = attribute.Users;
}
public bool Authorized(RequestContext requestContext)
{
return AuthorizeCore(requestContext.HttpContext);
}
}And whenever they wants to check permission they pass the actual attribute to this new class and calls the Authorized method. Much better than the previous solution, still it will not work when you create a Custom AuthorizationAttribute, as it will call the AuthorizeCore of the original AuthorizationAttribute rather than yours.
After discussing with the ASP.NET MVC Team and Levi Broderick explained me really well that AuthorizeAttribute was not really designed to be used in this kind of scenario.To solve it, yes we can use InvokeMethod of reflection, but invoking a protected method will not work in medium trust environment. So the only option that is left to monkey patch this issue is IL rewritting. The logic is, when checking the permission it will check whether a custom AuthorizeAttribute is used, if not it will follow the same as the MVCContrib is currently doing, if a custom Authorization attribute is used, it will runtime create a inherited class with a public method which we will call to check the permission, of course there is slight a performance overhead associated, but we can easily overcome it with proper caching. Now lets take a quick look how we are checking whether the user has permission of an action, assuming that the AuthorizeAttributes are collected from another service:
foreach (AuthorizeAttribute authorizationAttribute in authorizationAttributes)
{
if (authorizationAttribute != null)
{
try
{
Type currentAuthorizationAttributeType = authorizationAttribute.GetType();
IAuthorizeAttribute subclassedAttribute = (currentAuthorizationAttributeType == typeof(AuthorizeAttribute)) ?
new InternalAuthorize() : // No need to use Reflection.Emit when asp.net mvc built-in attribute is used
reflectedAuthorizeAttributeCache.GetAttribute(currentAuthorizationAttributeType);
subclassedAttribute.Order = authorizationAttribute.Order;
subclassedAttribute.Roles = authorizationAttribute.Roles;
subclassedAttribute.Users = authorizationAttribute.Users;
// Copy the remaining properties (if there is any)
objectCopier.Copy(authorizationAttribute, subclassedAttribute, "Order", "Roles", "Users" /* Excluded properties */);
allowed = subclassedAttribute.IsAuthorized(requestContext.HttpContext);
}
catch
{
// do not allow on exception
allowed = false;
}
if (!allowed)
{
break;
}
}
}As you can see when the default AuthorizeAttribute is used we are creating InternalAttribute, if not we are requesting a caching service to get the runtime version of this attribute, for each custom attribute we are using Reflection.Emit to create a subclass of that attribute and then we are caching it so that we can use the same attribute without recreating it with Reflection.Emit. Also to avoid naming collision, we created an interface IAuthorizeAttribute which we implements with IL. The IAuthorizeAttribute contains the same properties as the original AuthorizeAttribute:
public interface IAuthorizeAttribute
{
int Order
{
get;
set;
}
string Roles
{
get;
set;
}
string Users
{
get;
set;
}
bool IsAuthorized(HttpContextBase httpContext);
}Now here is the magic code that generates the class at runtime:
// (c) Copyright Telerik Corp.
// This source is subject to the Microsoft Public License.
// See http://www.microsoft.com/opensource/licenses.mspx#Ms-PL.
// All other rights reserved.
namespace Telerik.Web.Mvc.Infrastructure.Implementation
{
using System;
using System.Reflection;
using System.Reflection.Emit;
using System.Web;
public class AuthorizeAttributeBuilder : IAuthorizeAttributeBuilder
{
private static readonly Type authorizeAttributeType = typeof(IAuthorizeAttribute);
private static readonly ModuleBuilder module = CreateModuleBuilder();
public ConstructorInfo Build(Type parentType)
{
Guard.IsNotNull(parentType, "parentType");
string typeName = "$" + parentType.FullName.Replace(".", string.Empty);
TypeBuilder typeBuilder = module.DefineType(typeName, TypeAttributes.Class | TypeAttributes.Public | TypeAttributes.AutoClass | TypeAttributes.AnsiClass | TypeAttributes.BeforeFieldInit, parentType, new[] { authorizeAttributeType });
typeBuilder.DefineDefaultConstructor(MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.SpecialName | MethodAttributes.RTSpecialName);
typeBuilder.AddInterfaceImplementation(authorizeAttributeType);
WriteProperty(parentType, typeBuilder, "Order", typeof(int));
WriteProperty(parentType, typeBuilder, "Roles", typeof(string));
WriteProperty(parentType, typeBuilder, "Users", typeof(string));
WriteIsAuthorized(parentType, typeBuilder);
Type type = typeBuilder.CreateType();
return type.GetConstructor(Type.EmptyTypes);
}
private static void WriteProperty(Type parentType, TypeBuilder builder, string name, Type type)
{
string getName = "get_" + name;
string setName = "set_" + name;
MethodInfo parentGetMethod = parentType.GetMethod(getName, BindingFlags.Public | BindingFlags.Instance);
MethodBuilder implementedGetMethod = builder.DefineMethod(getName, MethodAttributes.Public | MethodAttributes.Virtual, type, Type.EmptyTypes);
ILGenerator getIl = implementedGetMethod.GetILGenerator();
getIl.Emit(OpCodes.Ldarg_0);
getIl.Emit(OpCodes.Call, parentGetMethod);
getIl.Emit(OpCodes.Ret);
MethodInfo interfaceGetMethod = authorizeAttributeType.GetMethod(getName, BindingFlags.Public | BindingFlags.Instance);
builder.DefineMethodOverride(implementedGetMethod, interfaceGetMethod);
MethodInfo parentSetMethod = parentType.GetMethod(setName, BindingFlags.Public | BindingFlags.Instance);
MethodBuilder implementedSetMethod = builder.DefineMethod(setName, MethodAttributes.Public | MethodAttributes.Virtual, typeof(void), new[] { type });
ILGenerator setIl = implementedSetMethod.GetILGenerator();
setIl.Emit(OpCodes.Ldarg_0);
setIl.Emit(OpCodes.Ldarg_1);
setIl.Emit(OpCodes.Call, parentSetMethod);
setIl.Emit(OpCodes.Ret);
MethodInfo interfaceSetMethod = authorizeAttributeType.GetMethod(setName, BindingFlags.Public | BindingFlags.Instance);
builder.DefineMethodOverride(implementedSetMethod, interfaceSetMethod);
}
private static void WriteIsAuthorized(Type parentType, TypeBuilder builder)
{
MethodInfo protectedMethod = parentType.GetMethod("AuthorizeCore", BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.InvokeMethod);
MethodBuilder implementedMethod = builder.DefineMethod("IsAuthorized", MethodAttributes.Public | MethodAttributes.Virtual, typeof(bool), new[] { typeof(HttpContextBase) });
ILGenerator il = implementedMethod.GetILGenerator();
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldarg_1);
il.Emit(OpCodes.Call, protectedMethod);
il.Emit(OpCodes.Ret);
MethodInfo interfaceMethod = authorizeAttributeType.GetMethod("IsAuthorized", BindingFlags.Public | BindingFlags.Instance);
builder.DefineMethodOverride(implementedMethod, interfaceMethod);
}
private static ModuleBuilder CreateModuleBuilder()
{
const string Name = "InheritedAuthorizeAttributes";
AssemblyName assemblyName = new AssemblyName(Name + "Assembly")
{
Version = typeof(AuthorizeAttributeBuilder).Assembly.GetName().Version
};
AssemblyBuilder assemblyBuilder = AppDomain.CurrentDomain.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Run);
ModuleBuilder moduleBuilder = assemblyBuilder.DefineDynamicModule(Name + "Module");
return moduleBuilder;
}
}
}There is only one public method Build where you have to pass the custom AuthorizeAttribute type, the rest will be taken care by it. And if you want to use the above code, you are permitted to do so, as it is licensed under MS-PL.
The moral of this story is though ASP.NET MVC is one of the most extensible framework by MS, yet there are some places where it requires some re-work, but it does not mean you would say some harsh word like some negative minded people of our community, instead you should look for the solution and share it.
Filed under: Asp.net, MVC, ASPNETMVC, ASP.NET MVC, Open Source, Telerik