7
\$\begingroup\$

I'm developing an ASP.Net web application, using MVC 5. This application authenticates via LDAP and will assign an Identity Role of "Admin" to a designated user.

The Admin user will be able to go to an administration page to add users with a role of "Admin", if the admin checkbox is not checked, then the role is "Employee". Admin can also update user role, or remove a user from the application. Only those added this users list will be able to log into the application.

I am using the UserManager class from ASP.Net Identity

http://localhost:xxxxx/Admin/Manage is the starting point.

I am sure my code is not optimal (less than 1 year experience), and I am looking for some critique on my code below. Particularly, I would like to figure out how to remove the self reference from my viewModel class. But please let me know if there's a better way to implement my solution.

User Interface (trying to focus on the app logic first before fixing this up): User Interface

Controller:

public class AdminController : Controller
{
    private ApplicationUserManager _userManager;

    public ApplicationUserManager UserManager
    {
        get { return _userManager ?? HttpContext.GetOwinContext()
                .GetUserManager<ApplicationUserManager>(); }
        private set { _userManager = value; }
    }

    [RestoreModelStateFromTempData]
    public ActionResult Manage()
    {
        var list = ManageViewModel.GetListOfUsers(UserManager);
        var viewModel = new ManageViewModel
        {
            Users = list,
        };

        return View(viewModel);
    }

    //Preserve model state across redirect
    [SetTempDataModelState]
    public ActionResult AddUser(ManageViewModel viewModel)
    {
        if (!ModelState.IsValid)
        {
            var list = ManageViewModel.GetListOfUsers(UserManager);

            var newViewModel = new ManageViewModel
            {
                Users = list,
            };

            return View("Manage",newViewModel);
        }

        var result = ManageViewModel
            .AddUserWithRole(viewModel.Id, UserManager, viewModel.GetRole(viewModel.IsAdmin));

        if (!string.IsNullOrEmpty(result))
        {
            ModelState.AddModelError("Error", result);
            return RedirectToAction("Manage");
        }

        return RedirectToAction("Manage");
    }


    public ActionResult RemoveUser(string id)
    {
        ManageViewModel.RemoveUser(id, UserManager);

        return RedirectToAction("Manage");

    }


    public ActionResult UpdateUserRole(string id, string role)
    {

        ManageViewModel.UpdateUserRole(id, role, UserManager);

        return RedirectToAction("Manage");
    }
}

viewModel:

public class ManageViewModel
{
    [Display(Name = "NetworkID")]
    [Required(ErrorMessage = "NetworkID is required!")]
    [RegularExpression(@"([A-Za-z0-9.-])+", ErrorMessage = "Invalid!")]
    public string Id { get; set; }

    public string FirstName { get; set; }

    public string LastName { get; set; }

    public bool IsAdmin { get; set; }

    public List<ManageViewModel> Users { get; set; }

    public string Role { get; set; }

    public string GetRole(bool isAdmin)
    {
        if (isAdmin)
        {
            return "Admin";
        }
        return "Employee";
    }

    public static List<ManageViewModel> GetListOfUsers(ApplicationUserManager userManager)
    {
        var users = userManager.Users.ToList();
        var list = new List<ManageViewModel>();

        foreach (var user in users)
        {
            var ldap = AdHelper.GetUser(user.UserName);

            if (ldap != null)
            {
                var isAdmin = userManager.IsInRole(user.Id, "Admin");

                var firstName = Convert.ToString(ldap.Properties["givenName"][0]);
                var lastName = Convert.ToString(ldap.Properties["sn"][0]);

                if (isAdmin)
                {
                    list.Add(new ManageViewModel
                    {
                        Id = user.UserName,
                        FirstName = firstName,
                        LastName = lastName,
                        IsAdmin = true,
                        Role="Admin",
                    });
                }
                else
                {
                    list.Add(new ManageViewModel
                    {
                        Id = user.UserName,
                        FirstName = firstName,
                        LastName = lastName,
                        IsAdmin = false,
                        Role="Employee",
                    });
                }
            }
        }
        return list;
    }

    internal static string RemoveUser(string id, ApplicationUserManager userManager)
    {
        var result = userManager.Delete(userManager.FindByName(id));
        if (!result.Succeeded)
        {
            throw new AccessViolationException("Delete user " + id +
                                                       " from application database failed with error(s): " + result.Errors);
        }
        return null;
    }

