User Authentication Bundle

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 PasswordResetRequestViewMode

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

-

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.

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

-
Why do you encrypt UserContact.Contact?

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

-
UserContact.ContactTypeId is not really an id - just rename it to ContactType.

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

-
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. :)

StackExchange Code Review Q#56241, answer score: 4