Fix reCaptcha LoginForm #17218

MikeAlhayek · 2024-12-11T17:13:51Z

Fix #17200

MikeAlhayek · 2024-12-11T17:14:13Z

@rjpowers10 can you please test out this PR?

…dCMS/OrchardCore into ma/fix-recaptcha-on-login

rjpowers10 · 2024-12-11T17:27:24Z

It's working after 5 failures now.

rjpowers10 · 2024-12-11T20:20:29Z

@MikeAlhayek I can't remember all the places a ReCaptcha is shown. Could there be other places with a similar issue, such as user registration?

MikeAlhayek · 2024-12-11T20:29:26Z

@rjpowers10 I don't know if there is an issue elsewhere. Guessing not since this issue came up after the LoginForm was added.

For now, we'll fix the known bug and worry about other "if any" when they are reported.

Guessing from your last screenshot you are reporting that the bug is indeed fixed

rjpowers10 · 2024-12-11T20:48:45Z

Yes, LGTM

Piedone · 2024-12-11T21:46:15Z

src/OrchardCore/OrchardCore.ReCaptcha.Core/ActionFilters/Detection/IPAddressRobotDetector.cs

-        faultyRequestCount++;
-        _memoryCache.Set(ipAddressKey, faultyRequestCount);
+
+        _memoryCache.Set(ipAddressKey, ++faultyRequestCount);


How about using IDistributedCache here instead, since this will be node-local, as you observed?

I'll do this is a separate PR along with my other notes.

MikeAlhayek · 2024-12-11T23:01:06Z

@rjpowers10 I made some change to the tag-helper to use shapes instead which allows reusing that shape from multiple places. Can you please give it one more try to make sure everything still works for you as expected?

sebastienros · 2024-12-11T23:13:09Z

src/OrchardCore.Modules/OrchardCore.ReCaptcha/Drivers/ReCaptchaLoginFormDisplayDriver.cs

-
-namespace OrchardCore.ReCaptcha.Drivers;
-
-public sealed class ReCaptchaLoginFormDisplayDriver : DisplayDriver<LoginForm>


You want a driver here to be able to extend the LoginForm with other things.

@sebastienros
No longer needed. We are rendering this using ShapeTableProvider. Look at the ReCaptchaShapeTableProvider class in this PR

I understand, but I think this should be driven by a driver. Or this is not extensible anymore.
What if you want another component extending the login form with another form or validation.

In order to fix linked bug, we have to remove it out of the DisplayDriver. LoginForm is still extensible. But we can't render the recaptcha from there as it screwes the robot counter.

If at least there was a way to take placement into account in this provider. Otherwise this is breaking. It seems like the shape from the driver was called "FormReCaptcha"

the driver was returning a shape result using the FormReCaptcha type. I think you can still use placement to targeting LoginForm_Edit. Maybe we should specify a differentiator.

src/docs/reference/core/Placement/README.md

src/docs/reference/modules/ReCaptcha/README.md

src/docs/releases/3.0.0.md

Co-authored-by: Sébastien Ros <sebastienros@gmail.com>

rjpowers10 · 2024-12-12T14:33:13Z

@MikeAlhayek I pulled your changes and the login form continues to work correctly. I fail 5 times and the ReCaptcha is shown on the 6th attempt.

However, registration is now broken. I wanted to confirm that registration is also counting the attempts correctly but I can't even get that far.

