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

Fixing FormatException when the login screen is posted with values other than true/false for RememberMe (Lombiq Technologies: OCORE-132) #14845

Merged
merged 7 commits into from
Dec 14, 2023
Merged

Fixing FormatException when the login screen is posted with values other than true/false for RememberMe (Lombiq Technologies: OCORE-132) #14845

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
75e1307
Fixing FormatException when the login screen is posted with values ot…
Piedone Dec 6, 2023
c4f78a3
Merge remote-tracking branch 'official/main' into issue/OCORE-132
Piedone Dec 13, 2023
8cbd8f0
Changing anti-cracking attempt model binding to use a custom model bi…
Piedone Dec 13, 2023
90f4035
Moving SafeBoolModelBinder to OrchardCore.Mvc.Core
Piedone Dec 13, 2023
b808524
Sealing internal classes
Piedone Dec 13, 2023
70c50d1
Formatting
Piedone Dec 14, 2023
771df0b
Merge remote-tracking branch 'origin/main' into issue/OCORE-132
Piedone Dec 14, 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
1 change: 0 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Users/Controllers/AccountController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
using Microsoft.Extensions.Localization;
using Microsoft.Extensions.Logging;
using OrchardCore.DisplayManagement.Notify;
using OrchardCore.Entities;
using OrchardCore.Modules;
using OrchardCore.Mvc.Core.Utilities;
using OrchardCore.Settings;
Expand Down
10 changes: 10 additions & 0 deletions src/OrchardCore.Modules/OrchardCore.Users/Views/Account/Login.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@using Microsoft.AspNetCore.Identity
@using Microsoft.AspNetCore.Mvc.ModelBinding
@using OrchardCore.Entities
@using OrchardCore.Settings
@using OrchardCore.Users
Expand All @@ -14,6 +15,15 @@
var allowResetPassword = (await SiteService.GetSiteSettingsAsync()).As<ResetPasswordSettings>().AllowResetPassword;
var loginProviders = (await SignInManager.GetExternalAuthenticationSchemesAsync()).ToList();
var disableLocalLogin = (await SiteService.GetSiteSettingsAsync()).As<LoginSettings>().DisableLocalLogin;

// When the value of the RememberMe field is set to anything else than "true" or "false" by the client, which is
// the case with automated cracking attempts, then otherwise we'd get a FormatException from the model binder.
if (!ViewContext.ModelState.IsValid &&
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't move the logic into the controller? The view should have a minimalistic logic that is related to the presentation

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a workaround solely for the <input asp-for="RememberMe" class="form-check-input" tabindex="3"> line below, that's where the exception is thrown, due to using the auto-generation of the editor with asp-for. With a different implementation of the field (like by having a checkbox implemented directly) the exception won't be thrown. So, for other implementations of this view the code wouldn't necessarily be appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

When I initially thought about this issue, I didn't think of creating a workaround like you did. If it's fine for all to accept this workaround, or I might propose another PR to fix it in a proper way

Copy link
Member Author

Choose a reason for hiding this comment

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

What did you think about? Alternatives I tried but didn't like better:

  • Nullable RememberMe but that would need a hand-crafted checkbox because the tag helper doesn't support nullable booleans.
  • Hand-crafted checkbox by itself.
  • Custom model binding but that would be just ugly.

ViewContext.ModelState.TryGetValue(nameof(LoginViewModel.RememberMe), out var rememberMeValue) &&
rememberMeValue.ValidationState == ModelValidationState.Invalid)
{
rememberMeValue.RawValue = "false";
}
}

<style asp-name="font-awesome" version="6"></style>
Expand Down