ApiAuthorizationDbContext updates

[StevenRasmussen]

Summary of the changes (Less than 80 chars)

• Updated ApiAuthorizationDbContext to follow best practices per @ajcvickers comment
• Added new classes to support using a different key type for IdentityUser<T>

Addresses #9548

[HaoK]

reopening to retrigger builds

[HaoK]

This looks reasonable to me, @ajcvickers does this match the current DbContext best practices?

[ajcvickers]

Note that this won't be usable with DbContext pooling. If IOptions<OperationalStoreOptions> needs to be resolved from the request scope for each context instance, then this is unavoidable. Nevertheless, people will complain.

[blowdart]

This is a big drawback and can't happen.

[StevenRasmussen]

I don't think I'm understanding this as I haven't yet used DbContext pooling. That being said, I guess I'm not seeing how this differs from the current implementation. Could you provide some clearer direction on what a possible solution could be?

[ajcvickers]

@StevenRasmussen Why is there a need to add IOptions<OperationalStoreOptions> operationalStoreOptions as a dependency here?

[javiercn]

This comes from IdentityServer and it has an impact on the DbSchema, so I'm not sure what can we do here.

[ajcvickers]

This looks incorrect to me. A public constructor should use DbContextOptions<TContext>. If this class is intended to also be a base type, then it should get a protected constructor that takes DbContextOptions.

[StevenRasmussen]

@ajcvickers - I just followed the pattern of all of the other existing contexts in the file. Let me know if you still think this should be changed.

[ajcvickers]

@StevenRasmussen Any context that may be both used directly and inherited from should have both constructors. If there are other context classes that have this requirement but do not follow this pattern, then they should be changed as well.

[HaoK]

@ajcvickers this PR is also trying to address the same issue I think #13064 we should probably pick one PR and close the other, which do you think is the one we should keep?

[anis3h]

When will this issue be fixed?
Thank you

[HaoK]

@StevenRasmussen this looks promising, if you can address Arthur's feedback I think we would take this

[StevenRasmussen]

@ajcvickers - I think it might help if I provide a high level view of the problem this is solving along with what actually changed here:

Problem: At the moment, ApiAuthorizationDbContext<TUser> forces TUser to extend IdentityUser instead of IdentityUser<TKey>. This means it is impossible to use Identity Server with ApplicationUser : IdentityUser<Guid> (or anything different from IdentityUser<string>).

What this PR changes:

1. Adds a new ApiAuthorizationDbContext<TUser, TKey> class that is identical to the existing ApiAuthorizationDbContext<TUser> implementation (same constructor args, etc) but allows specifying the TKey.
2. I updated the existing ApiAuthorizationDbContext<TUser> class to inherit from this new class with a hard coded value of string for the TKey generic param. Git is showing this change as basically several new lines of code when in reality all that was changed was moving all the logic to the new base class and hard coding the TKey param to string. You have some comments here around IOptions<OperationalStoreOptions> and that this might cause some issues with DbContext pooling. I can't say whether that's true or not (I claim ignorance here) but if that's the case then the issue exists today as I didn't change anything in this regard. The current ApiAuthorizationDbContext constructor already has an IOptions<OperationalStoreOptions> as one of its args.
3. Added a new IdentityDbContext<TUser, TKey> which is identical to the existing IdentityDbContext<TUser> class except with the new TKey generic param and constraint where TUser: IdentityUser<TKey>

I think we're getting stuck in the weeds here with perhaps other potential changes that may/may not be necessary (I don't know since I don't know enough about the inner workings of EF Core). That being said, if those changes are necessary, then I think they might better be addressed in a follow-up PR since it would be changing things in a more fundamental way. For example, if we were to change all the IdentityDbContext classes (that I modeled the new one after) to accept a constructor argument of DbContextOptions<T> instead of DbContextOptions then I think this would probably be a breaking change as there are 4 public classes in that same file with the same constructor signature:

1. IdentityDbContext
2. IdentityDbContext<TUser>
3. IdentityDbContext<TUser, TRole, TKey>
4. IdentityDbContextIdentityDbContext<TUser, TRole, TKey, TUserClaim, TUserRole, TUserLogin, TRoleClaim, TUserToken>

Anyway, hopefully that helps clear things up. Let me know if you still want/need anything addressed in this PR.

[ajcvickers]

@HaoK I think we probably need to cycle back with @javiercn here to understand what ApiAuthorizationDbContext is intended to be used for and what it's limitations are intended to be.

[javiercn]

Problem: At the moment, ApiAuthorizationDbContext<TUser> forces TUser to extend IdentityUser instead of IdentityUser<TKey>. This means it is impossible to use Identity Server with ApplicationUser : IdentityUser<Guid> (or anything different from IdentityUser<string>).

Just for clarity, ApiAuthorizationDbContext is a convenience class for the default scenario. We don't want to be living with a parallel class hierarchy to support all the options Identity does.

The recommendation for more customized contexts is to extend IdentityDbContext directly and add the IdentityServer OperationalDbSchema on top.

