Checking locked users using Identity 2.0

Question

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

1. it's the default in new projects and

2. 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 Locked property to the ApplicationUser class in Entity Framework code first:

public class ApplicationUser : IdentityUser
{
 public string FirstName { get; set; }
 public string Surname { get; set; }
 public bool Locked { get; set; }
 [NotMapped]
 public string FullName 
 { 
 get 
 {
 return FirstName + " " + Surname;
 } 
 }
}

Getting to the point of my question though, which is, is my modified ActionResult Login method 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.

public async Task<ActionResult> Login(LoginViewModel model, string returnUrl)
 {
 if (!ModelState.IsValid)
 {
 return View(model);
 }
 // MY LOCKED CHECK CODE:
 // Check if the user is locked out first
 // 1. Fail if it is
 // 2. If user isn't found then return invalid login atempt
 // 3. If the user is found and it isn't locked out then proceed with the login as usual
 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);
 }
 // END MY CODE
 // This doesn't count login failures towards account lockout
 // To enable password failures to trigger account lockout, change to shouldLockout: true
 var result = await SignInManager.PasswordSignInAsync(model.Email, model.Password, model.RememberMe, shouldLockout: false);
 switch (result)
 {
 case SignInStatus.Success:
 return RedirectToLocal(returnUrl);
 case SignInStatus.LockedOut:
 return View("Lockout");
 case SignInStatus.RequiresVerification:
 return RedirectToAction("SendCode", new { ReturnUrl = returnUrl, RememberMe = model.RememberMe });
 case SignInStatus.Failure:
 default:
 ModelState.AddModelError("", "Invalid login attempt.");
 return View(model);
 }
 }

• SirPython: Welcome to Code Review! I hope you get some helpful answers.
• paulpitchford: Thank you. I hope my first question is stuctured correctly and described well enough.
• SirPython: It looks just fine to me.

Answer 1

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");
 }

• paulpitchford: The extra information about StringComparison.OrdinalIgnoreCase was excellent.