3
\$\begingroup\$

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 (!user.IsApproved)
        {
            TempData[Keys.ACCOUNT_TEMPDATA_LOGINSTATUS] = LoginStatus.AccountNotAcctivated;
            return RedirectToAction("LoginFailed");
        }
        //if we got so far then everything is ok
        FormsAuthentication.SetAuthCookie(logModel.LoginUsername, false);
        if (Url.IsLocalUrl(returnUrl) && returnUrl.Length > 1 && returnUrl.StartsWith("/") && !returnUrl.StartsWith("//") && !returnUrl.StartsWith("/\\"))
        {
            return Redirect(returnUrl);
        }
        else
        {
            return RedirectToAction("Index", "Home");
        }
    }
  • Is it ok to retrieve all the user related information, specifically sensitive info like password? Should I first check if that username exists, then select the salt, compute the password hash and check if there is a user with that username and that password hash?
  • What would you improve?
\$\endgroup\$
2
\$\begingroup\$

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

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
        {
            _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 DetermineSuccessResult();
}

private LoginResult ConfirmUserIsActivated(User user)
{
    if (!user.IsApproved)
    {
        return new LoginResult(RedirectToAction("LoginFailed"), LoginStatus.AccountNotActivated);
    }

    return 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("/\\"));
}

The Login method still is doing too much, but at least it has moved the checking logic out into sub routines. You should now start to see a pattern emerging in the Login method: DoCheck -> CheckResult -> return if failed -> repeat. This can be moved into a chain event where if the first one fails, it returns, if it passes, it calls the next event:

[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("/\\"));
}

With this interation, you get rid of the RetrieveUserResult class.

There are a few more things I'd suggest, but I don't like giving too much information in these all at once.

Hope this helps.

\$\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.