Only focusing on this part
var user = UserManager.Users.Where(u => u.UserName == model.Email).SingleOrDefault(); if (user != null) { if (user.Locked) { return View("Lockout"); } } else { ModelState.AddModelError("", "Invalid login attempt."); return View(model); }
Are you aware that if the Users contains more than one user with the same Email the call to SingleOrDefault will throw an InvalidOperationException ? If you are 100 percent sure that this won't ever happen then there won't be a problem.
In its current state this linq query will for each user query the model for the Email property. If you store it in a variable the access will be much faster.
Checking first if the user == null will make it more obvious what is happening. As I first looked over this code I thought hey, how should the code after the if..else ever be reached.
Applying this will lead to
string email = model.Email;
var user = UserManager.Users.Where(u => u.UserName == email).SingleOrDefault();
if (user == null)
{
ModelState.AddModelError("", "Invalid login attempt.");
return View(model);
}
if (user.Locked)
{
return View("Lockout");
}This looks far better IMO and is more readable.
Usually a username isn't checked in a case sensitive way so instead of u.UserName == email you should use the string.equals() method.
This method takes as the third parameter a StringComparison enum which defines how the comparison will take place. The best one for all cultures, for instance using a turkish locale can make problems (does-your-code-pass-turkey-test), will be to use StringComparison.OrdinalIgnoreCase.
Applying this will lead to
string email = model.Email;
var user = UserManager.Users.Where(u => string.Equals(u.UserName, email, StringComparison.OrdinalIgnoreCase)).SingleOrDefault();
if (user == null)
{
ModelState.AddModelError("", "Invalid login attempt.");
return View(model);
}
if (user.Locked)
{
return View("Lockout");
}
manpreet![Tuteehub forum best answer]()
Best Answer
2 years ago
I'm writing a theoretical MVC application to aid in learning more about ASP Identity 2. In real terms I'm new to ASP Identity as a whole but thought I'd jump in in this release as
it's the default in new projects and
everything I read about it seems to mostly suit my needs.
There is one issue that I need to be able to overcome. That issue is locking out users. I want to allow administrators the facility to be able to lock out a user should they wish to restrict their access.
From what I've read so far, there is a temporary lockout solution for example when the user tries their password incorrectly x number of times for y number of minutes. This doesn't seem to be the recommended method for more long term solutions.
For a longer term solution, I've added a
Lockedproperty to the ApplicationUser class in Entity Framework code first:Getting to the point of my question though, which is, is my modified
ActionResult Loginmethod secure and efficient? I don't want to make unnecessary roundtrips to the database and I also don't want to unwillingly do anything unsecure.