Ocelot
Ocelot copied to clipboard
ThreeMammals
#657 Ability to set `UseDefaultCredentials` per route
Closes #657
- #657
Support setting WebClient.UseDefaultCredentials per route if needed.
Docs
- Microsoft Learn: WebClient.UseDefaultCredentials Property
@Sacrilege Hi Troy! Could you resolve merge conflicts please? And what issue is this PR related to?
The feature branch has been rebased onto ThreeMammals:develop successfully! Welcome to code review!
@raman-m
@Sacrilege Troy, What is related issue? Number?
https://github.com/ThreeMammals/Ocelot/issues/657
Waiting for this merge. in most of the cases users configure the gateway and api on same server and allow ui to call gateway and handle routing internally, hence this becomes a must when user identity with windows auth is required over https.
Looking for early merge on main branch as it's been very long since this was opened
If you all are waiting for me to merge this, I'm not authorized. I don't know why the maintainers haven't done anything with the PR in the last 2+ years. During that time I've moved to using bla-bla-bla from bla-bla-bla instead though.
Update 1
Personal information has been removed!
@pank789 commented on Oct 24
Soon... This PR is not reviewed. It has no new tests, unit tests or acceptance ones. Do you have an intention to help Troy to write tests?
@Sacrilege Do you agree to link this PR to #657 mentioned by @pank789 ? The scope of work will change after linking...
@RaynaldM @ggnaegi This feature has received a lot of interest from the community. I would say, PR is a good candidate for Dec'23 milestone, right? The logic inside is pretty simple. We need ensure .NET 6-7-8 support. And write more tests + docs... So, it should be developed more with medium efforts, in my opinion.
@ggnaegi I'm worrying only about the PR is not linked to any issues in backlog... But if we've agreed on delivery in Dec'23 then OK, let's develop it.
@tobiashoeft Hello, we are currently finalizing Nov-December'23 release. The PR is part of the Annual 2023 release later this year. I don't think we will be able to release this before March or April this year. We are 3 developers maintaining this project - after a very long span of inactivity - any help is welcome.
@raman-m fyi
Feature branch has been rebased!
I'm not sure what to do with these changes :point_down: but the file was removed in head repo.
HttpClientBuilder.cs
private static HttpClientHandler UseNonCookiesHandler(DownstreamRoute downstreamRoute)
{
var handler = new HttpClientHandler
{
AllowAutoRedirect = downstreamRoute.HttpHandlerOptions.AllowAutoRedirect,
UseCookies = downstreamRoute.HttpHandlerOptions.UseCookieContainer,
UseProxy = downstreamRoute.HttpHandlerOptions.UseProxy,
MaxConnectionsPerServer = downstreamRoute.HttpHandlerOptions.MaxConnectionsPerServer,
UseDefaultCredentials = downstreamRoute.HttpHandlerOptions.UseDefaultCredentials,
};
// !!!!!!!!!!!
// Dont' create the CookieContainer if UseCookies is not set or the HttpClient will complain
// under .Net Full Framework
if (downstreamRoute.HttpHandlerOptions.UseCookieContainer)
{
handler.CookieContainer = new CookieContainer();
}
// !!!!!!!!!!!
return handler;
}
@raman-m there: https://github.com/ThreeMammals/Ocelot/blob/develop/src%2FOcelot%2FRequester%2FMessageInvokerPool.cs#L64
@ggnaegi OK Thanks!
https://github.com/ThreeMammals/Ocelot/blob/d54836d51f6ea29a2013967e3649a23e19db28b3/src/Ocelot/Requester/MessageInvokerPool.cs#L75-L78
Not an issue!
@raman-m I can write the test cases if you want, but I should be authorized.
@Sacrilege
Dear Troy, Our teammate @ggnaegi would like to help you to write tests. Could you add him as Collaborator to your forked repo please? After adding his user he will be able to push commits to any branches of your fork. After merging the PR you can remove his user if you'd like to keep the repo secured.
@ggnaegi FYI You'll be authorized after adding you as Collaborator.
@raman-m Mocking UseDefaultCredentials in an acceptance test on linux hosts could be quite a pain...
@ggnaegi commented on Apr 3
Give me the link to the problematic code plz?! Now I don't understand the problem... Impossible to write tests? Will current tests for the feature fail in Linux environment on CircleCI?
Would you like to mentor this PR? If Yes, assign this PR to yourself!
Failed Ocelot.IntegrationTests.AdministrationTests.Should_return_OK_status_and_multiline_indented_json_response_with_json_options_for_custom_builder
A tiny fix for integration test is required.
Current problem in build 3803 ->
Scenario: Should return OK status and multiline indented json response with json options for custom builder
Given there is a configuration Ocelot.Configuration.File.FileConfiguration [Passed]
And given ocelot using builder is running System.Func`3[Microsoft.Extensions.DependencyInjection.IMvcCoreBuilder,System.Reflection.Assembly,Microsoft.Extensions.DependencyInjection.IMvcCoreBuilder] [Passed]
And given I have an ocelot token /administration [Passed]
And given I have added a token to my request [Passed]
When I get url on the api gateway /administration/configuration [Passed]
Then the status code should be OK [Passed]
Then the result have multi line indented json [Failed] [lines.Length should be 46 but was 47] [Details at 1 below]
Exceptions:
1. lines.Length should be 46 but was 47
at Ocelot.IntegrationTests.AdministrationTests.ThenTheResultHaveMultiLineIndentedJson() in /root/project/test/Ocelot.IntegrationTests/AdministrationTests.cs:line 887
at TestStack.BDDfy.StepActionFactory.<>c__DisplayClass1_0`1.<GetStepAction>b__0(Object o)
at TestStack.BDDfy.StepExecutor.Execute(Step step, Object testObject)
at TestStack.BDDfy.Processors.ScenarioExecutor.<>c__DisplayClass3_0.<ExecuteStep>b__0()
at TestStack.BDDfy.Processors.AsyncTestRunner.Run(Func`1 performStep)
at TestStack.BDDfy.Processors.ScenarioExecutor.ExecuteStep(Step step)
[xUnit.net 00:00:02.09] Ocelot.IntegrationTests.AdministrationTests.Should_return_OK_status_and_multiline_indented_json_response_with_json_options_for_custom_builder [FAIL]
The step lines.Length should be 46 but was 47 should be commented or removed at all.
The test is very unstable in different PRs and builds. So this step must be removed because not so important.