    public static string AddUserWithRole(string networkId, ApplicationUserManager userManager, string role)
    {
        var ldap = AdHelper.GetUser(networkId);

        if (ldap != null)
        {
            var user = new ApplicationUser() { UserName = networkId, Email = networkId + "@tamiu.edu" };

            //create new user in database
            var newUser = userManager.Create(user);
            if (newUser.Succeeded)
            {
                using (var rm = new RoleManager<IdentityRole>(new RoleStore<IdentityRole>(new ApplicationDbContext())))
                    if (!rm.RoleExists(role))
                    {
                        var roleResult = rm.Create(new IdentityRole(role));
                        if (!roleResult.Succeeded)
                            throw new ApplicationException("Creating role Admin failed with error(s): " + roleResult.Errors);
                    }
                var userResult = userManager.AddToRole(user.Id, role);
                if (!userResult.Succeeded)
                    throw new AccessViolationException("Adding user " + user.UserName +
                                                       " to Admin role failed with error(s): " + userResult.Errors);

            }
            else
            {
                return "User already exists!";
            }
            return null;
        }
        else
        {
            return "NetworkID is not valid!";
        }
    }

    public static void UpdateUserRole(string id, string role, ApplicationUserManager userManager)
    {
        var findId = userManager.FindByName(id);

        if (role == "Admin")
        {
            userManager.RemoveFromRole(findId.Id, "Admin");
            userManager.AddToRole(findId.Id, "Employee");
        }
        else
        {
            userManager.RemoveFromRole(findId.Id, "Employee");
            userManager.AddToRole(findId.Id, "Admin");
        }

    }
}

View:

@model App.ViewModels.ManageViewModel
@{
    ViewBag.Title = "Manage Application Users";
}

<div class="box">
    <h2>Manage Application Users</h2>

    <p>Add employee by NetworkId to enable application access.</p>

    <p>Specify access type:</p>
    <p><strong>Administrator</strong> role enables "Export to Excel" feature.</p>

    <table class="table table-hover">
        <tr>
            <th>
                @Html.LabelFor(m => m.Id)
            </th>
            <th class="text-center">
                @Html.Label("Name")
            </th>
            <th class="text-center">
                @Html.Label("Administrator")
            </th>
            <th></th>
        </tr>
        @using (Html.BeginForm("AddUser", "Admin"))
        {
            <tr>
                <td>
                    @Html.TextBoxFor(m => m.Id, "", new { @class = "form-control" })
                    @Html.ValidationMessageFor(m => m.Id)
                    @if (!ViewData.ModelState.IsValid)
                    {
                        @Html.ValidationMessage("Error")
                    }
                </td>
                <td></td>
                <td class="text-center">
                    @Html.CheckBoxFor(m => m.IsAdmin)
                </td>
                <td>
                    <button type="submit" class="btn btn-success">Add</button>
                </td>
            </tr>
        }

        @foreach (var user in Model.Users)

        {
            <tr>
                <td>
                    @user.Id
                </td>
                <td class="text-center">
                    @user.FirstName @user.LastName
                </td>
                <td class="text-center">
                    @Html.CheckBox("role", user.IsAdmin)
                </td>
                <td class="btn-group-vertical">
                    @using (Html.BeginForm("UpdateUserRole", "Admin"))
                    {
                        @Html.Hidden("id", user.Id)
                        @Html.Hidden("role", user.GetRole(user.IsAdmin))
                        <button type="submit" class="btn btn-info">Update</button>
                    }
                    @using (Html.BeginForm("RemoveUser", "Admin"))
                    {
                        @Html.Hidden("id", user.Id)
                        <button type="submit" class="btn btn-danger">Remove</button>
                    }
                </td>
            </tr>
        }

    </table>
</div>
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! It would be nice if you didn't mix your product name and specification in the title ;) Also, I recommend you to stress on improvement, because "far from done" rises alerts. The last thing is that thanks is implicit in every code review, so you should remove that. Other things look nice :) \$\endgroup\$ – Incomputable Aug 12 '16 at 18:28
  • \$\begingroup\$ First post here, thanks for the tips. Edits made. \$\endgroup\$ – Joe Cuevas Aug 12 '16 at 18:33
4
\$\begingroup\$

