6
\$\begingroup\$

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:

  1. Is there a more efficient practical way to do this?
  2. 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);
  }
 }
\$\endgroup\$
  • \$\begingroup\$ What does SignInManager do? My first thoughts would be the logic for using username oremail should be in this class or at least outside of the controller as this seems like a business rule which ideally should not be contained in a controller. \$\endgroup\$ – dreza Sep 22 '14 at 4:22
  • \$\begingroup\$ @dreza SignInManager is part of AspNet.Identity \$\endgroup\$ – aaronmallen Sep 22 '14 at 5:56
  • \$\begingroup\$ ah really. did not know that, awesome. So many ways to skin the cat! \$\endgroup\$ – dreza Sep 22 '14 at 6:59
6
\$\begingroup\$

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.

\$\endgroup\$
7
\$\begingroup\$

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!

\$\endgroup\$
2
\$\begingroup\$

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 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.
\$\endgroup\$
  • \$\begingroup\$ LockedOut status is part of AspNet.Identity. All of this code is generated when the project is created with the exception of my addition. I assure you it is properly formatted in my code, copy paste from visual studios to stack overflow has it's quirks. I agree the comments do explain the what not the why, however currently their more of a placeholder than anything else until I determine the most efficient route to take. As far as your actual answer is concerned I don't see any performance benefits vs mine. \$\endgroup\$ – aaronmallen Sep 22 '14 at 21:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.