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.

Sign up for GitHub

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

Jump to bottom

Cleanup ReCaptcha services #14333

Merged
merged 9 commits into from
Oct 19, 2023
Merged

Cleanup ReCaptcha services #14333

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9c81c1c
Change how ReCaptchaClient is registered
MikeAlhayek Sep 14, 2023
2278c7b
cleanup
MikeAlhayek Sep 14, 2023
6650e51
cleanup
MikeAlhayek Sep 14, 2023
4cc1dc4
Remove ReCaptchaClient
MikeAlhayek Sep 14, 2023
27b696b
cleanup driver
MikeAlhayek Sep 14, 2023
8142c91
cleanup
MikeAlhayek Sep 25, 2023
50bdaa1
Merge branch 'main' into ma/recaptcha
MikeAlhayek Oct 19, 2023
e419a60
Update ServiceCollectionExtensions.cs
sebastienros Oct 19, 2023
23d243b
Fix build and cleanup
MikeAlhayek Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/OrchardCore/OrchardCore.ReCaptcha.Core/ServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@ public static class ServiceCollectionExtensions
{
public static IServiceCollection AddReCaptcha(this IServiceCollection services, Action<ReCaptchaSettings> configure = null)
{
services.AddHttpClient<ReCaptchaClient>()
.AddTransientHttpErrorPolicy(policy => policy.WaitAndRetryAsync(3, attempt => TimeSpan.FromSeconds(0.5 * attempt)));
services.AddHttpClient<ReCaptchaClient>(ReCaptchaClient.Name, (sp, client) =>
{
var settings = sp.GetRequiredService<IOptions<ReCaptchaSettings>>().Value;
client.BaseAddress = new Uri(settings.ReCaptchaApiUri);
}).AddTransientHttpErrorPolicy(policy => policy.WaitAndRetryAsync(3, attempt => TimeSpan.FromSeconds(0.5 * attempt)));

services.AddTransient<IDetectRobots, IPAddressRobotDetector>();
services.AddTransient<IConfigureOptions<ReCaptchaSettings>, ReCaptchaSettingsConfiguration>();
services.AddTransient<ReCaptchaService>();
services.AddSingleton<ReCaptchaService>();
services.AddTagHelpers<ReCaptchaTagHelper>();

if (configure != null)
Expand Down
19 changes: 10 additions & 9 deletions src/OrchardCore/OrchardCore.ReCaptcha.Core/Services/ReCaptchaClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,23 @@
using System;
using System.Collections.Generic;
using System.Net.Http;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Newtonsoft.Json.Linq;
using OrchardCore.ReCaptcha.Configuration;

namespace OrchardCore.ReCaptcha.Services
{
public class ReCaptchaClient
{
private readonly HttpClient _httpClient;
public const string Name = "ReCaptcha";

private readonly IHttpClientFactory _httpClientFactory;
private readonly ILogger _logger;

public ReCaptchaClient(HttpClient httpClient, IOptions<ReCaptchaSettings> optionsAccessor, ILogger<ReCaptchaClient> logger)
public ReCaptchaClient(
IHttpClientFactory httpClientFactory,
ILogger<ReCaptchaClient> logger)
{
var options = optionsAccessor.Value;
_httpClient = httpClient;
_httpClient.BaseAddress = new Uri(options.ReCaptchaApiUri);
_httpClientFactory = httpClientFactory;
_logger = logger;
}

Expand All @@ -42,7 +41,9 @@ public async Task<bool> VerifyAsync(string responseToken, string secretKey)
});
try
{
var response = await _httpClient.PostAsync("siteverify", content);
var client = _httpClientFactory.CreateClient(Name);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is assuming that IHttpClientFactory is registered at the tenant level and not the host level so that each tenant for the same resolved name will get a different instance, because it has a custom configuration (Api Url).

But if we didn't set the base uri and have it instead in the POST call then we can even share the same client for all services in the tenant without base Uri.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here the options are evaluated every time the new client is created. So it would use the current tenant's options. We may have to remove the ReCaptchSettings from the singleton service too

Copy link
Member Author

@MikeAlhayek MikeAlhayek Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastienros I did some cleanup. I think ReCaptchaService is okay to make it scoped. Since there is no abstractions here, I removed ReCaptchaClient. I think it's over engineering and adds unnecessary complexity. Now, we register ReCaptchaService as scoped service which should be good in this case. Also, we now use IHttpClientFactory. I also replace Newtonsoft with System.Text.Json as an added bonus.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IHttpClientFactory, was it removed in the meantime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still use it. We register HttpClient for the type, so injecting HttpClient in the constructor will return the same instance for that type

var response = await client.PostAsync("siteverify", content);
response.EnsureSuccessStatusCode();
var responseJson = await response.Content.ReadAsStringAsync();
var responseModel = JObject.Parse(responseJson);
Expand Down
8 changes: 7 additions & 1 deletion src/OrchardCore/OrchardCore.ReCaptcha.Core/Services/ReCaptchaService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ public class ReCaptchaService
private readonly ILogger _logger;
protected readonly IStringLocalizer S;

public ReCaptchaService(ReCaptchaClient reCaptchaClient, IOptions<ReCaptchaSettings> optionsAccessor, IEnumerable<IDetectRobots> robotDetectors, IHttpContextAccessor httpContextAccessor, ILogger<ReCaptchaService> logger, IStringLocalizer<ReCaptchaService> stringLocalizer)
public ReCaptchaService(
ReCaptchaClient reCaptchaClient,
IOptions<ReCaptchaSettings> optionsAccessor,
IEnumerable<IDetectRobots> robotDetectors,
IHttpContextAccessor httpContextAccessor,
ILogger<ReCaptchaService> logger,
IStringLocalizer<ReCaptchaService> stringLocalizer)
{
_reCaptchaClient = reCaptchaClient;
_settings = optionsAccessor.Value;
Expand Down
Loading