Identity OAuth implementation with Owin

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

UserService.cs - Implementation of UserStore

public class UserService : UserStore
{}

RoleService.cs - Implementation of RoleStore

public class RoleService : RoleStore
{}

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
{
// identical to normal ClaimsIdentityFactory until:
if (manager.SupportsUserClaim)
{
var userClaims = await manager.GetClaimsAsync(user.Id).WithCurrentCultur

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 data = new Dictionary
    {
        { "email", user.Email },
        { "name", user.FirstName ?? string.Empty }
    };
    return new AuthenticationProperties(data);
}

Consider these members as part of the User class:

private readonly Lazy _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 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 claims = new List();

    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
    {
        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 data = new Dictionary

Or here:

IList claims = new List();

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!

StackExchange Code Review Q#110989, answer score: 4