Reduce Duplicated Code in Controller Actions

Question

To allow users and site admins to view/add/edit/delete data in my application I decided on this route:

routes.MapRoute("ClientRoute",
       "{account}/{controller}/{action}/{id}",
       new { controller = "Home", action = "Index", id = "" });

which results in routes like: mvcapp.net/1234/contact/add.

To keep users {except admins} from accessing other client's data I have added the following code in my controller actions.

...
   var model = repos.GetSomeData();
   if (User.IsInRole("Admin") == false) {
    if (account == Profile["Client"])
       return View(model);
    else
       return View("WrongClient");
    }
...

What is the best way to do this?

SOLUTION I WENT WITH

public class BaseController : Controller {
  protected override OnActionExecuting(ActionExecutingContect filterContext) {
      if (filterContext.RouteData.Values["account"] != null) {
         string client = filterContext.RouteData.Values["account"].ToString();
         if (User.IsInRole("admin") == false) {
            if (Profile.Clients.Contains(account) == false)
              filterContext.Result = new ViewResult() {ViewName = "WrongClient"};
          }
      }
  }
}

Answer 1

You can write your repository methods so that they only return client data for the appropriate account number. Just pass the account number to the repository method.

If you're concerned about passing user information into the repository method (as Jabe discusses in his comment below), then you can return an IQueryable from the repository, and run a Linq query against that to do your security trimming.

Answer 2

For this specific example, Azam Sharp has a possible solution on his blog. I literally stumbled across this article no more than five minutes ago. Hope it helps!

Answer 3

Is it actually necessary to have the client be part of the route considering you already have that information at your disposal from the profile?

As far as handling this it the repository level (as mentioned above) - this can get a little tricky, as could possibly prevent access to data needed internally for some business process as well as not letting a user access it. Of course, you could create separate filtered/unfiltered methods to handle this. Also, this may be the way to go no intermixing of client data is ever permissible.

Most of the time in our application, it is only the top level item that is being added/inserted/deleted which needs to have access restricted in some fashion. It is also only a limited set of data that needs restricted in this fashion, so I usually write code that throws an exception of some kind right in the controller should the access rules for that item be violated.

If the rules for your app are very similar there are a number of ways you can keep from repeating the code. A custom ActionFilter or custom Controller base class both come to mind. If each row in your db has a client id, or some such scheme, another option would be to have your domain objects each implement an interface exposing this id. You can then write reusable code (mixin-style extension methods, etc...) that uses this interface as a basis for applying your various security rules.

Answer 4

I'm not sure what Profile["AccountNumber"] is doing (or where Profile comes from), but assuming you can create that object anytime (or if its already created);

You can do the following (place this in your controller):

protected override void ExecuteCore()
{
    var model = repos.GetSomeData(int.Parse(base.RouteData.Values["client"])));
    if (User.IsInRole("Admin") == false && Profile["AccountNumber"].ToString() != model.AccountNumber)
    {
          ViewData["Error"] = "You can't access this page";
          View("WrongClient").ExecuteResult(ControllerContext);
    }
    else
          base.ExecuteCore();

}

You can put this in all the controllers you need, or have the controllers inherit a base controller class that implements this function.

Idea taken from: http://forums.asp.net/t/1382514.aspx