Categories

See More
Popular Forum

MBA (4887) B.Tech (1769) Engineering (1486) Class 12 (1030) Study Abroad (1004) Computer Science and Engineering (988) Business Management Studies (865) BBA (846) Diploma (746) CAT (651) B.Com (648) B.Sc (643) JEE Mains (618) Mechanical Engineering (574) Exam (525) India (462) Career (452) All Time Q&A (439) Mass Communication (427) BCA (417) Science (384) Computers & IT (Non-Engg) (383) Medicine & Health Sciences (381) Hotel Management (373) Civil Engineering (353) MCA (349) Tuteehub Top Questions (348) Distance (340) Colleges in India (334)
See More

Checking locked users using Identity 2.0

General Tech Learning Aids/Tools

Max. 2000 characters
Replies

usr_profile.png

User

( 5 months 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

  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
usr_profile.png

User

( 5 months ago )

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

what's your interest