Making HttpContext.User available to 3rd party code without Microsoft.AspNetCore.Http dependency

[robertmclaws]

Background and Motivation

On AspNetCore, when you need to access the HttpContext in a "service" class, you use an IHttpContextAccessor to get the thread-safe instance of the HttpContext used by the current thread, thanks to its internal use of AsyncLocal.

However, if you need to be able to access the HttpContext.User, AND the service class is in another assembly, you can no longer rely on ClaimsPrincipal.Current or Thread.CurrentPrincipal to get you that information. If you control the external assembly, you're now dependent on Microsoft.AspNetCore.Http in all of your libraries, which is unnecessary.

Proposed API

The proposal is a stand-alone class that does not affect the behavior or functionality of any existing APIs, which should make it quick to approve and get into the current release, even given the current timeframe for .NET 6's release.

The solution is to implement an identical pattern to IHttpContextAccessor for IPrincipal. This will require an Interface to be added to System.Security, and an HttpContextPrincipalAccessor in Microsoft.AspNetCore.Http.

namespace Microsoft.AspNetCore.Authentication
{
    public class AuthenticationOptions
    {
+       bool EnableThreadCurrentPrincipal { get; set; }
    }
}

Because the shipped implementation takes an IHttpContextAccessor in the constructor, it will pull in the thread's HttpContext to correctly pull the user from.

Usage Examples

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
        services
            .AddAuthentication(options => 
             {
                  options.EnableThreadCurrentPrincipal = true;
                  options.DefaultScheme = "Cookies";
             });
    }
}

Alternative Designs

• I considered making everything a ClaimsPrincipal, as that is the proper way to do things, but I considered that it could be a WindowsPrincipal instead, so IPrincipal is probably a better approach.
• The implementation would probably do a TryAddService() on IHttpContextAccessor, just in case the user forgot to add one.

Risks

• AFAIK there are no risks to the added API itself, just the timing.
• The IPrincipalAccessor API would need to get in the .NET Core Libraries, which risks the team not wanting to add it at this stage of the game.
• The AspNetCore team may decide not to add the HttpContextPrincipalAccessor at this stage of the game, even if the .NET Core team is cool with it.

I have the requisite pull requests ready to submit if the team is OK with this.

[robertmclaws]

@davidfowl @terrajobst think this might be possible to get into .NET 6.0? We used the pattern on Restier and it worked quite well.

[davidfowl]

A couple of things:

• I don't think IPrincipalAccessor belongs in ASP.NET Core, there should be an API proposal in .NET to add it and I'm not 100% sure it's something that fits into the BCL.
• Existing alternatives are Thread.CurrentPrincipal and ClaimsPrincipal.ClaimsPrincipalSelector.
  • Maybe there could be a way to opt-into setting Thread.CurrentPrincipal in the auth middleware. This is something we refused to do before but maybe it could be opt in and it would give you an agnostic way to ClaimsPrincipal.Current. It would still be someone's job to wrap it in an interface so it's mockable I guess but I'm not sure where that should live.
• What other tech would set the current principal besides unit tests? Is this interface solely for testing or do you have other technology that isn't tied to ASP.NET Core that needs to provide an implementation for the interface?

[terrajobst]

• I don't think IPrincipalAccessor belongs in ASP.NET Core, there should be an API proposal in .NET to add it and I'm not 100% sure it's something that fits into the BCL.

Maybe, maybe not. It's similar to ISystemClock in the sense that it doesn't really provide any additional capabilities but allows higher-level DI concepts to use them as exchange types. Considering the interface is very simple, I'm not opposed to it, so long we ship a sensible implementation too (and the implementation could reside at a higher layer, such as Microsoft.Extensions or ASP.NET Core).

/cc @bartonjs

[bartonjs]

My general stance, in Mr. Garrison's voice, is "interfaces are bad, mmkay?", because of their lack of evolvability. (I'm also pretty firmly in the camp that if you can't come up with a reasonable name ending in "-able" (or "-ible" for linguistic reasons) then you don't have an interface, you have an abstract class.)

