4
\$\begingroup\$

I wrote a user authentication program(s) for an MVC application. Before you ask part of the project specs are I have to store user information in company databases on servers that aren't the web server so the default membership classes are out of the question (as far as I am aware membership creates its own database). If you would be so kind and let me know how I did.

The Entities

public enum ContactType
{
  EmailPrimary,
  Email,
  PhonePrimary,
  Phone,
  Facebook,
  Twitter,
  Google,
  Github,
  BitBucket,
  Microsoft
}
public class User
{
  [Key]
  public string UserName { get; set; }
  public string NamePrefix { get; set; }
  public string NameFirst { get; set; }
  public string NameMiddle { get; set; }
  public string NameLast { get; set; }
  public string NameSuffix { get; set; }
  public DateTime? DateOfBirth { get; set; }
}
public class UserAccount
{
  [Key]
  public int AccountId { get; set; }
  public string UserName { get; set; }
  public string PasswordHash { get; set; }
  public bool Lock { get; set; }
  public DateTime Created { get; set; }
  public DateTime? Updated { get; set; }
  public virtual User User { get; set; }
}
public class UserContact
{
  [Key]
  public int ContactId { get; set; }
  public ContactType ContactTypeId { get; set; }
  public string Contact { get; set; }
  public string UserName { get; set; }
  public bool Active { get; set; }
  public DateTime Created { get; set; }
  public DateTime? Updated { get; set; }
  public virtual User User { get; set; }
}

The View Models

public class LogInViewModel
{
  public string UserName { get; set; }
  public string Password { get; set; }
  public bool RememberMe { get; set; }
}
public class RegisterViewModel
{
  public string UserName { get; set; }
  public string Password { get; set; }
  [Compare("Password", ErrorMessage="Passwords do not match.")]
  public string ConfirmPassword { get; set; }
  public string UserEmail { get; set; }
}
public class PasswordResetRequestViewModel
{
  public string UserName { get; set; }
}
public class PasswordResetModel
{
  public string Password { get; set; }
  [Compare("Password", ErrorMessage="Passwords do not match.")]
  public string ConfirmPassword { get; set; }
}

NOTE: Data annotations are on the TODO list for both the entities and the view models.

The Controller

private IAuthService authService = new  AuthService();
public ActionResult Index()
{
  return View();
}
[HttpPost()]
public ActionResult Index(LogInViewModel model)
{
  var user = authService.GetAccount(model.UserName);
  if (authService.IsLocked(user))
  {
    RedirectToAction("ResetPassword");
  }
  if (ModelState.IsValid)
  {
   switch (authService.AttemptLogin(user, model.Password).IsAuthenticated)
   {
     case true:
       FormsAuthentication.SetAuthCookie(user.UserName, model.RememberMe);
       // UNDONE: Where does the user go after login?
       break;
     case false:
       ModelState.AddModelError("", "Username or Password incorrect.");
       break;
   }
  }
  return View(model);
}
public ActionResult Register()
{
  return View();
}
[HttpPost()]
public ActionResult Register(RegisterViewModel model)
{
  var newUser = authService.CreateAccount(model);
  if (newUser != null)
  {
    // TODO: Send verification email
    RedirectToAction("Index");
  }
  return View(model);
}
public ActionResult PasswordResetRequest()
{
  return View();
}
[HttpPost()]
public ActionResult PasswordResetRequest(PasswordResetRequestViewModel model)
{
  var passwordResetRequest = authService.CreatePasswordResetRequest(model.UserName);
  if (passwordResetRequest != null)
  {
    // TODO: Send password reset email
  }
  return View(model);
}
public ActionResult PasswordReset(string id)
{
  return View();
}
[HttpPost()]
public ActionResult PasswordReset(string id, PasswordResetModel model)
{
  string cipher = id.Substring(0, 16);
  string userName = StringCipher.Decrypt(id.Substring(16, id.Length - 16), cipher);
  if (authService.ResetUserPassword(userName, model.Password) != null)
  {
    // TODO: do something to indicate password reset success.
    // either log the user in directly from here or
    // redirect them to log in with new password.
  }
  return View(model);
}

NOTE: The idea is if they need to reset their password an email will be sent with an URL and an id that is an encrypted string that equates to their UserName. I do have some concerns with this method.

The Interface/Service Class

