Enable IClaimsTransformer to be specified in DI instead of only options

[jburbach]

I'm not sure whether to post this here or in the EF repo, but I figured I'd start here.

ASP.NET Core RC2 WebAPI/SPA using windows auth, and I'm attempting to do a very basic claims transformation by looking the ID up in my database with my DBContext and adding claims I find there to the windows principal. I am using a repository pattern with EF, all registered as Scoped lifetime, the dbcontext should as well by default, so all methods are sharing the same dbcontext on each request. I get a lot of random EF errors on my _users.FindByUsername() method that say things like

Invalid attempt to call ReadAsync when reader is closed.
InvalidOperationException: BeginExecuteReader requires an open and available Connection. The connection's current state is open.
InvalidOperationException: The connection was not closed. The connection's current state is connecting.

I also get this error at startup, but then can refresh the page and get my SPA like I should:

InvalidOperationException: An attempt was made to use the context while it is being configured. A DbContext instance cannot be used inside OnConfiguring since it is still being configured at this point.

I think this is related to the transform running async and competing with other threads trying to access the db at the same time? I get these errors randomly under load, nothing consistent. If I take the DB lookup out of my claimstransform, zero errors. I tried making my user lookup async and awaiting it, which lessens the amount of errors but doesn't completely solve the issue.

My question is, is what I'm doing not recommended? Should I not be trying to use EF with a ClaimsTransform, or should I have a separate context just for this user lookup? Do I need to scope my dbcontext/repositories differently in DI? I'm curious what the envisioned recommended pattern is for something like this, and if this is possibly an EF Core issue that should work but doesn't.

Apologies if there is something obvious here I'm missing, I've looked through all the documentation I can find for best practices and didn't find anything that recommends something different.

using System;
using System.Security.Claims;
using System.Threading.Tasks;
using MyApp.Models.Repositories.Interfaces;
using Microsoft.AspNetCore.Authentication;

namespace MyApp.Authorization
{
    public class ClaimsTransformer : IClaimsTransformer
    {
        private readonly IUserRepository _users;

        public ClaimsTransformer(IUserRepository users)
        {
            _users = users;           
        }

        public async Task<ClaimsPrincipal> TransformAsync(ClaimsTransformationContext context)
        {
            System.Security.Principal.WindowsIdentity windowsIdentity = null;

            foreach (var i in context.Principal.Identities)
            {
                //windows token
                if (i.GetType() == typeof(System.Security.Principal.WindowsIdentity))
                {
                    windowsIdentity = (System.Security.Principal.WindowsIdentity)i;
                }
            }

            if (windowsIdentity != null)
            {
                //find user in database
                var appUser = _users.FindByUsername(windowsIdentity.Name);

                if (appUser != null)
                {

                    //add all claims from security profile
                    foreach (var p in appUser.SecurityProfile.Permissions)
                    {
                        ((ClaimsIdentity)context.Principal.Identity).AddClaim(new Claim(p.Permission, "true"));
                    }

                }

            }

            return await Task.FromResult(context.Principal);
        }
    }
}

[muratg]

cc @HaoK @divega

[HaoK]

@jburbach Can you include what your IUserRepository service looks like and how you register it in DI?

[Eilon]

Closing because we cannot reproduce this. Please re-open if you have more info. Thanks!

[jburbach]

@Eilon @HaoK My apologies for not responding quicker. Left for vacation and wasn't checking messages.

I register it like so
services.AddScoped<IUserRepository, UserRepository>();

and here is my UserRepo

using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using AutoMapper;
using EmployeeManager.Models.Repositories.Interfaces;
using Microsoft.EntityFrameworkCore;

namespace EmployeeManager.Models.Repositories
{
    public class UserRepository : IUserRepository
    {

        private IMapper _mapper { get; set; }
        private EmployeesAppContext _db { get; set; }

        public UserRepository(EmployeesAppContext dbcontext, IMapper mapper)
        {
            _db = dbcontext;
            _mapper = mapper;
        }