Like ISystemClock, I don't think it makes sense for the BCL... things in the BCL that need the current principal (which, ideally, would be 0) are going to use Thread.CurrentPrincipal, because the proposal really only makes sense for a DI environment, and the BCL doesn't have one. (My recollection was that our end belief was that something like ISystemClock should be defined along with the DI system, so it'd be in Microsoft.Extensions somewhere)

Despite the name saying "Thread", Thread.CurrentPrincipal is backed by an async local (https://source.dot.net/#System.Private.CoreLib/Thread.cs,af38dde3f1bd6b93), so my personal preference would be to take @davidfowl's "maybe" approach ("Maybe there could be a way to opt-into setting Thread.CurrentPrincipal in the auth middleware.")

[robertmclaws]

Thanks for weighing in on this everyone! Totally agree with @bartonjs, I'm usually vehemently against interfaces. I'm glad there is some consensus on setting ThreadPrincipal.Current, because that's exactly what I'm doing in Restier, and I was worried it was the wrong approach.

I avoided doing that this proposal because I assumed it was going to be some sort of SecureString-esque third rail for the team, but setting it directly in the Authorization pipeline instead of this pattern would be a much better approach.

@davidfowl to answer your question, we use it for a lot more than just unit tests. Up until now Restier only ran on .NET Framework, and BurnRate's business logic was in a separate assembly leveraging ClaimsPrincipal.Current for filtering data based on claims. When we migrated BurnRate to .NET Core via Restier's new AspNetCore version, the business logic was failing due to ClaimsPrincipal.Current not being set.

I searched for a solution but didn't find any, so I came up with this pattern to try to match what you were doing with HttpContextAccessor, and maintained compatibility by setting ClaimsPrincipal.ClaimsPrincipalSelector in Restier's pipeline.

Alternate Solution

So would we add a line under this one in AuthenticationMiddleware.cs that said ClaimsPrincipal.ClaimsPrincipalSelector = () => context.User;?

[davidfowl]

So would we add a line under this one in AuthenticationMiddleware.cs that said ClaimsPrincipal.ClaimsPrincipalSelector = () => context.User;?

No, we would set Thread.CurrentPrincipal there. ClaimsPrincipalSelector uses Thread.CurrentPrincipal by default. This would be opt in though via a flag somewhere. Probably AuthentictionOptions. Another reason for the option would be the performance hit for too many async locals. The performance degrades when there are more than 3 of them. Today we have:

• HttpContextAccessor
• Activity.Current
• The request logging scope
• Now the auth middleware
• Culture

For prior art here, we set the current culture (and UI culture) in the localization middleware by default without an option (https://github.com/dotnet/aspnetcore/blob/3eb0fa8abe62f9a937a78890762908159174ff91/src/Middleware/Localization/src/RequestLocalizationMiddleware.cs#L136-L140).

cc @blowdart @Tratcher @HaoK for feedback.

[HaoK]

So to summarize the current proposal we add something like

authenticationOptions.ShouldSetThreadCurrentPrincipalToo = true // defaults to false (better name TBD)

which would opt the middleware (and authorization logic whenever HttpContext.User is set) to also update Thread.CurrentPrincipal at the same time.

Sounds reasonable to me!

[Tratcher]

How likely is that Identity instance to leak onto background threads and outlive the request? For WindowsIdentity specifically we dispose it at the end of each request, so if it had leaked to a background thread you'd get ODEs.

[Tratcher]

• I considered making everything a ClaimsPrincipal, as that is the proper way to do things, but I considered that it could be a WindowsPrincipal instead, so IPrincipal is probably a better approach.

WindowsPrincipal already derives from ClaimsPrincipal.

Would Thread.CurrentPrincipal get reverted when the middleware exited?

[davidfowl]

authenticationOptions.ShouldSetThreadCurrentPrincipalToo = true // defaults to false (better name TBD)

But with a less bad name.

How likely is that Identity instance to leak onto background threads and outlive the request? For WindowsIdentity specifically we dispose it at the end of each request, so if it had leaked to a background thread you'd get ODEs.

You know the answer to this question 😄.

Would Thread.CurrentPrincipal get reverted when the middleware exited?

It doesn't need to, async methods revert the execution context.

[davidfowl]

I updated the API proposal.

[blowdart]

We worked very hard to avoid using Thread.CurrentPrincipal or indeed .Current in Core, because of threading and potential async issues, and required library owners to take a principal as a parameter.

It seems like a massive step back to return to that way of doing things.

[davidfowl]

We worked very hard to avoid using Thread.CurrentPrincipal or indeed .Current in Core, because of threading and potential async issues, and required library owners to take a principal as a parameter.

It seems like a massive step back to return to that way of doing things.

That's why it's opt in.

I should mention, the alternative is for us to do nothing and for customers to set the claims principal selector like so:

public class Program
{
    public static void Main(string[] args)
    {
        // This only works because we use a static async local as the backing field.
        var contextAccessor = new HttpContextAccessor();
        ClaimsPrincipal.ClaimsPrincipalSelector = () => contextAccessor.HttpContext.User;

        CreateHostBuilder(args).Build().Run();
    }

    public static IHostBuilder CreateHostBuilder(string[] args) =>
        Host.CreateDefaultBuilder(args)
            .ConfigureWebHostDefaults(webBuilder =>
            {
                webBuilder.UseStartup<Startup>();
            });
}

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
        services.AddHttpContextAccessor();        
    }
}

The above approach will avoid another async local and will let users opt into this behavior in a more explicit way. We could document this approach.

The other approach is to have users set Thread.CurrentPrincipal in middleware themselves:

app.UseAuthentication();

app.Use(async (context, next) => 
{
    Thread.CurrentPrincipal = context.User;
    await next(context);
});

app.UseAuthorization();

This is something you can do today without an added feature.

[blowdart]

Yea, if it can be done already I'm rather dead set against backing anything based on .CurrentAnything into asp.net, or worse corefx. It had reliability issues before and I'm not convinced it'd be problematic again.

[davidfowl]

@robertmclaws Can you try those solutions and let us know if it works out? As @blowdart says, we try really really hard to avoid doing this by default especially in the box.

[robertmclaws]

I'm all about having a way to do it in the framework vs having to look something up.

The challenge @blowdart for situations like Restier (where the class processing the HttpRequest is 100% decoupled from any concept of HTTP) is that passing the HttpContext.User as a parameter up the chain would require every method to take a ClaimsPrincipal, even those that don't use them.

That's why my original approach was basically to be able to pull it out of DI if you need it, the same as an IHttpContextAccessor.

Another alternative could be to wrap @davidfowl's first suggestion from his previous post into a clean extension method:

public static class HttpServiceCollectionExtensions
{
    public static IServiceCollection AddClaimsPrincipal(this IServiceCollection services)
    {
        services.AddHttpContextAccessor();        

        // This only works because we use a static async local as the backing field.
        var contextAccessor = new HttpContextAccessor();
        ClaimsPrincipal.ClaimsPrincipalSelector = () => contextAccessor.HttpContext.User;
    }
}

The idea is not to "bake" it into the pipeline, but to make it available if people need it.

My question would then be @blowdart, if HttpContextAccessor and Thread.CurrentPrincipal BOTH use AsyncLocal internally... why is one OK and the other is not?

[robertmclaws]

@davidfowl The point of the API Proposal was that I already have a solution that works. I can't be the only person in the same situation that can't get this any other way. So my original API proposal was NOT to set Thread.CurrentPrincipal, but to access the IPrincipal from the injected HttpContextAccessor instead, and pull out the IPrincipalAccessor when you need it. That way, I don't need a dependency on Microsoft.AspNetCore.Http in order to get the Principal out of the container.

[blowdart]

We had instances of other pieces of code setting the current principal after asp.net had, or doing strange things with it (the old ws-trust stuff) and if memory serves in asp.net core we moved everything to hang off the request because of this.

Moving back I wouldn't be able to make guarantees around correctness without a lot of work (and it feels like hobbling aspnet and net6/7 with a legacy approach)

[robertmclaws]

@blowdart fair enough. Do you have any other suggestions on how to get the HttpContext.User into the DI container so that it can be retrieved like any other service?

[blowdart]

I don't I'm afraid, I defer that to @davidfowl, We've never treated the current request identity as a discreet item at all, it's always been part of the request.

[davidfowl]

@davidfowl The point of the API Proposal was that I already have a solution that works. I can't be the only person in the same situation that can't get this any other way. So my original API proposal was NOT to set Thread.CurrentPrincipal, but to access the IPrincipal from the injected HttpContextAccessor instead, and pull out the IPrincipalAccessor when you need it. That way, I don't need a dependency on Microsoft.AspNetCore.Http in order to get the Principal out of the container.

Yes but as the above conversation revealed, we don't have any plans to offer this interface. Because there are existing well known async locals that can be used instead that are already defined by the framework. Nothing in the framework needs to use an IPrincipalAccessor, this service is convenience for users that want to avoid the ASP.NET Core dependency (which is why it's strange trying to define it inside of ASP.NET Core).