System.InvalidOperationException: The shape type 'FormReCaptcha' is not found for the theme 'TheAdmin'
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.ExecuteAsync(DisplayContext context) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 130
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.ExecuteAsync(DisplayContext context) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 189
   at OrchardCore.DisplayManagement.Zones.ZoneShapes.ContentZone(IDisplayHelper DisplayAsync, Object Shape, IShapeFactory ShapeFactory) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Zones\ZoneShapes.cs:line 51
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.<ProcessAsync>g__Awaited|13_0(Task`1 task) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 300
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.ExecuteAsync(DisplayContext context) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 135
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.ExecuteAsync(DisplayContext context) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 189
   at CallSite.Target(Closure, CallSite, Object)
   at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0)
   at AspNetCoreGeneratedDocument.Views_RegisterUserForm_Edit.<ExecuteAsync>b__12_0() in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore.Modules\OrchardCore.Users\Views\RegisterUserForm.Edit.cshtml:line 11
   at Microsoft.AspNetCore.Razor.Runtime.TagHelpers.TagHelperExecutionContext.GetChildContentAsync(Boolean useCachedResult, HtmlEncoder encoder)
   at Microsoft.AspNetCore.Mvc.TagHelpers.RenderAtEndOfFormTagHelper.ProcessAsync(TagHelperContext context, TagHelperOutput output)
   at Microsoft.AspNetCore.Razor.Runtime.TagHelpers.TagHelperRunner.<RunAsync>g__Awaited|0_0(Task task, TagHelperExecutionContext executionContext, Int32 i, Int32 count)
   at AspNetCoreGeneratedDocument.Views_RegisterUserForm_Edit.ExecuteAsync()
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageCoreAsync(IRazorPage page, ViewContext context)
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageAsync(IRazorPage page, ViewContext context, Boolean invokeViewStarts)
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderAsync(ViewContext context)
   at Microsoft.AspNetCore.Mvc.ViewFeatures.HtmlHelper.RenderPartialCoreAsync(String partialViewName, Object model, ViewDataDictionary viewData, TextWriter writer)
   at Microsoft.AspNetCore.Mvc.ViewFeatures.HtmlHelper.PartialAsync(String partialViewName, Object model, ViewDataDictionary viewData)
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.<ProcessAsync>g__Awaited|13_0(Task`1 task) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 300
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.ExecuteAsync(DisplayContext context) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 135
   at OrchardCore.DisplayManagement.Implementation.DefaultHtmlDisplay.ExecuteAsync(DisplayContext context) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.DisplayManagement\Implementation\DefaultHtmlDisplay.cs:line 189
   at CallSite.Target(Closure, CallSite, Object)
   at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0)
   at AspNetCoreGeneratedDocument.Views_Registration_Register.ExecuteAsync() in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore.Modules\OrchardCore.Users\Views\Registration\Register.cshtml:line 2
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageCoreAsync(IRazorPage page, ViewContext context)
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageAsync(IRazorPage page, ViewContext context, Boolean invokeViewStarts)
   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderAsync(ViewContext context)
   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ViewContext viewContext, String contentType, Nullable`1 statusCode)
   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ViewContext viewContext, String contentType, Nullable`1 statusCode)
   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ActionContext actionContext, IView view, ViewDataDictionary viewData, ITempDataDictionary tempData, String contentType, Nullable`1 statusCode)
   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewResultExecutor.ExecuteAsync(ActionContext context, ViewResult result)
   at Microsoft.AspNetCore.Mvc.ViewResult.ExecuteResultAsync(ActionContext context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResultFilterAsync>g__Awaited|30_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext httpContext, Boolean retry)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at OrchardCore.Liquid.ScriptsMiddleware.Invoke(HttpContext httpContext) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore.Modules\OrchardCore.Liquid\ScriptsMiddleware.cs:line 68
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at OrchardCore.Diagnostics.DiagnosticsStartupFilter.<>c__DisplayClass3_0.<<Configure>b__1>d.MoveNext() in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore.Modules\OrchardCore.Diagnostics\DiagnosticsStartupFilter.cs:line 33
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at OrchardCore.ContentPreview.PreviewStartupFilter.<>c.<<Configure>b__0_1>d.MoveNext() in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore.Modules\OrchardCore.ContentPreview\PreviewStartupFilter.cs:line 15
--- End of stack trace from previous location ---
   at OrchardCore.Modules.ModularTenantRouterMiddleware.Invoke(HttpContext httpContext) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantRouterMiddleware.cs:line 45
   at OrchardCore.Modules.ModularTenantContainerMiddleware.<>c__DisplayClass4_0.<<Invoke>b__0>d.MoveNext() in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 60
--- End of stack trace from previous location ---
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 286
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 291
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 296
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 301
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 302
   at OrchardCore.Modules.ModularTenantContainerMiddleware.Invoke(HttpContext httpContext) in C:\Users\ryan.powers\source\repos\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 58
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

rjpowers10 · 2024-12-12T14:35:17Z

@MikeAlhayek by the way Google has test keys for ReCaptcha you could plug in to make this easier to test yourself.

https://developers.google.com/recaptcha/docs/faq#id-like-to-run-automated-tests-with-recaptcha.-what-should-i-do

MikeAlhayek · 2024-12-12T16:24:00Z

@rjpowers10

@MikeAlhayek by the way Google has test keys for ReCaptcha you could plug in to make this easier to test yourself.

Thank you for the tip. I am aware and using this to test it myself. But needed you for a second round of testing. As you can see you spotted issues when I did not :)

registration is now broken. I wanted to confirm that registration is also counting the attempts correctly but I can't even get that far.

I fix this issue in all the views, like UserRegistration, UserForgotPassword and UserResetPassword.

Please try it out one more time to make sure you don't spot any additional issues.

Thank you!

rjpowers10 · 2024-12-12T16:34:57Z

I'll take another look.

rjpowers10 · 2024-12-12T18:18:27Z

I pulled the latest.

/login is still counting correctly.

/register no longer throws an exception for the missing template but it's still not working in that the ReCaptcha is never displayed no matter how many failures. Just doing some quick debugging, it looks like registration is calling DetectRobot but it's never calling FlagAsRobot so the counter is never incremented. Not sure if that's from your changes or if it was like that before.

Another observation (again, not sure if this is due to your changes or if it was like this before). Successfully logging in will call IsNotARobot to reset the counter. However, successfully registering does not. Not sure if this is intentional.

MikeAlhayek · 2024-12-12T18:39:45Z

@rjpowers10

Not sure if that's from your changes or if it was like that before.

This is from before. The threshold used for PreventRobots is complicated and it is not implemented everywhere.

Please provide thoughts on removing the PreventRobots model and simplify the implementation #17200 (comment)

MikeAlhayek · 2024-12-13T05:56:41Z

This PR was replaced by #17229

Fix reCaptcha LoginForm

b3e2964

Fix #17200

MikeAlhayek requested a review from Piedone December 11, 2024 17:13

Merge branch 'main' into ma/fix-recaptcha-on-login

07801e3

MikeAlhayek added 2 commits December 11, 2024 09:15

cleanup

d8d7624

Merge branch 'ma/fix-recaptcha-on-login' of https://github.com/Orchar…

8c0cf55

…dCMS/OrchardCore into ma/fix-recaptcha-on-login

cleanup

49a8ef1

Piedone requested changes Dec 11, 2024

View reviewed changes

MikeAlhayek added 3 commits December 11, 2024 14:49

Add ReCaptcha shape

898f287

add release notes

997c4e4

fix broken doc link

c955d71

sebastienros reviewed Dec 11, 2024

View reviewed changes

MikeAlhayek and others added 3 commits December 11, 2024 15:21

Update src/docs/reference/modules/ReCaptcha/README.md

3cd3084

Co-authored-by: Sébastien Ros <sebastienros@gmail.com>

Update src/docs/reference/modules/ReCaptcha/README.md

0f799db

Co-authored-by: Sébastien Ros <sebastienros@gmail.com>

remove bad line

ea66b06

MikeAlhayek requested review from sebastienros and Piedone December 11, 2024 23:27

MikeAlhayek added 4 commits December 11, 2024 17:37

DisplayManagement document

8a20c63

fix broken link

e0c59d5

fix mkdocs file

2685000

core module

fea9b06

Fix ReCaptch insjection in all drivers

9e067d0

improvements

9c45576

MikeAlhayek added 3 commits December 12, 2024 11:51

use a better counter and distributed cache

22ff3c7

Merge branch 'main' into ma/fix-recaptcha-on-login

fcb0488

fix build

c908657

MikeAlhayek closed this Dec 13, 2024

This was referenced Jan 1, 2025

Monthly community metrics report for 2024-12-01..2024-12-31 #17293

Open

Monthly community metrics report for 2024-12-01..2024-12-31 iaspnetcore/OrchardCore#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reCaptcha LoginForm #17218

Fix reCaptcha LoginForm #17218

MikeAlhayek commented Dec 11, 2024

MikeAlhayek commented Dec 11, 2024

rjpowers10 commented Dec 11, 2024

rjpowers10 commented Dec 11, 2024

MikeAlhayek commented Dec 11, 2024

rjpowers10 commented Dec 11, 2024

Piedone Dec 11, 2024

MikeAlhayek Dec 11, 2024

MikeAlhayek commented Dec 11, 2024

sebastienros Dec 11, 2024

MikeAlhayek Dec 11, 2024

sebastienros Dec 11, 2024

MikeAlhayek Dec 11, 2024

sebastienros Dec 12, 2024

MikeAlhayek Dec 12, 2024

rjpowers10 commented Dec 12, 2024

rjpowers10 commented Dec 12, 2024

MikeAlhayek commented Dec 12, 2024 •

edited

Loading

rjpowers10 commented Dec 12, 2024

rjpowers10 commented Dec 12, 2024

MikeAlhayek commented Dec 12, 2024 •

edited

Loading

MikeAlhayek commented Dec 13, 2024


		namespace OrchardCore.ReCaptcha.Drivers;

		public sealed class ReCaptchaLoginFormDisplayDriver : DisplayDriver<LoginForm>

Fix reCaptcha LoginForm #17218

Fix reCaptcha LoginForm #17218

Conversation

MikeAlhayek commented Dec 11, 2024

MikeAlhayek commented Dec 11, 2024

rjpowers10 commented Dec 11, 2024

rjpowers10 commented Dec 11, 2024

MikeAlhayek commented Dec 11, 2024

rjpowers10 commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeAlhayek commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjpowers10 commented Dec 12, 2024

rjpowers10 commented Dec 12, 2024

MikeAlhayek commented Dec 12, 2024 • edited Loading

rjpowers10 commented Dec 12, 2024

rjpowers10 commented Dec 12, 2024

MikeAlhayek commented Dec 12, 2024 • edited Loading

MikeAlhayek commented Dec 13, 2024

MikeAlhayek commented Dec 12, 2024 •

edited

Loading

MikeAlhayek commented Dec 12, 2024 •

edited

Loading