is there a better way to implement role based acton and view in mvc than if/else?

Question

So I have three roles, administrators, companies and employees in my mvc .net application that uses asp.net membership in a separate database. I moved the .net membership in a different database for now because everytime I modify the model, the .net membership tables are getting deleted.

Anyway, I am handling different roles using if/else in the action method. For example, in Index() action, I check if the user is in administrators role, then create model and linq query based on that. If user in companies role, different query and if user in employees role, different query. check code below. The model created after the if condition is passed to the View.

I feel like this is not the best way to handle roles. Is this the best way to handle roles? I am considering different areas as well, but I use same views for the different roles, i think it may not be productive.

Any suggestion/idea greatly appreciated.

[Authorize]
    public class CompanyController : Controller
    {
        private MyDBContext db = new MyDBContext();

        //
        // GET: /Company/

        public ViewResult Index()
        {
            var viewModel = new CompanyIndexViewModel();
            if (Roles.IsUserInRole("administrators")) {
                viewModel = new CompanyIndexViewModel { Companies = db.Companies.ToList() };
            }
            else if (Roles.IsUserInRole("companies")) {
                viewModel = new CompanyIndexViewModel { Companies = db.Companies.Where(c => c.Username.ToLower().Equals(this.User.Identity.Name.ToLower())).ToList() };
            }
            else if (Roles.IsUserInRole("employees")) {
                string userName = this.User.Identity.Name.ToLower();
                var companies = db.Companies.Where(c => c.Appointments.Any(a =>
                                   a.Employee.Username.ToLower() == userName)).ToList();
                viewModel = new CompanyIndexViewModel { Companies = companies.ToList() };
            }

            return View(viewModel);
        }
....

Answer 1

There is two things I would do:

Firstly, what StanK said and move it out of the controller action. However, I would move it out of the Controller all together. This sort of logic shouldn't really reside in the Controller to begin with (whether it be in an action, or a private method in the controller).

Think about it this way: What if your logic for who gets to see what companies changes.. you'll have to change it in all sorts of different places.

Secondly, I would create a constructor for the CompanyIndexViewModel that took in a list of Company instead of initializing it inline like that. Does the CompanyIndexViewModel contain anything else besides companies?

// your controller
public ViewResult Index()
{
    var viewModel = CompanyIndexViewModel(CompanyService.GetCompaniesForCurrentUser());
    return View(viewModel);
}

Ideally, you would also have your controller depend on an interface representing the "CompanyService", and have that injected into your controller.

Take a look at this blog which outlines using Ninject with MVC 3. It's ridiculously simple to setup for something that is so powerful for you later on.

If you take one thing away from what I've said above, it's probably best to start with moving your logic out of the controller.

Answer 2

I would move the code that builds the list of Companies to it's own method to tidy up the controller action , which would also make the logic that determines the list of Companies for the current user re-useable.

e.g.

private List<Company> GetCompaniesForCurrentUser()
{
    var userName = this.User.Identity.Name.ToLower();

    if (Roles.IsUserInRole("administrators"))
        return db.Companies.ToList();

    if (Roles.IsUserInRole("companies"))
        return db.Companies.Where(c => c.Username.ToLower().Equals(userName)).ToList();

    if (Roles.IsUserInRole("employees"))
        return db.Companies.Where(c => c.Appointments.Any(a =>
        a.Employee.Username.ToLower() == userName)).ToList();

    throw new AuthorizationException("User " + userName + " is not authorised.");

}


public ViewResult Index()
{
    var viewModel = new CompanyIndexViewModel { Companies = GetCompaniesForCurrentUser() };
    return View(viewModel);
}