This isn't about making stuff work per se, you can already add any service you'd like to the DI container and any service can use it. This is about an "official" exchange type for doing so. The solution I offered above allow the same exchange types to work.

Thanks for weighing in on this everyone! Totally agree with @bartonjs, I'm usually vehemently against interfaces. I'm glad there is some consensus on setting ThreadPrincipal.Current, because that's exactly what I'm doing in Restier, and I was worried it was the wrong approach.

This solution is really bad because it's capturing the HttpContext in the static callback, permanently rooting that reference. It also doesn't work well if there are multiple hosts in the same process that might define their own ClaimsPrincipalSelector. That's why it really belongs in Program.cs and why it shouldn't be an extension method on anything. This is code you wire up once, in the application entry point.

The middleware approach is much cleaner as it makes everything work even across hosts in a safer way without touching the ClaimsPrincipalSelector static. Then your IClaimsPrincipalAccessor implementation just looks like this:

public interface IClaimsPrincipalAccessor 
{
    ClaimsPrincipal ClaimsPrincipal { get; } 
}

public class ClaimsPrincipalAccessor : IClaimsPrincipalAccessor
{
    public ClaimsPrincipal ClaimsPrincipal => ClaimsPrincipal.Current;
}

[robertmclaws]

@davidfowl you are right, with the middleware approach, the ClaimsPrincipalAccessor seems unnecessary.

