Problem
There are 3 window groups to add users and the application uses MVC 4. Here’s my approach to add security attributes to my controllers and pass user’s role to the view in order to enable/disable controls. I’m looking for ways to approve the code.
Webauthorization.cs (the logic to check user’s role & if not in any role redirect)
public enum UserAccessLevel { SuperAdmin, Admin, StandardUser };
public class WebAuthorization : FilterAttribute, IAuthorizationFilter
{
private UserAccessLevel accessLevel;
public WebAuthorization(UserAccessLevel AccessLevel = UserAccessLevel.StandardUser)
{
accessLevel = AccessLevel;
}
public void OnAuthorization(AuthorizationContext filterContext)
{
string debugMode = ConfigurationManager.AppSettings["DebugMode"] == null ? "OFF" : ConfigurationManager.AppSettings["DebugMode"].ToUpper();
if (debugMode != "ON")
{
if (filterContext.HttpContext.Request.IsAuthenticated)
{
if (accessLevel == UserAccessLevel.SuperAdmin)
{
if (!System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin"))
{
ViewResult result = new ViewResult();
result.ViewName = "UnauthorizedAccess";
result.ViewBag.ErrorMessage = "You are not authorized to use this page. Please contact the Network Administrator.";
filterContext.Result = result;
}
}
else if (accessLevel == UserAccessLevel.Admin)
{
if (!System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin") && !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_Admin"))
{
ViewResult result = new ViewResult();
result.ViewName = "UnauthorizedAccess";
result.ViewBag.ErrorMessage = "You are not authorized to use this page. Please contact the Network Administrator.";
filterContext.Result = result;
}
}
else if (accessLevel == UserAccessLevel.StandardUser)
{
if (!System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin") && !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_Admin") && !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_InputUser"))
{
ViewResult result = new ViewResult();
result.ViewName = "UnauthorizedAccess";
result.ViewBag.ErrorMessage = "You are not authorized to use this page. Please contact the Network Administrator.";
filterContext.Result = result;
}
}
}
}
HomeController.cs (how the above security is implemented in the controller)
[WebAuthorization(UserAccessLevel.StandardUser)]
public class HomeController : Controller
{
private string debugMode;
private string isAdminUser;
private string isSuperAdminUser;
private string vanityRole;
public HomeController()
{
debugMode = ConfigurationManager.AppSettings["DebugMode"] == null ? "OFF" : ConfigurationManager.AppSettings["DebugMode"].ToUpper();
isAdminUser = debugMode.Equals("ON") ? "true" : System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_Admin").ToString().ToLower();
isSuperAdminUser = debugMode.Equals("ON") ? "true" : System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin").ToString().ToLower();
vanityRole = System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin") ? "Super Administrator" : System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_Admin") ? "Administrator" : "Standard User";
vanityRole = debugMode.Equals("ON") ? vanityRole + " (Developer)" : vanityRole;
}
public ActionResult Index()
{
ViewBag.DebugMode = debugMode;
ViewBag.IsAdminUser = isAdminUser;
ViewBag.IsSuperAdminUser = isSuperAdminUser;
ViewBag.VanityRole = vanityRole;
return View();
}
Index.chtml (Use ViewBag to check role)
........
var isAdmin = false;
if ("@ViewBag.DebugMode" === "ON" || "@ViewBag.IsAdminUser" === "true" || "@ViewBag.IsSuperAdminUser" === "true")
isAdmin = true;
bankstatus(bsurl, bscontainer, 'Lead', data.Current_Status_ID, isAdmin);
}
Web.config (settings here)
<system.web>
<compilation debug="true" targetFramework="4.0" />
<authentication mode="Windows" />
<roleManager enabled="true" defaultProvider="AspNetWindowsTokenRoleProvider" />
<authorization>
<deny users="?" />
</authorization>
Solution
First thing that I saw when I looked at your code was the massive if statement nesting.
there are two if statements that I saw right away that can be merged to prevent indenting more than necessary
if (debugMode != "ON")
{
if (filterContext.HttpContext.Request.IsAuthenticated)
{
change it to this
if (debugMode != "ON" && filterContext.HttpContext.Request.IsAuthenticated)
{
The next thing that I would do is to put the following into a method, you are calling it 3 times here.
ViewResult result = new ViewResult();
result.ViewName = "UnauthorizedAccess";
result.ViewBag.ErrorMessage = "You are not authorized to use this page. Please contact the Network Administrator.";
filterContext.Result = result;
any time that you do the exact same thing more than once, you should probably put it into a method that you can call when needed. DON’T COPY PAST CODE.
public void UnauthorizedAccessMessage()
{
ViewResult result = new ViewResult();
result.ViewName = "UnauthorizedAccess";
result.ViewBag.ErrorMessage = "You are not authorized to use this page. Please contact the Network Administrator.";
filterContext.Result = result;
}
you should probably name it something better though.
I also took the liberty of merging as many if statements as I could, and I came up with this as the end result of everything that I did.
public void OnAuthorization(AuthorizationContext filterContext)
{
string debugMode = ConfigurationManager.AppSettings["DebugMode"] == null ? "OFF" : ConfigurationManager.AppSettings["DebugMode"].ToUpper();
if (debugMode != "ON"
&& filterContext.HttpContext.Request.IsAuthenticated
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_Admin")
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin"))
{
if (accessLevel == UserAccessLevel.SuperAdmin )
{
UnauthorizedAccessMessage()
}
else if (accessLevel == UserAccessLevel.Admin)
{
UnauthorizedAccessMessage()
}
else if (accessLevel == UserAccessLevel.StandardUser
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_InputUser"))
{
UnauthorizedAccessMessage()
}
}
}
public void UnauthorizedAccessMessage()
{
ViewResult result = new ViewResult();
result.ViewName = "UnauthorizedAccess";
result.ViewBag.ErrorMessage = "You are not authorized to use this page. Please contact the Network Administrator.";
filterContext.Result = result;
}
I went a little bit farther, but I don’t know if this is going to make it much easier, but it does use less indentation(kind of) and less curly braces.
public void OnAuthorization(AuthorizationContext filterContext)
{
string debugMode = ConfigurationManager.AppSettings["DebugMode"] == null ? "OFF" : ConfigurationManager.AppSettings["DebugMode"].ToUpper();
if (debugMode != "ON"
&& filterContext.HttpContext.Request.IsAuthenticated
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_Admin")
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin")
&& (accessLevel == UserAccessLevel.SuperAdmin
|| accessLevel == UserAccessLevel.Admin
|| (accessLevel == UserAccessLevel.StandardUser
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_InputUser"))))
{
UnauthorizedAccessMessage()
}
}
This kind of looks a little intimidating to me though, and there are a lot of conditionals in that statement. Use with caution, I have not tested this.
Something else that we can do to get rid of some of the nesting, or another condition in our ternary statement is to use a Compiler Directive. basically look for a special compiler flag and then run code based on what the flag says.
#if DEBUG
if (filterContext.HttpContext.Request.IsAuthenticated
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_Admin")
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_BANK_SuperAdmin")
&& (accessLevel == UserAccessLevel.SuperAdmin
|| accessLevel == UserAccessLevel.Admin
|| (accessLevel == UserAccessLevel.StandardUser
&& !System.Web.Security.Roles.IsUserInRole("RICS_LeadManagement_Bank_InputUser"))))
{
UnauthorizedAccessMessage()
}
#endif
Now this code will only run when your Compiler is set to Debug mode. I didn’t use the cleaner code for the example but it’s super simple, just add the #if DEBUG and #endif at the beginning and the end, then Debug away. when you go to release, just make sure to switch the compiler to Release and release away the code inside this Compiler Directive won’t be included in the release version.