Also, containing data access layer code in your view model is considered bad practice from my understanding See this answer on SOthis answer on SO. So further to this I would consider a number of changes (based on whether you consider the complexity worth it),
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),
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),
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 db_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";
}
}
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 db.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";
}
}
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";
}
}
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),
- Abstracting your services behind an interface for better separation of concerns.
- Injecting your services into your controllers using tools such as Inject, Unit, AutoFac etc
- 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 db.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";
}
}
lang-cs