The code is literally this:

    public class MyContext : IdentityDbContext<IdentityUser<int>>, IPersistedGrantDbContext where TUser : IdentityUser
    {
        private readonly IOptions<OperationalStoreOptions> _operationalStoreOptions;

        public ApiAuthorizationDbContext(DbContextOptions options, IOptions<OperationalStoreOptions> operationalStoreOptions) : base(options) => _operationalStoreOptions = operationalStoreOptions;

        public DbSet<PersistedGrant> PersistedGrants { get; set; }

        public DbSet<DeviceFlowCodes> DeviceFlowCodes { get; set; }

        Task<int> IPersistedGrantDbContext.SaveChangesAsync() => base.SaveChangesAsync();

        protected override void OnModelCreating(ModelBuilder builder)
        {
            base.OnModelCreating(builder);
            builder.ConfigurePersistedGrantContext(_operationalStoreOptions.Value);
        }
    }

[StevenRasmussen]

@javiercn - Thanks for the code... which actually demonstrates the current issue nicely ;). Right now we can't do that because IdentityDbContext<TUser> has a type constraint which requires TUser : IdentityUser. The problem is that IdentityUser is already hard coded with a key type of string:

This PR actually allows you to do as you are suggesting because it introduces a new IdentityDbContext which allows a different key type other than string. Here is the class being introduced:

public class IdentityDbContext<TUser, TKey> : IdentityDbContext<TUser, IdentityRole<TKey>, TKey> where TUser : IdentityUser<TKey>
        where TKey : IEquatable<TKey>
    {
        /// <summary>
        /// Initializes a new instance of <see cref="IdentityDbContext"/>.
        /// </summary>
        /// <param name="options">The options to be used by a <see cref="DbContext"/>.</param>
        public IdentityDbContext(DbContextOptions options) : base(options) { }

        /// <summary>
        /// Initializes a new instance of the <see cref="IdentityDbContext" /> class.
        /// </summary>
        protected IdentityDbContext() { }
    }

FYI - Here is a screenshot of the error using your code above:

[javiercn]

@javiercn - Thanks for the code... which actually demonstrates the current issue nicely ;). Right now we can't do that because IdentityDbContext<TUser> has a type constraint which requires TUser : IdentityUser. The problem is that IdentityUser is already hard coded with a key type of string:

Sure, I wasn't specifically talking about the IdentityDbContext changes, I'll let @HaoK and @ajcvickers speak for that. The only thing I want us to avoid is introducing a parallel class hierarchy for ApiAuthorizationDbContext since it will be expensive to test and maintain over time and the alternative to extend IdentityDbContext directly is much cheaper.

[HaoK]

If I'm hearing @javiercn correctly, it sounds like its mostly meant as a convenience class, and its not too much of a burden for them to plug in their own derived IdentityDbContext, so there isn't too much value in adding generics. Does that sound accurate?

[javiercn]

If I'm hearing @javiercn correctly, it sounds like its mostly meant as a convenience class, and its not too much of a burden for them to plug in their own derived IdentityDbContext, so there isn't too much value in adding generics. Does that sound accurate?

Yep, but any change to IdentityDbContext I'll let you be the judge of.

[HaoK]

@ajcvickers sounds like we are probably fine with things the way they are for 5.0 then here?

[ajcvickers]

Agreed

[HaoK]

Thanks for the PR @StevenRasmussen but I don't think we are looking to add generic support for this db context when its pretty straightforward to directly extend IdentityDbContext and use that instead

[StevenRasmussen]

@HaoK, @ajcvickers - To be clear: the changes to the ApiAuthorizationDbContext could be considered a "convenience" whereas the changes to the IdentityDbContext are absolutely necessary in order to allow an IdentityUser that has a key other than a string. This is currently NOT possible.

That being said, I don't think the changes to the ApiAuthorizationDbContext should be excluded. By doing so we'd basically be putting the onus on every developer that uses anything but a string as their PK for their IdentityUser to create this same exact class. I can't imagine that too many DBA's are excited about using an nvarchar as their primary key for users. I feel like this 2nd use case (int or guid PK) is more the "best practice" than how it's currently implemented using a string. Anyway, just my 2 cents.

[HaoK]

What do you mean? Can't an app just add AppDbContext : IdentityDbContext<MyUser, IdentityRole<int>, int> where MyUser is IdentityUser<int>. What does the new generic identity dbcontext provide that isn't possible without it?

[StevenRasmussen]

Doh! Ok... you're correct. It's been several months since I've been working in this area. That being said... I'm trying to remember the issue that I ran into that sparked this whole thing and I think it has to do with using all of the default IdentityServer plumbing when registering everything in your web project. Let me try to work this out again and I'll post my results here. Thanks for your patience.

[Webreaper]

Coming to this late, but I'm trying to convert my app from Blazor Server using a customised User class that uses an int for a key, instead of a string. I now want to change this to a hosted Blazor WASM app, but continue to use the same DB and IDs etc., with the custom IdentityUser with int key. @HaoK, you appear to be saying that I can do this using a derived IdentityDbContext with my new user class, but that doesn't appear to fit with the call to .AddApiAuthorization which expects my DB Context to derive from ApiAuthorizationDbContext.

I've seen the answer on this SO post which implies that I can build my own derived ApiAuthorizationDbContext base class, but I can't get it to work. Does anyone have a working example of this? I'm currently in a balloon-squeezing situation where every change I make introduces more breakage, so it would be great if somebody could point me at a working sample of how to achieve this. :)

[ghost]

Hi @Webreaper. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.