best practice for authorization code repeating in a controller

Question

I have a controller that has method GetSignatories(), AddMe(), RemoveMe(), AddUser(), RemoveUser() and more to come in witch I have to verify every time if the contract is existing, if the user have access to the contract and if the requested version is existing. I'm wondering where should I put this code? In a Service or extracted in an other method of my controller? my problem is that I soometime return Unathorized or NotFound and don't know what would be the best way to do this.

Here's my controller :

public partial class ContractController : Controller
    {

        private ISession _session;
        private IAuthenticationService _authService;

        public V1Controller(ISession session, IAuthenticationService authService)
        {
            _session = session;
            _authService = authService;
        }

        [HttpPost]
        [Authorize(Roles = "User")]
        [ValidateAntiForgeryToken]
        public virtual ActionResult GetSignatories(string contractId, int version)
        {
            //NOTE Should be extracted
            var contract = _session.Single<Contract>(contractId);
            if (contract == null) return HttpNotFound();
            var user = _authService.LoggedUser();
            if (contract.CreatedBy == null || !contract.CreatedBy.Id.HasValue || contract.CreatedBy.Id.Value != user.Id) return new HttpUnauthorizedResult();
            if (contract.Versions.Count < version) return HttpNotFound();
            /*NOTE  END Should be extracted */

            return Json(contract.CurrentVersion.Tokens.Select(x => x.User).ToList());
        }

        [HttpPost]
        [Authorize(Roles = "User")]
        [ValidateAntiForgeryToken]
        public virtual ActionResult AddMe(string contractId, int version)
        {
            //NOTE Should be extracted
            var contract = _session.Single<Contract>(contractId);
            if (contract == null) return HttpNotFound();
            var user = _authService.LoggedUser();
            if (contract.CreatedBy == null || !contract.CreatedBy.Id.HasValue || contract.CreatedBy.Id.Value != user.Id) return new HttpUnauthorizedResult();
            if (contract.Versions.Count < version) return HttpNotFound();
            /*NOTE  END Should be extracted */

            return AddUserToContract(contract, new UserSummary(user));
        }

        [HttpPost]
        [Authorize(Roles = "User")]
        [ValidateAntiForgeryToken]
        public virtual ActionResult RemoveMe(string contractId, int version)
        {
            //NOTE Should be extracted
            var contract = _session.Single<Contract>(contractId);
            if (contract == null) return HttpNotFound();
            var user = _authService.LoggedUser();
            if (contract.CreatedBy == null || !contract.CreatedBy.Id.HasValue || contract.CreatedBy.Id.Value != user.Id) return new HttpUnauthorizedResult();
            if (contract.Versions.Count < version) return HttpNotFound();
            /*NOTE  END Should be extracted */

            return RemoveUserFromContract(contract, new UserSummary(user));
        }

        [HttpPost]
        [Authorize(Roles = "User")]
        [ValidateAntiForgeryToken]
        public virtual ActionResult AddUser(string contractId, int version, UserSummary userSummary)
        {
            //NOTE Should be extracted
            var contract = _session.Single<Contract>(contractId);
            if (contract == null) return HttpNotFound();
            var user = _authService.LoggedUser();
            if (contract.CreatedBy == null || !contract.CreatedBy.Id.HasValue || contract.CreatedBy.Id.Value != user.Id) return new HttpUnauthorizedResult();
            if (contract.Versions.Count < version) return HttpNotFound();
            /*NOTE  END Should be extracted */


            return AddUserToContract(contract, userSummary);
        }

        [HttpPost]
        [Authorize(Roles = "User")]
        [ValidateAntiForgeryToken]
        public virtual ActionResult RemoveUser(string contractId, int version, UserSummary userSummary)
        {
            //NOTE Should be extracted
            var contract = _session.Single<Contract>(contractId);
            if (contract == null) return HttpNotFound();
            var user = _authService.LoggedUser();
            if (contract.CreatedBy == null || !contract.CreatedBy.Id.HasValue || contract.CreatedBy.Id.Value != user.Id) return new HttpUnauthorizedResult();
            if (contract.Versions.Count < version) return HttpNotFound();
            /*NOTE  END Should be extracted */


            return RemoveUserFromContract(contract, userSummary);
        }
}

For those who might be looking how to register the model binder in the global :

