This blog post is about an ASP.NET MVC workaround we implemented in a previous project. We solved the problem by enforcing using a class that extends one of ASP.NET MVC classes, which in itself created another problem, as new developers joining the project may always use the old class. The solution to this problem was not something that I invented, but it’s also not a very common practice.

So, if you are interested, here’s the entire story…

Detecting Session Timeout In AJAX Requests

We wanted to solve a problem where in an AJAX heavy ASP.NET MVC application, if the user triggers an AJAX action after staying inactive for longer than our application timeout, the call to the controller action, which normally gets a JSON response, would instead get the HTML of the login page.

This is a known issue in ASP.NET (particularly System.Web). A feature that’s on by default is returning a redirect to the login page instead of a HTTP Unauthorized Status code (401). After the redirect the response returned is a successful (HTTP Status Code 200) load of the login page. That means even our Angular.JS error interceptors (or jQuery handlers, etc.) don’t notice there was an error.

A fix for this was turning this feature off. We inherited the Authorize attribute as below:

 /// 
 /// Suppresses Forms Authentication Redirects for Ajax requests
 ///     so the client side interceptors can handle it.
 /// 
 [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method,
     Inherited = true, AllowMultiple = true)]
 public class AuthorizeRedirectAttribute : AuthorizeAttribute
 {
     protected override void HandleUnauthorizedRequest(AuthorizationContext filterContext)
     {
         var context = filterContext.HttpContext;
         if (context.Request.IsAjaxRequest())
         {
             context.Response.SuppressFormsAuthenticationRedirect = true;
         }
         base.HandleUnauthorizedRequest(filterContext);
     }
 }

SuppressFormsAuthenticationRedirect is the property that disables the login redirect. Microsoft set it to false by default so that it’s backwards compatible.

ASP.NET MVC doesn’t recognize AJAX requests through Request.IsAjaxRequest() via Accept header or so. It does via checking X-Requested-With header. Most AJAX-capable frameworks like jQuery and others offer a way to intercept all requests and add extra headers, for example, in that app, we configure Angular.JS to include the header with some code similar to this:

 app.config(['$httpProvider', function ($httpProvider) {
   $httpProvider.defaults.headers.common["X-Requested-With"] = "XMLHttpRequest";
 }]);

Enforcing The Convention

The obvious problem with the previous solution is that we are ignoring The Power Of Defaults. Any other developer who may join the project needs to know that using Authorize is a no-no, and even for old devs (myself included),
it’s very easy to just forget and use Authorize not AuthorizeRedirect just out of habit.

Solving this problem was quite easy though, we added the following test to our Unit Tests project:

 [TestClass]
 public class AuthorizeRedirectTests
 {
     [TestMethod]
     public void All_Controllers_And_Actions_Do_Not_Inherit_Authorize_Directly()
     {
         var controllerType = typeof(IController);
         var invalidAuthorizeFilter = typeof(AuthorizeAttribute);
         var correctAuthorizeFilter = typeof (AuthorizeRedirectAttribute);
         var webProjectAssembly = correctAuthorizeFilter.Assembly;
         var controllers = webProjectAssembly
             .GetTypes()
             .Where(controllerType.IsAssignableFrom);
         Func>
             invalidAttributes = member =>
                 member
                     .GetCustomAttributes(invalidAuthorizeFilter)
                     // Note that AuthorizeRedirectAttribute inherits from AuthorizeAttribute
                     .SkipWhile(correctAuthorizeFilter.IsInstanceOfType);
         foreach (var controller in controllers)
         {
             Assert.IsTrue(invalidAttributes(controller).IsNullOrEmpty(),
                 "Controller {0} should not use {1} directly, " +
                 "use {2} instead",
                 controller.Name,
                 invalidAuthorizeFilter.Name, correctAuthorizeFilter.Name);
             var actions = controller
                 // Simple check, might get some false positives but they won't hurt
                 .GetMethods(BindingFlags.Instance | BindingFlags.Public);
             foreach (var action in actions)
             {
                 Assert.IsTrue(invalidAttributes(action).IsNullOrEmpty(),
                     "Controller Action {0}.{1} should not use {2} directly, " +
                     "use {3} instead",
                     controller.Name, action.Name,
                     invalidAuthorizeFilter.Name, correctAuthorizeFilter.Name);
             }
         }
     }
 }

I hope the code is self explanatory. We check the web project assembly for all ASP.NET MVC Controllers, then we check the Controllers and all their Action methods for existence of the AuthorizeAttribute. We filter those that use the correct attribute (AuthorizeRedirectAttribute), and we Assert that there are no the no Controllers or action remaining, otherwise, we tell the developer which Controller or Action needs to be fixed, and how to fix it as well.

