Testing for Exceptions and Expectations

One of the things that I frequently have need to do is make sure that my methods throw proper exceptions when exceptional conditions occur.  .NET unit testing frameworks support typically support this using attributes.  For example:

[Test]
[ExpectedException(typeof(ArgumentNullException))]
public void ShouldThrowAnArgumentNullExceptionWhenANullParameterIsSupplied()
{
    var foo = new Foo();
    
    foo.Bar( null );
}

This works wonders for simple cases, but what about those cases where I want to make sure that some expectations are met.  Because the verification code comes after the invocation of the method under test, it won’t get run when an exception is thrown.  One way around this is to set up your mock objects and verify your expectations in your test initialize/cleanup methods.  This works because the cleanup code is run even for tests that throw exceptions.

private Bar MockBar { get; set; }

[TestInitialize]
public void MyTestInitialize()
{
    this.MockBar = MockRepository.GenerateMock<Bar>();
}

[TestCleanup]
public void  MyTestCleanup()
{
    this.MockBar.VerifyAllExpectations();
}

[Test]
public void ShouldThrowAnArgumentNullExceptionWhenANullParameterIsSupplied()
{
    // Baz should never be called when Bar is passed a null parameter
    this.MockBar.Expect( b => b.Baz() ).Repeat.Never();

    var foo = new Foo();

    foo.Bar( null );  
}

This works well when your tests share similar dependencies.  You could even extend this so that each test sets up its own or uses shared private methods to set up groups of expectations and the verification step checks to make sure that a mock object exists before it verifies it’s expectations.  This is a valid way solve the problem, but one I’ve found that can get excessively complicated and comes with code smells (you end up with a fair amount of conditional logic in the cleanup code).

An alternative to this is to not use the ExpectedException attribute, but to build the logic of catching and validating the exception into your test.

[Test]
public void ShouldThrowAnArgumentNullExceptionWhenANullParameterIsSupplied()
{
    var mockBar = MockRepository.GenerateMock<Bar>();

    // Baz should never be called when Bar is passed a null parameter
    mockBar.Expect( b => b.Baz() ).Repeat.Never();

    var foo = new Foo();

    try
    {
        foo.Bar( null );
        throw new AssertFailedException(); // test fails due to no exception thrown
    }
    catch (ArgumentNullException) // only catch the one exception we expect
    {
    }

    mockBar.VerifyAllExpectations();   
}

This is, in a word, ugly. It has the advantage of flexibility and keeping the expectation code specific and local to the current test. You could also, if you wanted, refactor to share expectation set up/verification between tests. There's still a code smell here: you're repeating a pattern whenever you want this type of test, but I think it's more in keeping with the single responsibility principle since you're cleanup code really is only concerned with clean up that necessarily affects all tests and isn't being abused to (selectively) run verification on each test's mocks. If only we could make it a little less ugly by encapsulating the pattern. Voila! A couple of helper classes to the rescue!

public class AssertionFailedException : Exception
{
    public AssertionFailedException() : base() { }
    public AssertionFailedException( string message )
        : base( message ) { }
}

public static class AssertException
{
    public static void IsThrown<T>( Action action ) where T : Exception
    {
        try
        {
            action();
            throw new AssertionFailedException( string.Format( "An expected exception of type {0} was not thrown", typeof(T).Name ) );
        }
        catch (T)
        {
        }
    }
}

Now we have a much cleaner way to assert that an exception has been thrown without polluting our clean up methods. In cases where it isn't appropriate, because our tests don’t share a lot of dependencies, to use the test initialization/cleanup methods to create and enforce our expecations, this mechanism works very well. As usual, you are still able to refactor your setup/verification for dependent objects as needed for tests that use the same code.

[Test]
public void ShouldThrowAnArgumentNullExceptionWhenANullParameterIsSupplied()
{
    var mockBar = MockRepository.GenerateMock<Bar>();

    // Baz should never be called when Bar is passed a null parameter
    mockBar.Expect( b => b.Baz() ).Repeat.Never();

    var foo = new Foo();

    AssertException.IsThrown<ArgumentNullException>( () => foo.Bar( null ) );

    mockBar.VerifyAllExpectations();   
}

