Thursday, December 1, 2011

RequireHttpsAttribute breaks my app in Cassini

I was reading blog entry yesterday by Troy Hunt on Transport Layer protection. Beyond being a good reminder of some basic security practices, it reminded me of a little trick I use to get a site to require secure HTTP for production and staging installs, but still be able to debug it locally using Cassini (which doesn’t support HTTPS).  An alternative way to run your app locally would be to use IIS Express with an actual certificate, self-signed or otherwise, but this way is relatively simple and has the added advantage that you know that the attribute is applied globally.
I take advantage of GlobalFilters introduced in MVC 3.  The default template for your Global.asax.cs file has a method called RegisterGlobalFilters.  I’ve cloned this method as:

public static void RegisterProductionOnlyFilters( GlobalFilterCollection filters )
{
    filters.Add( new RequireHttpsAttribute() );
}

This method is then called in Application_Start() as follows:

var requestIsLocal = false;
try
{
    requestIsLocal = this.Context.Request.IsLocal;
}
// if the request isn't available, we catch the exception and know we're on prod/staging
catch (NullReferenceException) { }
 
if (!requestIsLocal)
{
    RegisterProductionOnlyFilters( GlobalFilters.Filters );
}

Now the RequireHttpsAttribute is applied as a global filter, but only when not running on localhost. To complete security, we use Web.config transformations to add requireSSL=”true” to our auth and session cookies (all cookies created server-side, that is).
<system.web>
    <authentication mode="Forms">
        <forms requireSSL="true" xdt:Transform="SetAttributes(requireSSL)" />
    </authentication>
    <httpCookies requireSSL="true" xdt:Transform="InsertAfter(/configuration/system.web/authentication)" />
</system.web>

Voila, our actions and cookies are now protected, but only when running in our production and staging environments.


Monday, October 24, 2011

Storing documents in a database using EntityFramework Code First

Our team has been developing a workflow application that allows our Board of Regents to submit requests for information to one or more Regents institutions and allow those institutions to respond back in return.  There are a number of other requirements, but one I'd like to focus on is the need to allow the Board to upload a document that one or more institutions must download, fill in, then upload as their response. We expose the data in the application via an API that each institution can use with their own workflow applications to process requests for information internally.  We've also developed a companion application that our institution will use internally and make available to the other institutions as desired. The applications serve a limited number of users and has relatively light data requirements so we chose a database-centric approach for simplicity.
We're using EntityFramework Code First (actually, database first, code first, but that's a different post). We've been through a couple of iterations on the approach and I thought I'd share our lessons learned.

First Approach
Our first approach was to use a single table per type holding both metadata and content for each type of attachment.  In our case we have three different types of documents, referred to internally as attachments, since that is how they are delivered via our web site: request attachments, response attachments, and a final response or "letter" attachment.  Using a single-table-per-type approach allows us to easily model the relationships between the main entity (Request/Response) and their related attachments (RequestAttachment and RequestLetter, and ResponseAttachment, respectively).  Even though the table schemas are identical, we segregate them into separate tables in order to keep the relationships and the models simple.
This approach is pretty clean.  On the model side we can refactor the code into a shared base class that all of the attachment entity models can derive from.  This fits in well with our standard model as well, in which we use an artificial primary key (identity) ID column and a Version (timestamp) column for concurrency control.  To be honest, we didn't really give this a lot of thought because it seemed a relatively straightforward mapping.
In developing the first, Regents, application our model seemed to work very well.  There were hints, however, that our choice might not be optimal.  First, we only need the attachment metadata to render links to the attachments in our views.  However, because of the model we're forced to either have two models per table, one for metadata-only and the other for the entire model, or we must load all of the data for each request.
Second, our "list" screens don't even reference the attachments, but our API needs to have a mechanism to "list" all requests to support synchronization between the main and client apps.   Again, we have a choice: our repository can be more complex, supporting different queries for different types of lists or we can use the same queries, but in some cases load extraneous data.  We chose to use a simpler repository and eager load attachments along with requests for information.  During development of the initial application, this seemed like a reasonable choice because the attachments tend to be small and the number of requests per "list" screen is small.  Better to do one request and get more information than needed than fire off many requests for a small amount of data was our thought.

