User Login Logic

Question

I want to see if this is as streamlined as possible. Most of the logic is accomplished through methods in the model. The controller does one of four things:

1. redirects new users to registration
2. sends locked users to reset password
3. logs the user in and redirects them to where they should go
4. fails the login and allows them to try again

The Controller

using System.Web.Mvc;
using System.Web.Security;
using Authenticator.Models;

namespace Authenticator.Controllers
{
    public class HomeController : Controller
    {
        // Get index
        public ActionResult Index()
        {
            return View();
        }

        [HttpPost()]
        public ActionResult Index(LoginViewModel model)
        {
            // Redirect User to Register if they do not exist;
            if (model.VerifyAccountExists() == false)
            {
                return RedirectToAction("Register");
            }

            // Determine if account is locked and redirect user to reset password.
            if (model.VerifyLock())
            {
                return RedirectToAction("PasswordReset");
            }

            if (model.VerifyPassword())
            {
                model.PassLogin();
                FormsAuthentication.SetAuthCookie(model.UserId, model.RememberMe);
                return Redirect(model.GetAppUrl());
            }
            else 
            {
                model.FailLogin();
                ModelState.AddModelError("", "Incorrect Network ID or Password.");
            }

            return View(model);
        }

        // shared view data
        protected override void OnActionExecuted(ActionExecutedContext filterContext)
        {
            base.OnActionExecuted(filterContext);
            ViewBag.BodyClass = "authenticator";  
        }
    }
}

The View Model

using System;
using System.Linq;
using System.Data;
using System.Web.Helpers;

namespace Authenticator.Models
{
    public class LoginViewModel
    {
        public string UserId { get; set; }
        public string Password { get; set; }
        public bool RememberMe { get; set; }

        public bool VerifyAccountExists()
        {
            using (var db = new WebContext())
            {
                if (db.UserAccounts.Count(p => p.UserId == UserId) > 0)
                {
                    return true;
                }
                else
                {
                    return false;
                }
            }
        }
        public bool VerifyPassword()
        {
            if (VerifyAccountExists() == false)
            {
                return false;
            }
            using (var db = new WebContext())
            {
                UserAccount account = db.UserAccounts.Find(UserId);
                return Crypto.VerifyHashedPassword(account.PasswordHash, Password);
            }
        }
        public bool VerifyLock()
        {
            if (VerifyAccountExists() == false)
            {
                return false;
            }

            using (var db = new WebContext())
            {
                UserAccount account = db.UserAccounts.Find(UserId);
                return account.Lock;
            }

        }
        public bool FailLogin()
        {
            if (VerifyAccountExists() == false)
            {
                return false;
            }
            using (var db = new WebContext())
            {
                UserAccount account = db.UserAccounts.Find(UserId);
                switch (account.FailedAttempts)
                    {
                        case 1 - 3:
                            account.FailedAttempts = account.FailedAttempts + 1;
                            account.DateUpdated = DateTime.Now;
                            break;
                        default:
                            account.FailedAttempts = 4;
                            account.Lock = true;
                            account.DateUpdated = DateTime.Now;
                            break;
                    }
                    db.Entry(account).State = EntityState.Modified;
                    db.SaveChanges();
                    return true;
            }
        }
        public bool PassLogin()
        {
            if (VerifyAccountExists() == false)
            {
                return false;
            }
            using (var db = new WebContext())
            {
                UserAccount account = db.UserAccounts.Find(UserId);
                account.FailedAttempts = 0;
                account.Lock = false;
                account.DateLastLogin = DateTime.Now;
                account.DateUpdated = DateTime.Now;
                db.Entry(account).State = EntityState.Modified;
                db.SaveChanges();
                return true;
            }
        }
        public string GetAppUrl()
        {
            using (var db = new WebContext())
            {
                var query = (from a in db.Permissions
                             join b in db.UserPermissions on a.Id equals b.PermissionId
                             join c in db.Applications on a.Name equals c.AppName
                             where a.Type == PermissionType.AppAccess & b.UserId == UserId
                             select c).ToList();
                switch (query.Count)
                {
                    case 0:
                        return "http://azshisp11/Tickets/Create?subject==Request%20For%20Application%20Access";
                        // TODO: code ticket logic to except this argument
                    case 1:
                        return query[0].AppUrl;
                    default:
                        return "http://azshisp11/Dashboard/" + UserId;
                }
            }
        }
    }
}

Known Issues

• I am aware that the GetAppUrl() method needs to be adjusted and have addressed that issue here.

Answer 1

Viewmodels from my understanding should be as lean as free of domain logic as possible. This is because they are a link from your domain model to your view and so can provide a layer/separation between the two if your view incorporates elements from various places/models.

Hence, all the logic/methods you have in your view model I would pull out into a Service class that is responsible for performing these as required.

