patterncsharpMinor
Asp.net MVC login method
Viewed 0 times
mvcmethodloginnetasp
Problem
I wrote this login method:
```
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
if (!ModelState.IsValid)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
return RedirectToAction("LoginFailed");
}
User user;
try
{
user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
}
catch(Exception ex)
{
//log it
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.DatabaseException;
return RedirectToAction("LoginFailed");
}
if (user == null)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
return RedirectToAction("LoginFailed");
}
//if we got so far it means that username does exist
//is user blocked?
if (user.IsLockedOut)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.AccountBlocked;
return RedirectToAction("LoginFailed");
}
//is the password correct?
string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
if (!String.Equals(passHash, user.Password))
{
user.FailedPasswordAttemptCount++;
try
{
_usersRepository.Update(user);
}
catch(Exception ex)
{
//just log it, not a big deal the FailedPasswordAttemptCount could not be updated
user.FailedPasswordAttemptCount--;
}
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
TempData[Keys.ACCOUNT_TEMPDATA_INVALIDATTEMPTS] = user.FailedPasswordAttemptCount;
return RedirectToAction("LoginFailed");
}
//is the account activated?
if
```
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
if (!ModelState.IsValid)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
return RedirectToAction("LoginFailed");
}
User user;
try
{
user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
}
catch(Exception ex)
{
//log it
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.DatabaseException;
return RedirectToAction("LoginFailed");
}
if (user == null)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
return RedirectToAction("LoginFailed");
}
//if we got so far it means that username does exist
//is user blocked?
if (user.IsLockedOut)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.AccountBlocked;
return RedirectToAction("LoginFailed");
}
//is the password correct?
string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
if (!String.Equals(passHash, user.Password))
{
user.FailedPasswordAttemptCount++;
try
{
_usersRepository.Update(user);
}
catch(Exception ex)
{
//just log it, not a big deal the FailedPasswordAttemptCount could not be updated
user.FailedPasswordAttemptCount--;
}
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.InvalidCredentials;
TempData[Keys.ACCOUNT_TEMPDATA_INVALIDATTEMPTS] = user.FailedPasswordAttemptCount;
return RedirectToAction("LoginFailed");
}
//is the account activated?
if
Solution
If you can, I would encrypt the password entered before it is passed back to the service. This will eliminate any chance it could be intercepted. As for the rest of your first question, I think the how you process is up to you. I don't think there is a right or wrong answer.
Now onto improvements:
First off, I think you code is doing too much. This is very evident by then number of if/else and return statements.
I think a better approach would be to pull out all of the logic into a number of separate methods, or even better classes that all work together to process the login, and decide what the appropriate course of action is. I'm going to tackle the multiple method approach. If you want to see how multiple classes can be implemented let me know, and I'll update the post then.
There are two important pieces of data that you require, the LoginStatus and the ActionResult Keeping this in mind, I would first create a class call LoginResult, which contains the two pieces of information in it
This will allow for each method to return the important data.
Now that you have the first building block, I would start by pulling out every if/else statement in your original code into its own method, returning a LoginResult. There is another important piece of data returned from one of the checks, and that is user, so add a UserLoginResult class:
```
internal class LoginResult
{
public ActionResult ActionResult { get; private set; }
public LoginStatus LoginStatus { get; private set; }
public LoginResult(ActionResult actionResult, LoginStatus loginStatus)
{
ActionResult = actionResult;
LoginStatus = loginStatus;
}
}
internal class RetrieveUserResult
{
public LoginResult Result { get; private set; }
public User User { get; private set; }
public RetrieveUserResult(LoginResult loginResult, User user)
{
Result = loginResult;
User = user;
}
}
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
_returnUrl = returnUrl;
_logModel = logModel;
var result = EnsureModalStateIsValid();
if (result.LoginStatus == LoginStatus.InvalidCredentials)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
return result.ActionResult;
}
var retrieveResult = TryToRetrieveUserFromRepository();
if (retrieveResult.User == null)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
return result.ActionResult;
}
var lockedResult = CheckIfUserIsLockedOut(retrieveResult.User);
if (lockedResult.LoginStatus == LoginStatus.AccountBlocked)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = lockedResult.LoginStatus;
return lockedResult.ActionResult;
}
var passwordResult = ConfirmUserPasswordMatches(retrieveResult.User);
if (passwordResult.LoginStatus == LoginStatus.InvalidCredentials)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = passwordResult.LoginStatus;
return passwordResult.ActionResult;
}
//is the account activated?
var activatedResult = ConfirmUserIsActivated(retrieveResult.User);
if (activatedResult.LoginStatus == LoginStatus.AccountNotActivated)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = activatedResult.LoginStatus;
return activatedResult.ActionResult;
}
return activatedResult.ActionResult;
}
private LoginResult EnsureModalStateIsValid()
{
if (!ModelState.IsValid)
{
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials);
}
// Should come up with a dummy Action for this case
return DetermineSuccessResult();
}
private RetrieveUserResult TryToRetrieveUserFromRepository()
{
try
{
var user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
return new RetrieveUserResult(new LoginResult(RedirectToAction("Index", "Home"), LoginStatus.LoginOK), user);
}
catch (Exception ex)
{
return new RetrieveUserResult(DetermineSuccessResult(), null);
}
}
private LoginResult CheckIfUserIsLockedOut(User user)
{
if (user.IsLockedOut)
{
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountBlocked);
}
return DetermineSuccessResult();
}
private LoginResult ConfirmUserPasswordMatches(User user)
{
string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
if (!String.Equals(passHash, user.Password))
{
user.FailedPasswordAttemptCount++;
try
{
_usersReposit
Now onto improvements:
First off, I think you code is doing too much. This is very evident by then number of if/else and return statements.
I think a better approach would be to pull out all of the logic into a number of separate methods, or even better classes that all work together to process the login, and decide what the appropriate course of action is. I'm going to tackle the multiple method approach. If you want to see how multiple classes can be implemented let me know, and I'll update the post then.
There are two important pieces of data that you require, the LoginStatus and the ActionResult Keeping this in mind, I would first create a class call LoginResult, which contains the two pieces of information in it
internal class LoginResult
{
public ActionResult ActionResult { get; private set; }
public LoginStatus LoginStatus { get; private set; }
public LoginResult(ActionResult actionResult, LoginStatus loginStatus)
{
ActionResult = actionResult;
LoginStatus = loginStatus;
}
}This will allow for each method to return the important data.
Now that you have the first building block, I would start by pulling out every if/else statement in your original code into its own method, returning a LoginResult. There is another important piece of data returned from one of the checks, and that is user, so add a UserLoginResult class:
```
internal class LoginResult
{
public ActionResult ActionResult { get; private set; }
public LoginStatus LoginStatus { get; private set; }
public LoginResult(ActionResult actionResult, LoginStatus loginStatus)
{
ActionResult = actionResult;
LoginStatus = loginStatus;
}
}
internal class RetrieveUserResult
{
public LoginResult Result { get; private set; }
public User User { get; private set; }
public RetrieveUserResult(LoginResult loginResult, User user)
{
Result = loginResult;
User = user;
}
}
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
_returnUrl = returnUrl;
_logModel = logModel;
var result = EnsureModalStateIsValid();
if (result.LoginStatus == LoginStatus.InvalidCredentials)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
return result.ActionResult;
}
var retrieveResult = TryToRetrieveUserFromRepository();
if (retrieveResult.User == null)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
return result.ActionResult;
}
var lockedResult = CheckIfUserIsLockedOut(retrieveResult.User);
if (lockedResult.LoginStatus == LoginStatus.AccountBlocked)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = lockedResult.LoginStatus;
return lockedResult.ActionResult;
}
var passwordResult = ConfirmUserPasswordMatches(retrieveResult.User);
if (passwordResult.LoginStatus == LoginStatus.InvalidCredentials)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = passwordResult.LoginStatus;
return passwordResult.ActionResult;
}
//is the account activated?
var activatedResult = ConfirmUserIsActivated(retrieveResult.User);
if (activatedResult.LoginStatus == LoginStatus.AccountNotActivated)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = activatedResult.LoginStatus;
return activatedResult.ActionResult;
}
return activatedResult.ActionResult;
}
private LoginResult EnsureModalStateIsValid()
{
if (!ModelState.IsValid)
{
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials);
}
// Should come up with a dummy Action for this case
return DetermineSuccessResult();
}
private RetrieveUserResult TryToRetrieveUserFromRepository()
{
try
{
var user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
return new RetrieveUserResult(new LoginResult(RedirectToAction("Index", "Home"), LoginStatus.LoginOK), user);
}
catch (Exception ex)
{
return new RetrieveUserResult(DetermineSuccessResult(), null);
}
}
private LoginResult CheckIfUserIsLockedOut(User user)
{
if (user.IsLockedOut)
{
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountBlocked);
}
return DetermineSuccessResult();
}
private LoginResult ConfirmUserPasswordMatches(User user)
{
string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
if (!String.Equals(passHash, user.Password))
{
user.FailedPasswordAttemptCount++;
try
{
_usersReposit
Code Snippets
internal class LoginResult
{
public ActionResult ActionResult { get; private set; }
public LoginStatus LoginStatus { get; private set; }
public LoginResult(ActionResult actionResult, LoginStatus loginStatus)
{
ActionResult = actionResult;
LoginStatus = loginStatus;
}
}internal class LoginResult
{
public ActionResult ActionResult { get; private set; }
public LoginStatus LoginStatus { get; private set; }
public LoginResult(ActionResult actionResult, LoginStatus loginStatus)
{
ActionResult = actionResult;
LoginStatus = loginStatus;
}
}
internal class RetrieveUserResult
{
public LoginResult Result { get; private set; }
public User User { get; private set; }
public RetrieveUserResult(LoginResult loginResult, User user)
{
Result = loginResult;
User = user;
}
}
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
_returnUrl = returnUrl;
_logModel = logModel;
var result = EnsureModalStateIsValid();
if (result.LoginStatus == LoginStatus.InvalidCredentials)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
return result.ActionResult;
}
var retrieveResult = TryToRetrieveUserFromRepository();
if (retrieveResult.User == null)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
return result.ActionResult;
}
var lockedResult = CheckIfUserIsLockedOut(retrieveResult.User);
if (lockedResult.LoginStatus == LoginStatus.AccountBlocked)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = lockedResult.LoginStatus;
return lockedResult.ActionResult;
}
var passwordResult = ConfirmUserPasswordMatches(retrieveResult.User);
if (passwordResult.LoginStatus == LoginStatus.InvalidCredentials)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = passwordResult.LoginStatus;
return passwordResult.ActionResult;
}
//is the account activated?
var activatedResult = ConfirmUserIsActivated(retrieveResult.User);
if (activatedResult.LoginStatus == LoginStatus.AccountNotActivated)
{
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = activatedResult.LoginStatus;
return activatedResult.ActionResult;
}
return activatedResult.ActionResult;
}
private LoginResult EnsureModalStateIsValid()
{
if (!ModelState.IsValid)
{
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials);
}
// Should come up with a dummy Action for this case
return DetermineSuccessResult();
}
private RetrieveUserResult TryToRetrieveUserFromRepository()
{
try
{
var user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
return new RetrieveUserResult(new LoginResult(RedirectToAction("Index", "Home"), LoginStatus.LoginOK), user);
}
catch (Exception ex)
{
return new RetrieveUserResult(DetermineSuccessResult(), null);
}
}
private LoginResult CheckIfUserIsLockedOut(User user)
{
if (user.IsLockedOut)
{
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountBlocked);
}
return D[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Login(LoginModel logModel, string returnUrl)
{
_returnUrl = returnUrl;
_logModel = logModel;
var result = Process();
TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = result.LoginStatus;
return result.ActionResult;
}
private LoginResult Process()
{
LoginResult result = EnsureModalStateIsValid();
return result.LoginStatus == LoginStatus.InvalidCredentials ? result : TryToRetrieveUserFromRepository();
}
private LoginResult EnsureModalStateIsValid()
{
return !ModelState.IsValid
? new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials)
: DetermineSuccessResult();
// Should come up with a dummy Action for this case
}
private LoginResult TryToRetrieveUserFromRepository()
{
try
{
var user = _usersRepository.SingleOrDefault(u => String.Equals(u.Username, logModel.LoginUsername));
return retrieveUserResult.User == null ? result : CheckIfUserIsLockedOut(retrieveUserResult.User);
}
catch (Exception ex)
{
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.DatabaseException);
}
}
private LoginResult CheckIfUserIsLockedOut(User user)
{
return user.IsLockedOut
? new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountBlocked)
: ConfirmUserPasswordMatches(user);
}
private LoginResult ConfirmUserPasswordMatches(User user)
{
//is the password correct?
string passHash = ComputePasswordHash(logModel.LoginPassword, user.Salt);
if (!String.Equals(passHash, user.Password))
{
user.FailedPasswordAttemptCount++;
try
{
_usersRepository.Update(user);
}
catch (Exception ex)
{
//just log it, not a big deal the FailedPasswordAttemptCount could not be updated
user.FailedPasswordAttemptCount--;
}
TempData[Keys.ACCOUNT_TEMPDATA_INVALIDATTEMPTS] = user.FailedPasswordAttemptCount;
return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.InvalidCredentials);
}
return ConfirmUserIsActivated(user);
}
private LoginResult ConfirmUserIsActivated(User user)
{
return !user.IsApproved
? new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountNotActivated)
: DetermineSuccessResult();
}
private LoginResult DetermineSuccessResult()
{
FormsAuthentication.SetAuthCookie(_logModel.LoginUsername, false);
return new LoginResult(ReturnUrlIsValid() ? Redirect(_returnUrl) : RedirectToAction("Index", "Home"),
LoginStatus.LoginOK);
}
private bool ReturnUrlIsValid()
{
return (Url.IsLocalUrl(_returnUrl) && _returnUrl.Length > 1 && _returnUrl.StartsWith("/") &&
!_returnUrl.StartsWith("//") && !_returnUrl.StartsWith("/\\"));
}Context
StackExchange Code Review Q#15303, answer score: 2
Revisions (0)
No revisions yet.