HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
identityaddressemailloginusernameeitherallowforaspnetconfigure

Problem

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 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 en

Solution

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!

Code Snippets

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;
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;
}
var user = db.Users.Where(p => p.UserName == model.Username || p.Email = model.Username)
    .SingleOrDefault();
if(user != null)
{
    model.Username = user.UserName.
}

Context

StackExchange Code Review Q#63523, answer score: 8

Revisions (0)

No revisions yet.