Generic Arguments missing from IdentityUser

[joshmouch]

In Identity 2.0, we had a lot of flexibility in extending the Identity* entities. That flexibility gave us further flexibility in how EF code first migrations interacted with Identity 2.0.

That seems to be completely missing in Identity 3.0.
For one, the following:

IdentityUser<Key, UserLogin, UserRole, UserClaim>

is changed to:

IdentityUser<Key>

This means I can no longer inherit from UserLogin, UserRole, and UserClaim and have the changes get migrated to the database using EF code first migrations.

For example:

public class MyUserRole : IdentityUserRole<Guid>
public class MyUserLogin : IdentityUserLogin<Guid>
public class MyUserClaim : IdentityUserClaim<Guid>
public class MyUser : IdentityUser<Guid, MyUserLogin, MyUserRole, MyUserClaim>
public class MyRole : IdentityRole<Guid, MyUserRole>
public class MyDataContext : DbContext (I set up the Identity entities in this class)

[divega]

@joshmouch thanks for the feedback on this. This change was intentional but I think it is something that we can discuss.

The goal of the change was to not have as many generic arguments leaking into all our types and extension methods. The main disadvantage is that the extensibility scenario you describe now requires replacing the Entity Framework provider with your own code.

cc @rustd @HaoK

[joshmouch]

@divega
As far as I know not having the generic arguments means you can't extend IdentityUserLogin, IdentityUserRole, or UserClaim, because the schema that will get created when EF looks at MyUser : IdentityUser, will have properties of the base types (e.g. IdentityUserLogin rather than MyUserLogin).

I tried for hours to get EF7 to treat MyUser.UserLogins as a type MyUserLogin through the ModuleBuilder.BaseType, but I didn't have any luck. (To get EF to use MyUserLogin for the database schema instead of IdentityUserLogin and to return type MyUserLogin when you reference the property MyUser.UserLogins).

[slaneyrw]

If I could add a voice to this discussion ( also see issue #567 - closed by HaoK ), the current state of the IdentityUser not only lacks extensibility, but it's implementation also couples the Identity* types into UserStore and UserManager and IdentityDbContext.

I'm trying to convert an existing v2 application( in the context of ASP.NET 4 -> 5 ) to access the feasibility.

In order for me to replace IdentityUserClaim I have to replace IdentityUser, IdentityDbContext, then (because UserStore has a generic constraint for TUser on IdentityUser ), replace UserStore as well, finally subclass UserManager to replace ALL the claims functions, working around the useful private/internal functions hidden.

Version 3 is much more closed than version 2 to the point of being a one size fits some / black box.

[joshmouch]

@slaneyrw Thanks for the support! Identity 2.0 definitely made it a design goal to make things one size fits all.

@divega I see you've set 3.0.0.-rc1 as a milestone for fixing this. Can you briefly describe what the public signature portion of the change will be so we can make sure it covers all the bases?

cc @divega @rustd @HaoK

[WiredUK]

I could be wrong, but I don't have a problem with having one or two (or several!) generic versions of the IdentityDbContext class to allow for those people who want to extend the model. I think that can be done pretty cleanly?

[joshmouch]

@WiredUK I'm with you. I think the concern is that many classes get touched. But it still seems clean as it's all "generically typed" touching.

[WiredUK]

@joshmouch I've taken a closer look at the code and it's super easy to extend it. Would a PR to do that be welcome here?

[joshmouch]

@divega @rustd @WiredUK I'm not very familiar with how this github development works in general. It sounds like the team may already be working on it for RC1. I think you'd have to ask them if they would welcome a PR.

[slaneyrw]

This missed RC1, is it going to be targeted for RC2 ?

[joshmouch]

@slaneyrw @divega What do you think the chances are this will make into RC2? And when in December to you approximate RC2 being available? I was kind of relying on this being available in RC1, and now I'm scrambling on what to do about it not being available. Worst case I'll have to download the code and make the changes myself, but that's adding work to an already accelerated project.

[divega]

@joshmouch, @slaneyrw and others, the reason we had this tagged as a discussion and assigned to me was that I was planning to do a write up detailing the team's thinking on the generic arguments. Unfortunately I was busy and the write up missed the RC1 deadline.

In the interest of clarifying this and setting realistic expectations, I am just going to try to summarize the situation quickly:

After discussing this a few times as a team we still don't feel like we can commit to have the generic additional arguments on entity types as we had in 2.0.

Obviously one of the concerns is that the generic arguments add complexity to the codebase. However the most important issue is that we don't understand the whole set of scenarios in which the generic arguments are supposed to enable extending the data model without affecting the functionality of the ASP.NET Identity API negatively.

Given that the goals aren't clearly defined it is hard to:

1. Understand whether it is possible to achieve them
2. Evaluate the amount of complexity it would bring
3. Add adequate test coverage to prove that the extensibility model works

@slaneyrw's PR we received recently (#643, very much appreciated!) adds the generic arguments on the EF entity types but unfortunately falls short of addressing our other concerns.

I would like to keep this issue as an open discussion and see if your feedback help us understand better. I wouldn't discard a change of mind but I wanted to clarify that we are currently not planning to do it.

[joshmouch]

I think you nailed it with "we don't understand the whole set of scenarios".
You've essentially hard-coded multiple table schemas with the way things are.
Isn't it enough to know that somebody may want to add a column to IdentityUserRole, whatever their reasons?
I'll try to come up with some more concrete scenarios...

[joshmouch]

Here's one broken scenario: I override UserManager in order to add logic so that users can't re-use passwords they've previously used. I use an Int32 as the TKey.
In this class, I call UserManager.FindByIdAsync, which is hard coded to use a string now, rather than TKey.

[HaoK]

Keys are always string at the UserManager level in Identity 3. The store is free to convert the string into whatever type it wants for storage but those key generics are definitely gone in the UserManager APIs

[slaneyrw]

@divega In both this issue and #567 you have been given concrete examples of why changing the 3 hardcoded aspects ( Claims, Roles and Logins ) has value. In our existing application that uses v2 we have extended both Claims and Logins

Claims: Added Issuer to we can track ( and make decisions on ) claims that originate of 3rd party identity systems, like Google, Facebook or our own OAuth 2 server.

Logins: We have also added an additional vector to our external login provider link so we can have a 1 to many relationship between a 3rd party login provider and our application.

You also have been shown that the complexity is already there in the system, you have just hardcoded the concrete coupling of the 3 types.

How do you want me to modify the pull request to address your concerns ?

As it stands, I cannot recommend upgrading any of our ASP.NET solutions to ASP.NET 5 as identity is unsuitable for our corporate use

EDIT

I'm adding tests now, and have found I missed IdentityDbContext and RoleStore extensions

[slaneyrw]

@divega @joshmouch
Tests are completed. There is a now a full test suite in the Microsoft.AspNet.Identity.EntityFramework.InMemory.Test project that uses each extensibility point. I have also included some extra tests to prove that the stores are using the extra information, not just round tripping the information

The key point here is that the API surfaced has an extra layer beneath the current UserStore, RoleStore, IdentityUser and IdentityDbContext. All the current code using UserStore<TUser, TRole, TContext, TKey> et al does not change, nor does the extra generic types leak into their codebase.

If a dev needs to change the shape of IdentityUser then they will need to do more work as the shape of the data layer will change, that has ramifications that cannot be avoided

[candoumbe]

Nothing new on this ?

[torrentx]

Yup. Same concerns. After banging my head for a few hours converting from 2.0 to this version, I came to this thread. Not having the ability to overwrite these base classes like version 2, makes it a just a fancy example and not something we can use as a framework. :(

[HaoK]

104f216