public static void RegisterModelBinders()
    {
        var session = (ISession)DependencyResolver.Current.GetService(typeof(ISession));
        var authService = (IAuthenticationService)DependencyResolver.Current.GetService(typeof(IAuthenticationService));
        System.Web.Mvc.ModelBinders.Binders[typeof (Contract)] = new ContractModelBinder(session, authService);
    }

For those looking on how to unit test the ModelBinder here's a good start : http://stackoverflow.com/questions/1992629/unit-testing-custom-model-binder-in-asp-net-mvc-2 — VinnyG, Apr 09 '11 at 18:57

score 2 · Accepted Answer · answered Apr 09 '11 at 15:57

Indeed you have pretty repeating code. There are many ways to refactor this code, one of which consists of writing a custom model binder for the Contract model:

public class ContractModelBinder : DefaultModelBinder
{
    private readonly ISession _session;
    private readonly IAuthenticationService _authService;
    public ContractModelBinder(ISession session, IAuthenticationService authService)
    {
        _session = session;
        _authService = authService;
    }

    public override object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
    {
        string contractId = null;
        int version = 0;
        var contractIdValue = bindingContext.ValueProvider.GetValue("contractId");
        var versionValue = bindingContext.ValueProvider.GetValue("version");
        if (versionValue != null)
        {
            version = int.Parse(versionValue.AttemptedValue);
        }
        if (contractIdValue != null)
        {
            contractId = contractIdValue.AttemptedValue;
        }

        var contract = _session.Single<Contract>(contractId);
        if (contract == null)
        {
            throw new HttpException(404, "Not found");
        }
        var user = _authService.LoggedUser();
        if (contract.CreatedBy == null || 
            !contract.CreatedBy.Id.HasValue || 
            contract.CreatedBy.Id.Value != user.Id
        )
        {
            throw new HttpException(401, "Unauthorized");
        }

        if (contract.Versions.Count < version)
        {
            throw new HttpException(404, "Not found");
        }
        return contract;
    }
}

Once you have registered this model binder with the Contract model in Application_Start your controller simply becomes:

public class ContractController : Controller
{
    [HttpPost]
    [Authorize(Roles = "User")]
    [ValidateAntiForgeryToken]
    public ActionResult GetSignatories(Contract contract)
    {
        return Json(contract.CurrentVersion.Tokens.Select(x => x.User).ToList());
    }

    [HttpPost]
    [Authorize(Roles = "User")]
    [ValidateAntiForgeryToken]
    public ActionResult AddMe(Contract contract)
    {
        var user = contract.CreatedBy;
        return AddUserToContract(contract, new UserSummary(user));
    }

    [HttpPost]
    [Authorize(Roles = "User")]
    [ValidateAntiForgeryToken]
    public ActionResult RemoveMe(Contract contract)
    {
        var user = contract.CreatedBy;
        return RemoveUserFromContract(contract, new UserSummary(user));
    }

    [HttpPost]
    [Authorize(Roles = "User")]
    [ValidateAntiForgeryToken]
    public ActionResult AddUser(Contract contract, UserSummary userSummary)
    {
        return AddUserToContract(contract, userSummary);
    }

    [HttpPost]
    [Authorize(Roles = "User")]
    [ValidateAntiForgeryToken]
    public ActionResult RemoveUser(Contract contract, UserSummary userSummary)
    {
        return RemoveUserFromContract(contract, userSummary);
    }
}

We successfully put it on a diet.