Cracks Appear
At the point where we started on the synchronization support in our API, it became apparent that this approach was not going to work.  To reduce the chance of inconsistent data between client and master applications, we minimize the amount of data stored in the client application, pulling the details from the master application as necessary.  To ensure consistency, client applications, at least ours, will periodically synchronize the glue that connects entities in the client with their corresponding entities in the master application.
Unfortunately our approach to attachments meant that whenever this synchronization occurred, all of the attachments for all requests we being eagerly loaded.  The obvious fix for this was to back off to lazy loading, but this is less optimal for the master application, forcing it to make more requests whenever a "list" view is generated.  What we'd really like is to eager load the attachment metadata, but lazy load the attachment content since it is only needed at the point where an actual download of a particular attachment occurs.

Current Solution
The approach that we settled on (for now) is to use one table per type and one shared table for all content.  The first table includes the attachment metadata, including a foreign keys to the content and the related request or response (it acts as a join table with additional metadata).  The shared table contains only the value of the content.  The attachment metadata (join table) is eagerly loaded for most queries, but the attachment content is not.  The attachment content is only loaded when handling a download request for an attachment.

Some Notes

There are a couple of things that you should be aware of before adopting this approach.  First, if your data is larger than ours it might be better to use either a hybrid DB/file system approach or investigate using FileStream data in MSSQL.  You can find a nice introductory article by Jacob Sebastian at SimpleTalk and Paul S. Randal has a nice white paper on MSDN on using FileStream data.  Our solution fits into the sweet spot for table-based attachment storage.  Yours might not and the extra complexity of a hybrid or FileStream solution might be worth the effort. Second, all of this discussion is predicated on an ASP.NET MVC architecture and particularly one supporting an API used to synchronize with external applications.  I think the approach we are using is better than our original even for a single application, but we didn't really discover the original's drawbacks until we implemented the API.

Please let me know if you have any suggestions for how we could improve on this or if you've found it helpful.

Friday, May 13, 2011

Sharing Razor Functions Across Views

Scott Guthrie wrote recently about using the @helper syntax within Razor, part of which was how to share these helpers across views.  I had a couple of functions defined in my Razor views that were repeated in various views and wondered if I could get this to work as well.  Turns out that it’s dead simple to do.
First, add an App_Code folder to your project (ok I know this seems like a throw-back, don’t blame me).  Next create a partial view named Razor.cshtml (or something else) in the App_Code folder.  Put your function into this folder as a static method.
@using System.Web.Mvc;

@functions
{
      public static  MvcHtmlString Checked<T>( IEnumerable<T> collection, T item)
      {
          if (collection != null && collection.Contains( item ))
          {
              return new MvcHtmlString( "checked=\"checked\"" );
          }
          return new MvcHtmlString( string.Empty );
      }  
}
Compile your solution, then open the view where you want to use the function. Add the code, using the name of the view as the class for the method
<div class="editor-label">
    @Html.LabelFor( model => model.RoleID )
</div>
<div class="editor-field">
    @foreach (var role in Model.AvailableRoles)
    {
        <div class="role-selections">
            <label>
                 <input type="checkbox"
                             name="RoleID"
                             value="@role.ID"
                             @Razor.Checked(Model.RoleID,role.ID) />
                 @role.Name.MakeTitle()
            </label>
        </div> 
    }
    <div>
    @Html.ValidationMessageFor( model => model.RoleID )
    </div>
</div>

It's important to make sure you open the view after the solution has been compiled or the new class might not be visible. Note, in the process of developing this particular example I realized that this code could easily be made into an HtmlHelper and I've since refactored to that, but it's still a good technique for making common Razor functions available across views.

Tuesday, April 26, 2011

Default authorization filter provider

