Skip to content
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

HttpContext lost between HttpClientWrapper and DelegatingHandler #1252

Open
amweiss opened this issue Jun 1, 2020 · 17 comments
Open

HttpContext lost between HttpClientWrapper and DelegatingHandler #1252

amweiss opened this issue Jun 1, 2020 · 17 comments
Assignees
Labels
bug Identified as a potential bug feature A new feature

Comments

@amweiss
Copy link

amweiss commented Jun 1, 2020

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

  • Version: 15.0.7
  • Platform: dotnet 3.1.300
  • Subsystem: MacOS
@amweiss amweiss changed the title Context lost between HttpClientWrapper and DelegatingHandler Context lost between HttpClientWrapper and DelegatingHandler as of 15.0.7 Jun 1, 2020
@davipsr
Copy link

davipsr commented Jun 2, 2020

Same issue here, version 16.0.1

@jlukawska
Copy link
Contributor

Hello,
The reason is that after changes done in fe3e8bd 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
Copy link

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:

  • Obtain an accessToken for ClientID A (from IdentityServer4)

  • Use accessToken A towards Route 1 in Ocelot

  • Observe that HttpContext.User.Claims contained claims present in accessToken A

  • Obtain an accessToken for ClientID B (from IdentityServer4)

  • Use accessToken B towards Route 1 in Ocelot

  • Observe that HttpContext.User.Claims contained claims present in accessToken A

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
Copy link
Contributor

@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
Copy link
Contributor

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

@AgentShark
Copy link

AgentShark commented Jun 26, 2023

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();
  1. 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();
  1. 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
Copy link
Member

raman-m commented Jun 26, 2023

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
Copy link

AgentShark commented Jun 26, 2023

@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
Copy link
Member

raman-m commented Jun 28, 2023

@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 amweiss closed this as not planned Won't fix, can't repro, duplicate, stale Jun 28, 2023
@amweiss
Copy link
Author

amweiss commented Jun 28, 2023

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

@raman-m raman-m assigned jlukawska and unassigned amweiss Jun 28, 2023
@raman-m
Copy link
Member

raman-m commented Jun 28, 2023

@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.

@raman-m raman-m reopened this Jun 28, 2023
@AgentShark
Copy link

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

@raman-m raman-m changed the title Context lost between HttpClientWrapper and DelegatingHandler as of 15.0.7 HttpContext lost between HttpClientWrapper and DelegatingHandler Jun 28, 2023
@raman-m
Copy link
Member

raman-m commented Jun 28, 2023

@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
Copy link
Member

raman-m commented Jul 3, 2023

TODO

  • Create a new issue labeled as a feature request for the design of middleware to be integrated into the Ocelot pipeline as the final step before sending the request

@raman-m raman-m added bug Identified as a potential bug feature A new feature accepted Bug or feature would be accepted as a PR or is being worked on labels Jul 7, 2023
@raman-m
Copy link
Member

raman-m commented Jul 7, 2023

+ Accepted 

...due to open PR #1268

@raman-m
Copy link
Member

raman-m commented May 16, 2024

@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.

@raman-m raman-m removed the accepted Bug or feature would be accepted as a PR or is being worked on label May 16, 2024
@ggnaegi
Copy link
Member

ggnaegi commented May 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug feature A new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants