PBKDF2 authorization

I've discovered that using hashed passwords with salts is a much better idea than MD5/SHA256, so I'm not hashing them with PBKDF2. However, I'm wondering if this is a correct approach to authorizing my user. I also have logic for logging authorizations. When the same IP has entered an incorrect login/pass, it becomes banned for 5 minutes.

• Administrator a is passing Username and Password.

• Checking if Username exists.

• Checking if the 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");
}

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?

StackExchange Code Review Q#20507, answer score: 3