public class LoginService
{
    public bool VerifyAccountExists()
    {
        using (var db = new WebContext())
        {
            if (db.UserAccounts.Count(p => p.UserId == UserId) > 0)
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }
}

Also, containing data access layer code in your view model is considered bad practice from my understanding See this answer on SO. So further to this I would consider a number of changes (based on whether you consider the complexity worth it),

1. Abstracting your services behind an interface for better separation of concerns.
2. Injecting your services into your controllers using tools such as Inject, Unit, AutoFac etc
3. Injecting your database into your LoginService

An example of a starting point for a refactored solution might be along these lines:

public interface ILoginService
{
    User GetUser(int userId);
    bool IsLocked(User user);
    AuthenticatedUser AttemptLogin(user, string password);

    // NOTE:  I would prefer this to be seperate from the service and part of a UnitOfWork class
    void SaveChanges();
}

public class LoginService
{
    private readonly WebContext _context;

    public LoginService(WebContext context)
    {
        _context = context;
    }

    public void SaveChanges()
    {
        _context.SaveChanges();
    }

    public User GetUser(int userId)
    {
        return _context.UserAccounts.SingleOrDefault(userId);
    }

    public bool IsLocked(User user)
    {
        return user.Lock;
    }

    public AuthenticatedUser AttemptLogin(User user, string password)
    {
        if(IsValidPassword(user, password))
        {
            RegisterSuccessLoginAttempt(user);

            return new AuthenticatedUser(
                user,
                GetUserWelcomeUrl(user),
                true);
            );
        }
        else 
        {
            RegisterFailedLoginAttempt();
            return new AuthenticatedUser(user);
        }
    }

    private string GetUserWelcomeUrl(User user) 
    {
        var query = (from a in _context.Permissions
                     join b in _context.UserPermissions on a.Id equals b.PermissionId
                     join c in _context.Applications on a.Name equals c.AppName
                     where a.Type == PermissionType.AppAccess & b.UserId == UserId
                     select c).ToList();

        // NOTE:  I would pull these urls from a config file, or a IConfiguration class passed into this service
        switch (query.Count)
        {
            case 0:
                return "http://azshisp11/Tickets/Create?subject==Request%20For%20Application%20Access";
                // TODO: code ticket logic to except this argument
            case 1:
                return query[0].AppUrl;
            default:
                return "http://azshisp11/Dashboard/" + UserId;
        }
    }

    private void RegisterSuccessLoginAttempt(User user)
    {
        user.FailedAttempts = 0;
        user.Lock = false;
        user.DateLastLogin = DateTime.Now;
        user.DateUpdated = DateTime.Now;
    }

    private void RegisterFailedLoginAttempt(User user)
    {
        switch (user.FailedAttempts)
        {
            case 1 - 3:
                user.FailedAttempts = account.FailedAttempts + 1;
                user.DateUpdated = DateTime.Now;
                break;
            default:
                user.FailedAttempts = 4;
                user.Lock = true;
                user.DateUpdated = DateTime.Now;
                break;
        }
    }

    private bool IsValidPassword(User user, string password)
    {
        return Crypto.VerifyHashedPassword(user.PasswordHash, password);
    }
}

public class AuthenticatedUser
{
    public string HomeUrl { get; private set; }
    public User User { get; private set; }
    public bool IsAuthenticated { get; private set; }

    public AuthenticatedUser(
        User user) : this(user, string.empty, false)
        {       
        }

    public AuthenticatedUser(
        User user,
        string welcomeUrl,
        bool isAuthenticated)
    {
        User = user;
        WelcomeUrl = welcomeUrl;
        IsAuthenticated = isAuthenticated;
    }       
}


public class HomeController : Controller
{
    private readonly ILoginService _loginService;

    public HomeController(ILoginService loginService)
    {
        _loginService = loginService;
    }

    // Get index
    public ActionResult Index()
    {
        return View();
    }

    [HttpPost]
    public ActionResult Index(LoginViewModel model)
    {   
        var user = _loginService.GetUser(model.UserId);

        if(user == null)
        {
            return RedirectToAction("Register");
        }

        // Possibly this should instead by part of Authenticated User rather than an extra check here???
        if(_loginService.IsLocked(user))        
        {
            return RedirectToAction("PasswordReset");
        }

        var authenticatedUser = _loginService.AttemptLogin(user, model.Password);       
        _loginService.SaveChanges();

        if (!authenticatedUser.IsAuthenticated)
        {   
            ModelState.AddModelError("", "Incorrect Network ID or Password.");      
            return View(model); 
        }

        FormsAuthentication.SetAuthCookie(model.UserId, model.RememberMe);                  
        return Redirect(authenticatedUser.HomeUrl);

    }

    // shared view data
    protected override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        base.OnActionExecuted(filterContext);
        ViewBag.BodyClass = "authenticator";  
    }
}

Answer 2

Sorry could not comment on the above answer but i would say that #dreza has covered everything.

1. Main important thing that his code is now unit testable.
2. As code is testable means loosely coupled.
3. ViewModel should have only data not a business logic which is correct, as you application gets more features it will become painful to maintain your code and all your business logic will be scattered all over the places.