Skip to main content

Asp.net mvcMVC login method

  • Is it ok to retrieve all the user related information, specifically sensitive info like password? Should iI first check if that username exists, thethen 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?

Asp.net mvc login method

  • Is it ok to retrieve all the user related information, specifically sensitive info like password? Should i first check if that username exists, the 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?

Asp.net MVC login method

  • 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?
Tweeted twitter.com/#!/StackCodeReview/status/242864922801868800
Source Link
gigi
  • 131
  • 1
  • 3

Asp.net mvc login method

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, the 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?