Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've discovered that using hashed passwords with salts is much better idea than MD5/SHA256 so I'm not hashing them with PBKDF2. However I'm wondering if this is correct approach to authorize my user. I also have logic for logging authorizations and when same IP did wrong login/pass it is banned for 5 minutes.

  • Administrator a is passing username and password
  • Checking if username exists
  • Checking if password is correct for this user
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Authorize(Administrator a)
{
    // Check if there are no failed login attempts in last 5 minutes
    if (!this.CanAdminLogin)
    {
        TempData["loginTooManyAttempts"] = true;
        return RedirectToAction("index", "home");
    }

    // If model is not validated return login view to show error messages (javascript disabled)
    if (!TryValidateModel(a))
    {
        return View("Authorize", a);
    }

    // Check if username exists, if not then log and show that login failed
    var admin = _db.Administrators.Where(x => x.Username == a.Username).SingleOrDefault();
    if (admin == null || admin.Username != a.Username)
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
        return RedirectToAction("index", "home");
    }

    // Username exists, check if passwords match
    ICryptoService cryptoService = new PBKDF2();
    string hash = cryptoService.Compute(a.Password, admin.PasswordSalt);
    if (hash == admin.Password)
    {
        this.LogAuthorization(a, true);
        Session["adminId"] = admin.ID;
    }
    else
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
    }

    // Login successfull
    return RedirectToAction("index", "home");
}
share|improve this question
    
I think code like _db.Administrators.Where(...) should be put in a repository to aid Unit Testing. –  rhughes May 23 '14 at 3:47

1 Answer 1

I really think that you should merge all of these if statements, make them an if/elseif/else statement. just move all the variable declarations to the top.

then it turns into this

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Authorize(Administrator a)
{
    var admin = _db.Administrators.Where(x => x.Username == a.Username).SingleOrDefault();
    ICryptoService cryptoService = new PBKDF2();
    string hash = cryptoService.Compute(a.Password, admin.PasswordSalt);
    if (!this.CanAdminLogin) // Check if there are no failed login attempts in last 5 minutes
    {
        TempData["loginTooManyAttempts"] = true;
        return RedirectToAction("index", "home");
    }
    else if (!TryValidateModel(a)) // If model is not validated return login view to show error messages (javascript disabled)
    {
        return View("Authorize", a);
    }
    else if (admin == null || admin.Username != a.Username) // Check if username exists, if not then log and show that login failed
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
        return RedirectToAction("index", "home");
    }
    else if (hash == admin.Password) // Username exists, check if passwords match
    {
        this.LogAuthorization(a, true);
        Session["adminId"] = admin.ID;
    }
    else
    {
        this.LogAuthorization(a, false);
        TempData["loginFailed"] = true;
    }

    // Login successfull
    return RedirectToAction("index", "home");
}

I also moved the comments to the end of the line that they attach to. Keep in mind that some of these comments are not needed at all.

The Comments that are needed here

// Check if there are no failed login attempts in last 5 minutes

This is only needed because otherwise I wouldn't have known why the Admin couldn't log in or that there was an instance that would change this boolean.

// If model is not validated return login view to show error messages (javascript disabled)

I can tell from the code what is going on, but I am not sure what (javascript disabled) has to do with this code?

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.