Set current principal if available

[twsouthwick]

No description provided.

[twsouthwick]

@davidfowl @Tratcher Is there a reason we shouldn't do this? The backing of the Thread.CurrentPrincipal is an async local so it should be handled fine and allow code written against that to just work. It also may be worth considering just adding it into ASP.NET Core itself.

[Tratcher]

@blowdart

[davidfowl]

I think this is fine from a compatibility perspective but not from a "do by default in ASP.NET Core" perspective:

• We've moved away from encouraging people using Thread.CurrentPrincipal. If we were going to do something like this, it would model IHttpContextAccessor.
• I've recently come to realize that having anything tied to the HttpContext on an async local is badness. There's no way to control the lifetime and how this thing flows. That can lead to concurrent access when it isn't supported.
• Performance, the more async locals the worse the performance. We try to avoid having more by default (we already have 2 by default).

[blowdart]

Yea, thread.currentprincipal is to be avoided as much as possible, it messes up badly with async as David says, it never worked well with impersonation (not that we're encouraging impersonation either), and 1st and 3rd party libraries made so many bad assumptions we pushed the AAD folks away from it.

I'm not sure we would even want this from a compat POV given its tendency to go wrong.

[twsouthwick]

That makes sense to not include it in the product itself and I get the desire to move away from it. However, since a lot of code currently does use it, what about it being something we make available with these adapters to opt into? This could either be per route (via metadata) or a global option.

I've created a PR that enables a request to be (logically) a single thread by limiting concurrency with #82. Would that make any of this better? We could only set Thread.CurrentPrincipal when single threaded behavior is enabled.

Additionally, what if we were to wrap the IPrincipal set to the current thread with a custom implementation to emit warnings if anyone were to access members from it?

[blowdart]

Single threaded? Woohoo welcome back STA :)

Yea, that'd work. I think an opt-in, with a dire warning is enough.

[twsouthwick]

@blowdart @davidfowl Thanks for the comments

I've updated this PR to do the following:

• Add metadata to be used to enable per-endpoint setting the Thread.CurrentPrincipal (which will then surface through ClaimsPrincipal.Current as well)
• A warning will be logged if this is set - it is currently a Warning, but that may end up being too noisy. Not sure if there's a better way, though
• Check if the logical single thread mode is enabled for the request. If not, it will log a warning that that should be enabled
• Add metadata to be used to enable per-endpoint logical single-threaded behavior using ConcurrentExclusiveSchedulerPair

I did also try overwriting the ClaimsPrincipal.Selector function to log warnings if it is accessed and to use HttpContextAccessor directly, but ended up removing it as the other warnings will probably be sufficient.

This is all opt-in and hopefully will help users understand it's not best practice but enable them to more easily migrate old code.

[blowdart]

I like these changes a lot. I appreciate your work here.

[davidfowl]

We shouldn’t have a wanting that logs per request, let’s get rid of it. Put something in the doc comments.