Fixing that a captcha is required on external login and registration, renaming IExternalLoginEventHandler to IExternalLoginUserHandler (Lombiq Technologies: GOV-44)
#17490
Conversation
This reverts commit c7ff204.
|
I think this duplicate for #17489 |
|
No, that's for to be included in a patch release, this is breaking for 3.0. Also see the description of that PR. |
|
It seems this targets the |
|
|
||
| public LoginFormEventEventHandler(ReCaptchaService reCaptchaService) | ||
| public LoginFormEventEventHandler(ReCaptchaService reCaptchaService, SignInManager<IUser> signInManager) |
There was a problem hiding this comment.
Please span over multiple lines while leaving the first line empty
src/OrchardCore.Modules/OrchardCore.ReCaptcha/Users/Handlers/RegistrationFormEventHandler.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs
Outdated
Show resolved
Hide resolved
| => _reCaptchaService.ValidateCaptchaAsync(reportError); | ||
| public override async Task RegistrationValidationAsync(Action<string, string> reportError) | ||
| { | ||
| if (await _signInManager.GetExternalLoginInfoAsync() == null) |
There was a problem hiding this comment.
It maybe be helpful to add a comment here explaining why not to validate when we're is an external am login.
And I would do if(... is not null)
{
return;
}
There was a problem hiding this comment.
I don't really see the point of doing an early return when the rest of the method is a single statement, since then it just makes it more verbose. But I changed these.
| => _reCaptchaService.ValidateCaptchaAsync(reportError); | ||
| public override async Task LoggingInAsync(string userName, Action<string, string> reportError) | ||
| { | ||
| if (await _signInManager.GetExternalLoginInfoAsync() == null) |
There was a problem hiding this comment.
Same as below, I would invert the condition and add a return. Also add a small comment.
|
The rename is because under #17422 (comment) you said you're not satisfied with an event handler generating these values, but I don't insist. |
|
Please backport this PR too once you merge it. |
src/OrchardCore.Modules/OrchardCore.ReCaptcha/Users/Handlers/LoginFormEventEventHandler.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.ReCaptcha/Users/Handlers/RegistrationFormEventHandler.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Mike Alhayek <[email protected]>
Co-authored-by: Mike Alhayek <[email protected]>
|
/backport to release/2.1 |
|
Started backporting to release/2.1: https://github.com/OrchardCMS/OrchardCore/actions/runs/13378469725 |
|
@MikeAlhayek backporting to "release/2.1" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixing variable names
Using index info to reconstruct a base tree...
M src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs
Falling back to patching base and 3-way merge...
Auto-merging src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs
CONFLICT (content): Merge conflict in src/OrchardCore.Modules/OrchardCore.Users/Controllers/ExternalAuthenticationsController.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fixing variable names
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
Fixes #17422.
Hotfix for 2.1.x: #17489.