2

Currently I have my code for Membership Roles in C# structured like this below

@if (WebApiApplication.CurrentUser.CurrentRole == Role.SysAdmin || WebApiApplication.CurrentUser.CurrentRole == Role.Coordinator)
{
   <li id="HomeMenu" class="@Model.GetMenuClass("HomeMenu")"><a href="@Url.Action("Index", "Home")"><i class="fa fa-home"></i>&nbsp;&nbsp;Home</a></li>
   <li id="InternsMenu" class="@Model.GetMenuClass("InternsMenu")"><a href="@Url.Action("Index", "Interns")"><i class="fa fa-user"></i>&nbsp;&nbsp;Interns</a></li>
   <li id="ProjectsMenu" class="@Model.GetMenuClass("ProjectsMenu")"><a href="@Url.Action("Index", "Projects")"><i class="fa fa-list-alt"></i>&nbsp;&nbsp;Projects</a></li>
   <li id="AssignmentsMenu" class="@Model.GetMenuClass("AssignmentsMenu")"><a href="@Url.Action("Index", "Assignments")"><i class="fa fa-paperclip"></i>&nbsp;&nbsp;Assignments</a></li>
}
@if (WebApiApplication.CurrentUser.CurrentRole == Role.User )
{
   <li id="HomeMenu" class="@Model.GetMenuClass("HomeMenu")"><a href="@Url.Action("Index", "Home")"><i class="fa fa-home"></i>&nbsp;&nbsp;Home</a></li>
   <li id="InternsMenu" class="@Model.GetMenuClass("InternsMenu")"><a href="@Url.Action("Index", "Interns")"><i class="fa fa-user"></i>&nbsp;&nbsp;Interns</a></li>
   <li id="ProjectsMenu" class="@Model.GetMenuClass("ProjectsMenu")"><a href="@Url.Action("Index", "Projects")"><i class="fa fa-list-alt"></i>&nbsp;&nbsp;Projects</a></li>
   <li id="AssignmentsMenu" class="@Model.GetMenuClass("AssignmentsMenu")"><a href="@Url.Action("Index", "Assignments")"><i class="fa fa-paperclip"></i>&nbsp;&nbsp;Assignments</a></li>
}

@if (WebApiApplication.CurrentUser.CurrentRole == Role.Intern)
{
    <li id="HomeMenu" class="@Model.GetMenuClass("HomeMenu")"><a href="@Url.Action("Index", "Home")"><i class="fa fa-home"></i>&nbsp;&nbsp;Home</a></li>
    <li id="TimecardMenu" class="@Model.GetMenuClass("TimecardMenu")"><a href="@Url.Action("Timecard", "Assigments")"><i class="fa fa-user"></i>&nbsp;&nbsp;Interns</a></li>
    <li id="FeedbackMenu" class="@Model.GetMenuClass("ProjectsMenu")"><a href="@Url.Action("Feedback", "Assigments")"><i class="fa fa-list-alt"></i>&nbsp;&nbsp;Projects</a></li>
    <li id="SupportMenu" class="@Model.GetMenuClass("SupportMenu")"><a href="@Url.Action("Index", "Support")"><i class="fa fa-question-circle"></i>&nbsp;&nbsp;Support</a></li>
}

What would be a better way to handle Roles logic instead of in Razor View ? Doing it in the C# model?