public interface IAuthService
{
  UserAccount GetAccount(string userName);
  UserAccount CreateAccount(RegisterViewModel newAccount);
  PasswordResetRequest CreatePasswordResetRequest(string userName);
  UserAccount ResetUserPassword(string userName, string password);
  bool IsLocked(UserAccount account);
  AuthenticatedUser AttemptLogin(UserAccount account, string password);
}
public class AuthService : IAuthService
{
  private WebContext db = new WebContext();
  public UserAccount GetAccount(string userName)
  {
    return db.UserAccounts.SingleOrDefault(u=> u.UserName == userName);
  }
  public UserAccount CreateAccount(RegisterViewModel newAccount)
  {
    var user = GetAccount(newAccount.UserName);
    if (user != null)
    {
      var accountBuilder = new UserAccountBuilder(newAccount);
      Account account = accountBuilder.CreateAccount();
      db.Entry(account.UserAccount).State = EntityState.Added;
      db.Entry(account.User).State = EntityState.Added;
      db.Entry(account.UserContact).State = EntityState.Added;
      db.SaveChanges();
      return account.UserAccount;
    }
    return null;
  }
  public PasswordResetRequest CreatePasswordResetRequest(string userName)
  {
    UserContact contact = db.UserContacts.FirstOrDefault(p => p.UserName == userName &
    p.ContactTypeId == ContactType.EmailPrimary);
    if (contact != null)
    {
      string cipher = StringCipher.GenerateCypher();
      return new PasswordResetRequest()
      {
        UserEmail = contact.Contact,
        Key = cipher + StringCipher.Encrypt(userName, cipher)
      };
    }
    return null;
  }
  public UserAccount ResetUserPassword(string userName, string password)
  {
    UserAccount account = GetAccount(userName);
    if (account != null)
    {
      account.PasswordHash = Crypto.HashPassword(password);
      account.Updated = DateTime.UtcNow;
      db.Entry(account).State = EntityState.Modified;
      db.SaveChanges();
      return account;
    }
    return null;
  }
  public bool IsLocked(UserAccount account)
  {
    return account.Lock;
  }
  public AuthenticatedUser AttemptLogin(UserAccount account, string password)
  {
    if (IsValidPassword(account, password))
    {
      return new AuthenticatedUser(account.User, true);
    }
    else
    {
      return new AuthenticatedUser(account.User);
    }
  }
  private bool IsValidPassword(UserAccount account, string password)
  {
    return Crypto.VerifyHashedPassword(account.PasswordHash, password);
  }
  private bool AccountAlreadyExists(string UserName)
  {
    if (db.UserAccounts.Count(p => p.UserName == UserName) == 0)
    {
      return false;
    }
    return true;
  }
}
public class AuthenticatedUser
{
  public User User { get; set; }
  public bool IsAuthenticated { get; private set; }
  public AuthenticatedUser(User user) : this(user, false)
  {
  }
  public AuthenticatedUser(User user, bool isAuthenticated)
  {
    User = user;
    IsAuthenticated = isAuthenticated;
  }
}
public class Account
{
  public UserAccount UserAccount { get; set; }
  public User User { get; set; }
  public UserContact UserContact { get; set; }
}
public class PasswordResetRequest
{
  public string UserEmail { get; set; }
  public string Key { get; set; }
}
internal class UserAccountBuilder
{
  private readonly RegisterViewModel newAccount;
  public UserAccountBuilder(RegisterViewModel account)
  {
    newAccount = account;
  }
  public Account CreateAccount()
  {
    return new Account()
    {
      UserAccount = CreateUserAccount(),
      User = CreateUser(),
      UserContact = CreateUserContact()
    };
  }
  private UserAccount CreateUserAccount()
  {
    return new UserAccount()
    {
      UserName = newAccount.UserName,
      PasswordHash = CreatePasswordHash(newAccount.Password),
      Lock = true,
      Created = DateTime.UtcNow
    };
  }
  private User CreateUser()
  {
    return new User()
    {
      UserName = newAccount.UserName
    };
  }
  private UserContact CreateUserContact()
  {
    string cipher = StringCipher.GenerateCypher();
    return new UserContact()
    {
      ContactTypeId = ContactType.EmailPrimary,
      Contact = cipher + StringCipher.Encrypt(newAccount.UserEmail, cipher),
      UserName = newAccount.UserName,
      Active = true,
      Created = DateTime.UtcNow
    };
  }
  private string CreatePasswordHash(string password)
  {
    return Crypto.HashPassword(password);
  }
}