        public User Add(User user)
        {
            _db.Users.Add(user);
            _db.SaveChanges();

            return user;
        }

        public ICollection<User> GetAll()
        {
            return
                _db.Users.Include(e => e.Employee)
                    .Include(e => e.SecurityProfile)
                    .Include(e => e.SecurityProfile.Permissions)
                    .Where(e => e.IsDeleted == false)
                    .ToList();
        }

        public User Find(int userId)
        {
            return _db.Users.Include(e => e.Employee).Include(e => e.SecurityProfile).Include(e => e.SecurityProfile.Permissions).Where(e => e.IsDeleted == false).FirstOrDefault(u => u.EmployeeId == userId);
        }

        //this method is used by middleware to setup security and this can execute on separate threads, 
        //so this needs to be async so we can await and prevent dbcontext collissions
        public async Task<User> FindByUsername(string username)
        {
            return await _db.Users.Include(e => e.Employee).Include(e => e.SecurityProfile).Include(e => e.SecurityProfile.Permissions).Where(e => e.IsDeleted == false).FirstOrDefaultAsync(u => u.Username == username);
        }

        public User Update(User user)
        {
            var original = Find(user.EmployeeId);
            original = _mapper.Map<User, User>(user); 

            _db.SaveChanges();

            return original;
        }
    }
}

[HaoK]

How are you registering EmployeesAppContext in Startup, my guess this is the root issue...

[kevinchalet]

The root cause of the issue you're seeing seems quite clear for me: your claims transformer - necessarily a singleton by definition, since it's attached to ClaimsTransformationOptions - relies on a scoped dependency which is disposed when the scope is closed (in this case, the HTTP request). When you make a second request, the claims transformer middleware invokes your transformer and indirectly tries to reuse the previously closed Entity Framework context, which causes an exception.

One way to solve this issue is to avoid injecting IUserRepository as a constructor parameter to ensure it's never reused. Instead, you can resolve it from the scoped dependency resolver:

public async Task<ClaimsPrincipal> TransformAsync(ClaimsTransformationContext context) {
    var repository = context.Context.RequestServices.GetRequiredService<IUserRepository>();

    // ...
}

Note that it's not specific to the claims transformation middleware: it impacts all the middleware using "services as options", which basically includes all the authentication middleware using the IXYZEvents model.

There was a ticket in aspnet/Options tracking a brilliant way to enable this kind of scenario, but it has never been implemented: aspnet/Options#11.

Related topic: aspnet-contrib/AspNet.Security.OpenIdConnect.Server#275

[HaoK]

Hrm it seems bad if claims transformation can't use the scoped DbContext easily. Seems like something we should look at in 1.0.1

[HaoK]

Although I guess the workaround to just get any scoped services from the context RequestServices doesn't seem that bad, but this seems like this will be a common pitfall.

[kevinchalet]

Hrm it seems bad if claims transformation can't use the scoped DbContext easily.

Well, it's not just claims transformation, it's everything using "services as options", which impacts the whole stack, not just the security middleware.

I guess the workaround to just get any scoped services from the context RequestServices doesn't seem that bad

Personally, I don't have any particular problem with the service locator pattern (at least, as long as it's not static), but many people consider that as an anti-pattern.

Having a cool sugar like the one that was suggested would have been great. Sadly, it's now too late for that.

[jburbach]

I'll modify my code and verify. I agree @HaoK, I think this will be a very common issue people have unless it's made VERY clear in your documentation that they can't do this.

[jburbach]

This appears to have corrected the issue. Thank you @PinpointTownes.

There is one other thing I'm running in to that I wasn't sure was related, but now I think it is. There are errors I get when the app first starts that appear to be trying to use the dbcontext to look up the user in the ClaimsTransformer, but the context isn't ready yet. This only happens on the initial browser launch, I can then just refresh and get my SPA and everything works fine from then on. Would you like to see this here or should I open another issue?

[jburbach]

To follow up, I don't think this issue is completely resolved for me. I get seemingly random 500 errors from my API calls that say this:

