3

our business has a number of sites that we manage and each of these sites have sites they are responsible for and so on. So everything is hierarchical as far as permissions go for our software too. If a person at site-X wants to edit stuff for site-X and any sub-site-X they should be allowed to. We also have applications roles, mainly admin, that will allow a person to edit everything as well as maintain the application.

I'm currently working on dealing with permissions for this application and I've got everything working, but I really hate it. Its clunky, not very testable and doesn't seem like its in the right place for my MVC application. I was hoping someone would have some thoughts on how I can refactor this code and make it most importantly more testable and perhaps make it a bit more usable.

Thank you in advance.

    public class OuController : BaseController {
    private readonly IOrganizationUnitRepository repo;

    public OUController(IOrganizationUnitRepository repo) {
      this.repo = repo;
    }

    public ActionResult Details(string site) {

      //Get the site we are viewing
      var ou = repo.GetOuByName(site);

      //make sure the site really exists
      if (ou != null) {

        //Get all the roles for the current user via the role provider
        //will return the sites they are able to manage along with
        //any application roles they have
        var roles = ((RolePrincipal)User).GetRoles().ToList();

        //Get all the parents of the current ou, this will include itself
        var parents = repo.GetParents(ou, new List<OU>());

        //create a new viewmodel object
        //ou is used for details obviously
        //parents are used for a breadcrumb
        var model = new OrganizationalViewModel(ou, parents);

        //if a user has no roles, there is no way he can possibly edit
        if (roles.Any()) {
          if(roles.Contains(InfoRoles.Administrator.ToString())) {

            model.CanEdit = true;

          } else if(parents == null) {

            //If there are no parents, check if this ou is in users list of roles
            model.CanEdit = roles.Contains(ou.DisplayName);

          } else {

            //check to see if any of the roles i have are parents of the current ou
            model.CanEdit = parents.Any(c => roles.Contains(c.DisplayName)); 

          }

        }

        return View("Details", model);

      }

      return View("NotFound");

    }
  }
}
Stecya
  • 22,896
  • 10
  • 72
  • 102

1 Answers1

2

Anything that looks like this:

((RolePrincipal)User).GetRoles().ToList()

... belongs in a class of its own (with an interface method like "GetCurrentRoles"), so it can be easily mocked.

Furthermore, this:

    //if a user has no roles, there is no way he can possibly edit
    if (roles.Any()) {
      if(roles.Contains(InfoRoles.Administrator.ToString())) {

        return true;

      } else if(parents == null) {

        //If there are no parents, check if this ou is in users list of roles
        return  roles.Contains(ou.DisplayName);

      } else {

        //check to see if any of the roles i have are parents of the current ou
        return  parents.Any(c => roles.Contains(c.DisplayName)); 

      }

... belongs in a utility class in a method called something like CanRolesEditOrganizationalView(IEnumerable<RolePrinciple> roles, ...). That way your controller can just say:

var roles = _sessionManager.GetCurrentRoles();
...
model.Edit = _orgViewRightsUtil.CanRolesEditOrganizationalView(roles, ...);
StriplingWarrior
  • 151,543
  • 27
  • 246
  • 315