Revisiting custom authorization in ASP.NET MVC

Several months ago I blogged about custom attributes based on the AuthorizeAttribute class in ASP.NET MVC. One of the bits that I got wrong or, at least, not as right as I would like, is caching. I didn’t extend the caching to allow it to take into account the new tests for authorization that I’d developed. The end result of this was that if you derived your permission to access the method based on the new test, you always got served a fresh page. It was never delivered from the cache.

In my application, this didn’t cause too much of a performance it. In fact, most of the actions protected in such a way are explicitly not cached anyway so the drawback rarely (if ever, I haven’t checked all the methods) came into play. I got asked about it recently on a StackOverflow answer so I thought I’d take a stab at improving the cache handling to account for the new tests.

There are a couple of things you need to be aware of. First, the code in OnCacheAuthorization must be thread-safe. Instances of the attribute are reused for multiple requests. In particular, this means that you cannot store information from the request in the attribute and expect it to remain inviolate during the life of the attribute. Another request may come along later and overwrite your stored request data and cause your code to return incorrect results. Since we’re talking about authorization this is a serious problem since it could lead to users getting access to information they should not be able to see.

Second, you need to be careful not to reuse the request data associated with the request that creates the cached entry. While there is a mechanism that allows you to pass data to the CacheValidationHandler method I created in the first post, you need to be careful to only pass data that actually applies to all requests, since you wouldn’t want to depend on the data being the same in the request that is served up from the cache. It may be an entirely different user making that request.

Our attribute still derives from AuthorizeAttribute, with the same modifications due to the fact that it is specific to a particular request and so doesn’t make sense to apply it at the class level or have multiples.

 AttributeUsage( AttributeTargets.Method, Inherited = true, AllowMultiple = false )]
 public class RoleOrOwnerAuthorizationAttribute : AuthorizationAttribute
 {
        private IDataContextFactory ContextFactory { get; set; }

        private string routeParameter = "id";
        /// <summary>
        /// The name of the routing parameter to use to identify the owner of the data (user id) in question.  Default is "id".
        /// </summary>
        public string RouteParameter
        {
            get { return this.routeParameter; }
            set { this.routeParameter = value; }
        }

        ...
}

The constructors, including reliance on my data context factory and its constructor injection, remain the same as well as the methods that I’ve refactored from the original AuthorizeAttribute. Notice, though, that I've made them virtual now in case we want to derive from this attribute.

protected IDataContextFactory ContextFactory { get; set; }

public RoleOrOwnerAuthorizationAttribute()
    : this( null )
{
}

protected virtual void CacheValidateHandler( HttpContext context, object data, ref HttpValidationStatus validationStatus )
{
    validationStatus = OnCacheAuthorization( new HttpContextWrapper( context ) );
}

 protected virtual void SetCachePolicy( AuthorizationContext filterContext )
{
    // ** 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 */);
}

Now I introduce a new, inner class CurrentRequest to help in abstracting the different between the authorization request that creates the cached copy and subsequent cache requests that need to be served from the cache, or not. It stores the OwnerID of the item being requested and the current Username of the user making the request. Our IsOwner method will be refactored to use this class instead of the AuthorizationContext for this data. Note that it has three constructors; the default constructor will be used for unit testing since I won’t always want to mock up either an AuthorizationContext or an HttpContextBase object in my tests.  The other constructors are used for requests made from OnAuthorization and OnCacheAuthorization, respectively.  Note the real difference is what context we have available to the code. In the first case we can use RouteValues to extract the OwnerID, in the second, we use the Request parameters directly.

public class CurrentRequest
{

    public CurrentRequest()
    {
        this.OwnerID = -1;
    }

