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.

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

Cleanup ReCaptcha services #14333

Merged
merged 9 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
using System;
using System.Threading.Tasks;
using OrchardCore.ContentManagement.Display.ContentDisplay;
using OrchardCore.ContentManagement.Display.Models;
using OrchardCore.DisplayManagement.Views;
Expand All @@ -20,21 +18,21 @@ public ReCaptchaPartDisplayDriver(ISiteService siteService)

public override IDisplayResult Display(ReCaptchaPart part, BuildPartDisplayContext context)
{
return Initialize("ReCaptchaPart", (Func<ReCaptchaPartViewModel, ValueTask>)(async m =>
return Initialize<ReCaptchaPartViewModel>("ReCaptchaPart", async model =>
{
var siteSettings = await _siteService.GetSiteSettingsAsync();
var settings = siteSettings.As<ReCaptchaSettings>();
m.SettingsAreConfigured = settings.IsValid();
})).Location("Detail", "Content");
model.SettingsAreConfigured = settings.IsValid();
}).Location("Detail", "Content");
}

public override IDisplayResult Edit(ReCaptchaPart part, BuildPartEditorContext context)
{
return Initialize<ReCaptchaPartViewModel>("ReCaptchaPart_Fields_Edit", async m =>
return Initialize<ReCaptchaPartViewModel>("ReCaptchaPart_Fields_Edit", async model =>
{
var siteSettings = await _siteService.GetSiteSettingsAsync();
var settings = siteSettings.As<ReCaptchaSettings>();
m.SettingsAreConfigured = settings.IsValid();
model.SettingsAreConfigured = settings.IsValid();
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.ReCaptcha/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public override void ConfigureServices(IServiceCollection services)
services.AddScoped<IPasswordRecoveryFormEvents, PasswordRecoveryFormEventEventHandler>();
services.Configure<MvcOptions>((options) =>
{
options.Filters.Add(typeof(ReCaptchaLoginFilter));
options.Filters.Add<ReCaptchaLoginFilter>();
});
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
using System;

namespace OrchardCore.ReCaptcha.Configuration
{
public class ReCaptchaSettings
Expand All @@ -17,9 +15,11 @@ public class ReCaptchaSettings
/// </summary>
public int DetectionThreshold { get; set; } = 5;

private bool? _isValid;

public bool IsValid()
{
return !string.IsNullOrWhiteSpace(SiteKey) && !string.IsNullOrWhiteSpace(SecretKey);
}
=> _isValid ??= !string.IsNullOrWhiteSpace(SiteKey)
&& !string.IsNullOrWhiteSpace(SecretKey)
&& !string.IsNullOrWhiteSpace(ReCaptchaApiUri);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ public static class ServiceCollectionExtensions
{
public static IServiceCollection AddReCaptcha(this IServiceCollection services, Action<ReCaptchaSettings> configure = null)
{
services.AddHttpClient<ReCaptchaClient>()
// c.f. https://learn.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests
services.AddScoped<ReCaptchaService>()
.AddHttpClient<ReCaptchaService>()
Copy link
Member

@jtkech jtkech Sep 30, 2023

Choose a reason for hiding this comment

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

If the typed client is now tied to ReCaptchaService, because it is also injected by ReCaptchaLoginFilter on each request, an HttpClient will be still created, by using the IHttpClientFactory but still on each request.

One way would be to make ReCaptchaService a singleton but not good to hold a fully built HttpClient a too long time.

I will add to #14348 the simple fix I did in #14335 which resolves the client lazily, because they are both related to #14117, I may change the lifetime of ReCaptchaService, at least a scoped service, maybe a singleton if it no longer injects the client, will see.

But not incompatible with what you did here because other changes and cleanups look good.

.AddTransientHttpErrorPolicy(policy => policy.WaitAndRetryAsync(3, attempt => TimeSpan.FromSeconds(0.5 * attempt)));

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

services.AddTagHelpers<ReCaptchaTagHelper>();

if (configure != null)
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace OrchardCore.ReCaptcha.Services;

public class ReCaptchaResponse
{
public bool Success { get; set; }
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Net.Http.Json;
using System.Text.Json;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Localization;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
Expand All @@ -15,20 +17,28 @@ namespace OrchardCore.ReCaptcha.Services
{
public class ReCaptchaService
{
private readonly ReCaptchaSettings _settings;
private static readonly JsonSerializerOptions _jsonSerializerOptions = new()
{
PropertyNamingPolicy = SnakeCaseNamingPolicy.Instance,
};

private readonly ReCaptchaSettings _reCaptchaSettings;
private readonly HttpClient _httpClient;
private readonly IEnumerable<IDetectRobots> _robotDetectors;
private readonly IHttpContextAccessor _httpContextAccessor;
private readonly ILogger _logger;
protected readonly IStringLocalizer S;

public ReCaptchaService(
HttpClient httpClient,
IOptions<ReCaptchaSettings> optionsAccessor,
IEnumerable<IDetectRobots> robotDetectors,
IHttpContextAccessor httpContextAccessor,
ILogger<ReCaptchaService> logger,
IStringLocalizer<ReCaptchaService> stringLocalizer)
{
_settings = optionsAccessor.Value;
_reCaptchaSettings = optionsAccessor.Value;
_httpClient = httpClient;
_robotDetectors = robotDetectors;
_httpContextAccessor = httpContextAccessor;
_logger = logger;
Expand All @@ -39,54 +49,43 @@ public ReCaptchaService(
/// Flags the behavior as that of a robot.
/// </summary>
public void MaybeThisIsARobot()
{
_robotDetectors.Invoke(i => i.FlagAsRobot(), _logger);
}
=> _robotDetectors.Invoke(i => i.FlagAsRobot(), _logger);

/// <summary>
/// Determines if the request has been made by a robot.
/// </summary>
/// <returns>Yes (true) or no (false).</returns>
public bool IsThisARobot()
{
var result = _robotDetectors.Invoke(i => i.DetectRobot(), _logger);
return result.Any(a => a.IsRobot);
}
=> _robotDetectors.Invoke(i => i.DetectRobot(), _logger)
.Any(a => a.IsRobot);

/// <summary>
/// Clears all robot markers, we are dealing with a human.
/// </summary>
/// <returns></returns>
public void ThisIsAHuman()
{
_robotDetectors.Invoke(i => i.IsNotARobot(), _logger);
}
=> _robotDetectors.Invoke(i => i.IsNotARobot(), _logger);

/// <summary>
/// Verifies the ReCaptcha response with the ReCaptcha webservice.
/// </summary>
/// <param name="reCaptchaResponse"></param>
/// <returns></returns>
public Task<bool> VerifyCaptchaResponseAsync(string reCaptchaResponse)
{
if (string.IsNullOrWhiteSpace(reCaptchaResponse))
{
return Task.FromResult(false);
}
public async Task<bool> VerifyCaptchaResponseAsync(string reCaptchaResponse)
=> !string.IsNullOrWhiteSpace(reCaptchaResponse)
&& _reCaptchaSettings.IsValid()
&& await VerifyAsync(reCaptchaResponse);

var reCaptchaClient = _httpContextAccessor.HttpContext.RequestServices.GetRequiredService<ReCaptchaClient>();
return reCaptchaClient.VerifyAsync(reCaptchaResponse, _settings.SecretKey);
}

/// <summary>
/// Validates the captcha that is in the Form of the current request.
/// </summary>
/// <param name="reportError">Lambda for reporting errors.</param>
public async Task<bool> ValidateCaptchaAsync(Action<string, string> reportError)
{
if (!_settings.IsValid())
if (!_reCaptchaSettings.IsValid())
{
_logger.LogWarning("The ReCaptcha settings are not valid");
_logger.LogWarning("The ReCaptcha settings are invalid");
return false;
}

Expand All @@ -99,14 +98,43 @@ public async Task<bool> ValidateCaptchaAsync(Action<string, string> reportError)
reCaptchaResponse = _httpContextAccessor.HttpContext.Request.Form[Constants.ReCaptchaServerResponseHeaderName].ToString();
}

var isValid = !string.IsNullOrEmpty(reCaptchaResponse) && await VerifyCaptchaResponseAsync(reCaptchaResponse);
var isValid = await VerifyCaptchaResponseAsync(reCaptchaResponse);

if (!isValid)
{
reportError("ReCaptcha", S["Failed to validate captcha"]);
reportError("ReCaptcha", S["Failed to validate ReCaptcha"]);
}

return isValid;
}

/// <summary>
/// Verifies the supplied token with ReCaptcha Api.
/// </summary>
/// <param name="responseToken">Token received from the ReCaptcha UI.</param>
/// <returns>A boolean indicating if the token is valid.</returns>
private async Task<bool> VerifyAsync(string responseToken)
{
try
{
var content = new FormUrlEncodedContent(new Dictionary<string, string>
{
{ "secret", _reCaptchaSettings.SecretKey },
{ "response", responseToken }
});

var response = await _httpClient.PostAsync($"{_reCaptchaSettings.ReCaptchaApiUri.TrimEnd('/')}/siteverify", content);
response.EnsureSuccessStatusCode();
var result = await response.Content.ReadFromJsonAsync<ReCaptchaResponse>(_jsonSerializerOptions);

return result.Success;
}
catch (HttpRequestException e)
{
_logger.LogError(e, "Could not contact Google to verify ReCaptcha.");
}

return false;
}
}
}