Timothy Fisher
  • 185
  • 1
  • 11
  • I do see it uses like this on this page http://www.asp.net/web-pages/overview/security/16-adding-security-and-membership – Timothy Fisher Jul 08 '16 at 01:13
  • 2
    That code should not be in a view. Your logic should be in the controller and you pass an appropriate model to the view (a collection of menu items) and use a simple loop to generate the links –  Jul 08 '16 at 01:15
  • Well, IMO you are just repeating yourself over and over with just some different links for some role(s) You can certainly do this but just think about maintaining it in the SDLC as time goes on. – Tom Stickel Jul 08 '16 at 01:16
  • @StephenMuecke Is there an example of this on the web that i can look at that you know of? – Timothy Fisher Jul 08 '16 at 01:17
  • I wouldn't put the roles directly into the view like that; I'd create a model property like `ShouldShowXXXX` (or similar). In the controller you'd check for roles and assign to the appropriate `ShouldShowXXX`s accordingly. – Kenneth K. Jul 08 '16 at 01:24
  • look for examples like http://stackoverflow.com/questions/21948909/asp-net-mvc-controller-for-layout – Tom Stickel Jul 08 '16 at 01:24
  • 1
    Just create a class with properties you need to generate the menu items (e.g. `string DisplayName`, `string ControllerName`, `string `ActionName` etc. And in the controller generate a collection of the class based on the role and pass that collection to the view to generate the links in a loop. –  Jul 08 '16 at 01:31
  • To add to what Stephen said, doing it that way makes it much easier to test because now all your logic is in a controller action and not in the view. – Fran Jul 08 '16 at 01:55

1 Answers1

0

I think something like this is OK to handle in the View as it handles the display of the menu items in your page.

As others mentioned above in the comments, it is a good idea to define properties in your view model that map to users roles and use that instead.

You could use a switch statement instead and group the options that apply to different roles. For example, I did noticed in your example that roles SysAdmin, Coordinator and User all have the same menu items, all roles have access to the Home menu and only role Intern should see the TimeCard, Feedback and Support menus.

See example below:

<li id="HomeMenu" class="@Model.GetMenuClass("HomeMenu")"><a href="@Url.Action("Index", "Home")"><i class="fa fa-home"></i>&nbsp;&nbsp;Home</a></li>

    @switch (WebApiApplication.CurrentUser.CurrentRole)
        {
          case Role.SysAdmin:
          case Role.Coordinator:
          case Role.User:
            <li id="InternsMenu" class="@Model.GetMenuClass("InternsMenu")"><a href="@Url.Action("Index", "Interns")"><i class="fa fa-user"></i>&nbsp;&nbsp;Interns</a></li>
            <li id="ProjectsMenu" class="@Model.GetMenuClass("ProjectsMenu")"><a href="@Url.Action("Index", "Projects")"><i class="fa fa-list-alt"></i>&nbsp;&nbsp;Projects</a></li>
            <li id="AssignmentsMenu" class="@Model.GetMenuClass("AssignmentsMenu")"><a href="@Url.Action("Index", "Assignments")"><i class="fa fa-paperclip"></i>&nbsp;&nbsp;Assignments</a></li>
            break;
          case Role.Intern:
            <li id="TimecardMenu" class="@Model.GetMenuClass("TimecardMenu")"><a href="@Url.Action("Timecard", "Assigments")"><i class="fa fa-user"></i>&nbsp;&nbsp;Interns</a></li>
            <li id="FeedbackMenu" class="@Model.GetMenuClass("ProjectsMenu")"><a href="@Url.Action("Feedback", "Assigments")"><i class="fa fa-list-alt"></i>&nbsp;&nbsp;Projects</a></li>
            <li id="SupportMenu" class="@Model.GetMenuClass("SupportMenu")"><a href="@Url.Action("Index", "Support")"><i class="fa fa-question-circle"></i>&nbsp;&nbsp;Support</a></li>
            break;
      }
Ricardo Sanchez
  • 6,079
  • 3
  • 24
  • 31
  • 1
    I think it is totally fine to put it in the view, if the menus and roles barely change. It looks easy and clear to me. It can be put into a layout so it can easily reused. And you can do simple changes without requiring recomplile the whole project. – Stephen Zeng Jul 08 '16 at 02:21
  • This isn't a good solution because it only works on this view. What if there is a link on another page to the same controller action. Is every view going to be littered with case statements? A better solution is outlined here. http://stackoverflow.com/questions/8197551/how-to-hide-action-link-if-user-has-no-rights-to-action – Fran Jul 08 '16 at 04:25
  • @Fran A good solution to the problem you are exposing is what Stephen mentioned above, add the above case statements to your shared layout view. The layout view is intended to be used for common assets (e.g. navigation/menu, footer, etc) to be shared with other pages. – Ricardo Sanchez Jul 08 '16 at 05:17
  • I don't think @StephenMuecke is proposing that at all. He's saying there should be no logic in the view and that any logic should be a controller action that then passes back the viewable menu items by role. – Fran Jul 08 '16 at 13:02
  • @Fran "I think it is totally fine to put it in the view, if the menus and roles barely change. It looks easy and clear to me. It can be put into a layout so it can easily reused. And you can do simple changes without requiring recomplile the whole project. – Stephen Zeng " totally agree with that and the reason I offered such solution. – Ricardo Sanchez Jul 08 '16 at 15:23
  • While in the short term it is easier to use case statement in razor view, I see why it is bad. View is supposed to be dumb/stupid . Relying on the Razor View engine seems to be what a ton of developers do, I think it would be better in the back-end – Timothy Fisher Jul 08 '16 at 21:27