-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IHttpContextAccessor.HttpContext.User is not always authenticated (when it should) when invoked by IModelCacheKeyFactory #2833
Comments
We've seen this happen in Restier (a downstream library to OData) and determined that it happens because OData disposes of the We solved this by taking the original suggested solution on StackOverflow and making it Middleware instead. We also optionally have a You can check it out here: We use Middleware instead of other techniques because Middleware uses the On its face, it would seem that one of these two techniques might mitigate your problem. HOWEVER...In looking at how you have structured everything, I would say that the entire implementation is flawed, and the fact that it ever worked but only on slow machines is the actual symptom of the REAL and very different problem. There is one key detail I am missing before definitively pointing to the problem. When you say this is a "multi-tenant" solution, are all the tenants in the same database, or are you connecting to a different distinct database instance per tenant? |
@robertmclaws thanks for your contribution.
As I said I'm working with normal API Controllers, not with OData controllers.
AFAIK that's exactly how it's always been done everywhere. Anyway, I'm not discarding any option, I'll take into consideration your observations.
That's not what I meant; maybe I haven't been precise enough, sorry, English is not my first language. I'm not sure if it's meaningfull or not, but we upgraded from 6.0.25 to 6.0.26 in the same period: maybe I'll try to revert to 6.0.25, even if frankly it seems a bit far-fetched.
Sorry, I was a bit misleading with that sentence, my bad. |
Ok, thanks for the context of the situation. Then I was correct in my assessment, you are creating complexity that is not actually necessary in the application, regardless of how many people say that this is how you do it. In ASP.NET Core, the following three things are SCOPED by default, meaning there is a single instance per request, and the same instance is injected throughout the request lifecycle:
That means that the Those types of situations are for when you are in a TRUE multi-tenant environment, and each tenant may have a different EDM model or connection string. That is not the case here. One thing you need to consider is that, no matter how many THOUSANDS of tests you have verifying this situation, an incorrect pattern may not manifest its problems until much later. I believe what happened here is that Microsoft fixed a bug with a runtime that is almost out of support and exposed the flaw in your setup. You can do this whole thing without tying In addition, there are so many unnecessary memory allocations and commands in the UserId processing that you're slowing everything down on every request. Your code should look like this: // RWM: Put this in your services startup.
services.AddDbContext<MyDbContext>(
(serviceProvider, options) =>
{
var connString = DbContextUtils.GetApplicationDbContextConnectionString(configuration);
options.UseSqlServer(connString, x => x.MigrationsAssembly("My.Migrations"));
});
///<summary>
/// Dramatically simplified context that doesn't need to worry about getting polluted by
/// a lack of separation of concerns.
///</summary>
public abstract partial class MyDbContext: DbContext
{
///<summary>
/// The ID for the currently logged-in user. Is 0 for unauthenticated users, and null when the instance
/// is first created or the claim is not found.
///</summary>
public long? UserId { get; set; }
}
public static class MyClaimsExtensions
{
///<summary>
/// The safe way to get the UserId from the currently executing request.
///</summary>
public static long? GetUserId(this HttpContext httpContext)
{
if (httpContext.User?.Identity?.IsAuthenticated != true) return 0;
long.TryParse(httpContext.User?.Claims?.FirstOrDefault(c => c.Type == ClaimTypes.Sid)?.Value, out var userId);
// RWM: If it's still null after you call this method then there is a problem somewhere else on the stack.
return userId is not null ? (long)userId : null;
}
}
[Route("api/My")]
[ApiController]
[Authorize]
public class MyController : ControllerBase
{
///<summary>
/// The <see cref="MyDbContext" /> instance for this request.
///</summary>
public MyDbContext DbContext { get; private set; }
///<summary>
/// The <see cref="MyDbContext" /> instance for this request.
///</summary>
public MyController(MyDbContext dbContext)
{
DbContext = dbContext;
}
[HttpGet]
[Route("MyTestAction")]
public async Task<ActionResult> MyTestAction()
{
// RWM: Follow a clean separation of concerns by bridging the User Claim to the DbContext before using it.
dbContext.UserId = HttpContext.GetUserId();
/// TODO: RWM: Do some other stuff.
}
} This has the benefits of:
I hope this structure helps. If you REALLY can't implement it this way for whatever reason (and you really should try to do it the way I suggested), then still get rid of the ModelCache and just call my |
AFAIK, that's not how EF works :)
That's exactly how I solved temporarily the problem a few days ago. Anyway, I just realized that in the attempt to simplify my explanation, I trivialized a bit too much the context, omitted important aspects, and I'm somehow misleading you. |
What I explained is exactly how EF works, and I proved it with a link to the source code. I understand that you think EFCore needs to work this way, but your entire set of assumptions is based off of needing Dynamic Modeling. If your team misunderstood how that works, and it turned out you DON'T need that, then every downstream assumption is also incorrect.
That last statement is not correct. As I previously explained, Dependency Injection handles getting an instance of the If you are not dynamically changing the EDM data model itself (by adjusting types at RUNTIME) and are just using the Again, the implementation of the On slower machines it may not have completed by the time the The fix I described solves the problem because by the point in the process where the UserId is assigned to the Alternatively, you could just use my At any rate, this is not the correct place to post the issue because this is a problem with your chosen architecture and not OData. |
The problem I'm facing seems to be very similar to #2294, but happens in generic API (non-OData) controllers that uses HttpContext-aware
DbContext
instances.The problem is very odd, because:
.... with exactly same DB, same source code, same .NET version, same SDK.
The only factor that changes among the test environments I'm using, is the hardware.
Some machines are radically slower than others, and it seems that the problems happen much more frequently on slower ones.
This raises the suspicion that there is some kind of race-condition scenario happening under the hood in the initialization of
HttpContext.User
AFAIK, I'm following all the known best practices related to the usage of
DbContext
in those scenarios that needs a "contextualization" of the queries for the current ClaimsPrincipal.The
DbContext
needs to know the "current" user, so that the Global Filters (injected into it) can use the user-id to contextualize the executed DB queries.In order to do so, I'm:
IHttpContextAccessor
IModelCacheKeyFactory
[FromServices]
Code-wise, my scenario is (very similar to) this:
DbContext
, with aIHttpContextAccessor
injected into the constructor. The following sample shows a direct injection for simplicity, but the same problem arises even when "wrapping" theIHttpContextAccessor
into another "current user resolver" service (scoped, transient or singleton... doesn't matter)OneModelPerUserModelCacheKeyFactory
, an implementor of `IModelCacheKeyFactory'I suspect the problem is here.
It seems that the
IModelCacheKeyFactory
is invoked in a moment whenHttpContext.User
is not "ready" yet.[FromServices]
into the action method of an API ControllerAssemblies affected
It's unclear to me were the problem could be, but here below are the most meaningfull versions of the components I'm using
Reproduce steps
Since the problems happens at random, It's not easy to reproduce.
On some machines it always happens.
On other machines it never happens.
The only pattern I can see, is that it happens more frequently in slower machines, and almost never happen in faster machines.
Expected result
HttpContext.User
should be ready for usage from any injected service after the authentication process has been executed.Actual result
HttpContext.User
seems to be "not always ready" when it's injected in other services.The text was updated successfully, but these errors were encountered: