-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
#1396 Lost context User
in MultiplexingMiddleware
#1462
#1396 Lost context User
in MultiplexingMiddleware
#1462
Conversation
Link #1396 |
Please do the following:
|
ea7b0d8
to
2f2b834
Compare
@Ugway77 If you are going to contribute in future... Is this PR related to some issue in backlog? |
@Kation commented on May 28, 2021: Could you review the current solution and confirm please that the solution fixes #1396 (your user scenario)? |
@tmkhan @aliprogrammer69 @andrei-manulife |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff samples
+ No unit and acceptance tests!
- No unit and acceptance tests!
! No unit and acceptance tests!
# No unit and acceptance tests!
@@ No unit and acceptance tests! @@
LaTeX samples
$\color{green}{No\ unit\ and\ acceptance\ tests!}$ $\color{red}{No\ unit\ and\ acceptance\ tests!}$ $\color{orange}{No\ unit\ and\ acceptance\ tests!}$
LaTeX samples with size
$\color{orange}{No\ unit\ and\ acceptance\ tests!}$
$\color{orange}{No\ unit\ and\ acceptance\ tests!}$
$\color{orange}{No\ unit\ and\ acceptance\ tests!}$
$\color{orange}{No\ unit\ and\ acceptance\ tests!}$
Proposed changes must be covered by tests!
Current tests should not be failed.
User
in MultiplexingMiddleware
@ggnaegi I will take care of this PR having assigned High priority
We have feature branch: Ugway77:fix_contextuser_lost_after_multiplexing |
2f2b834
to
0c4c838
Compare
InformationClass: Ocelot.Multiplexer.MultiplexingMiddleware Line coverage: 54.1% (+1.0) |
Please, review and I'm going to merge. 1st, User can setup Ocelot app authentication (app instance access) in own way, so seems this is out of the scope of our responsibility, but we forward 2nd, downstream route can authenticate if 3rd, I paid my attention to entire Ocelot pipeline.
is defined before AuthenticationMiddleware :
So, because of that pipeline middleware order we can't get authenticated user of current request when multiplexing. But in theory we can setup Ocelot to authenticate each request (per app instance) and forward this user. But it is impossible because of pipeline design. Look at my acceptance test: it has a lot of coding hacks to finally get auth user of the request. Also, seems Ocelot breaks regular ASP.NET conveyor and my custom auth setup in AddMvcCore have failed (per app instance),https://github.com/Ugway77/Ocelot/blob/fix_contextuser_lost_after_multiplexing/test/Ocelot.AcceptanceTests/AggregateTests.cs#L560-L567 and I have to authenticate manually right in the place. This is bad. I expect that other ASP.NET middlewares are broken too: it will be impossible to setup their DI services. |
Fixes #1396
Current Behavior
If using RouteClaimsRequirement then Claims are lost after passing
MultiplexingMiddleware
because it creates newHttpContext
and does not copyUser
property.Proposed Changes
User
property when creating newHttpContext
inMultiplexingMiddleware
and, maybe, use sourceHttpContext
and simplify code if only one downstream route found.