InvalidOperationException: An attempt was made to use the context while it is being configured. A DbContext instance cannot be used inside OnConfiguring since it is still being configured at this point.

This is the same error I get on startup, and it happens on the FindUsername call in the ClaimsTransformer. I think that it's from the ClaimsTransformer trying to use the dbcontext before it is ready to be used. No errors occur at all if I remove the claims transformation.

My EF registration is just the default setup:

services.AddDbContext<EmployeesAppContext>(options => options.UseSqlServer(Configuration["ConnectionStrings:DefaultConnection"]));

Thoughts?

[jburbach]

In fact, even if I replace the database lookup with something simple like this, which just gets a list of possible permissions from an Enum and adds them to the identity:

var perms = Enum.GetNames(t); foreach (var p in perms) ((ClaimsIdentity)context.Principal.Identity).AddClaim(new Claim(p, "true"));

On startup I get an error that says:

InvalidOperationException: Collection was modified; enumeration operation may not execute.

Like something is trying to access the identity while I'm populating it. I think the async nature of claims transformation is either conflicting with some other design or perhaps it's not being awaited like it should be?

[kevinchalet]

Any chance you could share your updated transformer and your Startup class (or better a complete repro?) 😄

[jburbach]

I can't publish my full code, but if it comes to it and you guys need a solution I can try to put something together that reproduces the issue. Was hoping maybe there would be an "aha moment" before it came to that though. :)

[adamholdenyall]

There is an overload to app.UseClaimsTransformation that takes in a delegate to get your claims transfomer, and that seems to run on a per-request basis. So your ClaimsTransformer does not need to be a singleton. You still have to use the context as a ServiceLocator, but it can be done outside of your ClaimsTransformer, which can still be registered as scoped.

I added this block above my app.UseMvc in Configure:

app.UseClaimsTransformation((context) =>
{
    IClaimsTransformer transformer = context.Context.RequestServices.GetRequiredService<IClaimsTransformer>();
    return transformer.TransformAsync(context);
});

I just got this working with a rudimentary example, so not sure if there would be issues when used with EF.

[jburbach]

@adamholdenyall That's it! Once we eliminate the claimstransformer as a singleton and have it recreated every request, no more errors and it appears to play perfectly with EF now.

I think this is even more evidence that they need to marry this up like @PinpointTownes suggested earlier. The fact that using RequestServices.GetRequiredService to request the repo inside the transformer didn't even work and I had to abandon the whole options pattern completely is a little concerning. I envision this causing a LOT of confusion for developers.

[Eilon]

We should give some more love to claims transformation.

[HaoK]

One possible improvement would be to add a new options flag like PreferDITransformer (but with a better name), which would be used instead of the instance found in the options and enable per request transformation:

   services.AddScoped<IClaimsTransform, YourTransform>();
   ....
   app.UseClaimsTransformation(o => o.PreferDITransformer = true);

[kevinchalet]

@HaoK alternatively, why not a TransformerType property corresponding to the service type resolved from the DI container? It would allow using the claims transformation middleware multiple times in a pipeline while being able to use separate transformers.

services.AddScoped<IClaimsTransformer, Transformer1>();
services.AddScoped<Transformer2>();
services.AddScoped<Transformer3>();

app.UseClaimsTransformation(options => options.TransformerType = typeof(IClaimsTransformer));
app.UseClaimsTransformation(options => options.TransformerType = typeof(Transformer2));
app.UseClaimsTransformation(options => options.TransformerType = typeof(Transformer3));

[HaoK]

That's not a bad idea, but if we were going that direction maybe we could just add a new ClaimsTransformationMiddleware<TClaimsTransformService> where TClaimsTransform : IClaimsTransformer and it'd just be app.UseClaimsTransformation<YourTransform> but you'd have to register your transformer as a service instead of using an options instance.

No need to add a flag this way, as you'd be opting into this behavior using the new generic middleware

[HaoK]

Will be handled as part of the big auth 2.0 change, ClaimsTransformation will become a pure service, tracked via #1061