ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.32k stars 1.63k forks source link

HttpContext lost between HttpClientWrapper and DelegatingHandler #1252

Open amweiss opened 4 years ago

amweiss commented 4 years ago

Expected Behavior / New Feature

A DelegatingHandler with IHttpContextAccessor injected can access the current context correctly.

Actual Behavior / Motivation for New Feature

As of version 15.0.7, a custom DelegatingHandler we use now only has access to an empty context and thus httpContextAccessor.HttpContext.User.FindFirst(ClaimTypes.NameIdentifier)?.Value is now null instead of the name like it was in 15.0.6

Steps to Reproduce the Problem

  1. Create a DelegatingHandler that takes in IHttpContextAccessor as an injected dependency.
  2. Register the handler on the route.
  3. Add JWT auth to the route.
  4. In the SendAsync method, call httpContextAccessor.HttpContext.User.FindFirst(ClaimTypes.NameIdentifier)?.Value
  5. See that the value is null

Specifications

davipsr commented 4 years ago

Same issue here, version 16.0.1

jlukawska commented 4 years ago

Hello, The reason is that after changes done in fe3e8bd23a666daa31ed2e8b00dce2c4630c23a7 a new HttpContext instance is created for every request (or even more instances for aggregated routes). AuthenticationMiddleware (and others) receives a new HttpContext instance, but HttpContextAccessor still has a reference to the original instance. That's why HttpContext obtained by HttpContextAccessor doesn't have any claims.

I've proposed a solution (see pull request #1268 ) where the proper HttpContext will be added as a property to a delegating handler class if the class implements the specified interface.

bjartekh commented 3 years ago

I tried the code submitted in #1268 (by implementing IDelegatingHandlerWithHttpContext in my DelegatingHandler) but I experienced that HttpContext.User.Claims were not updated after the first request (so any subsequent requests would use the same user claims).

Steps to reproduce:

Is this a scenario you've tested?

My workaround-hack for getting user claims to be available in the DelegatingHandler, was to fetch the claims from the httpContext in PreAuthorizationMiddleware and then set them in a custom request scoped RequestData object.

And when I needed that data in the DelegatingHandler, I would just obtain it like this: var rqData = (RequestData)_httpContextAccessor.HttpContext?.RequestServices.GetService(typeof(RequestData));

jlukawska commented 3 years ago

@bjartekh thanks, you're right. There's a problem with caching. I'll look into it if I find time.

Unfortunately Ocelot seems to be abandoned after breaking changes with HttpContext :/

jlukawska commented 3 years ago

@bjartekh if you are interested, I've just fixed the problem with cache.

AgentShark commented 1 year ago

The Ocelot project seems dead, but if anyone experiences this issue, here is a workaround.

Set the IHttpContextAccessor.Context yourself.

  1. Register the IHttpContextAccessor dependency.

    // Startup.cs
    services.AddHttpContextAccessor();
  2. In your Ocelot configuration add this code to any Ocelot pipeline middleware that runs after AuthenticationMiddleware. Example used is PreQueryStringBuilderMiddleware, but you can also use PreAuthorizationMiddleware or AuthorizationMiddleware.

    // Startup.cs
    app.UseOcelot(oc =>
    {
    oc.PreQueryStringBuilderMiddleware = async (context, next) => 
    {
        var contextAccessor = context.RequestServices.GetRequiredService<IHttpContextAccessor>();
        contextAccessor.HttpContext = context;
    
        await next.Invoke();
    };
    }).Wait();
  3. Then inject IHttpContextAccessor into any DelegatingHandler as normal and the context will have a user identity based on the downstream route called.

Why it works Ocelot creates a new HttpContext and calls its middleware pipeline for each downstream route needed to fulfill the request. In the case of aggregators, Ocelot creates multiple contexts, one for each downstream route. Each HttpContext goes through Ocelot's AuthenticationMiddleware and we save the context after authentication happens to use later in the downstream call pipeline in a delegating handler. The concrete HttpContextAccessor saves the context in an AsyncLocal<T> variable, thus the context is not shared between calls because each downstream call happens in a new async scope.