How would you folks feel about shipping the middleware approach as an extension method in the framework?

  public static IApplicationBuilder UseThreadPrincipals(this IApplicationBuilder app)
  {
      app.Use(async (context, next) =>
      {
          Thread.CurrentPrincipal = context.User;
          await next();
      });
      return app;
  }

@blowdart I know you're not a huge fan of baking it in, but honestly if it's this easy to do anyway, wouldn't you rather do it a simple and official way, rather having people like me invent sh!ttier and less performant ways to do it?

[davidfowl]

So the options are:

1. Do nothing in the framework and document this showing the 3 different approaches above:
   • The middleware to set Thread.CurrentPrincipal
   • Setting the ClaimsPrincipalSelector in Program.Main.
   • Using a custom interface like IClaimsPrincipalAccessor and implementing an HttpContextClaimsPrincipalAccessor that uses the IHttpContextAccessor.
   • Manually passing around the ClaimsPrincipal.
2. Make it an optional part of the AuthN middleware:
   • We don't make the performance worse by default by adding another async local into the mix.
   • The Thread.CurrentPrincipal needs to be set after the AuthN middleware runs.
   • Shared library code that works on the User can work between .NET Framework and .NET Core (I'm dubious of this just working because most web based library code depends on HttpContext.Current).

The hardest thing about 2 is it feels a little like an endorsement of Thread.CurrentPrincipal and ClaimsPrincipal.Current when trying to access the current user from assemblies that don't want to reference ASP.NET Core. At least with the first approach it feels less like strict guidance and more like a smattering of options.

[Kahbazi]

Make it an optional part of the AuthN middleware:

@davidfowl and also AuthZ middleware?

[davidfowl]

Why would it be part of the authz middleware?

[Kahbazi]

Why would it be part of the authz middleware?

In our application we don't have a default authN scheme so we don't even use AuthN middleware. Each endpoint has one or multiple authN scheme which would be authenticated in AuthZ middleware and HttpContext.User would be filled here :

https://github.com/dotnet/aspnetcore/blob/9fa3421e1302bce7dbc50269edbd3072907e7832/src/Security/Authorization/Policy/src/PolicyEvaluator.cs#L58

[davidfowl]

Tricky, you really want to set this outside of arbitrary components like this. In fact, maybe I'd argue that we shouldn't be setting the context.User here and instead the middleware should be responsible for doing that.

[Kahbazi]

In fact, maybe I'd argue that we shouldn't be setting the context.User here and instead the middleware should be responsible for doing that.

Which middleware? AuthZ?

[HaoK]

In fact, maybe I'd argue that we shouldn't be setting the context.User here and instead the middleware should be responsible for doing that.

Yeah this always felt odd to me too, but we did this to keep the same behavior as what [Authorize] always did

[davidfowl]

We should fix it and move the assignment to the callers

[poke]

Remembering how many issues we had in the past with the usage of IHttpContextAccessor where users incorrectly used it all over places because it was there, I would also agree that we should hesitate to add another thing alike that for the current principal. If Thread.CurrentPrincipal is backed by an async local, then this would include that.

I’m more than fine with adding a dedicated article in the documentation about how one could possibly approach this. Seeing the different ideas in this issue which often just span a few lines to integrate makes it clear IMO that we don’t need to expand ASP.NET Core by default and send the wrong message that it’s okay to do this. Instead, people can just copy over the relevant pieces into their own code, making this behavior and the downsides (which should also be clearly documented) very explicit.

As for libraries like Restier that want to rely on this: Usually, these libraries will need some kind of integration package for ASP.NET Core anyway, so they could always add their own capabilities there. If you control the full library, switching to a custom interface to access the principal should be possible too. And if you really need, the library could provide a way to set up Thread.CurrentPrincipal or similar.

As a framework, ASP.NET Core and .NET (Core) itself should make it clear that the static .Currents are meant to be legacy though. So I would agree that we should avoid advertising their usage here by providing quick opt-ins that will be seen as an invitation to enable it by default.

[MattJeanes]

Echoing my reply to @davidfowl's Twitter post, our team uses an authentication workflow (in our case using attributes on controllers) which sets a custom user DTO in a scope-injected helper class which we can then access from within our libraries by injecting the helper and retrieving the user DTO again. The DTO contains things like the username/id, permissions and roles etc that we need in our application.

This works really well for us but the two problems I see with our approach however is primarily that it is our own class and wouldn't work within any public libraries from NuGet for example where adding a more generic class built into [ASP].NET Core might, and there is discovery problems with our approach in the sense of there isn't any documentation (that I've come across at least) that tells you that this is an approach you can take to solve this problem.

[robertmclaws]

One pattern that I liked quite a bit from David's Twitter thread was this:

...
    services.AddHttpContextAccessor();
    services.AddScoped(sp => sp.GetService<IHttpContextAccessor>().Context.User;
...

Implementing it means that every business logic service would have to be scoped so it could accept a ClaimsPrincipal as a constructor parameter, but in in the cases where that is possible, it would be pretty clean and not require a direct HTTP dependency.

From the compatibility standpoint of helping people from .NET Framework to .NET Core (which sometimes means having the same code, i.e. business logic, run on both for a while), Thread.CurrentPrincipal funneling into ClaimsPrincipal.Current is the most consistent solution, and should have a specific and documented way get wired in as easily as possible..

Moving away from Thread.CurrentPrincipal may have seemed like a good idea at the time, but as David's Twitter thread showed, leaving people to their own devices just made the problem worse. If mutability and thread-safety were the issue, we now have tools we didn't back then. A combination of AsyncLocal and refactoring Thread.CurrentPrincipal to use C# Records might a safe and cross-device way of doing things.

If too many AsyncLocals cause performance issues, then we could flip the script. If you think about it. an HttpContext is essentially just a container for information passing down the current thread. Console and Windows apps don't typically have this problem because they do not usually have concurrent-but-different users executing at the same time. But sometimes batch job Console apps need to get tokens that impersonate users, and run different users on different threads.

If you push that concept down into the GenericHost, then an AsyncLocal-backed ThreadContext could create that generic thread storage object, and HttpContext could inherit from it to retrieve HTTP-specific things from that object. HttpContextAccessor would be adjusted to use the THAT AsyncLocal instead, and you get a consistent implementation across all of .NET with no change in performance.

[davidfowl]

I'm less worried about the async local performance for any other situation than the default. That's why it wouldn't be on by default. This problem exists for http context accessor, culture, windows principal, activity, logging scopes, thread current user. Removing one of those won't significantly change anything.

[davidfowl]

The other interesting thing about this is that the raw claims principal is usually fine if you're only reading it in a single place but the moment you need to transform claims, you want to store your own scoped service/async local with the projected data.

[robertmclaws]

The architecture we use at BurnRate stores the UserId in Auth0's user metadata, so it comes over the wire in the signed JWT, and you don't have to do DB lookups to get it. We transform Claims in the JWT middleware process, so the ClaimsPrincipal that comes out the other side of the built-in middleware has everything it needs. That usually just entails splitting a comma-separated list of roles into individual Role claims.

[davidfowl]

That usually just entails splitting a comma-separated list of roles into individual Role claims.

Do you parse then every time you want to consume it? Where do you store the preprocessed claims?

[Tratcher]

...
    services.AddHttpContextAccessor();
    services.AddScoped(sp => sp.GetService<IHttpContextAccessor>().Context.User;
...

Note this pattern doesn't account for the fact that the user can and will change during the lifetime of a request (e.g. by the AuthN middleware). If the scoped ClaimsPrincipal service were accessed too soon it would cache the wrong value for the user for the duration of the request.

[mqudsi]

Can I ask a dumb question? Why should it be possible to get the user associated with the current http request without a dependency on ASP.NET Core? It feels like an abstraction for the sake of having an abstraction, at least from ASP.NET Core’s perspective. I get that’s what Restier needs, but does that make it right?

Let’s say there is an abstraction that makes sense where there are multiple authenticated request providers and only one of which is ASP.NET Core; shouldn’t this abstraction (and the extension method configuring the .NET Core pipeline to support it) live in whatever library is abstracting over all that, then?

If it’s just about migrating legacy projects.. then there are lots of examples of things that were easier to do before and this probably isn’t the most egregious; plus, there’s a reason why all of ASP.NET Core is so compartmentalized as compared to its predecessor, and that’s mostly a good thing (TM), no?

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[davidfowl]

I wrote some notes on async locals and why they need to be designed intentionally https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#asynclocalt

This comes from experience with the HttpContextAccessor and the issues we’ve seen customers run into over the years.

[davidfowl]

Given the issues we've seen with HttpContextAccessor, I'm feeling sure that using Thread.CurrentPrincipal would end up in the same boat:

• ClaimsPrincipal isn't thread safe
• WindowsIdentity is disposable (and disposed in the request pipeline and other pipelines)

When used from arbitrary library code that is ignorant of this, it can be extremely problematic. I'm not sure of a 100% safe way to do this given the current APIs and that makes me not want to have anything built into the pipeline for this. Maybe we can start with documentation here.

Using Thread.CurrentPrincipal is the most pragmatic approach (with those caveats listed):

1. There must be no concurrent access to Thread.CurrentPrincipal (this is very subtle as code you don't control will capture the execution context and extend the lifetime of it)
2. If the current user is a WindowsPrincipal then it must be used within the lifetime of the request (that can't guarantee statically). This is to avoid the disposal race.

If you haven't yet read the above comment, please do as it details how these could happen and what they might look like.

[robertmclaws]

Can I ask a dumb question? Why should it be possible to get the user associated with the current http request without a dependency on ASP.NET Core? It feels like an abstraction for the sake of having an abstraction, at least from ASP.NET Core’s perspective. I get that’s what Restier needs, but does that make it right?

Because in a library of business logic that can use LINQ queries to filter down resultsets (whether using Restier or not), an end user needs to be able to get/read/act on the current permission set. The standard for that has been a ClaimsPrincipal (regardless of whether or not ClaimsPrincipal.Current is thread-safe or not, it still contains claims processing logic).

This way, if I'm executing on an out-of-band runtime (like a MessageBus), each message on the queue gets a new thread and a new Thread.CurrentPrincipal, and the business logic code still works.

Ironically, @davidfowl I'm seeing the same issue with the Features cache in Restier. Every so often a request fails because a feature is disposed while OData 7.X on APp.NET Core is trying to access it.

Y'all are smarter than me, but happy to help however I can. Maybe we can find a good solution for .NET 8 together.

Thanks so much, and I hope y'all have a very Happy Christmas / Kwanzaa / Hanukkah / Festivus / etc. 🤜🏻

[HamsterExAstris]

• ClaimsPrincipal isn't thread safe

Is a ClaimsPrincipal instance (as opposed to ClaimsPrincipal.CurrentPrincipal) really not thread-safe? I thought that passing the user as a method parameter was considered safe.

i.e. could the following controller action run into issues if the services both read the ClaimsPrincipal retrieved from ControllerBase.User simultaneously? Or is it only an issue if ThingThatUsesPrincipalAsync mutates the ClaimsPrincipal?

[HttpPost]
public ActionResult DoThing()
{
    var task1 = _service.ThingThatUsesPrincipalAsync(User);
    var task2 = _service.ThingThatUsesPrincipalAsync(User);
    await Task.WhenAll(task1, task2);
    return Ok();
}

[Tratcher]

@HamsterExAstris mutation is certainly not thread safe. Most types don't guarantee even reading is thread safe, though they are in practice. HttpContext is an example of a type that isn't thread safe for reads either since it generates many fields lazily.

[davidfowl]

Right. So, I just took a quick look at ClaimsPrincipal and WindowsPrincipal. ClaimsPrincipal is mutable and you can run into trouble if the identities are changed while it's being read, but it looks pretty safe to be read concurrently. WindowsPrincipal does have lazy properties but it is protected by a lock. WindowsPrincipal is still disposable so if you do end up trying to use it outside of the context where it was set, there's an inherent race condition that you cannot protect against. Of course, this doesn't say anything about types derived from ClaimsPrincipal.

Given that, I think documenting this:

Setting Thread.CurrentPrincipal in middleware:

public static IApplicationBuilder UseThreadCurrentPrincipal(this IApplicationBuilder app) =>
    app.Use(async (context, next) =>
    {
        Thread.CurrentPrincipal = context.User;
        await next();
    });

Would be a good place to start here.

[ericsampson]

@davidfowl if I set the Thread.CurrentPrincipal in an AuthenticationHandler > HandleAuthenticateAsync(), it gets reverted to the default instance of GenericPrincipal by the time it arrives in the next middleware.

I haven't yet set things up to run the aspnetcore code locally in order to step through in the debugger, but just looking at the code in AuthenticationMiddleware and various other things, I cannot figure out why this would be happening.

Any thoughts? I'm probably missing something obvious :) Thanks!

[davidfowl]

@davidfowl if I set the Thread.CurrentPrincipal in an AuthenticationHandler > HandleAuthenticateAsync(), it gets reverted to the default instance of GenericPrincipal by the time it arrives in the next middleware.

Yep. That's the wrong place to set it 😄. Async locals can't be set from anywhere, you have to understand how the execution context is set and reverted to make sure they "survive" long enough that they can be observed. Async methods revert the execution context when they exit, that's why it needs to be set in a middleware. I can't think of any other component that doesn't revert execution context in the framework.

Is there a reason you're trying to set it in a custom authentication handler?

[ericsampson]

Oh shoot, reading your response immediately made it clear what I was forgetting, and why it works when set in middleware before doing the await next, but not where I was setting it--because the execution context only flows downwards, not upwards. I already knew that, but just didn't put 2 and 2 together for some reason. Thanks!

The reason it's like this is that I'm porting a large (poorly-written) Framework API over to Core, and that's what the existing code was doing. I'm trying to minimize changes for now and just do a straight port to Core--the current code uses Thread.CurrentPrincipal in a million places.

Setting it in the next middleware like you showed fixed the issue, I was just trying to figure out what I was misunderstanding :)

Cheers!

[ericsampson]

regarding the original issue, I think that adding the snippet that David posted to the docs somewhere would be helpful/sufficient, rather than baking in new functionality to be maintained forever for a situation that is a) not a good pattern to encourage, b) most useful for porting existing old codebases, and c) cleanly solved using a simple 4-line snippet. I don't know if there is existing docs around Framework migration, but if so perhaps it could be put there.