NOTE: I didn't feel it necessary to include the context models. If you disagree with this decision I would be more than happy to post them. Rest assured WebContext is an instance of a context class. I also didn't feel the need to include the Helper classes I've create for encryption as they are readily available online. Anytime I call StringCipher I am referring to that helper class.

Now that you've taken the time to read all that (and thank you for that), please "let me have it" so to speak. As the tags of this post indicate I am in no way amazingly experienced with C# and have only a year of .NET experience under my belt. What "best practices" am I killing here that make you cringe? What can I do differently more efficiently? How would you make this code better, or perhaps more importantly how would you write this differently?

\$\endgroup\$
  • \$\begingroup\$ Is there any particular reason why do you have Username on all tables: User, UserAccount and UserContact? \$\endgroup\$ – dreza Jul 6 '14 at 7:39
  • \$\begingroup\$ It's is the User PK and FK every where else. \$\endgroup\$ – aaronmallen Jul 6 '14 at 14:25
3
\$\begingroup\$

or perhaps more importantly how would you write this differently?

I would just use ASP.NET Identity (http://www.asp.net/identity). Microsoft did a lot of the thinking for you there, no need to re-invent the wheel and you can just plugin your custom model in their system. There is a lot more security wise that you are currently missing.

How would you make this code better

I don't want to go into the security parts. I just looked at your code, method by method and did a review that way. That might be the most helpful for now, because in the above I said drop this code and use the code provided by Microsoft :-)

The Entities

Small things:

Make the Updated properties not nullable, this saves you null checking. We can easily say that when the account is created it is also updated. Might be just a matter of preference.

You might want a soft delete (IsDeleted), but that is totally depended upon the requirements.

The Controller

The method Index(LogInViewModel model)

  • you do the isLocked check without knowing if the ModelState is valid.
  • If I see a switch statement I assume there are more then 3 cases, there are only 2 (true|false)

I would end up with something like this:

[HttpPost]
public ActionResult Index(LogInViewModel model)
{
    if (!ModelState.IsValid)
    {
        return View(model);
    }

    var user = authService.GetAccount(model.UserName);
    if (authService.IsLocked(user))
    {
        RedirectToAction("ResetPassword");
    }

    if (authService.AttemptLogin(user, model.Password).IsAuthenticated)
    {
        FormsAuthentication.SetAuthCookie(user.UserName, model.RememberMe);
        return View(model); // UNDONE: Where does the user go after login?
    }

    ModelState.AddModelError(string.Empty, "Username or Password incorrect.");
    return View(model);
}

The methods below aren't checking for a valid ModelState, while the login method is.

public ActionResult Register(RegisterViewModel model) { }
public ActionResult PasswordReset(string id, PasswordResetModel model) { }
public ActionResult PasswordResetRequest(PasswordResetRequestViewModel model) { }

Responsibility of the controller methods: Not by definition a good or a bad thing, but currently there is a lot of the responsibility of your registration process in your controller. It's not delegating work to (others) services, but doing a lot of the work itself. It is a fat controller. So if you need the logic somewhere else you need the service and the controller, instead of only the services.

The Interface/Service Class

public UserAccount GetAccount(string userName)
{
    return db.UserAccounts.SingleOrDefault(u=> u.UserName == userName);
}

Small notes:

  • you are mixing FirstOrDefault and SingleOrDefault, might be more clear to just use one, because the result is the same in this case.
  • Might be best to perform toLowerCaseInvariant() and always store usernames in lower case. Or upper case if you prefer that. So that the username isn't case sensitive. But that's a requirements issue.

    public UserAccount CreateAccount(RegisterViewModel newAccount)

Why should your Authentication service be aware of the contract you have between the controller and the view model? Might be better to use simple parameters here, so newAccount.Username, newAccount.Password, etc. Or make an interface that should be implemented by the viewmodel class.

public bool IsLocked(UserAccount account)
{
    return account.Lock;
}

What is the difference between doing this, and just renaming the property on the account to IsLocked?

public AuthenticatedUser AttemptLogin(UserAccount account, string password)
{
    return new AuthenticatedUser(account.User, IsValidPassword(account, password));
}

Small one: that saves you an if statement/ complexity.

private bool AccountAlreadyExists(string userName)
{   
    return db.UserAccounts.Any(p => p.UserName.Equals(userName));
}

My personal preference over doing Count.

Running out of time so stopped here.

Some fast notes, without looking what is what:

The class UserAccountBuilder looks like some sort of factory method to handle the complexity in the data model. Might be better to check if the data model can be made less complex.

There currently isn't much exception checking/handeling, but that's probaly because it's work in progress.

\$\endgroup\$
4
\$\begingroup\$
  1. UserName as PK/CK of the User table and thus FK on all tables like UserContact may be a bad design choice. Especially if you have a lot of records, it may lead to index fragmentation.

    Suggestion: Add a [Key] public int UserId { get; set; } property and reference that in other tables. Set the username to be a unique index [Index(IsUnique = true)] public string UserName { get; set; }

  2. NOTE: The idea is if they need to reset their password an email will be sent with an URL and an id that is an encrypted string that equates to their UserName. I do have some concerns with this method.

    I think your concerns are absolutely valid. A huge problem with this approach is that yes, you create a one-time encryption key for the reset token every time it is requested, but all of the generated tokens will be valid forever. If someone ever intercepts a user's password reset link, they will be able to change the user's password until you change their username. A better approach would be to generate a secure one-time token and store that with the user's id/username in your database (add some spam protection (max. 5 current reset links or so), so that they cannot DOS your database by requesting billions of password reset links :)) and add an expiry date for the value.

    This is a really good read about mistakes that can be done when building a password reset functionality into a website.

  3. The naming of NameLast, NameMiddle, etc. looks a bit unusual to me. Most systems I have seen just use LastName, FirstName and so on.

  4. Why do you encrypt UserContact.Contact?

  5. In AccountAlreadyExists you are not really interested if there is more than one user with that username - you just want to know if there is any user matching the name. This results in a different SQL query (EXISTS instead of COUNT) that represents that intent better. (Of course, the performance impact should be negligible; Parameter name changed to camelCase for consistency.)

    private bool AccountAlreadyExists(string userName)
    {
        return db.UserAccounts.Any(p => p.UserName == userName);
    }
    
  6. UserContact.ContactTypeId is not really an id - just rename it to ContactType.

  7. You may want to consider moving the lifetime of WebContext to the HTTP request. If you then inject that into every service class that you use, all of them will use the same EF context, it's cache, etc. But more importantly, the context will be disposed correctly at the end of the request. If you do not want to do this, you should implement IDisposable in your AuthService and then dispose the webContext object. Of course, the auth service would then need to be disposed in the controller's Dispose method. Since this is much more work and needs to be done for every service and controller in your project, I would suggest using the "DbContext per request" approach.

  8. If you want to use Created and Modified columns on several tables, it might be worth looking into setting those values automatically. Saves some code in the services and helps you to not forget it. :)

