Make Roles optional at the Microsoft.Extensions.Identity layer

[HaoK]

High level idea is to enable something akin to:

Related to #651

I think this should be safe to do in 2.1, but we might want to consider doing something like this in RTM (as this is really finishing what we started in 651)

services.AddIdentityCore<TUser>() // UserManager related servives
   .AddRoles<TRole>() // RoleManager related services
   .AddEntityFrameworkStores<TContext>(storeOptions => { }) // All stores needed for all managers, optional schema tweaking
   .AddSignInManager() // SignInManager related stuff

Would be equivalent to today's (which we would still support):

services.AddIdentity<TUser, TRole>().AddEntityFrameworkStores<TContext>()

[divega]

@HaoK do you always need to configure in storeOptions in the call to AddEntityFrameworkStores to indicate whether the schema for roles needs to be included or can that be inferred automatically by the fact that you called AddRoles?

[HaoK]

Its an optional thing, I just was showing it to make it obvious that was where you will can opt into/out of things.

AddEntityFrameworkStores() would do something by default appropriately...

[HaoK]

But its likely there will be ways to mismatch things, like calling AddRoles() and then doing storeOptions.SupportsRoles = false... We won't be able to block all of those, but it should be easy to validate

[HaoK]

Basically misconfiguring storeOptions will cause manager apis to throw if they end up calling into store functionality that they disable

[HaoK]

Also related to #581

[HaoK]

After chatting with @ajcvickers today we agreed that this is something important to do in 2.0, some nice sideffects that fell out of the refactoring/exploring this. we can nuke all of the entities on TUser/TRole except for TKey... That cleans things up quite a bit.

[danroth27]

@blowdart @Eilon @DamianEdwards

we can nuke all of the entities on TUser/TRole except for TKey... That cleans things up quite a bit

Could you please clarify a bit more what this means?

[HaoK]

We had a bunch of generics that were on the base POCOs that were only needed for the navigation properties, see: https://github.com/aspnet/Identity/blob/dev/src/Microsoft.Extensions.Identity.Stores/IdentityUser.cs#L54

There will only be TKey now.

[HaoK]

To clarify the plan, we are leaving the existing AddIdentity alone that lives in Microsoft.AspNet.Identity, that will behave the same and add the same old user + roles scheme.

Since we are adding a new public AddIdentityCore API in Microsoft.Extensions.Identity.Core anyways, we might as well expose it the 'right way', meaning Users are the only required entity, with Roles being an add on.

[danroth27]

Ah, I was curious how those extra generic parameters got used since they don't appear on any of the type members. When you say they are currently needed for the navigation properties, what does that mean? Aren't navigation properties typically represented as properties on the type? Does EF Core do something special with the extra generic parameters in this case? Or does Identity?

Regardless, not having those extra generic parameters sounds like a good change.

[HaoK]

StoreOptions are gone, so there's none of that complexity. There will be a new UserStore base class that doesn't implement any of the Role interfaces, that will be what is used when roles isn't enabled. Otherwise, the usual roles + user store will be used.

We'll actually have proper base classes for 2.0 that do not have a TRole at all (IdentityDbContext/UserStore)

[HaoK]

The POCOs used to live inside of EF Core, and they had navigation properties. I removed them as part of the move to Extensions for preview 2. This is finishing the rest of the move as there's no way to actually use Extensions.Identity today without AspNet.Identity.

[HaoK]

@divega @ajcvickers Well, there's a tiny complication to removing the generics off IdentityUser... turns out we were using them to use reflection to automagically register the correct UserStore so they didn't have to manually add their own UserStore<MyUser, MyRole, MyUserClaims, MyUserLogins...>.

We added the smarts to infer generics in 1.1. We do have another generic type with the same information, the IdentityDbContext, but there's no hard requirement that they use our DbContext, as any DbContext works today. But I think this would cover the majority of use cases, so we'd be only impacting customers relying on the custom poco AddEntityFrameworkStores magic who were using their own DbContext that didn't derive from IdentityDbContext....

Thoughts?

[HaoK]

f555a26

[svallis]

@HaoK This was a breaking change for me and I'd like to add the functionality back in. The migration docs here says:

The Entity Framework Core navigation properties of the base IdentityUser POCO (Plain Old CLR Object) have been removed. If your 1.x project used these properties, manually add them back to the 2.0 project... snip

I was using the roles collection on the user and the users collection on the role, but they used to operate without needing to traverse the link table. How, specifically, can I restore that exact functionality?

My IdentityUser and IdentityRole classes are both in a library and implemented via an abstract class that implements IdentityUser, with the concrete class in the application itself deriving from the abstract class. I've been attempting to add the navigation properties to the abstract class without any success; however, I've done similar for non-collection based navigation properties for user accounts with builder.Entity<SomeClass>().HasOne(x => (TUser)x.User);, but the role and user relationships don't appear to work with anything like builder.Entity<TUser>().HasMany(x => (IList<TUserRole>)x.Roles); due to the link table being in the middle. The navigation properties for the above are defined as public IUser User { get; set; } and public IList<IRole> Roles { get; set; } respectively.

[HaoK]

You need the navigation property to be pointing at TUserRole, which is what it was in 1.x:

In 1.x: IdentityDbContext.OnModelCreating:

            builder.Entity<TUser>(b =>
            {
                b.HasMany(u => u.Roles).WithOne().HasForeignKey(ur => ur.UserId).IsRequired();
            });

           // The IdentityUser property:
           public virtual ICollection<TUserRole> Roles { get; } = new List<TUserRole>();

In 2.x:

            builder.Entity<TUser>(b =>
            {
                b.HasMany<TUserRole>().WithOne().HasForeignKey(ur => ur.UserId).IsRequired();
            });

           // Add a TUser property:
           public virtual ICollection<TUserRole> Roles { get; } = new List<TUserRole>();

[svallis]

Thanks @HaoK - for some reason I had in my head that the many to many relationship was exposed through the navigation properties without having to go through the link table, I guess due to the name of the properties. Appreciate the clarification.

[Ponant]

@HaoK , I think that will be for 2.1 Milestone (?)