Room For Improvement

The drawback of this is that our Unit Test project had to reference the ASP.NET MVC assemblies and gets more stuff than most tests should need. We can overcome this by moving our “convention” tests into another project completely, but for this project the conventions were very few and it seemed fine.

Of course the same method can be applied to any other convention you enforce in your project. One obvious example is ensuring all Controllers inherit from a custom base Controller class instead of the ASP.NET MVC class directly. I know people who already do this, as I mentioned in the opening the technique is not new by any means, but it’s worth even more popularity.

Speaking of improvement, the code for this test class was optimized a bit while writing this blog post, there is always room for improvement :)

IsNullOrEmpty()

In case you were reading the code carefully, the IsNullOrEmpty() method I used in assertions is a custom extension method we had in the project, a very simple one as you may expect:

 public static bool IsNullOrEmpty(this IEnumerable source)
 {
     return source == null || !source.Any();
 }

And That’s it!

I hope you found the technique useful if you haven’t used it before, or found the post a good place to reference it to those who didn’t.

Now that we're done, click this out ;)

, , , , ,
  • Abdelmawla Mohamed

    Nice workaround, we used this technique also to enforce using output encoding library for any string in html files.
    Note: applied it with php

  • http://www.joshka.net/ Joshua McKinney

    Hey Meligy,

    This is a cool idea. A possible improvement over ensuring that devs use the new AuthorizeRedirectAttribute rather than the out of the box AuthorizeAttribute might be to seperate the concern of suppressing the form redirect into separate Attribute that just only that (SuppressAjaxAuthenticationRedirectAttribute). Registering this as a global filter would allow you to leave the Authorize attributes on each method / controller as is, meaning that there’s no extra learning curve, possible wrong usage. You fall into the pit of success ;)

    • http://gurustop.net/ @Meligy

      Hey Josh,
      The idea is that every single `Controller` `Action` that returns a `JsonResult` should have this set correctly (while `Action`s that don’t will have no problem having it set as well, but this is not the point).

      We could override the `Json()` methods and check for that in them, but, the authorixzation check happens too early, before even accessing the `Action()` (which is by design, and is correct).

      If we have it as a separate attribute, it will be easy for developers to froget it, and there will not be an easy way to discover it except via a Unit Test as well (which needs to run the `Action()` method and if the result is JSON, check for the other attribute), so, it feels pretty much the same, if not more complex.

      • http://www.joshka.net/ Joshua McKinney

        I’m not sure that I follow your logic. Your test doesn’t test that the filter works on pages that return Json, only that you’re not using an obsolete Authorize filter. I contend that this is unnecessary and contrary to the idea that we should generally prefer composition over inheritance.

        The SuppressAjaxAuthenticationRedirectAttribute should run and do the check for AJAX after the Authorization filter runs and returns a HttpUnauthorizedResult, regardless of the fact that the Action’s method never runs. It only needs to run before the FormsAuthenticationModule does the translation of the 401 to a 301.

        Another way of putting this is that we define a new convention (Supress…Attribute) that causes all unauthorized results to ignore the default convention in the forms authentication module for AJAX requests (401->301). If you apply the convention as a global filter that runs on every request, there is no need to do it per controller / action, and nothing for a developer to forget. It is an orthogonal concern to each authorization. This solves the ‘Power of Defaults’ problem that you mention, and doesn’t couple the Authorization failure handling (401/301) to the Authorization validating (user/role check).

        You can still have a unit test that checks that we’ve overridden the convention in a particular app, but it becomes an extremely simple one liner GlobalFilterCollection.ShouldContainInstanceOf().

        I’d also suggest that it might be neat to apply this convention to every new MVC app, and perhaps include it in the framework.

        • http://gurustop.net/ @Meligy

          In the previous reply I referred to the problem of not knowing whether the extra Attribute you suggetsed was required or not (because this depends on whether the Action returns JSON).

          Now that I think about it, maybe a better way was to still use another Attribute, and check (stillusing a Unit Test) that you are always using it if you are using the Authorize Attribute (which is a composition way of the same thing).

          Another better way is applying it globally. I’m all in for this really, and I think is the way to go. This should always be the case (since we check whether this is an AJAX request or not anyway, so, it doesn’t harm to run in all `Action()`s). Then you don’t even worry about checking it.

          I actually didn’t come with the original solution, and while I accepted it in our codebase (it was quite a late change and didn’t want to spend time arguing about it. Considered it “good enough”), I just wanted to prevent the obvious flaw in it via this test, then later thought that the idea of enforcing conventions via tests might be a good topic for a blog post, although addmittly this is not the best example (or maybe it is, as it shows you should try to avoid being in this situation in the first place).