Support forward proxy

[mikocot]

This feature allows to specify Proxy address to be used by the http client for the requests done from the web server.

This is required for many production environments for additional security.

User only needs to specify 'UseProxy' and 'ProxyAddress' in the settings

[jooni91]

First, let me thank you for taking the time to contribute to this project. I really appreciate it!

I really like this new feature.

I've taken the time to review this PR and overall it looks good. However, there is one potential performance/security issue with the approach of creating HttpClient without IHttpClientFactory. Or to be more specific, the creation of HttpClientHandler instances without pooling them and proper lifetime management.

Using IHttpClientFactory in a DI-enabled app avoids:

- Resource exhaustion problems by pooling HttpMessageHandler instances.
- Stale DNS problems by cycling HttpMessageHandler instances at regular intervals.

There is a built-in solution for what you're trying to achieve without the need to give up the usage of IHttpClientFactory.

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0#configure-the-httpmessagehandler

To make use of the RecaptchaSettings you can create a custom implementation of HttpClientHandler and request the settings in the constructor of that implementation. Then register that implementation with the generic overload of ConfigurePrimaryHttpMessageHandler.

Please could you have a look at this issue and make changes according to to fix it?

[mikocot]

Hm, good point, I actually expected a similar solution when I first looked into the problem, but for some reason couldn't find it in the docs. I'll look into that later and update it.

[mikocot]

@jooni91 the problem I see is that keeping current API, we only have access to IServiceCollection in AddRecaptchaService method extension. Therefore neither this, nor calling method, has access to the actual configuration from appsettings.json - unless we temprarily build a service provider and request IOption service (via BuildServiceProvider), which is possible, but not very nice solution or access config file directly, which is even worse. - see the changes.

[jooni91]

What we can do instead is to create our own implementation of HttpClientHandler and inject our settings in the constructor of that class, for example:

public class RecaptchaHttpClientHandler : HttpClientHandler
{
    public RecaptchaHttpClientHandler(IOptions<RecaptchaSettings> settings)
    {
        this.UseProxy = settings.Value.UseProxy;
        this.Proxy = new WebProxy(settings.Value.ProxyAdress, false);
    }
}

Then we can register it with the generic overload of ConfigurePrimaryHttpMessageHandler like so:

services.AddHttpClient(RecaptchaServiceConstants.RecaptchaServiceHttpClientName, client =>
{
    client.BaseAddress = new Uri(RecaptchaServiceConstants.GoogleRecaptchaEndpoint);
})
.ConfigurePrimaryHttpMessageHandler<RecaptchaHttpClientHandler>();

This should work.

[mikocot]

Yeah, that's well hidden now, but cleaner too.

[jooni91]

Everything looks good now. I've commented had a question on some changes that were made to a few unit tests, could you have a look at that?

[jooni91]

Is there a reason behind this change in these tests? The same change was made on lines 133 and 167.

[mikocot]

hm, it's a consequence of first this expression having to be removed to match the interface of the previous handler factory, and then me quickfixing it without checking the context :P. I think Moq is actually going to do the same thing in both versions, but indeed no point having this change. I just didn't look at the complete changeset in the end.

[jooni91]

I will merge this feature into the master branch asap.

[mikocot]

I will merge this feature into the master branch asap.

Thanks, not sure what's the pipeline here, but would be nice to have a release, even if a beta one relatively soon. Cheers!

[jooni91]

I'm sorry that it takes so long to get the merge done. Unfortunately, it just happened that I'm moving between countries currently, which requires all my focus.

By the end of this week I should be able to get back to work again.

[jooni91]

Back in business. I was about to merge the PR and noticed that you didn't sign your commits. I will ignore that this time, but signing them is recommended.