Best practice for authorization code repeating in controller

Question

I have a controller that has the GetSignatories (), AddMe (), RemoveMe (), AddUser (), RemoveUser () and others method to enter witch. I have to check every time if the contract exists, if the user has access to the contract and if the requested version exists. I am wondering where should I put this code? In the Service or retrieved in another method of my controller? my problem is that I return Unathorized or NotFound so quickly and don’t know what would be the best way to do this.

Here is 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 can see how to register model bindings globally:

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

Answer 1

Indeed, you have pretty duplicate code. There are many ways to reorganize this code, one of which is to write a custom middleware 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 Contract model middleware 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 have successfully put it on a diet .

Answer 2

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

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

ValidationResult can be an enumeration, just to signal to the calling method how to proceed:

 public enum ValidationResult { Valid, Unauthorized, NotFound } 

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 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 various HttpNotFound , etc. in 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()); }