Phil Haack recently posted about Conditional Filters in ASP.NET MVC3.  It turned out to be a very timely post because I happen to have a need that nearly matches the example in his introduction.  Nearly every action, except Login, in my current application requires that the user be authenticated.  Some actions require further authorization processing, but just about everything requires that you be logged in.

In the past I would have built a base controller, applied the default Authorize attribute to the controller, then derived everything but my AccountController (where login lives) from the base controller.  This seemed like the safest way to ensure that all my actions are protected, but anytime I have a controller that has mixed public/private actions, I’m limited.  I can’t use the base controller or I have to introduce two base controllers – one with the common code and another that derives from it, but enforces authorization.  In addition, in each of these controllers with mixed actions I have to remember to protect access rather than permit access in exceptional cases.  Clearly the latter would be more secure since, as I age, I get more forgetful.

Now, using a custom FilterProvider, I can get the behavior that I prefer: a default AuthorizeAttribute [filter] applied to each action UNLESS the controller or action has some other, specific AuthorizationAttribute-based “restriction” applied.  I say “restriction” because it could be that I make the action public rather than further restrict access.  It only takes three simple steps:

First, create a DefaultAuthorizationFilterProvider.  Using Phil’s example as a template I created the following class.  Note that it implements IFilterProvider.  In GetFilters, it inspects both the controller (from the ControllerContext) and the action (from the ActionDescriptor) to see if either have an attribute that derives from AuthorizeAttribute.  If so, it simply returns an empty set of filters.  If not, it adds a default AuthorizeAttribute to the set of filters with global scope.  This will ensure that an something that derives from AuthorizeAttribute is supplied as a filter to every action.

public class DefaultAuthorizationFilterProvider : IFilterProvider
{
    public IEnumerable<Filter> GetFilters( ControllerContext controllerContext, ActionDescriptor actionDescriptor )
    {
        var filters = new List<Filter>();
        var controllerType = controllerContext.Controller.GetType();
        if (!controllerType.GetCustomAttributes( typeof( AuthorizeAttribute ), true ).Any()
            && !actionDescriptor.GetCustomAttributes( typeof( AuthorizeAttribute ), true ).Any())
        {
            filters.Add( new Filter( new AuthorizeAttribute(), FilterScope.Global, null ) );
        }
        return filters;
    }
}

Next, just as in Phil’s post, add this FilterProvider to the collection of filter providers in the Application_Start method of Global.asax.cs.  Ours is a bit simpler because, frankly, the use case is simpler.  We don’t have complex conditions to map, just check if there is a attribute or not, we can live with the default constructor and haven’t got any set up to do.
FilterProviders.Providers.Add( new DefaultAuthorizationFilterProvider() );

Finally, we want to be able to easily permit public access.  To do that, I created a PublicAttribute class that derives from AuthorizeAttribute and overrides AuthorizeCore to simply return true for every authorization request.  Yes, you can still set it up with Roles and Users, but they will be ignored.  I can live with that.
[AttributeUsage( AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true )]
public class PublicAttribute : AuthorizeAttribute
{
    protected override bool AuthorizeCore( System.Web.HttpContextBase httpContext )
    {
        return true;
    }
}

Now, to allow public access to an action we need to apply the PublicAttribute to the method (or controller). Note that in the following only the Login methods (GET and POST) are public.  You still need to be logged in to Logout (maybe we could make this public? meh) or to change your password.  And, look, we get to inherit from our BaseController, taking advantage of any common code or dependencies that we may have!

public class AccountController : BaseController
{
    [HttpGet]
    [Public]
    public ActionResult Login()
    {
        ...
    }

    [HttpPost]
    [ValidateAntiForgeryToken]
    [Public]
    public ActionResult Login( LoginModel model, string returnUrl )
    {
       ...
    }

    [HttpGet]
    public ActionResult Logout()
    {
        ...
    }

    [HttpGet]
    public ActionResult ChangePassword()
    {
        ...
    }

    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult ChangePassword( ChangePasswordModel model )
    {
        ...
    }
}

Monday, March 21, 2011

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();   
}

Tuesday, March 15, 2011

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.