\$\endgroup\$
  • \$\begingroup\$ 4. The contact is the users email address, would you not recommend encryption here? \$\endgroup\$ – aaronmallen Jul 6 '14 at 15:34
  • \$\begingroup\$ I get the idea, but as the encryption key is saved directly alongside the actual encrypted value, I do not know if it really helps to protect the data a lot. In the case that someone gains access to the database, they will probably figure out the value somehow. By encrypting the mail address, you would have no efficient way of providing a "I forgot my password and username, but do know the mail address!" functionality, for example. \$\endgroup\$ – hangy Jul 6 '14 at 15:44
  • \$\begingroup\$ Question: when you hash a password it is fairly typical to keep the salt and password stored next to each other, what would be the difference here? Is it because I'm encrypting instead of hashing that your concerned? Also I regain all functionality simply by decrypting the string. \$\endgroup\$ – aaronmallen Jul 6 '14 at 15:50
  • 1
    \$\begingroup\$ We hash a password, because you do not want it to be reversible. We add a salt against precomputed hashes etc., and that salt has to be stored next to the hash, because it is required for any comparison. Honestly, I think encrypting the address does not hurt a lot (as you said, you can always decrypt it), but I do not see a huge benefit by doing so either. :) \$\endgroup\$ – hangy Jul 7 '14 at 8:19

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.