Identity OAuth implementation with Owin

Question

I am building an MVC 5 application using code-first Entity Framework 6 and a customised ASP.NET Identity model for authentication.

I will only be authenticating against external OAuth providers (Google, Facebook etc) and persisting the login and claims information in custom code-first EF entities.

Since this is the first time I have worked with the ASP.NET Identity model and Owin, I want to make sure I haven't made any major errors in the code I have so far, which does work and registers a new user, saves their claims and persists their login using a cookie.

I will try to keep the code as small as possible while still showing all relevant details. I am using AutoFac for dependency injection.

User.cs - Implementation of IdentityUser

public class User : IdentityUser<string, UserLogin, UserRole, UserClaim>
{
    public User()
    {
        // without this Id would be NULL when I create a new User
        base.Id = Guid.NewGuid().ToString();
    }

    public User(string userName)
        : this()
    {
        base.UserName = userName;
    }

    .... other properties
}

UserLogin, UserRole and UserClaim are simply classes that implement their relevant IdentityUserX base class.

DbContext.cs - Standard DbContext implementing IdentityDbContext with above classes

public class DbContext : IdentityDbContext<User, Role, string, UserLogin, UserRole, UserClaim>
{}

UserService.cs - Implementation of UserStore

public class UserService : UserStore<User, Role, string, UserLogin, UserRole, UserClaim>
{}

RoleService.cs - Implementation of RoleStore

public class RoleService : RoleStore<Role, string, UserRole>
{}

ApplicationClaimsIdentityFactory.cs - reimplementation of ClaimsIdentityFactory. Needed because I was getting duplicate claims added because the Name and NameIdentifier claims are saved into the database, and re-added at sign in.

public class ApplicationClaimsIdentityFactory : ClaimsIdentityFactory<User, string>
{
    // identical to normal ClaimsIdentityFactory until:
        if (manager.SupportsUserClaim)
        {
            var userClaims = await manager.GetClaimsAsync(user.Id).WithCurrentCulture();
            foreach (var userClaim in userClaims)
            {
                if (!id.HasClaim(userClaim.Type, userClaim.Value))
                {
                    id.AddClaim(userClaim);
                }
            }
        }
        return id;
    }
}

ApplicationUserManager.cs - Implementation of UserManager specifying to use custom ClaimsIdentityFactory

public class ApplicationUserManager : UserManager<User, string>
{
    public ApplicationUserManager(UserStore<User, Role, string, UserLogin, UserRole, UserClaim> store) : base(store)
    {
        this.ClaimsIdentityFactory = new ApplicationClaimsIdentityFactory();
    }

    public static ApplicationUserManager Create(IdentityFactoryOptions<ApplicationUserManager>options, IOwinContext context)
    {
        var manager = new ApplicationUserManager(new UserService(context.Get<DbContext>()));
        return manager;
    }
}

ApplicationSignInManager.cs - Implementation of SignInManager with new entities

public class ApplicationSignInManager: SignInManager<User, string>
{
    public ApplicationSignInManager(ApplicationUserManager userManager, IAuthenticationManager authManager)
        : base(userManager, authManager)
    {
    }

    public static ApplicationSignInManager Create(IdentityFactoryOptions<ApplicationSignInManager> options, IOwinContext context)
    {
        return new ApplicationSignInManager(context.GetUserManager<ApplicationUserManager>(), context.Authentication);
    }
}

Startup.Auth.cs

public partial class Startup
{
    static Startup()
    {
        PublicClientId = "self";
        ExternalAuthPageUrl = "Account/ExternalAuthRequestCallback";
        var applicationOAuthProvider = new ApplicationOAuthProvider(PublicClientId, ExternalAuthPageUrl);
        OAuthOptions = new OAuthAuthorizationServerOptions
        {
            Provider = applicationOAuthProvider,
            RefreshTokenProvider = applicationOAuthProvider,
            AuthorizeEndpointPath = new PathString("/Account/ExternalLogin"),
            AccessTokenExpireTimeSpan = TimeSpan.FromDays(30),
            AllowInsecureHttp = false
        };
    }

    public static OAuthAuthorizationServerOptions OAuthOptions { get; private set; }
    public static string PublicClientId { get; private set; }
    public static string ExternalAuthPageUrl { get; private set; }

    public void ConfigureAuth(IAppBuilder app)
    {            
        app.UseCookieAuthentication(new CookieAuthenticationOptions
        {
            AuthenticationType = DefaultAuthenticationTypes.ApplicationCookie,
            LoginPath = new PathString("/Account/Login"),
            Provider = new CookieAuthenticationProvider
            {
                OnValidateIdentity = SecurityStampValidator.OnValidateIdentity<ApplicationUserManager, User>(
                                        validateInterval: TimeSpan.FromMinutes(15),
                                        regenerateIdentity: (manager, user) => user.GenerateUserIdentityAsync(manager))
            },
            SlidingExpiration = true,
            ExpireTimeSpan = TimeSpan.FromMinutes(30)
        });

        app.UseExternalSignInCookie(DefaultAuthenticationTypes.ExternalCookie);

        app.UseOAuthBearerTokens(OAuthOptions);

        var authOptions = new GoogleOAuth2AuthenticationOptions();
        authOptions.ClientId = "OAuthKey";
        authOptions.ClientSecret = "OAuthSecret";
        authOptions.Scope.Add("email");

        app.UseGoogleAuthentication(authOptions);
    }
}