Let's take about the ManageViewModel for a moment, because it has a lot of things you can do better.

public bool IsAdmin { get; set; }

public string Role { get; set; }

public string GetRole(bool isAdmin)
{
    if (isAdmin)
    {
        return "Admin";
    }
    return "Employee";
}

With C# properties you can really simplify how this whole thing goes:

public bool IsAdmin { get; set; }

public string Role
{
    get
    {
        if (IsAdmin)
        {
            return "Admin";
        }
        return "Employee";
    }
}

Now you never have to set Role in your code. This gets rid of GetRole entirely as well.

So this if block:

if (isAdmin)
{
    list.Add(new ManageViewModel
    {
        Id = user.UserName,
        FirstName = firstName,
        LastName = lastName,
        IsAdmin = true,
        Role="Admin",
    });
}
else
{
    list.Add(new ManageViewModel
    {
        Id = user.UserName,
        FirstName = firstName,
        LastName = lastName,
        IsAdmin = false,
        Role="Employee",
    });
}

Becomes:

list.Add(new ManageViewModel
{
    Id = user.UserName,
    FirstName = firstName,
    LastName = lastName,
    IsAdmin = isAdmin
});

The if is gone, entirely.


Next we're only going to look at:

public class ManageViewModel
{
    [Display(Name = "NetworkID")]
    [Required(ErrorMessage = "NetworkID is required!")]
    [RegularExpression(@"([A-Za-z0-9.-])+", ErrorMessage = "Invalid!")]
    public string Id { get; set; }

    public string FirstName { get; set; }

    public string LastName { get; set; }

    public bool IsAdmin { get; set; }

    public List<ManageViewModel> Users { get; set; }

    public string Role { get; set; }

And not even at any conventions, but at what does and doesn't make sense.

You have a List<ManageViewModel> Users inside the ManageViewModel...that's fishy, really fishy.

The great thing about Razor, MVC and C# is that you don't need to do that. I can see why you're doing it, but we're going to remove the List from that ManageViewModel and rename it to UserViewModel.

public class UserViewModel
{
    [Display(Name = "NetworkID")]
    [Required(ErrorMessage = "NetworkID is required!")]
    [RegularExpression(@"([A-Za-z0-9.-])+", ErrorMessage = "Invalid!")]
    public string Id { get; set; }

    public string FirstName { get; set; }

    public string LastName { get; set; }

    public bool IsAdmin { get; set; }

    public string Role
    {
        get
        {
            if (IsAdmin)
            {
                return "Admin";
            }
            return "Employee";
        }
    }

    public static List<UserViewModel> GetListOfUsers(ApplicationUserManager userManager)
    {
        var users = userManager.Users.ToList();
        var list = new List<UserViewModel>();

        foreach (var user in users)
        {
            var ldap = AdHelper.GetUser(user.UserName);

            if (ldap != null)
            {
                var isAdmin = userManager.IsInRole(user.Id, "Admin");

                var firstName = Convert.ToString(ldap.Properties["givenName"][0]);
                var lastName = Convert.ToString(ldap.Properties["sn"][0]);

                list.Add(new ManageViewModel
                {
                    Id = user.UserName,
                    FirstName = firstName,
                    LastName = lastName,
                    IsAdmin = isAdmin
                });
            }
        }
        return list;
    }

Alright, that works. Let's move on to where we're using the ViewModel from there.


[RestoreModelStateFromTempData]
public ActionResult Manage()
{
    var list = ManageViewModel.GetListOfUsers(UserManager);
    var viewModel = new ManageViewModel
    {
        Users = list,
    };

    return View(viewModel);
}

Aha! Now we know what we did wrong. We're not even going to make a ViewModel instance for this.

[RestoreModelStateFromTempData]
public ActionResult Manage()
{
    return View(UserViewModel.GetListOfUsers(UserManager));
}

And we just need to modify our Razor a bit:

@model App.ViewModels.ManageViewModel

To:

@model System.Collections.Generic.IEnumerable<App.ViewModels.UserViewModel>

And:

@foreach (var user in Model.Users)
{

To:

@foreach (var user in Model)
{

And bam, we cleaned a lot up with just a little work. :)


This is a great start, and hopefully you learned something from this answer. I implore you to find other ways to clean it up (there are two that I see right now) and get you thinking about how to use properties in C# more effectively. Good job, and good luck! :)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.