    public CurrentRequest( AuthorizationContext filterContext, string routeParameter )
        : this()
    {
        if (filterContext.RouteData.Values.ContainsKey( routeParameter ))
        {
            this.OwnerID = Convert.ToInt32( filterContext.RouteData.Values[routeParameter] );
        }

        this.Username = filterContext.HttpContext.User.Identity.Name;
    }

    public CurrentRequest( HttpContextBase httpContext, string routeParameter )
    {
        if (!string.IsNullOrEmpty( routeParameter ))
        {
            if (httpContext.Request.Params[routeParameter] != null)
            {
                this.OwnerID = GetOwnerID( httpContext.Request.Params[routeParameter] );
            }
            else if (string.Equals( "id", routeParameter, StringComparison.OrdinalIgnoreCase ))
            {
                // id may be last element in path if not included as a parameter
                this.OwnerID = GetOwnerID( httpContext.Request.Path.Split( '/' ).Last() );
            }
        }

        this.Username = httpContext.User.Identity.Name;
    }

    private int GetOwnerID( string id )
    {
        int ownerID;
        if (!int.TryParse( id, out ownerID ))
        {
            ownerID = -1;
        }
        return ownerID;
    }

    public int OwnerID { get; set; }
    public string Username { get; set; }
}

NOTE: An earlier version of this post did not handle getting the route parameter from the HttpContext properly, if the route parameter was the "id" property.  This version relies on the default route set up to place the id parameter at the end of the path but, with that caveat, now handles an "id" parameter properly.

The OnAuthorization method changes slightly to use this new class. Instead of simply passing the AuthorizationContext, we pass a CurrentRequest object constructed from it using the RouteParameter on the attribute.

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

    if (AuthorizeCore( filterContext.HttpContext ))
    {
        SetCachePolicy( filterContext );
    }
    else if (!filterContext.HttpContext.User.Identity.IsAuthenticated)
    {
        // auth failed, redirect to login page
        filterContext.Result = new HttpUnauthorizedResult();
    }
    else if (filterContext.HttpContext.User.IsInRole( "SuperUser" ) || IsOwner( new CurrentRequest( filterContext, this.RouteParameter ) ))
    {
        SetCachePolicy( filterContext );
    }
    else
    {
        ViewDataDictionary viewData = new ViewDataDictionary();
        viewData.Add( "Message", "You do not have sufficient privileges for this operation." );
        filterContext.Result = new ViewResult { MasterName = this.MasterName, ViewName = this.ViewName, ViewData = viewData };
    }
}

This allows us to simplify IsOwner significantly as the CurrentRequest class now takes care of unpacking the data from the request. Note how we are able to use the ContextFactory (it’s thread-safe) to create a new data context within the method, ensuring that we don’t conflict with other threads that may be using the attribute. The data context itself needn’t be thread-safe since we create a new one each time and it’s local to the method.

private bool IsOwner( CurrentRequest requestData )
{
    using (var dc = this.ContextFactory.GetDataContextWrapper())
    {
        return dc.Table<User>().Where( p => p.UserName == requestData.Username && p.UserID == requestData.OwnerID ).Any();
    }
}

Lastly, we need to hook our IsOwner check into the authorization system.  We do this by overriding OnCacheAuthorization and doing our check when the base class method would normally invalidate the request because the user is not in the correct role. When they are allowed by the role, we needn’t perform our check.

protected override HttpValidationStatus OnCacheAuthorization( HttpContextBase httpContext )
{
    var status = base.OnCacheAuthorization( httpContext );
    if (status == HttpValidationStatus.IgnoreThisRequest && IsOwner( new CurrentRequest( httpContext, this.RouteParameter ) ))
    {
        status = HttpValidationStatus.Valid;
    }
    return status;
}

Note here that we use the constructor on CurrentRequest that takes an HttpContextBase (wrapper around HttpContext) instead because that's all we have available to use when being evaluated by the caching system. Because we've abstracted IsOwner to use this object, it doesn't need to know how we got the values that it will be checking.

Now the owners of the data will have the same cache-based performance as the administrative users who access the data via their roles.