AccountController.cs

[AllowAnonymous]
// displays a list of available external authenticators for the user to pick
public ActionResult Login(string returnUrl = "/")
{
    IEnumerable<AuthenticationDescription> descriptions = Request.GetOwinContext().Authentication.GetExternalAuthenticationTypes();
    IList<ExternalProviderViewModel> providers = new List<ExternalProviderViewModel>();

    foreach (AuthenticationDescription description in descriptions)
    {
        ExternalProviderViewModel model = new ExternalProviderViewModel
        {
            Url = Url.Action("ExternalLogin", new
            {
                provider = description.AuthenticationType,
                response_type = "token",
                client_id = Startup.PublicClientId,
                redirect_uri = new Uri(Request.Url, returnUrl).AbsoluteUri + Startup.ExternalAuthPageUrl
            })
        };

        providers.Add(model);
    }
    return View(providers);
}

// Once a user picks a provider
[AllowAnonymous]
public ActionResult ExternalLogin(string provider, string error = null)
{
    return new ChallengeResult(loginProvider.Name, Url.Action("ExternalLoginCallback", "Account"));
}

// On return from provider
[AllowAnonymous]
public async Task<ActionResult> ExternalLoginCallback(string returnUrl)
{
    var loginInfo = await _authManager.GetExternalLoginInfoAsync();
    if (loginInfo == null)
    {
        return RedirectToAction("Login");
    }

    // Sign in the user with this external login provider if the user already has a login
    var result = await _signInManager.ExternalSignInAsync(loginInfo, true);
    switch (result)
    {
        case SignInStatus.Success:
            return RedirectToLocal(returnUrl);
        case SignInStatus.LockedOut:
            return View("Lockout");
        default:
            // If the user does not have an account, then prompt the user to create an account
            ViewBag.ReturnUrl = returnUrl;
            ViewBag.LoginProvider = loginInfo.Login.LoginProvider;
            return View("ExternalLoginConfirmation", new ExternalLoginConfirmationViewModel { Email = loginInfo.Email });
    }
}

// The External Login Confirmation view just confirms the user's email address, then submits to:

[HttpPost]
[AllowAnonymous]
[ValidateAntiForgeryToken]
public async Task<ActionResult> ExternalLoginConfirmation(ExternalLoginConfirmationViewModel model, string returnUrl)
{
    if (ModelState.IsValid)
    {
        // Get the information about the user from the external login provider
        var info = await _authManager.GetExternalLoginInfoAsync();
        var user = new User { UserName = model.Email, Email = model.Email };
        var result = await _userManager.CreateAsync(user);
        if (result.Succeeded)
        {
            result = await _userManager.AddLoginAsync(user.Id, info.Login);
            if (result.Succeeded)
            {
                await _signInManager.SignInAsync(user, true, true);
                ExternalLoginModel externalLogin = ExternalLoginModel.FromIdentity( info);
                OwinHelper.SignIn(Request.GetOwinContext(), user, externalLogin, _userManager);

                return RedirectToLocal(returnUrl);
            }
        }
    }
    ViewBag.ReturnUrl = returnUrl;
    return View(model);
}

OwinHelper.cs

public static class OwinHelper
{
    public static void SignIn(IOwinContext owinContext, User user, ExternalLoginModel externalLogin, ApplicationUserManager userManager)
    {
        owinContext.Authentication.SignOut(DefaultAuthenticationTypes.ExternalCookie);
        var oAuthIdentity = CreateIdentity(user, OAuthDefaults.AuthenticationType);
        var cookieIdentity = CreateIdentity(user, CookieAuthenticationDefaults.AuthenticationType);
        SaveClaims(oAuthIdentity, user, userManager);
        var properties = CreateProperties(user);
        owinContext.Authentication.SignIn(properties, oAuthIdentity, cookieIdentity);
    }

    private static ClaimsIdentity CreateIdentity(User user, string authenticationType)
    {
        IList<Claim> claims = new List<Claim>();

        claims.Add(new Claim(ClaimTypes.NameIdentifier, user.Id, null, ClaimsIdentity.DefaultIssuer, "Provider"));
        claims.Add(new Claim(ClaimTypes.Name, user.FullName, null, ClaimsIdentity.DefaultIssuer, "Provider"));
        claims.Add(new Claim(ClaimTypes.Email, user.Email, null, ClaimsIdentity.DefaultIssuer, "Provider"));
        claims.Add(new Claim(ClaimTypes.GivenName, user.FullName, null, ClaimsIdentity.DefaultIssuer, "Provider"));

        return new ClaimsIdentity(claims, authenticationType);
    }

