1
\$\begingroup\$

I was wondering whether it is a good aproach to have Identity 2.0 UserManager next to UnitOfWork pattern? I am using UserManager only in login and register as it gives freedom of not taking care of password hashing (also provides options to email user etc.) and user-to-role management. Although for the rest of the project I am using UOW to maintain users and roles (e.g. to edit user information (not password).

These are my Login and Registermethods:

[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult >Login(UserLoginViewModel model, string returnUrl)
{
    if (!this.ModelState.IsValid)
    {
        return this.View();
    }

    var user = (await this.unitOfWork.UserRepository.Find(u =>
        u.UserName == model.Login || u.UstId == model.Login)).FirstOrDefault();
    if (user == null)
    {
        this.ModelState.AddModelError("Login", Properties.Error.UserDoesntExist);
        return this.View();
    }
    var userManager = this.HttpContext.GetOwinContext().GetUserManager<LeaveUserManager>();
    var authManager = this.HttpContext.GetOwinContext().Authentication;

    if (!await userManager.CheckPasswordAsync(user, model.Password))
    {
        this.ModelState.AddModelError("Password", Properties.Error.WrongPassword);
        return this.View();
    }

    var ident = userManager.CreateIdentity(user, DefaultAuthenticationTypes.ApplicationCookie);
    authManager.SignIn(new AuthenticationProperties { IsPersistent = false }, ident);

    this.TempData["SuccessMessage"] = "Logged in";
    if (!string.IsNullOrEmpty(returnUrl) && this.Url.IsLocalUrl(returnUrl))
    {
        return this.Redirect(returnUrl);
    }
    return this.RedirectToAction("Index");
}

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Register(UserRegisterViewModel model)
{
    if (!this.ModelState.IsValid)
    {
        return this.View();
    }
    var userManager = this.HttpContext.GetOwinContext().GetUserManager<LeaveUserManager>();

    var user = new User(model);
    var result = userManager.Create(user, model.Password);
    if (!result.Succeeded)
    {
        result.Errors.ForEach(e => this.ModelState.AddModelError(string.Empty, e));
        return this.View();
    }

    var userRole = this.unitOfWork.UserRoleRepository.Find(ur => ur.Name == "User").Result.FirstOrDefault();
    if (userRole == null)
    {
        userRole = new UserRole("User");

        this.unitOfWork.UserRoleRepository.Add(userRole);
        this.unitOfWork.Complete();
    }
    userManager.AddToRole(user.Id, "User");

    this.TempData["SuccessMessage"] = "Registered";
    return this.RedirectToAction("Index");
}

And assigning user to role also uses UserManager:

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Assign(AssignRoleViewModel model)
{
    if (!this.ModelState.IsValid)
    {
        return this.View(model);
    }
    var userManager = this.HttpContext.GetOwinContext().GetUserManager<LeaveUserManager>();

    userManager.RemoveFromRoles(model.Id, userManager.GetRoles(model.Id).ToArray());
    if (model.SelectedRoles.Any())
    {
        userManager.AddToRoles(model.Id, model.SelectedRoles.ToArray());
    }
    return this.RedirectToAction("Details", new { id = model.Id });
}

Although Edit method uses uow:

[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult> Edit(User model)
{
    if (this.ModelState.IsValid)
    {
        var user = await this.unitOfWork.UserRepository.Get(model.Id);
        user.EditUser(model);
        await this.unitOfWork.Complete();
        return this.RedirectToAction("Index");
    }
    var userList = this.unitOfWork.UserRepository.GetAll().Result;
    this.ViewBag.SuperiorId = new SelectList(userList, "Id", "FirstName", model.SuperiorId);
    return this.View(model);
}

And my UserManager:

public class LeaveUserManager : UserManager<User>
{
    public LeaveUserManager(IUserStore<User> store) : base(store)
    {
    }

    public static LeaveUserManager Create(
        IdentityFactoryOptions<LeaveUserManager> options, IOwinContext context)
    {
        var manager = new LeaveUserManager(
            new UserStore<User>(context.Get<VacationsContext>()))
        {
            EmailService = new LeaveEmailService()
        };
        return manager;
    }
}

Is it OK to leave it like this or I should create my own userManager which will be using uow and repositories?
I am using ASP.NET MVC5 with EF6.


2018-03-20 edit:
I have removed .Result from all async methods and replaced with await.
I have dependency injected all managers, dbcontext and uow using Ninject library.

    private static void RegisterService(IKernel kernel)
    {
        kernel.Bind<VacationsContext>().ToSelf().InSingletonScope();
        kernel.Bind<RoleManager<UserRole>>().ToSelf().InRequestScope();
        kernel.Bind<LeaveUserManager>().ToMethod(c => LeaveUserManager.Create(kernel.Get<VacationsContext>())).InRequestScope();
        kernel.Bind<IAuthenticationManager>().ToMethod(c => HttpContext.Current.GetOwinContext().Authentication).InRequestScope();
        kernel.Bind<IUserStore<User>>().To<UserStore<User>>().InRequestScope().WithConstructorArgument("context", kernel.Get<VacationsContext>());
        kernel.Bind<IUnitOfWork>().To<UnitOfWork>().InRequestScope();
    }

Injection works in controllers:

public HomeController(LeaveUserManager userManager, IAuthenticationManager authManager, IUnitOfWork unitOfWork)
{
    this.leaveUserManager = userManager;
    this.authenticationManager = authManager;
    this.unitOfWork = unitOfWork;
}

And if noone says that it is some kind of violation - then I will leave LeaveUserManager as it is - using it's own way to database access layer (skipping my custom uow).

\$\endgroup\$
  • \$\begingroup\$ UserManager is more of an implementation concern that you can encapsulate behind your own abstraction in whatever structure/pattern you want. \$\endgroup\$ – Nkosi Mar 19 '18 at 16:58
  • 1
    \$\begingroup\$ Avoid .Result within async methods as those blocking calls can lead to deadlocks \$\endgroup\$ – Nkosi Mar 19 '18 at 17:34
  • \$\begingroup\$ you should post edits to your code as a new question(review request) so that you can get more feedback. \$\endgroup\$ – Malachi Mar 20 '18 at 13:12
  • \$\begingroup\$ @Malachi, I think that this is now pretty clear for me, but maybe it would be a good idea to modify question title that this also references to DI and Service Locator anti-pattern. Morelike for others than for me, what do you think? \$\endgroup\$ – Marek Mar 20 '18 at 13:30
  • \$\begingroup\$ Oh, I see you didn't actually show the changes you made just added other code pertinent to the review. You did good. As you were...lol \$\endgroup\$ – Malachi Mar 20 '18 at 13:32
2
\$\begingroup\$

First, avoid .Result within async methods

[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult> Edit(User model) {
    //...

    var userList = this.unitOfWork.UserRepository.GetAll().Result;

    //...
}

as those blocking calls can lead to deadlocks. Make the function async all the way through

[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult> Edit(User model) {
    if (this.ModelState.IsValid) {
        var user = await this.unitOfWork.UserRepository.Get(model.Id);
        user.EditUser(model);
        await this.unitOfWork.Complete();
        return this.RedirectToAction("Index");
    }
    var userList = await this.unitOfWork.UserRepository.GetAll();
    this.ViewBag.SuperiorId = new SelectList(userList, "Id", "FirstName", model.SuperiorId);
    return this.View(model);
}

Next, it appears that DI and service locator is being mixed.

var userManager = this.HttpContext.GetOwinContext().GetUserManager<LeaveUserManager>();
var authManager = this.HttpContext.GetOwinContext().Authentication;

could be seen as a service locator anti-pattern within the action being called.

Assuming the unitOfWork is being injected into the controller then user manager and authentication should be explicitly injected as well.

Is it ok to leave it like this or I should create my own userManager which will be using uow and repositories?

Entity Framework is already built following the UOW/Repository pattern and Identity Framework is built on top of it.

UserManager and by extension Identity Framework is more of an implementation concern/detail that you can encapsulate behind your own abstraction in whatever structure/pattern you want.

Instead of focusing on that pattern consider just creating a service to perform the desired functionality and inject that into your dependent classes.

\$\endgroup\$
  • \$\begingroup\$ Thanks for pointing out using .Result in async methods. I have fixed this in all project :). If I understood correctly - It would be good for me to create a service that would take some of the functionality (that I need) from userManager and would use my UoW? I know that EF is based on UoW and Repos, but in this case I have my custom UoW and user Manager is not using my custom uow. \$\endgroup\$ – Marek Mar 19 '18 at 22:05
  • \$\begingroup\$ @MarekCzaplicki That works perfectly, and saves you from having to build your own service. My only issue is that classes should depend on abstractions and not concretions. While injecting the class in you scenario works, it is not a very good design. \$\endgroup\$ – Nkosi Mar 20 '18 at 12:55
  • \$\begingroup\$ do you mean like I should use UserManager instead of LeaveUserManager, or you mean something else? \$\endgroup\$ – Marek Mar 20 '18 at 13:13
  • \$\begingroup\$ Not necessarily. An interface would be as abstract as you can get, but like I said before, what you have can work. \$\endgroup\$ – Nkosi Mar 20 '18 at 13:16
  • \$\begingroup\$ In case of UserManager there is no interface, but I could still use it, because it is still more abstract than LeaveUserManager. I am pretty new to Dependency Injection in practice, so I just want to understand how things should work in live project \$\endgroup\$ – Marek Mar 20 '18 at 13:21

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.