#1252 Fix HttpContext in DelegatingHandler #1268

jlukawska · 2020-06-23T11:11:48Z

Fixes #1252

The problem

After changes from fe3e8bd there is a new instance of HttpContext created for each request (or even more instances for aggregated routes). If HttpContextAccessor is used to obtain a HttpContext instance in a delegating handler class, it will return the original HttpContext without e.g. authentication data that is added to the new instance.

Proposed Changes

To obtain a HttpContext, a delegating handler class should implement IDelegatingHandlerWithHttpContext interface
DelegatingHandlerHandlerFactory will add the valid HttpContext as a property to the delegating handler class

Conflicts: src/Ocelot/Requester/HttpClientBuilder.cs src/Ocelot/Requester/HttpClientWrapper.cs Cherry picked from ThreeMammals#1268

raman-m · 2023-06-26T15:01:05Z

Hi J!
Thanks for a great PR!

I've merged latest top-commits from develop, resolved merge conflicts, fixed a few compile errors.
But all acceptance ClaimsInDelegatingHandlerTests are failing because of this error 😞

Message: 
System.Net.Http.HttpRequestException : Response status code does not indicate success: 400 (Bad Request).

  Stack Trace: 
HttpResponseMessage.EnsureSuccessStatusCode()
Steps.GivenIHaveAToken(String url, String username) line 918
Steps.GivenIHaveAToken(String url) line 897
<>c__DisplayClass1_0`1.<GetStepAction>b__0(Object o)
StepExecutor.Execute(Step step, Object testObject)
<>c__DisplayClass3_0.<ExecuteStep>b__0()
AsyncTestRunner.Run(Func`1 performStep)
ScenarioExecutor.ExecuteStep(Step step)
ExceptionProcessor.Process(Story story)
Engine.Run()
BDDfyExtensions.BDDfy(Object testObject, String scenarioTitle, String caller)
ClaimsInDelegatingHandlerTests.should_expose_claims_in_route_specific_delegating_handler() line 144
RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

I believe a small fix in test setup section should fix the problem of 400 Bad Request.

Could you have a look to it and fix the issue please?

raman-m · 2023-08-01T16:20:21Z

@jlukawska
The feature branch has been rebased onto ThreeMammals:develop successfully!
We are going to code review stage...

raman-m · 2023-08-01T16:27:20Z

@davipsr @bjartekh @AgentShark
Could you review the code please?
Your opinion is prioritized for sure because of past discussions of the #1252 issue.

@amweiss
Because you're the #1252 reporter, I believe this PR will be interesting to look into.

wast · 2023-08-01T18:51:56Z

src/Ocelot/Requester/HttpClientWrapper.cs

-        public HttpClientWrapper(HttpClient client)
+        public DelegatingHandler ClientMainHandler { get; }
+
+        public HttpClientWrapper(HttpClient client, DelegatingHandler clientMainHandler)


Why not making clientMainHandler default null?

Or define new overloaded method version. 😉

RaynaldM · 2023-08-02T11:14:47Z

src/Ocelot/Requester/DelegatingHandlerHandlerFactory.cs

@@ -34,7 +30,7 @@ public class DelegatingHandlerHandlerFactory : IDelegatingHandlerHandlerFactory
            _qoSFactory = qoSFactory;
        }

-        public Response<List<Func<DelegatingHandler>>> Get(DownstreamRoute downstreamRoute)
+        public Response<List<Func<DelegatingHandler>>> Get(DownstreamRoute downstreamRoute, HttpContext httpContext)


Suggested change

public Response<List<Func<DelegatingHandler>>> Get(DownstreamRoute downstreamRoute, HttpContext httpContext)

public Response<IList<Func<DelegatingHandler>>> Get(DownstreamRoute downstreamRoute, HttpContext httpContext)

src/Ocelot/Requester/DelegatingHandlerHandlerFactory.cs

RaynaldM · 2023-08-02T11:15:32Z

src/Ocelot/Requester/DelegatingHandlerHandlerFactory.cs

+                }
+
+                return delegatingHandler;
+            }).ToList();


Suggested change

}).ToList();

}).ToArray();

Do you hate lists?

Array is more efficient than list on ram usage

raman-m · 2023-08-03T12:40:00Z

@RaynaldM Hey Ray!
Thanks for code review!

When you've approved the PR, does it mean that all issues from your code review are minor?
I suggest not giving an approval if issues are major, and they should be fixed.
OK? 😉

…and unit tests

One line definition of the auto-property

…have its default value null

raman-m · 2023-10-13T19:30:56Z

Build 2170 has been failed after last changes in Polly provider.

@RaynaldM @jlukawska
Could you look into build logs and understand the problem please?

It seems we have to fix these tests:

All these tests belong to the ClaimsInDelegatingHandlerTests class.
I guess this is setup test problem, or test server cannot start.

jlukawska · 2024-05-13T20:32:17Z

This PR was created 4 years ago and almost all modified files no longer exist. As I saw in #1252 there is another idea to solve this issue. I'm closing this PR for now.

raman-m · 2024-05-16T11:10:08Z