Very short code indeed, but are you really sure the model binder is the right place for this? After all there is a lot of business logic. Putting it in a service class seems like a more common approach to me... — Adrian Grigore, Apr 09 '11 at 16:11
@Adrian Grigore, there's strictly 0 business logic. This is authorization logic and indeed an authorize filter might have been another possible solution but the advantage of a model binder is that you get the results in your controller actions so that you can directly use them. If you use a service you will still have lots of repeating logic in those actions and they won't be DRY enough. — Darin Dimitrov, Apr 09 '11 at 16:13
it seems like the best solution, how would you handle the HttpException? Where do you but the binder in the project folder? — VinnyG, Apr 09 '11 at 16:14
@VinnyG, personally I use a custom exception handler in Global.asax: http://stackoverflow.com/questions/4911212/how-to-implement-proper-http-error-handling-in-net-mvc-2/4912456#4912456 Because the model binder throws the proper HttpException the ErrorsController will take care of the rest. And you could place the binder for example in a `ModelBinders` folder within your project. — Darin Dimitrov, Apr 09 '11 at 16:15
I know it's not related to this question but what is the advantage of using private readonly ISession _session over not a readonly? — VinnyG, Apr 09 '11 at 16:35
@VinnyG, the advantage is that you are helping the compiler enforce semantics and allow him probably to optimize this code as it knows that the field will never be assigned a value in other place than the constructor. — Darin Dimitrov, Apr 09 '11 at 16:36
@VinnyG - making a field `readonly` makes it a runtime constant, i.e. it can only be assigned to once in the constructor of the class that contains the field. — Sergi Papaseit, Apr 09 '11 at 16:38
I stand corrected. The first time I skimmed through the OP, it seemed like this is about business logic. But you are right, it isn't really. — Adrian Grigore, Apr 09 '11 at 16:53
@VinnyG, yes and yes :-) It will be pretty easy because the binder takes only abstractions in its constructor that could be mocked in your unit test. — Darin Dimitrov, Apr 09 '11 at 17:20
How do I register my binder? I added my global code and I use Ninject but don't know how to do this, thanks again! — VinnyG, Apr 09 '11 at 17:35
@VinnyG, checkout Brad Wilson's blog post about the new [`IModelBinderProvider`](http://bradwilson.typepad.com/blog/2010/10/service-location-pt9-model-binders.html) and a sample [Ninject implementation](http://www.mauilse.nl/archive/2011/03/09/creating-an-imodelbinderprovider-in-mvc3-that-uses-dependency-injection.aspx). — Darin Dimitrov, Apr 09 '11 at 17:43
Now I need to do almost the same but I want to verify if the user is in the concerned users, not if it's the owner of the document, do I create a new binder? How should I name it? — VinnyG, Apr 10 '11 at 19:20

score 2 · Answer 2 · edited Nov 06 '13 at 20:46

One possible solution would be to create an IContractService interface that has two methods, one to get the contract and another to validate it:

public IContractService
{
    Contract GetContract(int id);
    ValidationResult ValidateContract(Contract contract);
}

ValidationResult could be an enumeration, just to signal the caller of the method how to proceed:

public enum ValidationResult
{
    Valid,
    Unauthorized,
    NotFound
}

A possible Implementation of IContractService:

public class ContractService : IContractService
{
    private readonly ISession _session;
    private readonly IAuthenticationService _authService;

    // Use Dependency Injection for this!
    public ContractService(ISession session, IAuthenticationService authService)
    {
       _session = session;
       _authService = authService;
    }

    public Contract GetContract(int id)
    {
        var contract = _session.Single<Contract>(contractId);

        // hanlde somwhere else whether it's null or not
        return contract;
    }

    public ValidationResult ValidateContract(Contract contract)
    {
        var user = _authService.LoggedUser();
        if (contract.CreatedBy == null || !contract.CreatedBy.Id.HasValue ||
            contract.CreatedBy.Id.Value != user.Id) 
              return ValidationResult.Unauthorized;

        if (contract.Versions.Count < version) 
            return ValidationResult.NotFound;

        return ValidationResult.Valid;
    }
}

Then in your controller you can still return the various HttpNotFound, etc to the view:

[HttpPost, Authorize(Roles = "User"), ValidateAntiForgeryToken]
public virtual ActionResult GetSignatories(string contractId, int version)
{
    //NOTE Should be extracted
    var contract = _contractService.GetContract(contractId);

    if (contract == null) 
        return HttpNotFound();

    var result = _contractService.ValidateContract(contract);

    if (result == ValidationResult.Unauthorized) 
        return new HttpUnauthorizedResult();

    if (result == ValidationResult.NotFound) 
        return HttpNotFound();

    return Json(contract.CurrentVersion.Tokens.Select(x => x.User).ToList());
}

how would you do the GetSignatories() Method in my controller? If I understand it, the service would only replace the 3 last lines and I would have as much code in my controller? — VinnyG, Apr 09 '11 at 16:24
That is correct actually. This mainly moves the validation logic out of the controller, where it doesn't belong. Returning `ActionResult` to the view is still job of the controller though. I'll update my answer to include the `GetSignatoires` action. — Sergi Papaseit, Apr 09 '11 at 16:32
I think both solution is good but I prefer Darin's since I have less code. Thanks for helping! — VinnyG, Apr 09 '11 at 17:10

best practice for authorization code repeating in a controller

2 Answers2