    private static void SaveClaims(ClaimsIdentity identity, User user, ApplicationUserManager userManager)
    {
        foreach (var claim in identity.Claims)
        {
            if (identity.HasClaim(claim.Type, claim.Value))
            {
                userManager.RemoveClaim(user.Id, claim);
            }
            userManager.AddClaim(user.Id, claim);
        }
    }

    public static AuthenticationProperties CreateProperties(User user)
    {
        IDictionary<string, string> data = new Dictionary<string, string>
        {
            { "email", user.Email },
            { "name", user.FirstName ?? string.Empty }
        };
        return new AuthenticationProperties(data);
    }
}

I think that's everything that's relevant. Like I said, I'm looking for a review of my Identity and OAuth implementation, not general code quality. I've removed a lot of Null checks and alike to try and shorten the code a bit.

Please let me know if there's something you feel I might have missed, and any comments on my code I'm happy to receive.

Answer 1

A static class named OwinHelper, with methods SignIn, CreateIdentity and CreateProperty, and SaveClaims, smells funny. "Helper" is a bad sign all by itself ("Manager" is also a sign), static is another bad sign, and the public members are a bad sign: the class has all the characteristics of a type with already too many responsibilities, that will scale by growing hair and tentacles.

These static "helper" methods probably belong as private members / implementation details of the types they're "helping".

public static AuthenticationProperties CreateProperties(User user)
{
    IDictionary<string, string> data = new Dictionary<string, string>
    {
        { "email", user.Email },
        { "name", user.FirstName ?? string.Empty }
    };
    return new AuthenticationProperties(data);
}

Consider these members as part of the User class:

private readonly Lazy<AuthenticationProperties> _authProperties;
public AuthenticationProperties AuthProperties { get { return _authProperties.Value; } }

I'm assuming the members you stripped from your post are private readonly and only ever assigned from the constructor: if the User type isn't immutable then you have another problem. Anyway the idea is to create that Lazy<T> in the constructor when you're receiving the Email and FirstName, and because your user is immutable you don't need to worry about whether these values are going to change by the time the AuthProperties are needed, so you can just cache an instance.

---

This method:

private static ClaimsIdentity CreateIdentity(User user, string authenticationType)
{
    IList<Claim> claims = new List<Claim>();

    claims.Add(new Claim(ClaimTypes.NameIdentifier, user.Id, null, ClaimsIdentity.DefaultIssuer, "Provider"));
    claims.Add(new Claim(ClaimTypes.Name, user.FullName, null, ClaimsIdentity.DefaultIssuer, "Provider"));
    claims.Add(new Claim(ClaimTypes.Email, user.Email, null, ClaimsIdentity.DefaultIssuer, "Provider"));
    claims.Add(new Claim(ClaimTypes.GivenName, user.FullName, null, ClaimsIdentity.DefaultIssuer, "Provider"));

    return new ClaimsIdentity(claims, authenticationType);
}

Could be simplified to this:

private static ClaimsIdentity CreateIdentity(User user, string authenticationType)
{
    var claims = new List<Claim>
    {
        new Claim(ClaimTypes.NameIdentifier, user.Id, null, ClaimsIdentity.DefaultIssuer, "Provider"),
        new Claim(ClaimTypes.Name, user.FullName, null, ClaimsIdentity.DefaultIssuer, "Provider"),
        new Claim(ClaimTypes.Email, user.Email, null, ClaimsIdentity.DefaultIssuer, "Provider"),
        new Claim(ClaimTypes.GivenName, user.FullName, null, ClaimsIdentity.DefaultIssuer, "Provider")
    };

    return new ClaimsIdentity(claims, authenticationType);
}

That's a collection initializer syntax calling .Add under the hood for you... but you know that, since you're using it to initialize a Dictionary a few lines further...

Notice I used var here, because I use it everywhere, var is awesome! it's obvious what the type is.

I find it ironic that you wouldn't specify var here:

IDictionary<string, string> data = new Dictionary<string, string>

Or here:

IList<Claim> claims = new List<Claim>();

But that you would do it here:

var oAuthIdentity = CreateIdentity(user, OAuthDefaults.AuthenticationType);
var cookieIdentity = CreateIdentity(user, CookieAuthenticationDefaults.AuthenticationType);
var properties = CreateProperties(user);

Or even here:

foreach (var claim in identity.Claims)

Don't get me wrong: I love var and I really abuse it a lot. I can live with a codebase that doesn't use it everywhere, but in a codebase that uses it sparingly, I like to see consistency, and I expect to see it where it's useful (i.e. when the type is redundant boilerplate clutter) - not where the type isn't obvious from the statement itself!