Learn more about AsyncLocal.

raman-m commented 1 year ago

Hi @AgentShark ! Thanks for the nice coding recipe!

Does it solve the problem with user claims (aka User property)?

What about to make it as an additional method for the IOcelotBuilder interface of DependencyInjection part of Ocelot? ...to use it like this:

services
    .AddOcelot()
    .AddHttpContextAccessor();

or Make it as an additional extension method of the IApplicationBuilder interface? ...to use it like this:

app.UseOcelot()
    .UseHttpContextAccessor();
AgentShark commented 1 year ago

@raman-m Yes it solves the problem with user claims. AddHttpContextAccessor() can be registered anywhere in services it is an extension of IServiceCollection. I don't think there is any such thing as UseHttpContextAccessor(). See Docs

raman-m commented 1 year ago

@amweiss Hi Adam!

I would like personally to thank you for reporting this issue being found in v15.x! Yeah, it was a large upgrade to .NET 5 with a lot of breaking changes made by Tom.

Are you still interested to collaborate on this issue?

Specifications

  • Version: 15.0.7
  • Platform: dotnet 3.1.300
  • Subsystem: MacOS

Are you able to upgrade your solution to .NET 7 with usage of Ocelot v19.0.2 (latest ver.)? Could you report that the bug is still in v19.0.2 please?

@davipsr and @bjartekh
Could I pleasantly ask you to check the latest v19.0.2 please?

@AgentShark What version of Ocelot do you use for your solution proposed in comment on June 26, 2023?

amweiss commented 1 year ago

I no longer use this project so I won't be going back to verify.

raman-m commented 1 year ago

@amweiss This is last notification for you...

I no longer use this project so I won't be going back to verify.

Sorry for disturbing you! You can unsubscribe from notifications for this issue. And I won't address you in discussions anymore.

AgentShark commented 1 year ago

@raman-m I use v18.0.0 where the issue is still present.

raman-m commented 1 year ago

@AgentShark wrote on June 26, 2023:

Yes it solves the problem with user claims.

How can we verify your solution? Do you have some time for contribution? Could you upload your app sample to GitHub please, as a separate repo? Also, you can fork the Ocelot repo, create feature branch, and upload your solution as sample in samples folder. And, please, make a reference to Ocelot project to be sure we test the latest version (.NET 7, v19.0.2).


I don't think there is any such thing as UseHttpContextAccessor(). See Docs

Okay... It seems you misunderstood me. I meant that we can wrap this your code:

app.UseOcelot(oc =>
{
    oc.PreQueryStringBuilderMiddleware = async (context, next) => 
    {
        var contextAccessor = context.RequestServices.GetRequiredService<IHttpContextAccessor>();
        contextAccessor.HttpContext = context;
        await next.Invoke();
    };
}).Wait();

...as UseAAA() extension for the IApplicationBuilder interface in the OcelotMiddlewareExtensions class. And/Or We can develop AddAAA() method for the OcelotBuilder class, which will wrap services.AddHttpContextAccessor() and Ocelot Middleware Configuration as in the Delegating Handlers feature. I believe we have to extend the OcelotPipelineConfiguration class.

Finally we need to update Ocelot docs:


See Docs (Access HttpContext from custom components)

Microsoft Learn: Access HttpContext in ASP.NET Core Exactly! I would say we need to follow the best practices by Microsoft. I believe your solution is easier one and solid, but it requires some project update with your proposed functionality.

raman-m commented 1 year ago

TODO

raman-m commented 1 year ago
+ Accepted 

...due to open PR #1268

raman-m commented 4 months ago

@ggnaegi, FYI, the linked pull request #1268 was closed a few days ago due to a significant number of merge conflicts. I require your assistance to determine the appropriate action for this issue. Since we've implemented the new MessageInvokerPool routing design in the system kernel, it appears this issue may no longer be pertinent. Ultimately, we must replicate the bug testing on the most recent version.

ggnaegi commented 4 months ago

@raman-m Do we have this issue in a multiplexing context?