Configure AspNet.Identity to allow for either username OR email address on login

Question 1

I am wondering if there is a more efficient route to take here. Using AspNet.Identity I would like to allow the user to sign in to the same text box using either their usernameor email. I went ahead and addressed this issue in theAccountController Login ActionResult`.

I run the check before calling:

var result = await SignInManager.PasswordSignInAsync(model.Username, 
                                                     model.Password, 
                                                     model.RememberMe, 
                                                     shouldLockout: false);

My method for checking username or email:

// Determine if user has entered their UserName or Email address.
// TODO: research if there is a more efficient way to do this.
using (var db = new ApplicationDbContext())
{
  model.Username = (db.Users.Any(p => p.UserName == model.Username)) ?
  model.Username :
  (db.Users.Any(p => p.Email == model.Username)) ?
  db.Users.SingleOrDefault(p => p.Email == model.Username).UserName :
  model.Username;
}

My concerns here:

Is there a more efficient practical way to do this?
Am I introducing any new security risks or performance risks by doing it this way?

I am including the entire ActionResult below for reference:

//
// POST: /Account/Login
[HttpPost]
[AllowAnonymous]
[ValidateAntiForgeryToken]
public async Task<ActionResult> Login(LoginViewModel model, string returnUrl)
{
  if (!ModelState.IsValid)
  {
    return View(model);
  }

  // Determine if user has entered their UserName or Email address.
  // TODO: research if there is a more efficient way to do this.
  using (var db = new ApplicationDbContext())
  {
    model.Username = (db.Users.Any(p => p.UserName == model.Username)) ?
    model.Username :
    (db.Users.Any(p => p.Email == model.Username)) ?
    db.Users.SingleOrDefault(p => p.Email == model.Username).UserName :
    model.Username;
  }

  // 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.Username, 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);
  }
 }

Question 2

I know this is an old ticket, but I think I might have an approach that uses only one round trip to the database.

using (var db = new ApplicationDbContext())
        {
            string myPhone = String.Join("", model.UserName.Split('(', '-', ')'));

            ApplicationUser myUser = (ApplicationUser)db.Users.FirstOrDefault(u =>
                                                                 u.UserName == model.UserName ||
                                                                 u.Email == model.UserName ||
                                                                 u.PhoneNumber == myPhone);

            if (myUser != null)
            {
                model.UserName = myUser.UserName;
            }
        }

I think this approach generates an OR query, and I believe the rules still apply that conditions are assessed in order, so whichever one proves TRUE first will exit the condition check without looking at any further conditions. I’ve also included a bit of formatting so someone can check against phone numbers ignoring common extra phone number characters.

I had to move the String.Join() method call out of the LINQ query because having it happen inside the query kept throwing an error.

Question 3

I added some formatting to that nested ternery to try to understand what it’s doing, and it still took me a while!

model.Username = (db.Users.Any(p => p.UserName == model.Username)) 
    ? model.Username 
    : (db.Users.Any(p => p.Email == model.Username)) 
        ? db.Users.SingleOrDefault(p => p.Email == model.Username).UserName 
        : model.Username;

It seems to have 3 possible paths, and only one of them actually does anything (since the other two just set model.Username to model.Username).

So it looks like this is actually trying to say:

if(!db.Users.Any(p => p.UserName == model.Username) 
    && db.Users.Any(p => p.Email == model.Username))
{
    model.Username = db.Users.Single(p => p.Email == model.Username).UserName;
}

(Not that I’ve changed SingleOrDefault to Single. This practically shouldn’t have an effect but is a slightly more appropriate method for the situation)

Okay, that’s a little better, and we could extract the conditional to its own method for readability. But it’s still problematic:

It’s making 3 database calls (via EF) to get one piece of information
It’s having to use some pretty roundabout logic to do this. For example, it relies on the fact that no valid email address could also be a valid username. This is domain logic, which shouldn’t be encoded (or repeated) here.

The first point could be improved on with some further rejigging:

var user = db.Users.Where(p => p.UserName == model.Username || p.Email = model.Username)
    .SingleOrDefault();
if(user != null)
{
    model.Username = user.UserName.
}

Here we’re relying on:

Uniqueness of usernames
Uniqueness of email addresses
No string can be valid as both a username and email address

Of those, only the first one is a new assumption compared to the original code

The second point is rather more difficult to deal with. While it would be possible to move the logic for matching against an individual username or email to a User class, it would be hard to do that in a way which EF would still be able to translate to SQL. To avoid adding a lot of complexity, it’s probably best not to try to do this. Do make sure your validation won’t allow for usernames and email addresses which could match, though!

Question 4

Slightly shorter:

using (var db = new ApplicationDbContext())
{
    model.Username = (db.Users.Any(p => p.Email == model.Username)) ?
       db.Users.SingleOrDefault(p => p.Email == model.Username).UserName :
       (db.Users.Any(p => p.UserName == model.Username)) ?
       model.Username : 
       null; // or another value to define missing user account :(
}

Security / performance risks:

From my limited knowledge your security seems “fine”, but the performance might suffer, because you are running at least 2 database queries before even hitting the “real login” method (in the SigninManager).

Nitpicks:

Standard indentation for c# is 4 spaces and not 2 😉
LockedOut status seems superfluous when the comment above states: “Does not count up to lockout”. You could remove:
```
case SignInStatus.LockedOut:
    return View("Lockout");
```
Your comments explain the “What?” and not the “Why?”. I can see from the code, that you are determining the user’s username / email.

Slightly shorter:

Security / performance risks:

Nitpicks:

Leave a Reply Cancel reply