I understand your decision to close this PR. However, you are welcome to open a new PR if you wish to contribute. It is crucial to use the new code base from the development branch. The internal Ocelot system kernel underwent a redesign last year, and we need to devise a new solution or re-investigate the reported issue.

jlukawska mentioned this pull request Jun 23, 2020

HttpContext lost between HttpClientWrapper and DelegatingHandler #1252

Open

jlukawska marked this pull request as draft April 28, 2021 11:24

jlukawska marked this pull request as ready for review May 7, 2021 10:39

stefancruz added a commit to stefancruz/Ocelot that referenced this pull request Dec 22, 2022

Merge branch 'fix/1252-user-context-in-delegating-handler'

3d1d351

Conflicts: src/Ocelot/Requester/HttpClientBuilder.cs src/Ocelot/Requester/HttpClientWrapper.cs Cherry picked from ThreeMammals#1268

raman-m changed the base branch from master to develop June 26, 2023 10:02

raman-m changed the title ~~Fix/1252 HttpContext in DelegatingHandler~~ #1252 Fix HttpContext in DelegatingHandler Jun 26, 2023

raman-m assigned jlukawska Jun 26, 2023

raman-m self-requested a review July 3, 2023 12:50

raman-m added bug Identified as a potential bug needs feedback Issue is waiting on feedback before acceptance labels Jul 19, 2023

raman-m force-pushed the fix/1252-user-context-in-delegating-handler branch from 3222fe3 to 55d2738 Compare August 1, 2023 15:20

raman-m requested review from binarymash, jps, geffzhang, RaynaldM, philproctor, anktsrkr, sharpn, abooraja, wast, yangzhongke and thiagoloureiro August 1, 2023 16:27

wast reviewed Aug 1, 2023

View reviewed changes

RaynaldM approved these changes Aug 2, 2023

View reviewed changes

jlukawska added 2 commits October 13, 2023 20:13

attach HttpContext to delegating handlers when necessary, acceptance …

c8e675f

…and unit tests

update docs

694c93f

jlukawska and others added 11 commits October 13, 2023 20:13

update HttpContext for HttpClient retrieved from cache

7b28872

Fix Compile errors

f876677

Fix problem after breaking change introduced in IdentityServer4

f84a7cf

Update ClaimsInDelegatingHandlerTests.cs

7c96fb5

One line definition of the auto-property

File-scoped namespaces

d6b9f95

Fix warnings and messages

7af478d

Improve reStructuredText by inline code blocks markup

c5ee79a

Use target-typed 'new'

bd24f0b

Remove and Sort Usings

3fa7549

CS0649 Field '_servicebuilder' is never assigned to, and will always …

88ffdc0

…have its default value null

Add required namespaces

6a8822a

raman-m force-pushed the fix/1252-user-context-in-delegating-handler branch from b67123a to 6a8822a Compare October 13, 2023 19:00

fix tests after merge

fe139a0

raman-m removed request for binarymash, jps, geffzhang, philproctor, anktsrkr, sharpn, abooraja, yangzhongke and thiagoloureiro May 13, 2024 17:41

jlukawska closed this May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1252 Fix HttpContext in DelegatingHandler #1268

#1252 Fix HttpContext in DelegatingHandler #1268

jlukawska commented Jun 23, 2020 •

edited by raman-m

Loading

raman-m commented Jun 26, 2023

raman-m commented Aug 1, 2023

raman-m commented Aug 1, 2023

wast Aug 1, 2023

raman-m Aug 3, 2023

RaynaldM Aug 2, 2023

RaynaldM Aug 2, 2023

raman-m Aug 3, 2023

RaynaldM May 13, 2024

raman-m commented Aug 3, 2023 •

edited

Loading

raman-m commented Oct 13, 2023

jlukawska commented May 13, 2024

raman-m commented May 16, 2024

	public Response<List<Func<DelegatingHandler>>> Get(DownstreamRoute downstreamRoute, HttpContext httpContext)
	public Response<IList<Func<DelegatingHandler>>> Get(DownstreamRoute downstreamRoute, HttpContext httpContext)

#1252 Fix HttpContext in DelegatingHandler #1268

#1252 Fix HttpContext in DelegatingHandler #1268

Conversation

jlukawska commented Jun 23, 2020 • edited by raman-m Loading

Fixes #1252

The problem

Proposed Changes

raman-m commented Jun 26, 2023

raman-m commented Aug 1, 2023

raman-m commented Aug 1, 2023

wast Aug 1, 2023

Choose a reason for hiding this comment

raman-m Aug 3, 2023

Choose a reason for hiding this comment

RaynaldM Aug 2, 2023

Choose a reason for hiding this comment

RaynaldM Aug 2, 2023

Choose a reason for hiding this comment

raman-m Aug 3, 2023

Choose a reason for hiding this comment

RaynaldM May 13, 2024

Choose a reason for hiding this comment

raman-m commented Aug 3, 2023 • edited Loading

raman-m commented Oct 13, 2023

jlukawska commented May 13, 2024

raman-m commented May 16, 2024

jlukawska commented Jun 23, 2020 •

edited by raman-m

Loading

raman-m commented Aug 3, 2023 •

edited

Loading